2007-10-23

Window Closing

Woopsi now supports window closing. Each window can now have a close gadget at the top-left (switched on or off by a bool in the window’s constructor) that will close the window when clicked.

Sounds easy to implement. In practice, not so much. First problem I encountered was a null pointer issue - if a window closes, and that window is the active window, everything falls apart. Woopsi will try to do more things with the active window once it has closed and been deleted.

Fixing that led me to think some more about the way events are routed throughout the gadget tree, which inevitably led me to refactor it somewhat. Each gadget can now contain an active gadget pointer. If that pointer is null, and the gadget’s parent has the current gadget as its active gadget, we know that the current gadget is active (and none of its children are). Events can now be routed through the tree simply by sending them to each active gadget pointer in turn, stopping when we hit a NULL and processing them on that gadget. Any events that should be processed along the way can be passed to every gadget we traverse through. Key press events are now sent to every active gadget along the trail, for example.

There are other improvements brought about by the changes - events are now fired more logically. Previously, clicking a gadget in an active window fired the click event for that window and the focus event. This doesn’t make any sense - the window itself hasn’t been clicked (the gadget has), and the window already has focus. We shouldn’t fire either of those events.

Conversely, events are bubbled back up the hierarchy automatically as required - if we click a button in an inactive window, the button tells all of the gadgets above it to handle the focus event. The window receives the notification, checks its focus state, and calls its focus event handler as it knows it does not currently have focus.

This still left me with null pointer problems, though. Clicking the close gadget caused the screen to delete the window before it handled the gadget’s release event. When the release event was processed, the clicked gadget pointer pointed to trashed memory, so everything fell over. This was a simple one to fix - just point the clickedGadget event to NULL.

However, there was yet another problem. Clicking the close gadget called the window’s close() function, which in turn called the screen’s closeChild() function. This function deleted the window. The problem here is that, as we exit the closeChild() function, we return to the window’s close() function. This function no longer exists. In fact, what we’re really doing is a complex version of “delete this”, then trying to carry on running the deleted code.

Not good.

Since gadgets need to have a “close()” method in order to simplify removing gadgets from the system, I had to come up with a method of deleting gadgets that entirely removed the gadget to be deleted from the control process. The solution was a deletion queue. Calling the close() method on a gadget now marks it as deleted and adds a pointer to that gadget to the parent’s deletion queue (actually a vector, since that required no extra coding and deletion order isn’t a concern). Each event now calls a “processDeletionQueue()” function after executing that will delete any queued gadgets.

One thing that has rapidly become apparent is that recursion is a real swine to debug if you haven’t got a debugger or a console to print to that doesn’t wrap around uselessly. PALib’s “PA_Print()” function scrolls lines off the top of the screen, then scrolls them back on the bottom again a short time later. I really need to fix the TextViewer clipping problem so that I can write a proper console gadget.

Here’s a screengrab of the latest demo:

Woopsi Close Buttons

Three of the windows have been constructed with close buttons switched on; one has been created with close buttons switched off. Woopsi automatically resizes and repositions the window title bars for the different situations.

At present, the close buttons are slightly thinner than the screen depth gadget. It looks better this way (less squat), but I’ve found that it makes the buttons rather more difficult to click. I’d prefer to go for accessibility than squarer buttons, so I might make them fatter soon.

New demo should appear on the SourceForge page soon (demo 0.23). The SVN code is already up to date.

Comments

Jeff on 2007-10-24 at 00:35 said:

While pondering the problems of close() it might be worth thinking about a hide() function. In another windowing system I’m familiar with, we had the close-button hooked up to a function that did a) hide the current window b) add it to the delete-queue. However, some classes of windows only did the first because they were relatively expensive to set up in the first place, and the next time you wanted a window of that class, you’d want all the content it already had. So, its nice to seperate out the automatic ‘he clicked this, it must mean delete’ behaviour in a redefinable method. ie, you need a clickedClose() method seperate from a close() one.

By focusing on hide(), we had to worry about focus maintenance, etc as you have done, but also about things like ‘does a hidden window get idle() events?’ (yes, it does) ‘does it get notifications about other system information changing?’ (yes it does). Etc.

When I suggested we had a delete queue, that wasn’t true. Instead, each window has a flag word with two bits, one for I’m busy, and one for I’m ready to die. Whenever the message processor was going to send an event to the window, it would copy the old flags (important), set the I’m busy bit, then call the window callback. On return, it would restore just the busy bit state to whatever it was previously, then look at the ‘I want to die’ bit. If the window wanted to die, that was the point it got deleted - unless it was busy in which case it remained alive (but presumably hidden).

oldflags = window->flags; window->flags |= BUSY; window->handle_event(…); window->flags = (window->flags &~BUSY) | (oldflag&BUSY); if (window->flags & IWANTTODIE) if ((window->flags &BUSY)==0) window->close();

In effect, it helps you avoid the problem of deleting objects where you have dangling pointers on the runtime stack. If you ever get all the way out to your main event loop and find a window still busy then you have a bug somewhere. And its simpler than maintaining a queue, though secretly of course, it is, its just that the queue is in the callstack.

Jeff on 2007-10-24 at 00:37 said:

Hmmm, “by focussing on hide(), we had to worry about focus maintenance” is pretty clumsy English, isn’t it? Obviously I meant to say

Since we had to consider hide() and invisible windows in general, we had to worry about managing the event focus…

ant on 2007-10-24 at 05:53 said:

Gadget hiding is on the list and already partially implemented. Nice to see that we’re thinking along the same lines!

Checking for a “ready for deletion” flag is a really good idea - I’ve already got the flag in thwlere, so can’t see why I missed that one. Busy status mght be good too.

If I make the close() function hide the gadget and only set the delete flag if the gadget has an “allowDeletion” property set to “true, that solves the hide issue. All of the basic events can be overridden anyway, of course.

I’m going to have to put you in the credits list for Woopsi, you know!

Jeff on 2007-10-24 at 23:38 said:

Happy to help. One other thought you might like to consider concerns the difference between ‘window->close()’ and ‘delete window;’

If you make your window->close() merely hide the gadget, what do you use to make it visible again? window->open() ? The terminology you use here can really cause problems later on unless you get your naming right up front.

We have hide() and show() to control visibility.

Our close() method calls hide() and sets the KILLME bit. That way, it goes away instantly from the users perspective, even if the data structures aren’t cleaned up immediately.

To actually kill a window, the framework C++-deletes it. The destructor is private so that no-one can call it because no-one else should call it because normal code can’t tell that there aren’t still pointers to the window in the callstack.

You can’t unclose() a window though technically you probably could, just force the KILLME bit off, but thats so pathologically bad that we don’t consider doing it. There really isn’t a good reason for allowing it that I can see.

Of course, I’m not saying this is the only way to do this - its what our design has evolved to. You could use a ref-counting mechanism instead but thats more complicated than it needs to be, in my opinion, and still falls back on needing to call a different method on the object rather than its destructor.

Another gotcha that caused us grief because we got it wrong up front was having a different data structure for window vs gadget (where gadgets only live in windows.) When we implemented tab panels, we ended up with a tab-content-gadget which has to maintain its own window object as well, so that the tab gadget could contain other gadgets. If we had designed, from the start, the notion that gadgets can contain gadgets, we’d have been better off.

(From what I can tell, Woopsi hasn’t made this mistake)

ant on 2007-10-25 at 08:55 said:

Yep, everything in Woopsi is a gadget, and all gadgets can have sub-gadgets. I was chatting with someone before I started Woopsi who mentioned that everything in Windows is a window, which made sense as it makes the whole thing a traditional tree structure. That’s the pattern I followed.

Hmm, you’re right about naming things. The close() function should delete the window, hide() should remove it from the screen, and show() should make it visible. At present, close() changes its behaviour depending on the closeType variable, but that isn’t correct - the close gadget should choose the correct method to call based on the closeType. Having functions change their function to do something very different is a bad plan. What was I thinking? (Actually, it was 1AM - I’d lost the capacity for thinking.)

I need to change that.

There is no open() function to complement the close() function, as it doesn’t make sense with the way that Woopsi works. Windows are opened by adding new window instances to screens, using a “Window* window = screen->addWindow(x, y, etc)” command. This way, Woopsi has more control over its internal processes and gadget lists. There’s no way for someone to add an unexpected gadget into the screen’s gadget list, since the gadget list is private.

You’d never call an open() function anyway, since a window is opened in its constructor - how could you call an open() function on a window that doesn’t exist yet? I suppose you could move all of the initialisation code into the open() function, and make the constructor empty. That would seem to be a bit of a waste of time, though…

One thing I haven’t done is implement “gadget closed”, “gadget hidden” and “gadget shown” events. If a window has been closed, how will the application know that it’s been closed? At the moment, it doesn’t, and if a developer has created pointers to a window that is subsequently closed by Woopsi, he won’t know that his pointer points at trashed memory.

ant on 2007-10-25 at 09:03 said:

Oh, I should also mention that the close() function doesn’t delete the window - it sets the isDeleted bit (same idea as KLLME), and adds itself into its parent’s deletion queue. The framework takes care of gadget deletion.

Setting the destructor to private is a good idea, but then how can the parent gadget delete the sub gadget? It could call a killme() method or something, which in turn would delete the gadget, but then you’d be calling “delete this” and the killme() method would need to be public.

ant on 2007-10-25 at 09:43 said:

Aha, it’s all to do with friend classes. Interesting, I need to read up on that.

ant on 2007-10-25 at 09:46 said:

…but then, classes with private destructors can’t be created on the stack. Is this necessarily a bad thing in this case? I have no idea…

ant on 2007-10-25 at 10:00 said:

Oh, I do care about that - if I can’t create objects on the stack that screws my “Woopsi woopsi” initialisation thing - I’d need to create a pointer instead (“Woopsi* woopsi = new Woopsi()“). Hmm, requires some thought. I suppose I could rename the current Woopsi into “GUISystem” or something, and create an intermediate object called “Woopsi” that acts as an interface. Jumping through hoops…

Jeff on 2007-10-26 at 00:49 said:

I’m not sure that you need to insist on the same behaviour for the top-level Woopsi instance, and the gadgets.

Unless, of course, deleting the Woopsi object would be how I invoke rebootlib

;-)

I see the Woopsi object as an Application, not a Window. You certainly never have more than one, do you? Singleton objects are definitely allowed to use a different mechanism to manage their destruction, since many people may have a pointer to the singleton, and its only when all of them are finished with it that it should really go.

(As to the ‘on Microsoft Windows, all gadgets are windows’, thats mostly true except that there are a bunch of significant places where it isn’t true. You can’t add a menubar to anything other than a top-level window. The status bar at the bottom of a window is a seperate gadget, but its not taken into account correctly when determining how much usable space is in the window itself, nor is the menubar)

ant on 2007-10-26 at 05:59 said:

The Woopsi object needs to be a gadget internally as it is the top node in the gadget tree. It uses a lot of the same functions as the other gadgets, and its children need a parent pointer to it. However, there is no reason why the user needs to treat it as a gadget, which is why I could put an intermediate interface in front of the gadget hierarchy and allow the user to interact with that. It could be a singleton, or for more flexibility it could be a normal object.