delete window crash

Looks like I am only posting messages to report crashes these days.

This time it happens, when I try to delete a window. I remember having this problem before, but wasn't it fixed in SVN some time ago?

Here is the backtrace:


#0 005A1B2E ZN8QuickGUI12VertexBuffer6updateEv() (C:\code\TinEngine\Core\Test\QuickGUI.dll:??)

#1 0059137F ZN8QuickGUI13QuadContainer6renderEv() (C:\code\TinEngine\Core\Test\QuickGUI.dll:??)

#2 00591470 ZN8QuickGUI13QuadContainer6renderEv() (C:\code\TinEngine\Core\Test\QuickGUI.dll:??)

#3 0057CC2A ZN8QuickGUI10GUIManager18renderQueueStartedEhRKSsRb() (C:\code\TinEngine\Core\Test\QuickGUI.dll:??)

#4 65127B9A ZN4Ogre12SceneManager22fireRenderQueueStartedEhRKSs() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#5 65127C87 ZN4Ogre12SceneManager35renderVisibleObjectsDefaultSequenceEv() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#6 65134FD9 ZN4Ogre12SceneManager12_renderSceneEPNS_6CameraEPNS_8ViewportEb() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#7 6A2D4A65 ZN4Ogre19TerrainSceneManager12_renderSceneEPNS_6CameraEPNS_8ViewportEb() (C:\code\TinEngine\Core\Test\Plugin_OctreeSceneManager.dll:??)

#8 64F30253 ZN4Ogre6Camera12_renderSceneEPNS_8ViewportEb() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#9 651B2A96 ZN4Ogre8Viewport6updateEv() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#10 650FA9B3 ZN4Ogre12RenderTarget6updateEv() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#11 650FEEAB ZN4Ogre12RenderWindow6updateEb() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#12 66F829C0 dllStopPlugin() (C:\code\TinEngine\Core\Test\RenderSystem_Direct3D9.dll:??)

#13 650FEE8D ZN4Ogre12RenderWindow6updateEv() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#14 650F5DB2 ZN4Ogre12RenderSystem23_updateAllRenderTargetsEv() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#15 65124E41 ZN4Ogre4Root14startRenderingEv() (C:\code\TinEngine\Core\Test\OgreMain.dll:??)

#16 00417B62 tcf::engine::Invoke() (??:??)

#17 00403BCC main() (??:??)



Lol, apparently I have posted exactly this bug a while ago:

http://www.ogre3d.org/phpBB2addons/viewtopic.php?t=5163

So, it hasn't been fixed yet.

Looks like I am only posting messages to report crashes these days.

Sorry about that.. I hope you don't get discouraged!

Have you tried GUIManager::destroyWidget, instead of "delete myWindow;"? The problem is GUIManager stores references to the widget with the mouse over it, and the active widget (last widget clicked, in focus), and the code assumes these values will always point to a valid widget. using GUIManager::destroyWidget function, these 2 references are set to the active sheet. Also, the GUIManager version adds the widget to be deleted on a freeList, which will be checked next frame. The reason for this is to safely allow the user to specify to delete a widget within one of its event handlers. (quick example: clicking on a window deletes it. You cannot delete the window before the event handler finishes executing, a crash will occur. Thus you have to mark it for deletion and delete it after executing the event handler fully)

The GUIManager::destroyWidget is meant to be the safest possible method to destroy a widget. If that still gives you problems then there is a bug I need to fix. I should add this information to the wiki.

If you have suggestions regarding the GUIManager holding references to always valid widgets, let me know. Sometimes I try to minimize comparisons per frame, and sometimes I add in comparisons for robustness.

I just thought of a good idea.. make the Widget destructor private! This would require users to destroy widgets safely at all times. (GUIManager would have to be a friend) This kind of follows the Ogre method for handling Entities/SceneNodes/etc. What do you think?

Looks like I am only posting messages to report crashes these days.

Sorry about that.. I hope you don't get discouraged!


I am pretty much frustration resistant and the overall design of QuickGUI still looks rather attractive (though I have to admit, that there are aspects of the implementation, that make me slightly nervous). I certainly will give this library some more time, before I start to look for alternatives.


Have you tried GUIManager::destroyWidget, instead of "delete myWindow;"? The problem is GUIManager stores references to the widget with the mouse over it, and the active widget (last widget clicked, in focus), and the code assumes these values will always point to a valid widget. using GUIManager::destroyWidget function, these 2 references are set to the active sheet. Also, the GUIManager version adds the widget to be deleted on a freeList, which will be checked next frame. The reason for this is to safely allow the user to specify to delete a widget within one of its event handlers. (quick example: clicking on a window deletes it. You cannot delete the window before the event handler finishes executing, a crash will occur. Thus you have to mark it for deletion and delete it after executing the event handler fully)

The GUIManager::destroyWidget is meant to be the safest possible method to destroy a widget. If that still gives you problems then there is a bug I need to fix. I should add this information to the wiki.


In fact I am getting exactly the same crash, when using destroyWidget.


If you have suggestions regarding the GUIManager holding references to always valid widgets, let me know. Sometimes I try to minimize comparisons per frame, and sometimes I add in comparisons for robustness.

I would always go for robustness. In this special case, if the widget destructor should not be used from outside the manager class, you should make it inaccessible for anything but the manager. That should avoid the whole problem. Another alternative would be having the relevant widget hold a pointer to its manager, so it can tell the manager, when it won't be available any more. But with the current design the first option is probably better.

Edit:



What do you think?



Good idea. I guess, I was too slow

Widgets do store pointers to GUIManager, since GUIManager maintains unique names of widgets, does the work of making a widget active (contain focus), stores the window (viewport?) dimensions, etc.

I had also thought of the method where the widget would notify the GUIManager it is not available.. I thought the free list would remove the need for this functionality, but now that I think about it.. we can make this function so that widgets that are disabled or other (maybe when hidden) can tell the GUIManager it is no longer participating in certain events like losing/gaining focus and mouse over/enter/leave.

Thanks for the great ideas!

Regarding the crash, I will see if I can reproduce this in the demo tonight. If you could list anything that might help me repro this, please do.

The bug sits way to deep in QuickGUI's entrails for me to make any sense of it (at least without throughout study of the code, which I don't have the time for right now):

Here is the code for creating the window:


QuickGUI::Sheet& Sheet = *QuickGUI::GUIManager::getSingleton().getDefaultSheet();

d_Main = Sheet.createWindow (QuickGUI::Rect (0, 0, 1, 0.4));
d_Main->hideTitlebar();


And here for deleting it:


if (d_Main)
{
QuickGUI::GUIManager::getSingleton().destroyWidget (d_Main);
d_Main = 0;
}


I easily reproduced this, found the error, and made a fix for it. However, I think I want to brush up on my implementation, especially since I'm planning a medium sized internal re-design.

If you want a quick fix for now, add this at the beginning of Panel deconstructor:

// Remove from parent QuadContainer if attached to one.
if( mOwner != NULL )
{
if(mOwner->getWidgetType() == Widget::TYPE_WINDOW)
mOwner->getQuadContainer()->removeChildWindowGroup(mID);
else if(mOwner->getWidgetType() == Widget::TYPE_PANEL)
mOwner->getQuadContainer()->removeChildPanelGroup(mID);
}


Problem was that I was not removing the QuadContainer from it's parent's container, which causes a problem when parent tries to tell it's child containers to render.

Working on a more elegant/robust solution..

I have applied the fix and it is in SVN, widgets should be destroyed properly. I have made a lot of changes, I wouldn't recommend updating SVN until after the removal of Singleton's from GUIManager and MouseCursor are complete, which should hopefully be tomorrow evening. I need to brush up on other GUI libs to see how rendering is done on multiple windows, to see if I'm heading in the right direction.

Your fix doesn't even compile with rev 124:
C:\code\Ogre3rd\QuickGUI_SVN\QuickGUI\src\QuickGUIPanel.cpp:37: error: 'class QuickGUI::Widget' has no member named 'getQuadContainer'

What version is it supposed to be used with?

Updated to rev 126 anyway. It's working. Looks like we are finally getting somewhere. Thanks.

Getting some real work done now. At least I have most of the input component of my console working again. A few issues, questions, feature requests:

- When I clear the TextBox after entering some text, the cursor is stuck at its old position. Shouldn't it be moved back to the left side automatically?

- Is there any way to set the text colour?

- I noticed, that QuickGUI doesn't have auto-repeat for keyboard-input (same as CEGUI). Are you planning to add this feature or I am supposed to implement it outside of QUickGUI (wouldn't be a problem)

- The names of the Widget methods are looking a bit inconsistent (sometimes a get/set prefix, sometimes not; setting the text in the TextBox is done by calling setText, but reading is done by calling getCaption, ...)

- I would like to see a method to set a widget to its "default size", e.g. a Textbox doesn't have a default width, but it has a "natural" height, which is defined by the height of the font and of the border. Something similar could probably be used for most buttons and some other widgets.

- Is there a way to stop a specific widget from losing the focus? I tried to use EVENT_LOSE_FOCUS to restore it immediately back to the widget. But that doesn't seem to work.

- When I clear the TextBox after entering some text, the cursor is stuck at its old position. Shouldn't it be moved back to the left side automatically?

Good find! I will fix this tonight. If you want a temp fix, make a call to "setCursorPosition(0)" inside TextBox::setText, inside the if statement checking for captions that are "".

- Is there any way to set the text colour?

I suggest briefly skimming the Wiki I put up, it should have useful information on it. All widgets that derive from Label have a Text Object, and it is gotten via Label::getText(). Color is set via the Text object, as well as the font, and you can even color character by character if you wanted. (color per character will not hold if you change the caption.. I need to implement a list of colors to store this state)

- I noticed, that QuickGUI doesn't have auto-repeat for keyboard-input (same as CEGUI). Are you planning to add this feature or I am supposed to implement it outside of QUickGUI (wouldn't be a problem)

I'd like your input on this. I actually did have plans to implement this, there are still more refinements I'd like to make to TextBox, most likely when I go to implement the MultiLineTextBox. (ie maxCharacterLength, Copy/Paste/Cut) I was thinking of imitating how text works on windows. You press a button, ie backspace, and it happens once, until a set time passes, and then it repeats at a set rate. I can add functions to modify the repeat rate. That or I can leave it up to the user..

- The names of the Widget methods are looking a bit inconsistent (sometimes a get/set prefix, sometimes not; setting the text in the TextBox is done by calling setText, but reading is done by calling getCaption, ...)


You have spotted the one obviously inconsistency I know of. If you want I could remove that, but it seems odd to *set the caption of a textbox*. (At least, seems odd to me)

- I would like to see a method to set a widget to its "default size", e.g. a Textbox doesn't have a default width, but it has a "natural" height, which is defined by the height of the font and of the border. Something similar could probably be used for most buttons and some other widgets.

You want the height of widgets to fit around the text dimensions? Its a good idea, I will need to think about how to incorporate this into the library. First thought would be to make an extra create function for text related widgets that accepted a text string instead of Size. How does that sound?

example:
Button* Panel::createButton(Point p, const Ogre::String& buttonText, Gui_Metrics_Mode gmm = GMM_RELATIVE, const Ogre::String& texture = "");

- Is there a way to stop a specific widget from losing the focus? I tried to use EVENT_LOSE_FOCUS to restore it immediately back to the widget. But that doesn't seem to work.

Sounds like a feature that should be added into GUIManager. What would be a good name for a function of GUIManager, which allows the user to supply a widget, which will remain active at all times, not allowing other widgets to become the active widget? (This would only work if you have 1 TextBox and it was the always active widget.. only the active widget has character injections) For now you can try to call GUIManager::setActiveWidget every frame and see if that works.

Thanks for the comments!

- Is there any way to set the text colour?

I suggest briefly skimming the Wiki I put up, it should have useful information on it. All widgets that derive from Label have a Text Object, and it is gotten via Label::getText(). Color is set via the Text object, as well as the font, and you can even color character by character if you wanted. (color per character will not hold if you change the caption.. I need to implement a list of colors to store this state)


Didn't notice that you had expanded the Wiki entry that much. Last time I looked it was much smaller


- I noticed, that QuickGUI doesn't have auto-repeat for keyboard-input (same as CEGUI). Are you planning to add this feature or I am supposed to implement it outside of QUickGUI (wouldn't be a problem)

I'd like your input on this. I actually did have plans to implement this, there are still more refinements I'd like to make to TextBox, most likely when I go to implement the MultiLineTextBox. (ie maxCharacterLength, Copy/Paste/Cut) I was thinking of imitating how text works on windows. You press a button, ie backspace, and it happens once, until a set time passes, and then it repeats at a set rate. I can add functions to modify the repeat rate. That or I can leave it up to the user.


That is a really interesting question. I guess it is mostly a matter of taste. But if you are going to implement it, it should be as flexible as possible. The implementation you mentioned is only one possible solution. Since we both agree that the main target for QuickGUI is game development, we can't assume that the behaviour as we know it from general purpose GUI systems (e.g. Windows) will automatically be sufficient (the suitability or rather the lack of suitability when it comes to any behaviour exhibit by anything Windows-related is an entirely different topic).
I have some ideas how to do it, but you might be better off for now, if you place it on your list for QuickGUI 2.0 . After all it can be easily implemented outside QuickGUI and its not like you don't have enough other things to fix and to add.


- I would like to see a method to set a widget to its "default size", e.g. a Textbox doesn't have a default width, but it has a "natural" height, which is defined by the height of the font and of the border. Something similar could probably be used for most buttons and some other widgets.

You want the height of widgets to fit around the text dimensions? Its a good idea, I will need to think about how to incorporate this into the library. First thought would be to make an extra create function for text related widgets that accepted a text string instead of Size. How does that sound?


That is certainly an option, though you already have a lot of create methods. I can't offer anything better right now.


- Is there a way to stop a specific widget from losing the focus? I tried to use EVENT_LOSE_FOCUS to restore it immediately back to the widget. But that doesn't seem to work.

Sounds like a feature that should be added into GUIManager. What would be a good name for a function of GUIManager, which allows the user to supply a widget, which will remain active at all times, not allowing other widgets to become the active widget? (This would only work if you have 1 TextBox and it was the always active widget.. only the active widget has character injections) For now you can try to call GUIManager::setActiveWidget every frame and see if that works.
!

I am not sure about this one. As mentioned above the usual GUI behaviour might not always what you want, if you write a game (things like active widget losing focus, when clicking on the background/sheet; clicking on a window automatically gives it the focus and so on). Simply disallowing a single window to lose the focus doesn't look flexible enough to me. I know, requested it. But after thinking about it again, I am not sure about it any more.
Anyway, I can live with the workaround you mentioned.

My manager needs to give me more work..

I just had a better thought regarding the default size idea:


protected:
Label::_matchHeightToFontHeight();
Label::_matchWidthToCaptionWidth();

public:
Label::setAutoFit(bool heightToFontHeight, bool widthToCaptionWidth);

Label::_notifyTextChanged();


Text will be implemented to notify its parent widget when it's caption/font changes.

I'm up for better function names, sometimes its hard to think up good, clear, intuitive names. But commenting should help some.

Actually I prefer your first version of the default size setting method (looks like I can't make up my mind today). With the second version you have to pass a dummy size argument at construction time and you need an additional step.

Furthermore the interface for the 2nd version looks like the widget would change its size, whenever its contents is changed. I can't think of an application, where this would be useful.

So what I had in mind was to allow the ability to match the height of the glyph, or match the width of the caption, or both.

By default, Label and derived widgets have a Text object, which is created with the default font, which can be changed at any time. Currently, the Owner Widget is not aware of any changes made to the Text object. (why I opted for a _notify method. But only if we need it..)

- In order to match the height, we need to supply the font being used, so its pixel height can be determined and used.

- In order to match the width, we need to supply both the font being used, and the caption to match.

The main problem is that we don't want to have to guess and supply dummy values which are later changed, depending on the Text properties. If you think up any elegant way to accomplish this that would be great. I'll be thinking about this in the background.

Think again about the notify method! When the user changes the text of a TextBox, this would change the size of the TextBox. But is there enough room for the TextBox to grow? The TextBox can not know, so it would need to inform it's owner. And the owner would need to know how to arrange all its widgets, so it can make room. If you continue this way, we end up in layout hell very fast.

Simply adding more overloaded create method should do. As an alternative I was thinking about adding another metrics mode, But the design of your create methods don't support this, because metrics mode doesn't distinguish between width and height. And no, I don't propose another redesign of the create methods to get seperate width and height metrics modes. These methods are already complicated enough.

Of all options your very first idea seems to be the cleanest (though if you want to use auto-width and auto-height independently, you probably need to tripple the number of create methods).

Another idea: Let the Rect class handle it. You could add additional constructors to it and the class would need to be able to store a special size value (-1), that indicates auto sizing:

Rect (Ogre::Real x, Ogre::Real y, Ogre::Real width, Ogre::Real hight);
Rect (Ogre::Real x, Ogre::Real x); // both default
Rect (Ogre::Real x, Ogre::Real y, Ogre::Real widthOrHeight, bool HeightDefault)

How bad of an idea is it to use 0 as the special value instead of -1? What instances are there where people want widget of width/height 0?

I think your latest idea is the best so far. We keep all regular methods, but using 0 (or -1) as a value for width or height implies auto sizing.

I'm not sure I understand your position on notify. Is there some alternate method you had in mind? notify only works on a small scale, when one object wants to inform another object that something has changed.

How about this: we create a ON_TEXT_CHANGED event and event handler, so my Label class can tap into this event and perform sizing? (I like this idea a lot.. the more events available the better)

Text::addOnTextChangedEventHandler(...);

I will make a TextEventArgs, which includes the current caption, font size, text color, and flags to know which changed.

You are right with simply using -1 or 0 as a default value (doesn't really matter which one). Its much easier and avoids problems with overloading ambiguity. I was about to propose the same.

Regarding the notify method: I can see, where this could come in handy with special widgets, e.g.

- a menu, which must be widened, when a menu item is widened.

- the vertical scrollbar of a multi-line text widget, which must be adjusted, when additional lines are added

- the horizontal scrollbar of a multi-line text widget, which must be adjusted, when the maximum line width is changed

These are applications, that escaped me, when I wrote this post (I was a bit in a hurry).

Beyond that, I don't see the point, though the reason given above should be good enough to implement a notify method.

The ON_TEXT_CHANGED event is something entirely different. Usually you would read the content of an input text widget only, when the return key is pressed. But I can imagine a situation, where the program wants to be informed instantly of any user input. I am not sure if that is possible with the current event system. If not, an additional event would be a good idea.

On the other hand using an event to inform the program, that the program just has changed a widget, sounds like overkill to me. From my experience on GUI programming I would say that these things are best dealt with outside the library.

I believe having lots of Events is a good thing. I try to program with minimal assumptions.. even if I can't think of an application off the top of my head, somebody else will probably find a need for it. Remember, each event is given a list of event handlers. The list by default is empty, so even if I have 30 event handler lists, in reality this could be 30 empty lists. If you've programmed using MSVC++ or C#, you can see how many events that each widget can have, its actually quite a bit. Even then, I found myself wishing there were other event handlers for use. (Unfortunately I've forgotten what I needed, its been a while..) From a user's perspective, it would take some effort to modify the library and support an event that hasn't been defined.

Before this discussion, I was already planning on adding more events for next release. So the the current list as I remember it would be:

Text:
ON_TEXT_CHANGED

Widget:
ON_SHOWN
ON_HIDDEN
ON_POSITION_CHANGED
ON_SIZE_CHANGED
ON_DISABLED
ON_ENABLED
ON_CHILD_ADDED
ON_CHILD_REMOVED

Also, the library subscribes to a lot of events internally to function properly. (Basic examples are TitleBar's close button Hiding the Window, or ComboBox selection notifying combobox, so that it can display the selection and hide the list)

I hope you don't find this a bad design decision.. these events happen under the hood, I'm just letting users have the ability to subscribe to them to perform any special effects or other functionality they may want.

I guess with MSVC++ you mean WinAPI or MFC? Sorry, but this stuff is total crap. Had only a short look at MFC and was running away screaming (don't really wanna talk about WinAPI). Haven't used C# either. My GUI experience consists of some Gtkmm coding, a tiny bit of Java coding (ugh!) and writing my own GUI library under RISC OS (and using this library a lot).

It is my firm believe that any piece of software should be as complex as necessary, but no more complex than that. IMHO simplicity and compactness are major design goals. Sometimes a software developer has to take a bold stand for the sake of compactness and refuse to add more stuff, that might some day be useful for someone.

But these are only my opinions and I am certainly not trying to impose my design philosophy on you. Got a bit off topic, sorry! If you want to add these events I have no problem with that (besides, if I were in your place, I would have added about half of it myself).

Alright, all the events are added! It actually created a lot of extra work for me to do, I decided to wipe the previous way I was firing events and make it easier to add events/event handlers. And now, all widgets function internally via use of event handlers!! Every widget file was touched, and even GUIManager was changed some. (mActiveWidget and mWidgetContainingMouse can't and shouldn't be NULL in any case!)

Anyhow, I didn't get where I wanted to, but I did a lot of cleaning, with more to come tomorrow. If you could update to latest revision and list all problems you encounter, that would help me out a lot. I didn't see any crashes or bugs in the demo, but I might have done something CB isn't accustomed to.

Actually I haven't tried the demo (would mean another round of wrestling with Code::Blocks' VC-importer, which I am not to keen on). However the library itself doesn't even compile:

I am getting: C:\code\Ogre3rd\QuickGUI_SVN\QuickGUI\src\QuickGUIComboBox.cpp:180: error: no matching function for call to `QuickGUI::ComboBox::fireEvent(QuickGUI::Widget::Event, QuickGUI::WidgetEventArgs)'

on

fireEvent(EVENT_SHOWN,WidgetEventArgs(this));

Which is what is supposed to happen. You can't bind a temporary to a non-const reference. The C++ standard explicitly disallows it. I know that MSVC is a bit slackly regarding these thing. If you want to produce standard conforming C++ source, you might want to look into disabling some of the MSVC specific extensions (don't know if that is possible).

So is this acceptable?


WidgetEventArgs args(this);
fireEvent(X,args);


I'm off to work now, I have committed the changes to SVN, regarding this new method of calling fireEvent, I hope it fixes the issue.

Yes, that should work.

Edit: btw. please don't blame Code::Blocks. It really hasn't got anything to do with it, since it is only an IDE: The underlying compiler is gcc (the MingW variant). And you are not changing the code for gcc's sake, but to become compatible with the C++ language. The original version wasn't valid C++.

I understand, I'm just blinded by .NET. Either way, I'll refer to them as compatability issues, regardless of who is at fault.

[edit] When the QuickGUI becomes mature enough, I might be tempted to remove my dependency of Visual Studio. How is the intellisense in Code Blocks? [/edit]

You mean probably the code-completion plug-in. Can't say much about it, since I have never used it (I prefer a simple working environment without all the bells and whistles). From what I read on the forums, it has some rough edges, but is mostly working as one would expect.

I have made a lot of changes/additions. The one bad part about the changes is that the List Widget is now broken, which means ComboBox and MenuList do not work correctly. I had already intended to change the List's design, but cleaning up the Widget class has made the List inoperable at the moment. For example, the List size is dynamically built based on number of ListItems added/removed. However in the future, the user will have to specify a set size. If more ListItems are added that cannot be viewed, a scrollbar will appear. Also, the Panel widget needs to be able to create List widgets, as the List widget is not only reserved for MenuList and ComboBox. Anyhow, thought I would update you on this, so you might not want to update to latest revision if you are using these widgets.

On the up side, GUIManager and MouseCursor are not Singleton, there are no setup/shutdown functions, and everything renders to a specific viewport, which can easily be changed if need be.

In the event that you do update to latest SVN, make sure you do the following:

mGUIManager = new GUIManager(myViewport);
mGUIManager->setSceneManager(mySceneManager);

Window::get/setBringToFrontOnFocus is also implemented.

Same problem as before, this time in QuickGUIListItem.cpp:

bool ListItem::onMouseButtonDown(const EventArgs& args)
{
return fireEvent(EVENT_MOUSE_BUTTON_DOWN,static_cast<EventArgs>(args));
}

Here you create a copy of args, which is another temporary. Looks like you are having a constness problem ...

Edit: And a slicing problem too, since EventArgs is only the base class of the actual type.

You're right.. I am still a beginner when it comes to const, and I'm finding that .NET has been letting me get away with incorrect syntax. I haven't even looked into using const functions yet, maybe when I get some time I'll see how that works.

I only saw 2 functions that had that issue, and I fixed them. They are in SVN now.


bool ListItem::onMouseButtonDown(const EventArgs& args)
{
WidgetEventArgs tempArgs(dynamic_cast<const WidgetEventArgs*>(&args)->widget);
return fireEvent(EVENT_MOUSE_BUTTON_DOWN,tempArgs);
}


I'm still considering if I should fix the List widget or not. Either way I will have to visit it after scrollBar's are implemented. I think I will use that autosize trick for Lists as well, if you specify a height and/or width of 0. That really is a great idea, we could probably use it in MultiLineTextBox as well, so the user can choose how many lines to display.

Are you sure that args is always a WidgetEventArgs here? Because if not, the method will crash (EventArgs) or probably not working as intended (any other type except .EventArgs and WidgetEventArgs).

The problem is, that you sometimes have an EventArgs argument as const and sometimes not. I would advise to sort that out as fast as possible. If you continue this way, you will only get into more trouble.

From what I see you are only using only args.handled in a non-const way inside the fireEvent method. Why are you passing the event on, when it has already been handled? Sounds a bit inconsistent to me.

Regarding the constness in general, I noticed a few oddities. You might want to look into that. Or, if the current SVN version compiles (haven't tested it yet) I could fix the other const problems and send you a patch (though I won't touch the fireEvent stuff).

fireEvent takes a non const EventArgs for a reason. In many cases, the GUIManager will create an EventArgs or derivative, and fire an event. For example, injectMouseButtonDown is used to fire events related to gaining focus, losing focus, and button down. I create a MouseEventArgs, and set properties such as mouse position, mouse button down, and there is also an offset (for inject mouse move). Same goes for KeyEventArgs, which contains the keycode and code point in the args.

For the most part, everything uses WidgetEventArgs or a derivate (thus can safely cast to WidgetEventArgs). I can't even think up a place that fires off an EventArg. Even so, I have added a "type" field, so in your own apps you can do a safety check before casting, if you desire. I do know the library enough to know when I have received a MouseEventArg or WidgetEventArg, etc.

Regarding the ListItem code, you are seeing a handler, which is called when a specific event is fired. This handler then fires an event, which is of the same event type. For example, ListItem child widgets, such as Image or NStateButton, have the ability to propogate their events to the ListItem widget. The ListItem, if set to allow events propogated up to it, will subscribe to the Image/NStateButton so that when the mouse goes down on the Image, the mouse down event is also fired (which is tied to other events, so that menu lists and combobox behaves properly, and the lists are hidden when a selection is made.)

Hope that sheds some light on why fireEvent takes a non-const EventArgs, and why event handlers take const EventArgs.

Hope that sheds some light on why fireEvent takes a non-const EventArgs, and why event handlers take const EventArgs.


Unfortunatly it doesn't. I don't understand your argument.


fireEvent takes a non const EventArgs for a reason. In many cases, the GUIManager will create an EventArgs or derivative, and fire an event. For example, injectMouseButtonDown is used to fire events related to gaining focus, losing focus, and button down. I create a MouseEventArgs, and set properties such as mouse position, mouse button down, and there is also an offset (for inject mouse move). Same goes for KeyEventArgs, which contains the keycode and code point in the args.


Sorry, don't see how that is an argument for non-constness. You are never modifying the EventArgs inside fireEvent or any event handler, right?
The only exception is args.handled and I don't really understand, why you want to tell an event handler, that the event it is receiving already has been handled. Either the handler is supposed to handle the event or it is not and if it is not, why send it to the handler in the first place?
If you really want that in, fine. Use non-const argument (and make it consistent then). But if not, I don't see what a non-const argument is good for in this place.


For the most part, everything uses WidgetEventArgs or a derivate (thus can safely cast to WidgetEventArgs). I can't even think up a place that fires off an EventArg. Even so, I have added a "type" field, so in your own apps you can do a safety check before casting, if you desire. I do know the library enough to know when I have received a MouseEventArg or WidgetEventArg, etc.


Uhm, a bit redundant since this is a buildin feature of the C++ language. You can always use dynamic_cast or typeid to check the type (dynamic_cast preferred).

Sorry, I really can't follow your arguments. Maybe I haven't looked closely enough into your event system yet. Don't want to step on your toes, but I think you are making a mistake here.

Sorry, I really can't follow your arguments. Maybe I haven't looked closely enough into your event system yet. Don't want to step on your toes, but I think you are making a mistake here.

It's fine, only good can come of these discussions, right? Aside from me looking silly, that is.

I think you're right, fireEvent should take a const arg, since the majority of changes come from right before the call to fireEvent. (aside from handled)

Regarding the handled flag, I don't know what the proper way is to handle this. Back when I was looking at CEGUI for reference, I got lost somewhere in the code bloat and billion layers of indirection, and never found anything useful.

So the ideal reason for the handled flag is to know if a particular injection (injectChar, MouseButtonDown, MouseMove, etc) was consumed and used by QuickGUI, right? So basically if a widget fires an event, and there is at least 1 event handler registered to handle the event, we know the event was consumed? (This is my current direction of thought)

Uhm, a bit redundant since this is a buildin feature of the C++ language. You can always use dynamic_cast or typeid to check the type (dynamic_cast preferred).

Not sure I understand this. I'm not familiar with what happens when you dynamic cast a Button into a Sheet, for example. All I was trying to say is that you can query the type, so you know what casting you can do, knowing that there will be no oddities.

It's fine, only good can come of these discussions, right? Aside from me looking silly, that is.


Or me looking silly, if I am wrong. Happens sometimes


So the ideal reason for the handled flag is to know if a particular injection (injectChar, MouseButtonDown, MouseMove, etc) was consumed and used by QuickGUI, right? So basically if a widget fires an event, and there is at least 1 event handler registered to handle the event, we know the event was consumed? (This is my current direction of thought)


Right, but you don't need to store this information in the EventArgs, since fireEvent and the event handler are returning it anyway.


Not sure I understand this. I'm not familiar with what happens when you dynamic cast a Button into a Sheet, for example.


Version 1:

dynamic_cast<WidgetEventArgs *> (&args);

This returns a 0-pointer, if args is of the wrong type.

Version 2:

dynamic_cast<WidgetEventArgs&> (args);

This throws an exception, if args is of the wrong type.

Awesome! Thanks for clarifying that, I don't know why I used pointers when I could have casted with a reference type.

I'll fix this up tonight.. good changes to be made!

Always glad, when I can help. Especially, if this gets me a better library.

Looks better now. Compiles without problems on gcc. And the new singleton-less design is much nicer.

New problem: When I am creating TextBoxes with QGUI_GMM_PIXELS they are not created in their window, but in the sheet (I guess it is the sheet, because they do not appear at the top of the window, but at the top of the screen).

Can you give me the code to reproduce? My setSize and setPosition functions need to be tested more. (In this case it looks like Widget::setPosition is causing the problem.)

It's a bit complex, but anyway, here it is:


void tcg::info_window::Add (const std::string& Handle, tcb::info_source *Source)
{
QuickGUI::Size Size (d_Window->getSize (QuickGUI::QGUI_GMM_PIXELS));

QuickGUI::Point Position (d_Window->getPosition (QuickGUI::QGUI_GMM_PIXELS));

const int Height = 20;

// window formatting
if (d_Sources.empty())
{
Position.y = (Position.y + Size.height) - Height;
Size.height = Height;

d_Root.addFrameListener (this);
}
else
{
Size.height += Height;
Position.y -= Height;
}

d_Window->setSize (Size.width, Size.height, QuickGUI::QGUI_GMM_PIXELS);
d_Window->setPosition (Position.x, Position.y, QuickGUI::QGUI_GMM_PIXELS);

// fix widget positions
int y = 0;
for (std::vector<std::pair<std::string, t_source> >::iterator Iter (d_Sources.begin());
Iter!=d_Sources.end(); ++Iter, y += Height)
if (Iter->second.first)
{
QuickGUI::Rect Rect (0, y, Size.width, Height);
Iter->second.first->setDimensions (Rect, QuickGUI::QGUI_GMM_PIXELS,
QuickGUI::QGUI_GMM_PIXELS);
}

// create text widget
QuickGUI::Rect Rect (0, Size.height - Height, Size.width, Height);

t_source Line (d_Window->createTextBox (Rect, QuickGUI::QGUI_GMM_PIXELS,
QuickGUI::QGUI_GMM_PIXELS), Source);

d_Sources.push_back (std::make_pair (Handle, Line));

d_Window->show();
}


I am able to reproduce this on my end. I have to run, I'll try and have it fixed by tomorrow. If you can see where my logic is faulty, I'm 100% sure the problem lies in Widget::setPosition. I fixed it in my local copy for TextBox, but then the TrackBar buttons are not positioned correctly. There must be some things I'm not taking into consideration (I convert to relative coordinates, then convert to abs and pix, and use pix for rendering)

I really need to add 2 more options for GuiMetricsMode:

GMM_PIXEL_RELATIVE
GMM_ABSOLUTE_RELATIVE

Might seem a little confusing, but I'll make sure comments are put in. Basically, when you are setting the position of a Widget, you have to decide if you want to position it relative to its parent, or relative to the viewport dimensions. After that, you decide the type of values to give (pixel or 0.0-1.0 values).

Here would be the list:
RELATIVE
ABSOLUTE (uses viewport top/left as origin for setPosition)
ABSOLUTE_RELATIVE (only useful for setting positions)
PIXEL (uses viewport top/left as origin for setPosition)
PIXEL_RELATIVE (only useful for setting positions)

The _RELATIVE enums are useless for setting the Size, and will just be used as the enums without _RELATIVE.

My code was bad because I was trying to satisfy both having viewport and parent as origin. At least now my head is clear.

Ok. I haven't finished implementing this yet, but I hope this is the last major design change we come across.. (I figured I might as well do this since I've already done a lot of other important changes, and its for the better anyway)

This is the new scheme I'm working to implement:


/**
* Allows setting size using 3 different types of values.
* Absolute: [0.0,1.0], where 1.0 is the full width or height of viewport.
* Relative: (-inf,+inf), where 1.0 is the full width or height of parent widget.
* Pixels: [0,x], where x is the width or height of the screen in viewport.
*/
typedef enum SizeMode
{
SIZE_ABSOLUTE = 0,
SIZE_RELATIVE ,
SIZE_PIXELS
};

/**
* Allows setting size using 3 different types of values, according to 2 origins.
* Absolute: [0.0,1.0], where 1.0 is the full width or height of viewport.
* Relative: (-inf,+inf), where 1.0 is the full width or height of parent widget.
* Pixels: [0,x], where x is the width or height of the screen in viewport.
* RELATIVE implies positioning is relative to Parent.
*/
typedef enum PositionMode
{
POSITION_ABSOLUTE = 0,
POSITION_PIXELS ,
POSITION_RELATIVE ,
POSITION_RELATIVE_ABSOLUTE ,
POSITION_RELATIVE_PIXELS
};


These will be within the Widget namespace, as opposed to the previous QuickGUI::Gui_Metrics_Mode enumerated type.

I think I can follow your logic (mostly).

btw. the typedef keyword isn't needed here.

Oh man!! That was way too confusing. I decided to scrap my attempts at implementing that, and will go with a simple creation of the function Widget::setScreenPosition. For all intents and purposes, setPosition will be relative to it's Parent's origin.

I reverted my code and then Tortoise did some weird crap. I thought it erased some revision in my repository! I was about to throw my laptop out the window (not really) when I wiped my code and did a re-checkout, and everything was fine again. I hate when tortoise tells me my files are revision 140, when they are obviously not revision 140. I'm scared of Tortoise now.. it told me I removed revision 124 (who knows why) and that my code was up to date. Long day..

setPosition works, and setScreenPosition is implemented and works also. The comments of setPosition state that the positioning is relative to the parent widget.

My problem was that the TrackBar button dragging was using setPosition improperly. Fixed also.