Re: accel activation on invisible pages

[Tim Janik <timj gtk org>]

On Mon, 8 Dec 2003, Owen Taylor wrote:

> On Sat, 2003-11-15 at 03:19, Tim Janik wrote:

> For reference, what Tim actually committed has something along the lines
> of:
>
>  gtk_menu_item_can_activate_accel:
>   GTK_WIDGET_IS_SENSITIVE (widget) && GTK_WIDGET_VISIBLE (widget) &&
>    gtk_widget_can_activate (widget->parent, signal_id);

that is (since you stripped the comments), menu items are required to:
a) be sensitive (like every activated widget should be)
b) be visible (note that they do not need to be realized or mapped)
c) need activation permission by their parent menu

>  gtk_menu_can_activate_accel:
>   gtk_widget_can_activate (menu->attach_widget, signal_id);

for menus, the comment is:
  /* menu items chain here to figure whether they can activate their accelerators.
   * despite ordinary widgets, menus allow accel activation even if invisible
   * since that's the usual case for submenus/popup-menus. however, the state
   * of the attach widget affects "activeness" of the menu.
   */

in contrast, ordinary widgets need:

  /* widgets must be onscreen for accels to take effect */
  return GTK_WIDGET_IS_SENSITIVE (widget) && GTK_WIDGET_DRAWABLE (widget) && gdk_window_is_viewable (widget->window);

i.e. they need not only be visible, but also be mapped (and therefore
realized).
it later occoured to me, that gdk_window_is_viewable() is redundant
here, since with the introduction of gtk_widget_set_child_visible(),
not being mapped propagates down the tree from parent to all its children
(and mapped is already checked by DRAWABLE).


on the practical side, these changes esentially mean:
- widgets are activatable if the user can *see* them
- menu items are activatable if the user *could see* them
  by popping up the menu

note that with the latter point, accelerator activation of
menus attached to invisible (unmapped) widgets (menubars)
is not possible.
for most applications, that's actually what we want (i've
seen applications crash because they didn't guard against
this situation), i'll get to a counter example later on.

> I'm not entirely happy with the details of this solution:
>
>  * The appearance of 'signal_id' strikes me as an unnecessary
>    complexity; a single object with multiple accelerators on
>    different signals is not something I've ever seen used. Having
>    those different accelerators act differently for activatation
>    should be even rarer.

it's there because the accelerator mechanism is parameterized
by signal ids. there're basically three aproaches:
- remove the signal_id from the accelerator mechanism, making
  it work only for ::activate.
  -> not feasible since it breaks source and binary compat
     (though in retrospect, this would have probably been
     the best approach)
- eliminate the signal_id from can_activate_accel, making the
  activation determination code blind against the kind of
  acceleratopr activation, and also any future user code that
  would want to change the default gtk behaviour
  -> i didn't go for this variant, because i have a hard time
     forseeing all future uses of this code, and i'd rather
     have a damn good reason before crippling *one* aspect
     of the accelerator API, leaving the rest at odds
- parameterize can_activate_accel with a signal_id just like
  the rest of the accel code is
  -> that's the variant i comitted

i'm not sure what you are suggesting. i got the impression that
part of your mail was suggesting we guard only ::activate
accelerators and leave the rest the way they are. that'd
definitely not be acceptable.

>  * We don't solve the problem of mnemonics at the same time -
>    activation of mnemonics on hidden notebook pages is a more
>    commonly reported problem then similar problems
>    with accelerators. (I don't think too many people use accel
>    groups for popup menus and then attach those accel groups
>    to the main window.)

last i tried, i couldn't reproduce this behaviour with HEAD.
that's due to:
      if (GTK_WIDGET_IS_SENSITIVE (widget) && GTK_WIDGET_MAPPED (widget))
in gtk_window_mnemonic_activate().
about attaching the menu, you're best off attaching it to the widget
that's actually going to popup the menu, so accel activation is
properly prevented if that widget disappears (e.g. because of notebook
pages). and of course all every-day menus part of a menubar hierarchy
are also properly attached (either to the menu items they are submenus
of, or to the menubar itself).

>  * At least the mnemonic issue is also related to the question
>    of focus/default on unmapped or insensitive widgets; see e.g:
>
>    69483 (gtk_widget_set_sensitive() do not remove the widget's focus)

isn't that simply being taken care of by having state propagation
remove focus from a widget if its toplevel is sensitive?

>    114635 (Don't focus unmapped widgets)

and here, by focussing unmapped widgets only if the toplevel
isn't mapped either?
(ok, there's the issue of people doing grab_focus on invisible
notebook pages before switching to them, but that's essentially
exploiting current gtk bugs).

>    Currently, nothing prevents the focus or default being put on
>    a unmapped page of a notebook; in itself that could be handled
>    by a 'can_activate()' approach, but that doesn't handle dynamic
>    changes.

i don't think there's a good reason to mix these issues up.
for one, activation is a different concept than focus cycling
(not visible menu items are a godd example to keep the
issues apart), and for another, can_activate_accels, or
can_activate() won't save the myriads of more general state
keeping issues gtk has (i.e. similar/worse problems exist
as soon as paired events come into play).

<side-rant>
the real problem here is actually that gtk never formaly specified
the different (sub-)states a widget(-tree) can have, with regards to
different kinds of paired events, realization stages, temporary
states, etc. and then enforced proper transition between them.
yes, we have a document with invariants, but those only (partly)
cover realization stages and don't lead to proper transitions.
this has caused:
- lots of bugs inside gtk
- lots of bugs (and instabilities) in gtk applications,
- many semiworking workarounds in third-party apps introducing
  their own problem fields
- a bunch of ugly hacks in the gtk code base (e.g. inter-widget
  special casing in the non-core widgets) which lead to constant
  decay of the code base and build up insurmountable barriers for
  widget implementations outside the gtk tree
and with that lots of grief amongst developers over the past years
which will continue for the foreseeable future.
a proper solution here could be the implementation of state
automatons and some major refactoring efforts, but that'd most
probably cause binary/source incompatibilities, the current
user/developer communities aren't willing to bear.
</side-rant>

sorry, something got me started here ;) the point is, the signal
i introduced was meant to accompany the accelerator system in
the same fashion the rest of it works. mnemonics are easily handled
or fixed inside the core, since their sphere of action is limited
to a single window-rooted widget tree.

>  * The use of the 'g_signal_accumulator_true_handled()' accumulator
>    doesn't seem right to me. This means that you can never reduce
>    the activatibility with a signal handler.

limitation technically is possible. you simply after-connect a signal
handler returning FALSE which stops the current emission.
the "after" is necessary due to a relict in the signal code:
  if (return_type != G_TYPE_NONE &&
      (signal_flags & (G_SIGNAL_RUN_FIRST | G_SIGNAL_RUN_LAST |
                       G_SIGNAL_RUN_CLEANUP)) == G_SIGNAL_RUN_FIRST)
    g_warning (G_STRLOC ": signal \"%s::%s\" has return type `%s' and
               is only G_SIGNAL_RUN_FIRST", class, signal, rtname...);

which is clearly outdated and inadequate with the newly uses of
accumulators.
i.e. it'd be a good idea to remove this check and turning the
signal into a RUN_FIRST signal.

> The right accumulator
>    would be some sort of 'always_stop' accumulator which always
>    returned FALSE.

i'm quite confident you're pretty unlikely to do this.
usually, if you wanted to prevent activation through accelerators,
the proper way would be to alter sensitivity of the respective
widgets, since it makes no sense to disable accelerator activation
but still allow mouse button activation. in fact, that'd be quite
irritating for the user.
a connection doing the opposite, i.e. enabling accelerator
activations where stock gtk prevents them, is likely to
some extend though:

>  * It's not completely clear to me why this is a signal; in the
>    first versions of GTK+, there was a general idea that all
>    virtual functions had an associated signal, but we've moved
>    away from that recently (especially with interfaces).

it is a signal so users can hook up their own handlers to
enable accelerator activation where stock gtk doesn't.
an example here could be galeon in full-screen mode (toggled
via F11). in full-screen mode, galeon's menu bar is not visible,
however, (sub-)menu activation still works via accelerators (with
gtk 2.2). with HEAD that is not the case anymore, due to the
accelerator activation checks propagating up the tree and
ultimately ending up at the invisible menubar.
as i said earlier, while this is desired stock behaviour (for
the sake of stability of apps not expecting activation in this
kind of situation), connecting a signal handler to the menubar
that returns just GTK_WIDGET_IS_SENSITIVE (menubar) will
reenable 2.2 behaviour for the apps that can handle it.

>    Clearly, making it a signal makes it easier on language bindings
>    to override; but doing that for a "random" subset of virtual
>    functions doesn't seem like a good policy to me.

above is the rationale for having a signal in this particular
case. beyond that, i didn't suggest a policy change at all.

> Leaving aside the question of focus and default, the minor
> modification to your scheme that would make sense to me is
> to have a 'can_activate()' vfunc (and signal?) that did:
>
>  gtk_widget_real_can_activate()
>   GTK_WIDGET_IS_SENSITIVE (widget) && GTK_WIDGET_MAPPED (widget) &&
>    (!widget->parent || gtk_widget_can_activate (widget->parent))

i don't see a reason for propagating the signal up ordinary widget
trees, unless you mean to also handle mnemonic activation that way
which is already handled fine by gtkwindow simply checking whether
an item is viewable (which it needs to do anyway to figure group
cycling).

>  gtk_menu_can_activate()
>   GTK_WIDGET_IS_SENSITIVE (widget) &&
>    (!menu->attach_widget || gtk_widget_can_activate
> (menu->attach_widget))

that's the logic CVS implements already.

> But how do we bring the focus and default into this picture?

as i said earlier, i don't think it's a good idea to mix these
up. it'd certainly complicate the accelerator checking logic
(which is meant to be user adjustable), and provides an extra
entry point for people to affect focus/default-ability besides
the existing mechanisms.
and i don't see the necessity either. unless you explain to
me where i'm wrong, my recommendations above regarding the bug
reports should handle things in a failry simple manner in the
core (gtkwidget.c and gtkwindow.c).

> (Note that we want for it to be possible to set the focus and
> default on a unmapped toplevel, so it's not exactly the same
> as the notion you've implemented for accelerators)

i already said, i don't think it's good to mix them up, not
even with mnemonic handling. ;)

>  A widget is activatible if
>   - the widget is sensitive
>   - the widget and all of its ancestors up to but not including
>     the toplevel are mapped

you may very well run into further bugs with this. so far, i've
*only* changed the way keyboard events are handled, and those
pretty naturaly require mapped and realized widgets.
what you are doing here is changing more generic mechanism that
wasn't bound to widget realization at all so far.
i wouldn't be surprised if an application did
gtk_widget_activate(foobutton) on unmapped windows and expects
it's signal handlers to be executed (depending on where exactly
you place the can-activate check, you might even end up breaking
gtk_toggle_button_set_active()). in contrast to grabbing focus
on invisible nmotebook pages, i'd not consider this exploiting
a gtk bug though.

>  An accelerator is activatible if
>   - the widget is activatible
>   - gtk_window_accelerators_active (toplevel) is TRUE, where
>     gtk_window_accelerators_active() is implemented in some way
>     that GtkMenu can chain the activatibility of its toplevel
>     to the activatibility of the attach widget.

uhm, that means passing the check up from GtkMenu to its toplevel
and then fudging around with signal handlers and stuff to get
back to the menu's attach widget? apart from that, it introduces
a whole new per-window-tree concept of activatable accelerators,
that i fail to see the necessity for (still).

> This is certainly sounding overcomplicated, but what I'm worried
> about is having different concepts introduced for:
>
>  accelerators
>  mnemonics
>  focus/default widget and gtk_widget_activate()

i think you're making a mistake here. my impression is, that you're
making everything more complicating by pretending entirely different
things to be the same thing.
1) mnemonics are dependent on the current window-rooted widget tree
   and can be checked or conflict-resolved (in case of groups) via
   viewable checks from gtkwindow.c very easily since you have a 1:1
   relationship between mnemonic candidate widgets and viewable on
   screen widgets.
2) that's not the case for accelerators, they are dispatched through
   closures hooked up to accel groups. the window can't possibly figure
   which widget or whether one should be activated, since:
   - it doesn't have access to the widget handles
   - the logic to determine which widget may be activated is different
     (invisible menu items)
   - customization needs to be possible (invisible menu bars)
3) mnemonic activation decision and deciding accelerator activation
   can not possibly be "reduced" to a single concept, since we already
   have ::mnemonic-activate which besides activation also handles the
   tree propagation
4) activation (gtk_widget_activate()) as implemented by gtk is a concept
   independant of realization/mapped state, gtk_toggle_button_set_active()
   is the prime example here.
5) focus/default widget handling is for the most part bound to "Live"
   i.e. realized and mapped widget trees, with some pre-realization
   convenience demands, i.e. queueing the when-realized-to-be default
   or focus widget.
   trying to set ::has-focus or ::has-default on two different widgets
   before adding them to the same window should make it obvious that
   pre-realization (or "pre-widget-tree-completed") setups of these
   concepts aren't anything more than convenince hints offered by gtk.

> > i'd apprechiate comments, especially if you can think of any
> > problems with this approach.
> > also, i'm pondering whether this change would be suitable for 2.2.
> > though an extra signal is an API addition, it does fix the
> > accel-activation-on-invisible-pages bug.
>
> No, I really don't think it's at all suitable for 2.2. (I'm not sure
> we are even going to make another 2.2.x release.)
>
> Have I confused the issue enough?

hm, we'll see. i think, if taken apart, each of the issues can be
solved in a much simpler manner (apart from focus/default just
being one artefact of a major state-enforcement-automaton lack
in gtk). but i can and have already been wrong, so i'm curiously
awaiting your reply to prove me (right/wrong/confused) ;)

> 							Owen
>

---
ciaoTJ