passing configuration to dialog Topic is solved

If you are using the main C++ distribution of wxWidgets, Feel free to ask any question related to wxWidgets development here. This means questions regarding to C++ and wxWidgets, not compile problems.
Post Reply
loptr
Earned some good credits
Earned some good credits
Posts: 110
Joined: Tue Jan 23, 2007 12:22 pm
Location: Kiel, Germany
Contact:

passing configuration to dialog

Post by loptr » Tue Jan 23, 2007 11:05 pm

and once again me.

the prob:
i need to pass an object containing the configuration data to a dialog. so far so good. and IN the dialog the configuration will be changed. still no prob, testing shows the right data after altering it (still in the dialog). but when i'm back in the main app, the data seems to be lost (the app hangs with an access error - just as if the object did not exist... - although the assignments worked in the dialog).

question:
i passed the config-object as a pointer

Code: Select all

 
ConfigDialog(Config* sCfg) { ... } //constructor
should i better pass it as a reference?

Code: Select all

ConfigDialog(Config& sCfg) { ... } 
or am i completely wrong? what the heck am i missing?

Auria
Site Admin
Site Admin
Posts: 6695
Joined: Thu Sep 28, 2006 12:23 am
Contact:

Post by Auria » Wed Jan 24, 2007 12:32 am

would be interesting to see how you create, store and pass Config, and how you alter it (i.e. code snippets or minimal code sample demonstrating issue?)

ScratchMonkey
Experienced Solver
Experienced Solver
Posts: 56
Joined: Mon Jan 01, 2007 10:34 pm
Location: San Pablo, CA
Contact:

Post by ScratchMonkey » Wed Jan 24, 2007 2:06 am

You might hide the copy/assignment operators for your Config class. (Move the declarations to the private section.) That often turns up unexpected and unwanted copy operations that lose modifications to the original.

As a rule, when defining a class, you should always declare the "big 4": Default constructor, destructor, copy constructor, and assignment operator. Any that you are willing to let the compiler provide should at least have a comment stating so. I've been burned many times by forgetting to follow this rule and discovering an undesirable copy being made.

loptr
Earned some good credits
Earned some good credits
Posts: 110
Joined: Tue Jan 23, 2007 12:22 pm
Location: Kiel, Germany
Contact:

Post by loptr » Wed Jan 24, 2007 7:27 am

@monkey:
thx for the hint. so far there was never need for me to redefine the copy constructor or the assignment operator. but i will try...
btw: i always declare ALL access functions as private (except for bools; actually i don't use funcs for bools und thus make the boolean attributes public), but that can't be the point here (though i admit that it might be better to also declare the bools private)

@auria:
its quite simple. i run through an array of spinCtrls, and if their value is greater than zero a new object is created (i'm at work now, but it must go like this):

Code: Select all

void AssignArmy() {
    Stone* tmpStone=new Stone*[stoneCount];
    int aux=0;

    for (int i=0; i<stoneCount; i++) {
        if (spins[i]->GetValue()>0) {
            for (int j=0; j<spins[i]->GetValue(); j++) {
                //Stone constructor takes values for team, value and position
                Stone* tS=new Stone(game->GetTeamOn(), i, -99);
                tmpStone[i]=tS;

                //cleanup
                delete tS;
                tS=NULL;
            }//for
        }//if

    //assign to the configClass
    if (game->GetTeamOn()==1)
        game->GetMyPlayer->SelectArmy(tmpStone, stoneCount);
    else
        game->GetOpponent()->selectArmy(tmpStone, stoneCount);

    //tmpStone can now be deleted
    delete tmpStone;
    tmpStone=NULL;
}//end of function; dialog ends modal immediately after this
as for the SelectArmy(), it only copies the submitted array:

Code: Select all

//in class Player:
void Player::SelectArmy(Stone* sStone[], int sCount) {
    army=sStone;
    armyStrength=sCount;
}//SelectArmy()
there must be a more elegant way to determine the size of the array, but i have difficulties getting it (since sizeof() always returns the size of ONE pointer to object of this class, no matter wether it is called on an array)
Last edited by loptr on Wed Jan 24, 2007 8:00 am, edited 1 time in total.

imekon
Earned a small fee
Earned a small fee
Posts: 10
Joined: Tue Jan 16, 2007 3:41 pm

Post by imekon » Wed Jan 24, 2007 7:34 am

Code: Select all

//Stone constructor takes values for team, value and position
Stone* tS=new Stone(game->GetTeamOn(), i, -99);
tmpStone[i]=tS;

//cleanup
delete tS;
tS=NULL; 
You create a Stone object, store its pointer in an array, then delete it. All you've done is store a non-existant pointer, a recipe for a crash later.

Surely you meant:

Code: Select all

//Stone constructor takes values for team, value and position
Stone* tS=new Stone(game->GetTeamOn(), i, -99);
tmpStone[i]=tS;

//cleanup
tS=NULL; 
Then you're storing the object pointer without deleting it.

loptr
Earned some good credits
Earned some good credits
Posts: 110
Joined: Tue Jan 23, 2007 12:22 pm
Location: Kiel, Germany
Contact:

Post by loptr » Wed Jan 24, 2007 7:59 am

as far as i know, the pointer is COPIED to the array. so the pointer in the array points to the Stone object on the heap tS had pointed to. since there is no further need for that pointer (tS), it should be safe to delete it. the object on the heap is not deleted - when i haven't an error in reasoning here...

otherwise: how comes that the values in tmpStone (the Stone[]) are correct at the end of the function (checked with a debugFunction), i.e. no access error or crash when trying to access the different objects by their respective pointers in the array?
that's what happens normally when you try to access a non-existent object, isn't it?
[EDIT]
and the assignment to game->GetPlayer() also works fine...
[/EDIT

nevertheless, i'll try out your suggestion when i'm home again. maybe that's the solution... ;)

Jorg
Moderator
Moderator
Posts: 3971
Joined: Fri Aug 27, 2004 9:38 pm
Location: Delft, Netherlands
Contact:

Post by Jorg » Wed Jan 24, 2007 8:14 am

Please beware (I do not know what language you originally developed in). Copying pointers in C++ is nothing more byut copying an address pointer, meaning where it points to. When you are freeing up one pointer that points to the class you are referencing on serveral places, it will be deleted on all places.

So,

SomeClass *A = new SomeClass();

SomeClass *B = A;

delete A;
B is now also invalid, although it still points to that memory location. This is called a "dangling pointer", meaning, it is there but points to data that is freed and can at any time be re-used by the memory manager.

The reason why the pointer still points to some data even after deleting one of the assigned pointers, is because the memory manager has not yet re-assigned that particular spot to another allocation. Very nasty things can happen, it can go good for a long time, but maybe at some point parts of that memory are re-used.

The reason why you can even read the data from the classes even when the pointer is freed and even call it's methods without problems, is that the C++ implementation of a class is that the code is always seperated from the data. Once you use data in your class, only the 'this' pointer points to the location your part of the data is stored.

Freeing up pointers like you do only works when there is a garbage collector like in Java, Python or C#. C++ is fast, but the penalty is you will have to manage pointers yourself.

To add, if you really want to make copies of your class to store in an array you can either use a copy-constructor or operator=. Also a std::vector<YouClass> can be used to store deep copied objects in an array. However, it is not recommended when your class also holds relations to other classes as pointers. You cannot control the lifetime easily when it is copied, so leaving it on the heap is safer.

Regards
- Jorgen
Forensic Software Engineer
Netherlands Forensic Insitute
http://english.forensischinstituut.nl/
-------------------------------------
Jorg's WasteBucket
http://www.xs4all.nl/~jorgb/wb

loptr
Earned some good credits
Earned some good credits
Posts: 110
Joined: Tue Jan 23, 2007 12:22 pm
Location: Kiel, Germany
Contact:

Post by loptr » Wed Jan 24, 2007 8:38 am

yeah, thx.

i also just came up that the array will (at the latest) be deleted when leaving the dialog...

idea is now to directly assign the objects to the player by replacing the old SelectArmy() by two new functions:

Code: Select all

void Player::SetArmy(int sSize) { army=new Stone*[sSize]; }

void Player::SetArmyMember(int offset, int sTeam, int sBase, int sPos) {
    army[offset]=new Stone(sTeam, sBase, sPos); //Stone has got a corresponding constructor
}
the original code would the be modified to fit this, of course...

Code: Select all

int aux=0;

void AssignArmy() {
    game->GetMyPlayer->SetArmy(stoneCount);

    for (int i=0; i<stoneCount; i++) {
        if (spins[i]->GetValue()>0) {
            for (int j=0; j<spins[i]->GetValue(); j++) {
                //set the player's army          
                game->GetMyPlayer->SetArmyMember(aux, game->GetTeamOn, i, -99);
                aux++;
            }//for
        }//if
    }//for
}//end of function; dialog ends modal immediately after this
this should work, no?

as for the copyConstructor and the operator. to be honest, i always tried to avoid overloading THEM, because i'm not quite sure of what to do (constructor and destructor is easy). already read some books and articles about it, but i'm still not sure... perhaps a little walkthrough might help (and please don't say: why, it's as easy as with the constructor/destructor). as mentioned elsewhere, i'm (at best) an intermediate programmer... ;)

Jorg
Moderator
Moderator
Posts: 3971
Joined: Fri Aug 27, 2004 9:38 pm
Location: Delft, Netherlands
Contact:

Post by Jorg » Wed Jan 24, 2007 9:30 am

Hi,

First of all, my personal opinion on passing config to a dialog should be kept as simple as possible. Which means;

- Create a method called ConfigDlg::PutData(YourClass *data)
- Create a method called ConfigDlg::GetData(YourClass *data)

- Instantiate your settings dialog like this;

Code: Select all

YourFrame::OnSettings()
{
    ConfigDlg *dlg = new ConfigDlg(...);
    dlg->PutData(aClass);
    if(dlg->ShowModal() == wxID_OK)
       dlg->GetData(aClass);

   dlg->Destroy();
}
So what you do is when the user selects CANCEL you do not put the data back in the class. When the user selects OK the data is stored in the class.

In PutData you sync all GUI elements with your class data. If needed create shadow classes to work on, if your config is complex. When the user presses OK, in GetData you sync back all the data to your class.

This is the most common way of manipulating your class. Also, if possible try to keep the NEW action similar to the EDIT action. What I mean is when you create a new class the user needs to fill in (e.g. a project item) and the user needs to edit it later, it is better to use one dialog for both actions.

- Jorgen
Forensic Software Engineer
Netherlands Forensic Insitute
http://english.forensischinstituut.nl/
-------------------------------------
Jorg's WasteBucket
http://www.xs4all.nl/~jorgb/wb

loptr
Earned some good credits
Earned some good credits
Posts: 110
Joined: Tue Jan 23, 2007 12:22 pm
Location: Kiel, Germany
Contact:

Post by loptr » Wed Jan 24, 2007 10:28 am

thx a lot. will try this...

Post Reply