Created attachment 169788 [details] [review]
Add _KEY

Created attachment 169789 [details] [review]
update keysyms

Note the order is backwards, for some reason bugzilla didn't take my first attachment.

Note, i'd like to do a patch which renames all of the constants to GDK_KEY_ in gtk3.  This is a lot cleaner - even in C, #define GDK_<random string> is just asking for namespace conflicts.

For application compatibility, I can fairly easily provide gdkkeysyms-compat.h.  Any thoughts/objections?

Review of attachment 169788 [details] [review]:

Mostly looks like it works as intended to me, specific comments below.

The general idea needs sign-off from a GTK+ maintainer - this patch is saying that these are "really" Gdk.KEY_Whatever for modern language bindings, and Gdk.Whatever for C and older language bindings is compatibility.

::: gdk/Makefile.am
@@ +20,3 @@
 	gdkconfig.h.win32 	\
 	gdkkeysyms-update.pl	\
+	gdkkeysyms-introspection	\

I think it's pretty confusing to have gdkkeysyms-introspection be a script and gdkkeysyms-introspection.h a header file. I'd call this make-gdkkeysms-introspection.py or something like that (you can claim that .py is an implementation detail, but it's not something you run manually and having the .py makes it more obvious what's going on. Note that existing .sh scripts.)

@@ +173,3 @@
 
+gdkkeysyms-introspection.h: gdkkeysyms.h gdkkeysyms-introspection Makefile
+	python $(srcdir)/gdkkeysyms-introspection $< $@

Can't use python like this without a configure.ac check - /usr/bin/python is something ancient on solaris apparently. Use AM_PATH_PYTHON([2.5]) in configure.ac and $(PYTHON) here

@@ +177,2 @@
 introspection_files = 		\
+	$(filter-out gdkkeysyms.h, $(gdk_public_h_sources))	\

Why can't you add gdkkeysyms-introspection.h here?

::: gdk/gdkkeysyms-introspection
@@ +46,3 @@
+        if not line.startswith(GDK_KEY_START):
+            continue
+        output_f.write('#define GDK_KEY_')

It's strange to have the old version and the new version not a constant

Created attachment 169808 [details] [review]
prefix key names with KEY_

Totally different patch; see commit message for details.

If we go with this, my suggested approach for gtk2 is to also add these #defines to gdkkeysyms.h, so apps can be ported and work with both.  (A patch to do so is pretty easy).

Review of attachment 169808 [details] [review]:

This one looks a lot better to me. Thanks for doing all the sed work to port gtk over. Not sure we really want to deprecated all the old keysyms in 2.x; thats a lot of deprecation to do so close to 2.22. Adding the new names to 2.x is a good idea though.

Committed to gtk3, working on gtk2.

Fix committed to gtk2 (with maximal compatiblity, gtkkeysyms.h does #include <gdk/gdkkeysyms-compat.h>).

So, while this is also on 2-x branch, if one wants to support _older_ gtk versions as well as gtk3, one does now have to

#if GTK_CHECK_VERSION (2, 22, 0)
#include <gdk/gdkkeysyms-compat.h>
#else
#include <gdk/gdkkeysyms.h>
#endif

IMHO, it would have been better to put the new keysym names in a NEW header (gdkkeysyms3.h maybe), and leave the compat defines in the header with the old name.

I think just including gdk/gdkkeysyms.h is supposed to work in 2.x and 3.

It includes the compat header in 2.x

Is gdkkeysysm intentionally still not in gdk.h? I'd think now that it has proper names, that shouldn't be a problem.

(In reply to comment #11)
> So, while this is also on 2-x branch, if one wants to support _older_ gtk
> versions as well as gtk3, one does now have to
> 
> #if GTK_CHECK_VERSION (2, 22, 0)
> #include <gdk/gdkkeysyms-compat.h>
> #else
> #include <gdk/gdkkeysyms.h>
> #endif
> 
> IMHO, it would have been better to put the new keysym names in a NEW header
> (gdkkeysyms3.h maybe), and leave the compat defines in the header with the old
> name.

Hmm, well we could just include them in gdk.h as comment #13 suggests.  That would reduce your code to:

#if GTK_CHECK_VERSION (2, 22, 0)
#include <gdk/gdkkeysyms-compat.h>
#endif

Is that too onerous?  If we went with your suggestion of gdkkeysyms3.h, I basically don't think anyone would ever use the new names, and we'd be stuck with namespace conflicts if someone added a key named WINDOW or whatever.

Colin: do you need 

#if GTK_CHECK_VERSION (2, 22, 0)
#include <gdk/gdkkeysyms-compat.h>
#endif

? It was my understanding that gdk-keysyms.h includes gdk-keysyms-compat.h in 2.x. Is that not the case ?

As for including in gdk.h, I'm open to doing that in 3.0, but we should probably leave things as they are in 2.x

gdk.h includes gdkkeysyms.h in master now

(In reply to comment #15)
> Colin: do you need 
> 
> #if GTK_CHECK_VERSION (2, 22, 0)
> #include <gdk/gdkkeysyms-compat.h>
> #endif
> 
> ? It was my understanding that gdk-keysyms.h includes gdk-keysyms-compat.h in
> 2.x. Is that not the case ?

Ah...right.  So we only want to include it in 3.  So:

#if GTK_CHECK_VERSION (2, 95, 0)
#include <gdk/gdkkeysyms-compat.h>
#endif

right?

> As for including in gdk.h, I'm open to doing that in 3.0, but we should
> probably leave things as they are in 2.x

Sounds fine.

(In reply to comment #17)
 
> Ah...right.  So we only want to include it in 3.  So:
> 
> #if GTK_CHECK_VERSION (2, 95, 0)
> #include <gdk/gdkkeysyms-compat.h>
> #endif
> 
> right?

Or just an unconditional 

#include <gdk/gdkkeysyms-compat.h> 

if you don't care about the extra include.