Created attachment 73645 [details] [review]
Patch fixing the bug

Now this patch really really has to go through tim or owen,
I have not much clue what's going on in that file, but the
patch seems to work :-)

Mitch, can you please explain in detail what your patch achieves and how it does it?

i might miss something, but it looks to me like you generally turned clearing/removal of binding entries into unbound entries which breaks bindings at lower priorities, if bindings at higher priorities got added and removed.
this could be fixed by adding a marks_unbound:1 flag to GtkBindingEntry, but i'd really like to hear the exact intentions from your first.

also, is there really a new keyword 'unbind' required for gtkrc?
currently binding and unbinding already works within the same style/binding_set just be specifying an empty binding body: {}
'unbind' might be ok to distinguish override-bindings-at-lower-prios-and-unbind from simply deleting a binding from the current set via {}. that's not what you implemented though.

I don't know if we absolutely need the new keyword, but clearing
a binding on the same level, so that the binding from the level below
becomes active again is different from having a special "unbind" binding
that lets the key event appear unhandled, no matter what binding it might
have triggered in the levels below.

I pondered adding "guint unbind : 1" to the struct, but thought
"signals == NULL" would have the same meaning :) Please correct
me if I'm wrong Does signals == NULL already mean something else?

(In reply to comment #3)
> I pondered adding "guint unbind : 1" to the struct, but thought
> "signals == NULL" would have the same meaning :) Please correct
> me if I'm wrong Does signals == NULL already mean something else?

that is just an implementation detail and can be freely redefined.
the way i read you patch, gtk_binding_entry_clear() will now not "delete" an existing binding, but install an effective 'consider-unbound' binding.
that would be an incompatible semantic change of the API.

is that accidental or really your intention (i.e. i'm asking again for a more detailed description of how your patch approaches the issue)?

Without looking at the patch, it seems to me there are two fairly
basic problems with the concept of unbinding:

1) Conceptually it just doesn't work to have a delete operation
in a gtkrc ... the whole gtkrc system is based upon priorized 
merging, so you can't delete, you can just override.

2) You really want to unbind a binding, not unbind a key.

Unfortunately, of course, bindings are semi-procedural and freeform,
so it's pretty hard to figure out how unbinding a binding would work.
I suppose you could just literally match on exact equality of the
sequence of binding, so, you'd have:

  unbind { "move-cursor" (logical-positions, -1, 0) }

Actually, the more useful thing would usually be to combine that
with a new binding, so:

  rebind "<ctrl>b" { "move-cursor" (logical-positions, -1, 0) }

And maybe a magic 'rebind "<none>" ...' or something like that.

If you take issue 1) into account, then the above doesn't
really mean "rebind", what it means is "bind, and if, when
scanning for matching bindings you encouter this binding, and
it doesn't match, ignore any future binding signals for the
same combination of action signals.

Doing that sounds pretty darn complex, so maybe a simpler hack
is to have a special binding that means:

 "If this matches, skip all further binding processing for this
  widget and pretend there were no matching bindings".

Which may effectively be what your patch does, but it's important
to keep in mind that it isn't really unbinding, it's just skipping
ahead in processing.

[ 
  The issues with unbinding/rebinding have there since day 1 of the
  keybinding system and are one of several reasons that nobody has
  ever done much beyond the simple Emacs key theme. The other reason
  is the priority of menus over keybindings... it's hard to swap
  out keybindings without changing menu shortcuts in a coordinated 
  fashion.
]

>  "If this matches, skip all further binding processing for this
>   widget and pretend there were no matching bindings".
>
> Which may effectively be what your patch does, but it's important
> to keep in mind that it isn't really unbinding, it's just skipping
> ahead in processing.

Yes, that's exactly what I intended to do with the patch. Introduce
a special "unbind binding" that simply stops processing, effectively
letting the key appear unbound.

And yes, abusing signals == NULL for this is really a hack, it probably
changes other semanticas too.

So, if that approach sounds OK to you guys, I'll modify the patch to
use an explicit "guint unbind : 1" and resumbit, with some comments
added :)

(In reply to comment #6)
> Yes, that's exactly what I intended to do with the patch. Introduce
> a special "unbind binding" that simply stops processing, effectively
> letting the key appear unbound.
> 
> And yes, abusing signals == NULL for this is really a hack, it probably
> changes other semanticas too.

yes, namely _clear, as i already outlined.

> So, if that approach sounds OK to you guys, I'll modify the patch to
> use an explicit "guint unbind : 1" and resumbit, with some comments
> added :)

jup. allthough you should use marks_unbound as a flag name, which is cleared than unbind i think (or skip_ahead:1 to go with owen's terminology). and you need to introduce a new _clear version, i.e. gtk_binding_entry_unbind(), which does exactly what _clear does, but also sets the marks_unbound flag. (that way the new rcfile semantics are also available programatically)

Created attachment 73833 [details] [review]
There we go

so...
i've revisited the binding entry API, and it doesn't make too much sense to me the way it currently is (anymore at least, might have had different ideas in 1998 ;) i think we need some deprecation/renaming action here:

+#ifndef DEPRECATED
 #define  gtk_binding_entry_add          gtk_binding_entry_clear
 void     gtk_binding_entry_clear        (GtkBindingSet  *binding_set,
                                          guint           keyval,
                                          GdkModifierType modifiers);
+void     gtk_binding_entry_add_signall  (GtkBindingSet  *binding_set,
+                                         guint           keyval,
+                                         GdkModifierType modifiers,
+                                         const gchar    *signal_name,
+                                         GSList         *binding_args);
+#endif

and have a public API of:

+void    gtk_binding_entry_skip         (GtkBindingSet  *binding_set,
+                                        guint           keyval,
+                                        GdkModifierType modifiers);
 void    gtk_binding_entry_add_signal   (GtkBindingSet  *binding_set,
                                         guint           keyval,
                                         GdkModifierType modifiers,
                                         const gchar    *signal_name,
                                         guint           n_args,
                                         ...);
-/* Non-public methods */
 void    gtk_binding_entry_remove       (GtkBindingSet  *binding_set,
                                         guint           keyval,
                                         GdkModifierType modifiers);
-void    gtk_binding_entry_add_signall  (GtkBindingSet  *binding_set,
+void   _gtk_binding_entry_add_signall  (GtkBindingSet  *binding_set,
                                         guint           keyval,
                                         GdkModifierType modifiers,
                                         const gchar    *signal_name,
                                         GSList         *binding_args);

so we essentially provide add_signal(), remove() and skip(). where skip() is what does the actual "unbinding".

also, we need some real docs on these functions, along with rcfile examples. i'll look into tackling this after we got your patch applied.
can you integrate those API changes into your patch please?

Created attachment 73844 [details] [review]
New patch including the deprecations

Created attachment 74035 [details] [review]
Includes changes to gtk.symbols

the patch looks very good now. in the presence of _skip() "marks_unbound:1" could also be called "skip:1" or so, but it's also ok to leave it as is i guess.
only two comments left this time:

> +  unbind = (scanner->token == GTK_RC_TOKEN_UNBIND);

this doesn't need extra parenthesis and can just read:

+  unbind = scanner->token == GTK_RC_TOKEN_UNBIND;

and i think it makes sense to also deprecate 
gtk_binding_parse_binding() and call it as _gtk_binding_parse_binding().

that's because using this function directly in applications can't fully be supported by gtk together with rcfile reparsing. applications supplying their own rcfiles can be supported however, so they should rather do that than calling gtk_binding_parse_binding() directly.

can you please commit with those changes applied Mitch?

Fixed in CVS:

2006-10-05  Michael Natterer  <mitch@imendio.com>

	* gtk/gtkrc.[ch]: added new scanner token "unbind" which gets
	rid of a key binding (in fact, it only lets it appear unbound).

	* gtk/gtkbindings.[ch] (struct GtkBindingEntry): added
	"guint marks_unbound : 1"

	(gtk_binding_entry_skip): new API which marks the entry as unbound.

	Changed code so it returns FALSE when "marks_unbound == TRUE" is
	encountered while activating bindings, effectively letting the
	binding appear unbound (regardless of still existing bindings in
	lower binding priority levels). Fixes bug #358329.

	(gtk_binding_entry_add)
	(gtk_binding_entry_clear)
	(gtk_binding_entry_add_signall)
	(gtk_binding_parse_binding): deprected these functions.

	(_gtk_binding_parse_binding)
	(_gtk_binding_entry_add_signall): new internal API.

	* gtk/gtk.symbols: changed accordingly.

added missing docs mentioned earlier. thusly closing bug.

Tue Oct 10 15:29:15 2006  Tim Janik  <timj@imendio.com>

        * gtk/tmpl/gtkbindings.sgml: documented GtkBinding*, #358329.