Re: Review of accelerator changes
- From: Tim Janik <timj gtk org>
- To: Owen Taylor <otaylor redhat com>
- Cc: Bill Haneman <Bill Haneman Sun COM>, Gtk+ Developers <gtk-devel-list gnome org>
- Subject: Re: Review of accelerator changes
- Date: Sat, 17 Nov 2001 20:54:10 +0100 (CET)
On 15 Nov 2001, Owen Taylor wrote:
> This is long, so let me provide a table of contents:
ok, i'll just strip the parts i don't want to answer ;)
> I. Introduction
> ===============
> But there was one aspect of the code that I had a lot of trouble
> figuring out how it works -- that was the connection between
> GtkAccelMap and GtkAccelGroup. It seemed to be scattered all
> through the code and done in ways that I just didn't expect
> and that didn't seem convenient. Part II explores how I expected
that's misinterpretation. basically accel group knows _nothing_
about accel map, and accel map knows how accels relate to groups
and how groups relate to windows. there's not much scattering going
on there though ;)
> it to work, how it actually does work, and how I think it
> could be made a lot simpler than it is currently.
you seem to be missing the case of installing an accelerator on
a menu item out of the blue though, which renders your idea pretty
much unusable (i've also been there at some early design stages though ;)
> II. GtkAccelMap and GtkAccelGroup
> =================================
>
> The path <=> closure connection is not done at all how I would expect.
> It seems to me a that it should be fairly simple problem to create
> an appropriate API.
>
> We have:
>
> GtkAccelGroup - a map of:
>
> <key/modifiers>
> <closure> <=> or
> <path>
>
> GtkAccelMap - a (global singleton) map of:
>
> <path> <=> <key/modifiers>
not quite, it's more like:
GtkAccelGroup:
GClosure <=> (key,modifiers) + [optional path_quark for negotiation]
GtkAccelMap - a global map (didn't see a point in creating a singleton object) of:
<accel-path> <=> <key/modifiers>
> To add entries to GtkAccelGroup we have:
>
> void gtk_accel_group_connect (GtkAccelGroup *accel_group,
> guint accel_key,
> GdkModifierType accel_mods,
> GtkAccelFlags accel_flags,
> GClosure *closure,
> GQuark accel_path_quark);
>
> To add entries to GtkAccelMap we have:
>
> GQuark gtk_accel_map_add_entry (const gchar *accel_path,
> guint accel_key,
> guint accel_mods);
>
> and to change them:
>
> gboolean gtk_accel_map_change_entry (const gchar *accel_path,
> guint accel_key,
> GdkModifierType accel_mods,
> gboolean replace);
>
> Now, there is complicated internal logic for how add_entry() and
> change_entry() work with keeping uniqueness of changeable accelerators
> on toplevels, but I would expect that to be handled via API's private
> to GtkAccelMap and GtkAccelGroup.
i don't quite understand... the "complicated internal logic" to detect/resolve
clashes is all internal... the toplevel involving part is also help private
through _gtk_window_query_nonaccels().
> Then for monitoring, via something like GtkAccelLabel, we have
> the signal on GtkAccelGroup:
>
> void (*accel_changed) (GtkAccelGroup *accel_group,
> guint keyval,
> GdkModifierType modifier,
> GClosure *accel_closure);
>
> Given a closure, I can find it's group with:
>
> GtkAccelGroup* gtk_accel_group_from_accel_closure (GClosure *closure);
>
> and then watch changes on that group. If I want to watch for changes
> in accelerators for a widget and the closures for that widget might
> change, then it's a little more tricky but still easy - I need
> something like:
>
> GSList* _gtk_widget_get_accel_closures (GtkWidget *widget);
> ::accel_closures_changed (GtkWidget *widget);
>
> All the above API exists, and makes a lot of sense to me, except
> for a few small quibbles. (E.g., I don't think
> _gtk_widget_get_accel_closures should be private.)
yes, this should become public as shown by the accesible group's hack ;)
> But unfortunately, things aren't this pretty. Instead of
> GtkAccelMap and GtkAccelGroup maintaining their relationship
> internally, it's spilled out into an ugly public API:
>
> void gtk_accel_map_add_notifer (const gchar *accel_path,
> gpointer notify_data,
> GtkAccelMapNotify notify_func,
> GtkAccelGroup *accel_group);
> void gtk_accel_map_remove_notifer (const gchar *accel_path,
> gpointer notify_data,
> GtkAccelMapNotify notify_func);
>
> Despite the "notifier" names, these functions aren't actually
> just about notification. The accel_group parameter here
> actually is used to figure out what AccelGroups a path is
> used in. If you don't call add_notifier(), then things won't
> work properly.
yes, we need special code to react to changes in the mapping:
<accel-path> <=> <key/modifiers>
the possible cases to handle here are:
1) need to remove a closure
2) need to install a new closure
3) need to reconnect a closure from one accelerator to another
all of these happen on the same group btw.
now creating a _new_ closure is actually the tricky part, since for
1) and 3), the accel_Group involved can be queried from the clposure.
installing a new closure can't happen out of the blue however, for
widgets they need to be specially created, and accel map has no
knowledge about doing that. and it'd be false for it to have that
knowledge, 'cause then accel-paths would only work for widgets.
the naming "notifer" which was intended to be "notifier" is somewhat
of a misnomer however, "Handler" might be more appropriate.
> Not only that, but to get propagation of changes in the AccelMap
> to work properly we have to connect a notifier that is responsible
> for handling them manually. There is about 100 lines of code
> in GtkWidget behind _gtk_widget_set_accel_path() responsible
> for this back propagation that would have to be duplicated
> by anyone else who wanted to use GtkAccelMap.
right, anyone going to support accel paths on objects other than
widgets will need to supply those ~100 lines (<cough>, <sweat>)
of code.
as for the rationale, this is not just some random inconvenient
code, but really embedds the widget specific parts of it, i.e.:
- widgets need special closures. their closures emit the "activate"
signal on widgets, and need to make sure to _not_ activate insensitive
widgets, so they need to be specially created
- widgets want to provide notification though ::accel-closures-changed
upon path changes that affect them
- widgets want to know about their accel closures, so they can hand
them out via gtk_widget_get_accel_closures()
- accel closures of widgets need to be invalidated once the widget
dies, since we don't want segfault-per-hotkey, do we? ;)
that's pretty much all the code does, it's widget specific on every
edge, and i don't really see good ways to have accel map/group do
that magically.
> Since GtkAccelMap already is quite familiar with the details
> of GtkAccelGroup, and has expectations of how things work,
> a private Map/Group API would be much simpler. All we'd
> need to add is
>
> gtk_accel_map_path_add_group (const gchar *path, /* Or a quark */
> GtkAccelGroup group);
> gtk_accel_map_path_remove_group (const gchar *path, /* Or a quark */
> GtkAccelGroup group);
>
> And GtkAccelMap would simply keep a list of groups for each path
> and handle everything from there.
it unfortunately can't, as much as i'd like to make this simplification.
the reason being the widget specific parts that need to be implemented as
outlined above.
> III. A ramble about gtk_widget_set_accel_path
> =============================================
>
> [ Don't expect much of a point to this section ]
erm, now, should i respond to this or not? ;)
> With the first change above, adding and watching changeable
> accelerators for closures would be quite easy. But we still
> have the question of how we set up the traditional
> widget::activate signals and associate them with paths.
>
> I might expect the API for this to be something like:
>
> void gtk_widget_add_changeable_accelerator (GtkWidget *widget,
> const gchar *accel_path,
> const gchar *accel_signal,
> GtkAccelGroup *accel_group);
>
> Or variants, say perhaps always using the activate_id signal, and
> setting a path and group for the widget.
ok, lemme extend a bit on this. this suggestions involves several changes
over the new code:
a) all widgets can have runtime changable accelerators
b) the signal upon accelerator activation:
- can be freely choosen
- must be specified
c) the accel group of a widget needs to be fixed (or should there
be a remove_changabel_accelerator variant)?
on (a), we did allow this in 1.2 in prior gtk versions, yes. i do not
believe this is of practical use however, since to benefit from runtime
changable accelerators, you need:
- something that displays the accelerator
- something that changes the accelerator
for menu items, that's GtkAccelLabel and GtkMenu, for all other widgets,
that's.... nothing. so people never actually used such accelerators for
anything but menu items, and we never got a feature request to enable
this for anything besides menu items. so i don't see a HUGE need to support
this.
on (b), yes, since the very beginning, accelerator signals could be
freely choosen. they still can. they also could be freely choosen for
runtime changebale accekerators. they can't anymore. why?
well, for reasons outlined under (a) already, there's never any
widget type besides a menu item, where runtime changable accels make
sense. now, how many VOID:VOID RUN_ACTION signals do menu items have
that people tend to connect to? right, one, namely ::activate.
on (c), the accel groups of menu items is the one set on their parent
menu via gtk_menu_set_accel_group(). that means that accel groups
of a menu item change upon parentation and when someone invokes
gtk_menu_set_accel_group() on their parent. thus they need to be
changable.
so in the end, i decided that we'd be better of by supporting
- runtime changable acclerators only for menu items, since they
can actually display and change them
- runtime changable accelerators that only activate
GtkWidgetClass.activate_signal (for the ones not so intimite
with gtk internals, GtkWidgetClass.activate_signal is simply
the signal_id of ::activate)
which still covers all cases of runtime changable accels in
current use and simplifies tihngs a LOT.
> The actual API is different - there is no public API on GtkWidget,
> instead it is set up as:
>
> void gtk_menu_item_set_accel_path (GtkMenuItem *menu_item,
> const gchar *accel_path);
> void gtk_menu_set_accel_path (GtkMenu *menu,
> const gchar *accel_path);
>
> Where gtk_menu_set_accel_path() allows GtkMenuItem to magically make
> up a path for all the menu items in the menu based on their
> labels. Now, let me say plainly, I think gtk_menu_set_accel_path() is
> a useless feature:
>
> - Menu item labels are translated, menu paths cannot be translated
we discussed that on irc and figured, the only possible ill-effect
this could have is that your previously saved accel bindings are
gone, once you change your locale. now people who change their
locale daily may bug us about this, but for the broad masses, this
shouldn't result in any actual harm.
> - Menu item labels contain mnemonics and markup, menu paths
> should not contain such.
yes, there are two functions on labels:
gtk_label_get_text() and gtk_label_get_label().
the later returns the label text with markup and mnemonics, while
the former just returns the plain text displayed.
gtk_menu_set_accel_path(), when creating the menu item path
(actually, this code is in _gtk_menu_item_refresh_accel_path()),
invokes the former function, so that mnemonics and markup are
not part of the accel path. (note that, even if it did call
gtk_label_get_label() and thus contained additoinal characters,
that wouldn't do much harm as the main purpose of an accel path
is just to give an action<->accel mapping a unique name)
> - If people have paths for their menus, they will have paths
> for the menu items.
i think this is too naive for a valid assumption.
people will create menus from hand and figure that they can't change
accelerators during runtime. "change" here, will often mean "install
initially" since users may want accelerators on menus/menu items
that the programmer didn't setup default accelerators for.
so we force the programmer to call gtk_menu_item_set_accel_path()
with a lengthy unique path for each menu item, just in case his users
will want to install accelerators.
i can imagine many programmers not wanting to do this.
as a consequence, i'm offering gtk_menu_set_accel_path(), so that's
only one more funciton call for the programmer, and it enables all
menu items for the user.
> But that being said, wanting to have gtk_menu_set_accel_path()
> does explain why gtk_widget_set_accel_path() API wouldn't work
> very well ... GtkMenuItem and GtkMenu can cooperate so that
> the magic path only takes effect if gtk_menu_item_set_accel_path()
> hasn't been called explicitely; this is harder for GtkWidget.
>
> And, gtk_menu_item_set_accel_path() does have other advantages:
>
> - It can get the accel group automatically from the GtkMenuItem
> - It is obvious what the correct signal to use is.
> IV. Small things, more or less important
> ========================================
>
> * With my suggestion above in mind (AccelGroup/AccelMap handle
> relationship automatically) it would be clearer if
> gtk_accel_group_connect() were split into:
>
> gtk_accel_group_connect (GtkAccelGroup *group,
> guint accel_key,
> GdkModifierType accel_mods,
> GtkAccelFlags accel_flags,
> GClosure *closure);
> gtk_accel_group_connect_path (GtkAccelGroup *group,
> GQuark accel_path_quark,
> GtkAccelFlags accel_flags,
> GClosure *closure);
>
> Since it _is_ conceptually only one or the other.
as stated above, i don't quite agree here.
i think it's conceptually unclean to have the accel gorup upon
gtk_accel_group_connect_path() automatically figure the accel key
and mods through the accel-path, but be completely dumb and unaware
of accel paths (their creation, changes) otherwise.
> * I consider using a GQuark for the path argument to gtk_accel_group_connect
> quite dubious. This is an implementation detail, and even if the
> caller actually has a quark, a one extra quark => string => quark
> conversion per accelerator will not hurt us.
yes, the reason i pass a quark here is burried deep in the negotiation code.
i don't actually want to make a huge amount of string comparisons or
hashtable lookups there with quarks i get a decent integer comparison, and
the accel-path changed notifier already has the quark available.
the problem might very well lie in the type and name i choosed, things
will still work quite well, if we have:
/* instead of GtkAccelMapNotify: */
typedef void (*GtkAccelMapHandler) (gpointer data,
/* this is the map_hint => */ GQuark accel_path_quark,
guint accel_key,
guint accel_mods,
GtkAccelGroup *accel_group,
guint old_accel_key,
guint old_accel_mods);
/* renaming gtk_accel_map_add_notifer and gtk_accel_map_remove_notifer:*/
void gtk_accel_map_add_handler (const gchar *accel_path,
gpointer notify_data,
GtkAccelMapNotify notify_func,
GtkAccelGroup *accel_group);
void gtk_accel_map_remove_handler (const gchar *accel_path,
gpointer notify_data,
GtkAccelMapNotify notify_func);
/* removing the quark from connect: */
void gtk_accel_group_connect (GtkAccelGroup *accel_group,
guint accel_key,
GdkModifierType accel_mods,
GtkAccelFlags accel_flags,
GClosure *closure);
/* new function: */
void gtk_accel_group_connect_with_hint (GtkAccelGroup *accel_group,
guint accel_key,
GdkModifierType accel_mods,
GtkAccelFlags accel_flags,
GClosure *closure
guint map_hint);
so only accel map handlers (GtkAccelMapHandler) are supposed to call
gtk_accel_group_connect_with_hint() and need to specify the quark as
hint. also, it should be clearer now, that GtkAccelGroup has absolutely
*0* knowledge of accel map. you might want to argue (or did to some
extend already) that it's better to have accel group know about accel
map, but i couldn't really figure a good API for that (though i started
out in that direction initially) because any way i choose, the accel
group will always have too little knowledge to handle things really on
its own (resulting in really bad API kludges on widgets for instance,
the gtk+-1.2 API is the prime example for this).
> * _gtk_widget_get_accel_closures() should be exported, see mail
> exchanged with Padraig earlier.
yes, definitely. i didn't respond in that thread because you answered
him correctly already ;)
> * Mispellings:
>
> refresh_accel_paths_froeach()
> gtk_accel_map_foreach_unfilterd()
> gtk_accel_map_add_notifer()
> gtk_accel_map_remove_notifer()
ok, after thinking about this for hours, and not being able to come
up with a good line of reasoning on why these functions are named the
way they are and should stay so, i guess i need to admit that these are
either typos on my part (can't imagine) or transfer errors when committing
into CVS (probably).
> * Instead of gtk_accel_group_disconnect, gtk_accel_group_disconnect_closure(),
accel_groupS_disconnect_closure, i.e.:
gboolean gtk_accel_groups_disconnect_closure (GClosure *closure);
not to be anal about a spelling error you made in response, but to
point out that this function automatically fetches the accel group
from the closure and disconnects it from there, so basically this
is a convenience function.
> I'd expect something like:
>
> gtk_accel_group_disconnect (GtkAccelGroup *group,
> GClosure *closure);
> gtk_accel_group_disconnect_key (GtkAccelGroup *group,
> guint accel_key,
> GdkModifierType accel_mods);
>
> Since AccelGroup, as the caller sets it up is a map from closure to
> _either_ key/mods or to path, so the closure is more primary.
yeah, i was actually pondering something like this. then decided that
a) the current way is good enough for the moment, b) i'd rather get
the widget/menu API and accel map negotiation right, c) most importantly,
owen is goint to catch this anyway and make a good suggestion.
so, i'm fine with that.
> * I find "acceleratable" to be a word that:
>
> a) Doesn't mean anything to me
yeah sure, but that's just because you have speed limits everywhere in
the states. this API is coming from germany however.
> b) Keeps on looking like a misspelling of GtkAcceleratorTable
yep, i figured that shortly after commited when concentrating on somehing
else. i was amused by the coincidence though ;)
> Can't we rename change gtk_accel_groups_from_acceleratable() back
> to the 1.2 gtk_accel_groups_from_object(), or perhaps
> gtk_accel_groups_from_activate_object(). Similarly, I'd like
> to rename the acceleratable paramete to gtk_accel_groups_activate().
sure, go ahead.
> * What is the use case for exporting gtk_accel_group_find()?
> It seems to me that you either want to go key => closure or
> closure => key and that a general iteration interface is
> a little clumsy.
at this point, GtkAccelLabel remains the only valid external user,
if you think the accel label should rather walk the list itself
(accessing GtkAccelGroup.priv_accels, which IMHO is not such a
good idea), we can make this private.
keep in mind that accel label shouldn't just use a private function
_gtk_accel_group_find(), since third party code will want to do
the same thing, e.g. the accessibility code.
> * If gtk_accel_group_query is internal, it should be called _gtk.
> If it is not internal, than it shouldn't have a comment above
> it that says so. In order to have reliable source/binary compatiblity
> we need to know exactly what is public and what isn't. Comments
> in header files are _not_ sufficient means of marking this.
>
> We need to either not export, or use #defines to hide the
> prototypes, as in Pango or GtkTextLayout.
it needs to be public for things like the widget part of the accel
implementation, which might very well be a menu action object in the
future. however, this function is not of much use besides accel map
handler implementation, which is why it's in the category i personally
like to call semi-public eventhough you keep jumping on that ;)
> * The arrangement of return values from gtk_accel_group_query()
> is wrong. (Yes, I know you don't think so, but I need to keep
> repeating this in case my point will get through some day ;-)
i can't quite understand this still. if you want to arrange the return values
differently, that leaves you with:
/* current */
GtkAccelGroupEntry* gtk_accel_group_query (GtkAccelGroup *accel_group,
guint accel_key,
GdkModifierType accel_mods,
guint *n_entries);
/* rearrange1 */
void gtk_accel_group_query (GtkAccelGroup *accel_group,
guint accel_key,
GdkModifierType accel_mods,
guint *n_entries,
GtkAccelGroupEntry *entries);
/* rearrange2 */
guint /*n_entries*/ gtk_accel_group_query (GtkAccelGroup *accel_group,
guint accel_key,
GdkModifierType accel_mods,
GtkAccelGroupEntry *entries);
for 1, that's just one more variable you have to take the adress of, and
remember whether &n_entris or &entries comes first.
for 2, that's of lesser use than my variant. i can do
GtkAccelGroupEntry* entries;
entries = gtk_accel_group_query (group, key, mods, NULL);
if (entries)
g_print ("first entry: %p\n", entry);
while you can do:
guint n_entries;
n_entries = gtk_accel_group_query (group, key, mods, NULL);
g_print ("number of entries: %u, but i can't access them", n_entries);
usually needs from personal experience are:
a) i want all elements: entries + n_entries
b) i need any element: entries
i usually do not just need to know how many there are but do nothing
with them.
> * The GQuark return from gtk_accel_map_add_entry() looks a bit pointless,
> since it is done as:
>
> return g_quark_try_string (entry->accel_path);
>
> The caller could do the g_quark_try_string() if they needed it,
> and most of the callers don't actually need the quark...
yep, it's more or less an artifact of code changes. it's probably best
to get rid of it and have gtkwidget.c call g_quark_try_string() on the
path it passed in, after calling this function.
> * I don't understand why gtk_accel_map_lookup_entry returns a
> GQuark rather than a boolean. The quark could easily be
> computed by the caller if they needed it; the one caller
> currently there doesn't use it at all.
same thing.
> * I never got an answer about my comment about the filters in
> GtkAccelMap
>
> > The 'ignore_filters' argument here seems a convuluted way of doing
> > things to me and I also question whether it's a good idea to force
> > parties installing filters to use gtk_accelerator_map_foreach().
> >
> > Perhaps we should add a 'const gchar *root' argument to
> > gtk_accelerator_map_save(), where NULL, means 'everything
> > except what has been disabled by gtk_accelerator_map_install_filter()',
> > but GLE could do "gtk_accelerator_map_save (filename, "<GLE>");
> > And maybe rename gtk_accelerator_map_install_filter()
> > gtk_accelerator_map_make_root_private().
i don't really see a use for this besides GLE, so i don't expect
many people to use this or wanting to bother at all.
as a result, i don't like the idea of introducing a "root" concept
for the common case where you want to save accels.
also the idea of a "root" is somewhat flawed, GLE has multiple windows
that come with menus, and the window type part of their paths should be
something like: "<GLE-Shell>", "<GLE-Editor>", etc...
which makes saving a whole lot more complex.
see gtk_accel_map_add_entry() on the path naming, btw, can we please
get http://developer.gnome.org/doc/API/2.0/gtk/index.html fixed?
> * We need doc comments for:
>
> gtk_accel_groups_from_acceleratable()
do we need this function to be public? anyone?
> gtk_accel_group_find()
> gtk_accel_groups_disconnect_closure()
or better yet, your variants (which is why this has no docs
yet ;)
> gtk_accel_group_query()
grumble, but this is a semi-public guru-entry point ;))
> * We need a Changes-2.0.txt entry, for things like g
> gtk_accel_group_get_default()
>
> * Didn't we agree to call ::accels_changed on GtkWindow, ::keys_changed?
yes, we did. i noticed after i commited.
> * In order to handle key propagation for GtkPlug, I'm going
> to have to add a private API like _gtk_window_list_keys(),
> which will have to look into GtkAccelGroup->priv_accels.
yeah, that's fine. plug/socket is code from hell that uses interfaces from
hell ;)
/me runs
> (Just warning you, so you don't accuse me of doing evil
> things later ;-)
i will indeed, probably not of this particular thing though ;)
> * API Bug #61285 - No way to have change-on-the-fly-menus with
> unmodified accels is still not resolved.
>
> You proposed simply always allowing unmodified accelerators when
> changeable accelerators are allowed. This significantly increases
> the chance of accidentally changing accelerators, and the harm that
> an accidentally installed accelerator does, but if change on the
> fly accelerators are off for novice users, this probably doesn't
> matter.
damn, i mean to finish this up with you on irc. ;(
change on the fly should definitely be on by default. yes, i know, that'S
not what windows does, but then i don't play a jingle everytime i open
a window or flush my toilet either. this is still gtk land and i'm not working
for MS, we've been having runtime chnagable accels since pre-1.0 and should
deinfitely preserve it.
i do buy your argument of accidental chnages however, but i'd like to
treat it with different measures:
- one 1.0/1.2 problem is that accidental accel installation can delete
this accel if it was setup for other menu items. this can be circumvented
with an rc-file property:
can-replace-accelerators = yes/no
i don't particularly care the default here
- accidental changes are probably way more likely without modifiers, so lets
just fix this issue with:
accelerators-need-modifiers = yes/no
it'll just become a gimp FAQ entry to put
accelerators-need-modifiers = no
into ~/.gtkrc ;)
> V. Some trivial comments:
> =========================
>
> * _gtk_menu_refresh_accel_paths should be static to gtkmenu.c
oh, ok, i used to call this from menu item, but that was unclean.
> * The replace_accels gboolean in gtk_menu_key_press() looks
> like it could be removed to advantage.
nope, it's intentionally there because it should become above mentioned
rc-file property.
> * gtk_accel_map_load/save_fd() have portability problems.
well, you didn't adress that in my initial proposal, how would
you go about them?
> * There is a // FIXME in gtK_accel_map_lookup_entry(), need
> to fix before 1.3.11
that can just be removed entirely.
> * GQuark quark_path - gtk_accel_map_add_entry (accel_path, 0, 0);
> if (!quark_path)
> return; /* pathalogic anyway */
>
> Are we going to start writing
>
> label = gtk_label_new ("Hello!");
> if (!label)
> return; /* pathalogic anyway */
>
> If a function cannot return a value, we should _not_ check for
> it. It just bloats code and makes the reader of the code
> work harder.
npoe, in this particular case, the real problem is actually that
gtk_accel_path_is_valid() isn't public and so gtkwidget.c can't do
a premature g_return_if_fail(gtk_accel_path_is_valid(path));
though, it doesn't really need to, because gtk_accel_map_add_entry()
will catch this anyway.
but if it failed, the rest of the code really shouldn't be executed.
yep, once a warning occoured on stderr you shouldn't rely on your
program beaving stable anymore anyways, so this extra return path
isn't strictly necessary, but it might occour deferred, and heck,
we don't need to crash if it's easily circumvented.
i'm not introducing a new convention of checking return values here,
but used my jugdement on whether it'd be harder
- for me to add the extra return; statement
- for hundreds of users filing a bug report after they lost their
documents.
> * Step 6) of internal_change_entry could be skipped
> if !can_change. This function would be easier to read
> with a few gotos. Things like:
>
> for (slist = removable ? replace_list : NULL; slist; slist = slist->next)
>
> To skip blocks of code don't make for pleasant reading.
there's really no point in using a goto then, or is goto preferrable
over:
if (removable)
for (slist = replace_list; slist; slist = slist->next)
?
as for skipping (6) on !can_change, well, make the above if (!removable)
into if (removable && can_change) then. it's quite unlikely to be triggered
though, as runtime changeable accels are not installed with the locked
flag.
> * In gtk_accel_path_is_valid(),
>
> "<<a>" is not a valid accel path, but "<a<>" is?
nope, i didn't want to add the full parsing code from item factory
there though. proper accel paths are documented, and this is only
for a g_return_if_fail() check which don't need to check every
insane case. if you want to take a look at code that tries
to catch any possible misuse, take gsignal.c, it goes to quite
some lengths in that regard, so much that it hurts readability
and gets ugly.
i'll not complain if you change it into checking real validity.
then, and after GQuark from add_entry() has been removed, it should
probably be exported so gtkwidget.c can return_if_fail() on it.
> * Extra ) in arguments descriptions for gtk_accel_map_add_notifier()
hm? i don't see an extra parenthesis there...
one thing i'd like to add is accel change handling in gtkmenu.c.
if an accelerator can't be changed (say its locked) gtkmenu.c
currently issues gdk_beep(), which provides feedback similar
to e.g. pressing backspace at the beginning of a shell prompt.
if this is something that the accessibility guys need to hook
into though, it should probably be a gtkmenu.c signal.
---
ciaoTJ
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
