widgetShown() and widgetHidden() events aren't recursive

[GoogleCodeExporter]

Just came across this issue over the weekend and figured that it should get
reported. Currently, the widgetShown() and widgetHidden() events aren't
particularly useful if they're used by a child widget of a Window or other
container types. Especially now that the base widget class can add and
remove other widgets, this could cause a lot of hack code to traverse
through all of the widget's parents and subscribing to their visibility
events, but that can be rather messy, especially when you're receiving
visibility events that are higher up than you care about (especially when a
parent in the chain isn't visible), and can potentially produce the wrong
results if done correctly (or should I say incorrectly?). For instance,
let's say that we have an ordering like this:
visible->invisible->visible->invisible. If we have to recurse through on
the last widget in the chain for events, and we fire off a shown event for
the second widget, the last widget will catch a shown event and perform it,
when it really shouldn't.

So, to fix this problem, I think that distributeShownEvent() and
distributeHiddenEvent() should also call each of the widget's children
widgets. Originally I was thinking that a parentShown/Hidden-type event
would be needed also if the user just wanted to ignore parent events and
keep doing what they're doing, but that isn't necessary since the user can
check the event's source for that. However, there is a small pitfall with
widgetShown() events if done like this, and that is that on widgetHidden()
and widgetShown() events, it should keep distributing down the chain to
every child widget where mVisible == true, else it should skip the widget
and move on to the next one. This way, events aren't shipped out farther
than they are useful, since if a widget was hidden when its parent was
hidden, each of its children already had the chance to get its hide event,
and even if it got a show event, it wouldn't be useful if it can't be shown.

Oh, and by the way, although there is a quick patch to this, I think that
it'd be better if the calling event could be determined correctly at any
point in the chain (not only for correctness, but that this would be more
useful to the GUIChan user, in case you want to utilize the event, but only
care about whether your widget is hidden or shown, instead of what the
parent or any grandparents are doing. For instance, this could be
potentially better for a widget which opens and closes a file for example,
where it might want to keep the file in memory so long as the widget is
visible, and not its ancestors, since that might signify that it might get
used again in the near future), instead of just getting the widget who the
WidgetListener is subscribed to. It might take a bit more code, but it'd
still be nice to keep who fired it at all stages of the recursion chain.
Currently, this type of behavior is a bit odd for GUIChan, so it might pay
to discuss what the best way might be to handle recursive events without
seeming tacked on in order to do it. I think that the best way to approach
this might be to create a new _distributeHidden/Shown method which
setVisible would call that would create the event, and then pass it to and
call each of the child distributeHidden/Shown events, which would now take
an event parameter in order to retain where the original event came from.

Original issue reported on code.google.com by irar...@gmail.com on 15 Jun 2009 at 4:27

[GoogleCodeExporter]

Perhaps they should be distributed to parent widgets. It's always difficult to
consider all possible outcomes, so I think we need to think about some real 
useful
scenarios and possible problems before we make a decision.

Personally I think it's quite awkward if a visible widget receives a hidden 
event.
It's different with mouse input and keyboard input as it actually occurs on the
parent widget as well as the source widget. 

Original comment by olof.nae...@gmail.com on 16 Jun 2009 at 1:27

[GoogleCodeExporter]

The way I'm taking it though is literally, since if a widget goes in or out of 
view
because of the parent, the parent should let the children know (and I currently 
have
a bug which isn't resolvable perfectly without this change as well). And since I
didn't provide a snippet last time, this is more or less what I'm proposing, 
just so
that you can get an idea of what I'm meaning a bit better (thought I'd describe 
it so
that alternate designs could get proposed, and also since the fix isn't obvious 
in
how it fixes this issue):

void Widget::setVisible(bool visible)
{
    if (!visible && isFocused())
        mFocusHandler->focusNone();

    Event event(this);

    if (visible)
        _distributeShownEvent(event);
    else if(!visible)
        _distributeHiddenEvent(event);

    mVisible = visible;
}

void Widget::_distributeHiddenEvent(Event event)
{
    std::list<WidgetListener*>::const_iterator iter;
    for (iter = mChildren.begin(); iter != mWidgetListeners.end(); ++iter)
    {
        if ((*iter)->mVisible == true)
            (*iter)->_distributeHiddenEvent(event);
    }

    distributeHiddenEvent(event);
}

void Widget::_distributeShownEvent(Event event)
{
    std::list<WidgetListener*>::const_iterator iter;
    for (iter = mChildren.begin(); iter != mWidgetListeners.end(); ++iter)
    {
        if ((*iter)->mVisible == true)
            (*iter)->_distributeShownEvent(event);
    }

    distributeShownEvent(event);
}

void Widget::distributeHiddenEvent(Event event)
{
    std::list<WidgetListener*>::const_iterator iter;
    for (iter = mWidgetListeners.begin(); iter != mWidgetListeners.end(); ++iter)
    {
        (*iter)->widgetHidden(event);
    }
}

void Widget::distributeShownEvent(Event event)
{
    std::list<WidgetListener*>::const_iterator iter;
    for (iter = mWidgetListeners.begin(); iter != mWidgetListeners.end(); ++iter)
    {
        (*iter)->widgetShown(event);
    }
}

Hope that helps out a little. Like I said though, this should cover it the way 
that
I'd expect it to work. And as for it being awkward for a visible widget to 
receive a
hidden event, the widget isn't actually visible if the parent's hidden, which 
is the
main point I was trying to make. There's no problem with keeping track of 
mVisible or
isVisible() the way it currently is, since that does keep the code a lot 
simpler, but
the current way these events are handled, it does cause a problem if the parent 
is
gaining visibility or hiding which then chains out to its children, and then 
they
aren't notified about this change. Take for example that we have a window and it
contains a widget inside of it which launches something like a right click 
menu. If
the parent loses visibility, the child widget should get informed that the 
parent
lost visibility so that it can close that menu. And like I pointed out through 
my
example earlier, if you just subscribe to the visibility events for all of the
ancestors, it can lead to getting some false positives from time to time. If 
there's
something that I could do to help you understand why what I'm doing here 
resolves
this issue, then feel free to let me know how I can help you with that, since 
I'd
like to be able to see visibility events handled this way, or some way similar, 
so
that situations like these can get handled better.

Original comment by irar...@gmail.com on 16 Jun 2009 at 5:37

[GoogleCodeExporter]

In Qt, show/hide events are not distributed to the parents (well, there is a
HideToParent event for that...). They are sent to widgets when their visibility
changes. This is not only (and not always) when setVisible() is called on a 
widget,
since it also depends on the visible state of their parents.

Similarly, isVisible() returns whether the widget is actually visible, not the
visible state internal to the widget. There is an isHidden() function that 
returns
whether a widget is explicitly hidden, ie. it doesn't return true when the 
widget is
merely invisible due to one of its parents being invisible.

Anyway, as I've said before, due to this whole Qt business I am not able to 
form an
objective opinion about this.

Original comment by b.lindeijer on 16 Jun 2009 at 5:54

[GoogleCodeExporter]

BTW, just for clarification, the events are not getting sent to the parents, 
but to
the children, just in case that was causing some confusion here.

Original comment by irar...@gmail.com on 16 Jun 2009 at 6:00

[GoogleCodeExporter]

No, that was in response to Olof, your reply came in while I was writing mine. 
:-)

Original comment by b.lindeijer on 16 Jun 2009 at 6:03

[GoogleCodeExporter]

Ok, just as a side comment, there should be a small change to the code that I
presented, in order to ensure proper event ordering:

void Widget::_distributeHiddenEvent(Event event)
{
    distributeHiddenEvent(event);

    std::list<WidgetListener*>::const_iterator iter;
    for (iter = mChildren.begin(); iter != mWidgetListeners.end(); ++iter)
    {
        if ((*iter)->mVisible == true)
            (*iter)->_distributeHiddenEvent(event);
    }
}

void Widget::_distributeShownEvent(Event event)
{
    distributeShownEvent(event);

    std::list<WidgetListener*>::const_iterator iter;
    for (iter = mChildren.begin(); iter != mWidgetListeners.end(); ++iter)
    {
        if ((*iter)->mVisible == true)
            (*iter)->_distributeShownEvent(event);
    }
}

I considered adding in an assignment of mVisible also before getting the 
children to
fire off their events in this fix, but I decided that that shouldn't be done 
until
after all of the children receive that event, so that they can have the same 
starting
state as the parent widget had. So it isn't really too big of a deal being left 
out,
since the parent can still set it if they want to in their own visible or 
hidden handler.

Original comment by irar...@gmail.com on 17 Jun 2009 at 4:10

[GoogleCodeExporter]

Ok, quick correction, since I probably got a little copy/paste happy there for 
a 
minute (and that some of the last comment isn't as clear as I was trying to get 
across). All of that was only meant for the _distributeShownEvent() function. 
The 
_distributeHideEvent() was perfectly fine as is. Same for the comment on 
setting 
mVisible, and everything afterwards.

Now, as to why that's important, shown events should get processed in order 
starting 
from the parent, then to each of its children down that chain, while for hidden 
events, it should go in the opposite direction and then bubble up to the 
parent. If 
you need an explaination as to why, then I can provide one here, but I hope 
that the 
logic as to why that ordering should happen in that manner shouldn't be too 
hard to 
figure out.

Original comment by irar...@gmail.com on 17 Jun 2009 at 5:08