GNOME Bugzilla – Bug 756751
Keypad decimal point patch prevents entry of comma in german keypad layout
Last modified: 2016-03-06 17:05:31 UTC
The german keyboard has a COMMA in the place of the numeric keypad where other national keyboards have the POINT symbol. The following two commits prevent comma entry when working on a windows machine with german locale setting and a german keyboard attached. Whe hitting the keypad COMMA symbol, a POINT gets inserted in all kind of widgets (GtkEntry, GtkTextView ...) First Commit: https://git.gnome.org/browse/gtk+/commit/gdk/win32?h=gtk-2-24&id=cb3eb7cd777b6cea239898efde3239c39144a955 Second Commit: https://git.gnome.org/browse/gtk+/commit/gdk/win32?h=gtk-2-24&id=1f74f12d9f8ecec321d7e662ff37127ca01c57c1 This problem occurs in a fresh build of Gtk+ 2.24.28. Rolling back the two commits solves the problem (verified on Windows-7/64-Bit). I do not know why these patches were applied, but they render the german keypad unuseable. The keypad layout cannot be constantly mapped to GDK_KEY_KP_Decimal ("."), because it depends the locale settings.
Googled this up: http://www.siao2.com/2006/09/13/752377.aspx
Nice article... but only half of the cake. Beware that the decimal separator entered via numeric keypad should correspond to other locale dependant functions like strod() and sprintf(). setting locale to de_DE will change the behaviour of strtod() and stprintf() to parse/format numbers with a COMMA. setting locale to ch_DE or en_US will change the behaviour of strtod() and stprintf() to parse/format numbers with a POINT. en_UK is even worse, because english locale use POINT as decimal separator and COMMA as thousands separator. quite the opposite of de_DE References (klick on Numbers LC_NUMERIC) : - http://lh.2xlibre.net/locale/de_DE - http://lh.2xlibre.net/locale/en_UK - http://lh.2xlibre.net/locale/de_CH - http://lh.2xlibre.net/locale/en_US GLib compensates this odd behaviour of libc with ascii_strtod(), ascii_dtostr() and ascii_formatd()
Well, what i have to say on the matter is that i'm not even sure what the right thing to do is.
So am i. Fact is that we didnt't have any conversion problems before the change, because the locale dependant correct decimal separator was copied from the underlying windows message. After the change, we always get POINTs, which causes trouble together with strtod() and sprintf(). Maybe a compile time option ? -- At least, you should put some comments into the code pointing the problem. Because the the consequences are not obvious when looking at the code.
Created attachment 313756 [details] [review] a patch Does this patch work for you ? (its against gtk3, but should more or less apply to gtk2 as well).
Thank you, I will check it out next week-end.
Ups, i'm running into compilation problems with the proposed patch on MinGW32/64-Bit. This is rather strange because when compiling our gtk-application in the same environment, we do not get any compiler errors. It also uses #include <locale.h> Any idea ? --- make[4]: Entering directory `/home/bb-slave/bsa_mingw64_slave/mingw64_gtk2/Build-2.24.MinGW.64x/gtk+-2.24.28/gdk' CC gdkkeyuni.lo In file included from gdkkeyuni.c:830:0: /opt/mingw64/x86_64-w64-mingw32/include/locale.h:15:9: error: expected expression before '#pragma' #pragma pack(push,_CRT_PACKING) ^ --- less /opt/mingw64/x86_64-w64-mingw32/include/locale.h /** * This file has no copyright assigned and is placed in the Public Domain. * This file is part of the mingw-w64 runtime package. * No warranty is given; refer to the file DISCLAIMER.PD within this package. */ #ifndef _INC_LOCALE #define _INC_LOCALE #include <crtdefs.h> #ifdef __cplusplus #include <stdio.h> #endif #pragma pack(push,_CRT_PACKING)
found it, do not put #pragma inside struct! https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37267
Sorry, i meant do not put #include <locale.h> inside struct definition.
Created attachment 314008 [details] [review] fixed patch Moved #include <locale.h> to top of file
OK, i can confirm that the patch is WORKING. Tested keypad data entry with GtkEntry and GtkTextView: - Win7/64Bit de_DE regional settings and keyboard layout -> Comma - Win7/64Bit de_CH regional settings and keyboard layout -> Point - Win7/64Bit fr_FR regional settings and keyboard layout -> Comma - Win7/64Bit en_US regional settings and keyboard layout -> Point Comments Typing the decimal separator on the keypad now renders the character that was set in the windows regional settings. While the french keyboard has a Point on the keypad layout, typing the decimal separator on the keypad renders a Comma now. But we think this behaviour is still consistend with strtod(), sprintf(), ...
For reference, the original discussion that led to cb3eb7cd777b6cea239898efde3239c39144a955: https://mail.gnome.org/archives/gtk-devel-list/2015-April/msg00025.html As for this patch, i have only one question: can locale change while the application is running? If not, then maybe make get_decimal_point() do its thing once and return cached result afterwards.
Hi guys. As the proposer of the original patch I've been asked if I can review this new amendment - though to be honest, I'm not sure what I should look for. From what I can remember, the problem I discovered in June was that (on Windows) the numeric keypad's decimal key was behaving differently from its behaviour in Linux and OS-X. In fact, I think it was getting completely ignored. So has the new patch superseded my earlier one from June? (i.e. is it just a better way of doing the same thing?) Or does it amend the functionality in some way? Or will it have the effect of restoring the original behaviour? (i.e. reverting my original patch?) What should I be looking for?
Instead of always emitting one character, pressing the decimal key will now emit differ different characters depending on how locale is configured.
@LRN - normally, the locale will be initialized very early during application startup. most libraries and applications do not deal with locale changes after the application has been initialized. Sybase client library f.e. will get completely messed up if you try to change locale after startup. So does FreeTDS. It is not necessary to handle changed locale settings after application startup. The user has to restart the application to work correctly. So caching the decimal_point character upon first use is completely sufficient. ---- More tests today: - CentOS/64Bit env LANG=de_DE -> Comma - CentOS/64Bit env LANG=de_CH -> Point - CentOS/64Bit env LANG=en_US -> Point
Caching the locale data would be possible, but I'd consider it a case of premature optimization. We call localeconv every time in g_ascii_strtod, and nobody ever reported a problem with that.
pushed to both gtk 2 and 3
Please, revert this patch. A similar change was made in xkeyboard-config some years ago, for similar reasons and it caused uproar, I was not the only one impacted. The change was reverted. http://cgit.freedesktop.org/xkeyboard-config/commit/?id=cf93beb47f9907946dd72ca4d481ffbbbadda53a Please, don't muck with the keyboard layout inside gtk: 1) I don't have a comma on my keypad, I have a dot. When I press this key, I expect to see the same character. 2) It makes gtk behave differently from other toolkits. gtk should just use whatever XKB (and its bazillion options) gives it. Thanks
FTR, the revert commit: http://cgit.freedesktop.org/xkeyboard-config/commit/?id=74e09c3a95961163e97a206d6847bec9dcabab6b
For what it's worth, in Windows the keypad key in question produces either a dot or a comma depending on your input language scheme. Even though it says ". Del" on the face of the physical key, pressing it will produce comma for some non-English keyboard layouts. So the "i don't have a comma on my keypad, i have a dot" logic conflicts with that of the OS. By the way, this bug says "Backend: Win32" and i've been implicitly assuming that this is about GTK+ input behaviour on Windows. However, the patch changes the common GDK code, i don't see any W32-specific #ifdefs there, and this talk of XKB makes me think: what did just happen, exactly? Did we change how GDK input works on other platforms?
I don't see any reason to make this win32-specific. And I don't see any reason to revert this patch
Again, my arguments: * consistency with other X11 toolkits and programs * XKB already allows changing the setting (and gnome-tweak-tool allows accessing that setting)
Beware, this patch here is a bugfix for another patch. See https://mail.gnome.org/archives/gtk-devel-list/2015-April/msg00025.html. If anyone needs to revert this patch, he should also revert the patch from the original discussion that lead to it. -- Changing xkb layout is only half of the story, because locale settings do not only influence language and keyboard, but also change behavior of standard C libary conversion functions like printf, strtod and friends. We have tested this on Win32 and X11 platforms and found both to be compatible with all tested locale settings. -- Don't you have the possibility to use either the Gdk key symbol GDK_KP_Decimal or alternatively check the key character in the event record. While the GDK_KP_Decimal is constant, only the key character may be '.' or ','.
Hello, this has been bugging me for the last month, and I've finally found this report. Please reopen this ticket, it needs serious reconsideration (at least for X11/Wayland). I'll try to explain from my non-english-speaker perspective: 1. This change introduces difference between gtk3 and non-gtk3 apps. E.g. in gedit (gtk3), KP_Decimal produces ",", but in Firefox (gtk2) or VLC (qt) it produces ".". That is extremely confusing for the end-user, and it makes user experience a hell, because you never know which characters is going to be displayed. If this is implemented somewhere, it should be lower in the stack, so that all applications behave consistently. 2. This change has been implemented and reverted several times in the past. Not just as referenced in comment 18, but look at bug 105161. It specifically says: "the official XFree86 interpretation is that KP_Decimal => . KP_Separator => , always, independent of locale" 3. While I don't object to the idea of KP_Decimal producing "," in some circumstances (provided it is consistent across all apps), it should not be dependent on your *locale*. It should be dependent on your *keyboard layout*. As someone who uses GNOME with a non-english locale (Czech), I don't want to have KP_Decimal hardcoded to ",". That would make it unusable for inputting e.g. english numbers, unless I use a full-english desktop. That's exactly what keyboard *layouts* are used for - they can be switched on the fly, and they change the representation of the physical keys, and that's exactly what we want to do here - print "," or "." based on what layout I'm using, not a static locale. So for Czech keyboard layout, it would print out ",", but for English keyboard layout, it would print out ".". 4. I have tested this behavior in Windows 10, and it behaves exactly as I described in 3). The character printed out is not dependent on system language, but on currently active keyboard layout. With Czech layout, it prints out ",", with English layout, it prints out ".". So overall this patch did not change the behavior to something (Windows) users expect, but it just changed one hardcoded value for another hardcoded value (unless you want to change your system language). It did not make the behavior consistent with what Windows does. I understand this does not bother English speakers too much, but please, for some of us this is extremely unpleasant. Many of non-English speaker are used to and often need to work with multiple languages. We need to be able to write in both. But I can no longer input Czech-style dates, English-style numbers or e.g. IP addresses using the numeric keypad, regardless of what keyboard layout I choose, just because I happen to want a translated desktop. That cannot sound right to you. Thanks a lot for reconsideration.
I'm willing to bet nothing will be done here until a stable version of Debian/Ubuntu starts shipping this version of gtk. So a couple more months for Ubuntu, about a year or so for Debian. Just like the xkeyboard-config patch really. Let's just wait for a larger number of users to hit this (I'm very well aware Gentoo is a niche distro, and that French Gentoo users with an azerty layout are even more of a niche).
Reopening as requested by Kamil on irc, explanation is in comment 24
Created attachment 318409 [details] [review] GDK W32: Use *input locale* not *system locale* for KP_Decimal Uses current input locale to convert GDK_KEY_KP_Decimal to a unicode character. This is the W32 version, other platforms will have to do it differently. It's only possible to do this way because GDK_KEY_KP_Decimal is already a special case, otherwise a full GDK_KEY_* -> VK_* map would have been needed. This fixes bug 756751. Again.
(In reply to Kamil Páral from comment #24) > 3. While I don't object to the idea of KP_Decimal producing "," in some > circumstances (provided it is consistent across all apps), it should not be > dependent on your *locale*. It should be dependent on your *keyboard > layout*. As someone who uses GNOME with a non-english locale (Czech), I > don't want to have KP_Decimal hardcoded to ",". That would make it unusable > for inputting e.g. english numbers, unless I use a full-english desktop. > That's exactly what keyboard *layouts* are used for - they can be switched > on the fly, and they change the representation of the physical keys, and > that's exactly what we want to do here - print "," or "." based on what > layout I'm using, not a static locale. So for Czech keyboard layout, it > would print out ",", but for English keyboard layout, it would print out ".". Note sure I agree, really. The hypothesis underlying the change is that you are entering numbers that are being parsed by strtod and its ilk. And those parsing functions use the locale to determine the decimal separator. Not the keyboard layout.
That's a discrepancy between libc (which depends on what you tell setlocale(), and whether you call it at all), OS locale (which, on W32, is global, at least on a per-user basis; there is API for accessing it) and input locale AKA keyboard layout (which changes frequently). I'm not sure there's a completely right thing that could be done. FWIW, MS PowerShell only accepts floating point numbers with '.' separator, but uses locale-dependent separator when printing them. PowerShell is the only very-windowsly-app-from-MS i had at hand that can do something with floating point numbers (other programs deal with integers) *and* is not specifically geared towards handling floating point numbers - unlike, say, Calculator, which just accepts any separator ('.' and ',' at least) on input and uses locale-dependent separator when showing numbers. I'd go with following the input locale, because generally people expect keyboard input to *only* depend on the input locale AKA keyboard layout, not on some setting that they hardly ever change or even know about. Applications that require a lot of numbers to be entered (LibreOffice Calc, for example) tend to handle this in a special way (specifically, Calc types '.' or ',' in response to VK_DECIMAL, depending on what your system locale is; but it also has varying cell format, which means that you might need to input '.' or ',' depending on the cell format, and that could lead to a mismatch between what VK_DECIMAL types and what you need to be typed). As for generic GTK apps, i'd like to point out that numeric entry widgets will refuse to let you type a wrong separator, thus the problem will not be > i've typed in a number and the separator was ignored > after i pressed Enter , but rather > the input field didn't accept ',' (or '.', depending on locale) > as a separator, i've had to type '.' (or ',') - annoying, but not fatal. Best thing would be to accept any separators (use custom strtod(), etc implementations?) when possible, but output numbers with locale-dependent separators. More work for the toolkit, but less frustration for its users.
(In reply to Matthias Clasen from comment #28) > Note sure I agree, really. The hypothesis underlying the change is that you > are entering numbers that are being parsed by strtod and its ilk. And those > parsing functions use the locale to determine the decimal separator. Not the > keyboard layout. Yes, but I did not meant that all those numbers need to be processed (by strtod or similar), sometimes I just need to type them out. For example, if I have cs_CZ system locale and cs input locale and start a gnome-calculator, I can input "3,14" just with the numeric keypad and the number will be parsed alright. If I switch to us input locale, it will produce "3.14" instead, and at this point it's completely understandable if gnome-calculator yells at me "not a valid number!". Of course, because I'm running it with cs_CZ system locale and indeed that is not a valid number. However, I don't always process numbers, often I just need to type them. For example, I can have a txt file where I need to type in a lot of english-style numbers (it might be an invoice from your english customer). Then, when using the us input locale, I expect the numpad to produce english-style numbers (as on Windows). I expect and want "." to be printed out in this case, I'm not trying to process it. The same use case is an english website that uses its own logic to process english-style numbers. Again, I want to be able to switch to the us input locale and conveniently input english-style numbers using the numpad. And another very common use case in our world is programming, where all numbers are typed in using english conventions, regardless of your system locale (because the programming language syntax defines it that way). So again, switching to the us input locale should allow me to write source code conveniently. (In reply to LRN from comment #29) > Applications that require a lot of numbers to be entered (LibreOffice Calc, > for example) tend to handle this in a special way Yes, this is actually the case even for gnome-calculator - it accepts both "." and "," as a decimal separator. Due to this being a very known difference between US and EU, most such applications accept both characters. I assume that's why so few people complain about this - it mostly works anyway. However, with the recent change of "." into "," under certain locales (like Czech), all those programming use cases and terminal input use cases stopped working and there's no "fix" for that. That's why I consider this change a net loss - most number-heavy GUI apps worked before and still work now, but many poweruser-related or multilanguage-related use cases are now broken.
Isn't this discussion simply to make the Point/Comma decision based on the "input locale" instead of the "system locale" ? What exactly is meant with the 'input locale' ?
input locale == keyboard layout. It controls how your keypresses are interpreted. This is extremely useful for people who type stuff in more than one language, because it allows one to type characters from different languages (or character sets) using the same keyboard, instead of having to buy a separate keyboard for each language. Input locale AKA keyboard layout is usually switched around by pressing some kind of key combo that the desktop environment recognizes. Usually it's a combination of <shift>, <ctrl>, <alt>, <super> keys, and sometimes the <space> key.
(In reply to Matthias Clasen from comment #28) > Note sure I agree, really. The hypothesis underlying the change is that you > are entering numbers that are being parsed by strtod and its ilk. I think this hypothesis is faulty. Unless the focus is in a widget specifically tailored for floating point number input, I don't think anyone can make any reasonable assumptions about the user's intent, even when using the keypad. Applications that care about those things (spreadsheets, scientific or financial applications, custom widgets, etc) can listen in on the symbol in the key event and act appropriately (e.g. the way LibreOffice Calc does), while regular text inputs should just get the character translated from the key as per the system/session settings.
The problem I originally reported was that (for Windows users) KP_DECIMAL simply wasn't implemented. Yes, technically it was doing the same thing in all locales - but only because it did nothing at all in any locale. The 'comma' key and 'dot' key were both available (from the alpha side of a keyboard). But from the numeric keypad, the 'dot' key was just getting ignored. Now that it's been implemented, it seems that different users need it to do different things. Unless they can reach an agreement about what they actually need, I'm not sure if this problem is actually solvable.
I've reverted the change. Not worth the amount of controversy. Sad for Fredy, I guess.
So you will always get dots '.' in all Gtk Widgets on Windows, with all international keyboard layouts, no matter what "input locale" was selected. I bet we're not finished yet. :-) --- btw. Here's a very nice and general writeup about the decimal marks and related: https://en.wikipedia.org/wiki/Decimal_mark --- Maybe i'll find some time next weekend to look for an alternative solution.
I thought that W32 case is actually going to be fixed by the patch in comment 27, which (IIUIC) will make gtk behave exactly the same as Windows frameworks (i.e. let the keyboard layout affect the separator key). On Linux it might be harder to achieve the same thing, so I'm personally glad this got reverted until there's a proper solution. But I didn't want to negatively affect the original reporter, if the patch is ready and can be applied as Windows-only. Either way, can we please get the commit reverted also in 3.18 branch?
@matthias - what patch did you actually revert now? Generally i agree to revert the patch in GDK, because it affects all operating systems. We have to work out a solution for Windows that uses the character that gets reported by the underlying operating system. --- A few other aspects to consider: - you can have multiple input devices, each of them one using different "input locales" for example USB Barcode-Scanners sending keycodes while scanning special characters within a barcode - every running application in the same user session can use a different "system locale" which affects how numbers are transformed and displayed to the user - most GTK Widgets directly display the character data of the keyboard event to modify and display not only numbers, but also the text entry - finally the application decides what the data is used for some applications need dots, to enter IP's some applications need decimal separators, to enter numbers some applications need the sign that is printed on the physical key, text f.e. Android solves this by invoking different entry widgets for numbers, e-mail adresses and text
(In reply to Fredy Paquet from comment #38) > @matthias - what patch did you actually revert now? https://git.gnome.org/browse/gtk+/commit/?id=4eb333801b517c58b896d82dcd129768e76d0e0e This is the one from comment 17 that affected all platforms.
Just a suggestion... but would it be feasible to add two new API functions - e.g. 1) 'set_kp_decimal_to_system_locale()' - and 2) 'set_kp_decimal_to_keyboard_locale()' Developers could then choose whichever translation is most appropriate for them (or even add something so it could be user-selectable).
(In reply to John E from comment #40) > Just a suggestion... but would it be feasible to add two new API functions - > e.g. > > 1) 'set_kp_decimal_to_system_locale()' - and > 2) 'set_kp_decimal_to_keyboard_locale()' > > Developers could then choose whichever translation is most appropriate for > them > Hmmm... maybe that's not such a great idea in a shared DLL :-(
Code path analysis (after revert of commit 4eb33380) win32/gdkkeys-win32.c, function update_keymap(), line 383ff starting from commit cb3eb7cd7, Line 383 - handle_special() will map VK_DECIMAL -> GDK_KEY_KP_Decimal now Line 385 - if (*ksymp == 0) is FALSE Line 391 - k = ToUnicodeEx() will no longer get executed Line 493 - control returned to caller with *ksymp == GDK_KEY_KP_Decimal gtkkeyuni.c, function gdk_keyval_to_unicode(), line 888ff - keyval == GDK_KEY_KP_Decimal - binary search in gdk_keysym_to_unicode_tab hits constant entry { 0xFFAE /* Decimal */, '.' }, Effect - on Windows: gdk_keyval_to_unicode() always return '.' - on other operating systems: return input language dependant '.' or ',' Discussion The call of ToUnicodeEx() which returned the input language dependant decimal mark character gets no longer executed. The upper level function gdk_keyval_to_unicode() has no way to determine the windows driver specific input language setting. The windows driver is holding the input locale setting in the variable _gdk_input_locale initialized to GetKeyboardLayout(0) by _gdk_windowing_init(). The variable is declared in win32/gdkprivate-win32.h. The GdkEventKey data structure and involved function parameters offer no possibility to pass the input locale from the lower level (win32) to the upper level (gdkkeyuni.c). I do not find any other 'input locale' options or settings within GDK. So gdk_keyval_to_unicode() should ask the lower level(s) for exceptions before applying the global gdk_keysym_to_unicode_tab, which is valid for all operating systems. References ToUnicodeEx - https://msdn.microsoft.com/en-us/library/windows/desktop/ms646320(v=vs.85).aspx GetKeyboardLayout - https://msdn.microsoft.com/en-us/library/windows/desktop/ms646296(v=vs.85).aspx
In order to make a proposal following the principles: - use "input locale" to deliver correct unicode for keyval - do not use "system locale" at all - affects win32 only - keep win32 specific code as far as possible in gdk/win32/ The patch works like this: - When typing the first character at the keyboard or when switching national keyboard layout, the decimal mark character will be cached in a static variable named "decimal_mark" within gdkkeys-win32.c (works because there exists only one keysym_tab in gdkkeys-win32.c) - in case of WIN32, gdk_keyval_to_unicode() asks gdkkeys-win32.c for the decimal_mark when converting GDK_KEY_KP_Decimal. Notes: - the local function declaration in gdkkeyuni.c is somewhat ugly
Created attachment 318668 [details] [review] GDK Win32 decimal mark patch Here's the patch
Review of attachment 318668 [details] [review]: Thanks for the patch; you probably want to set the resolution to "REOPENED", not "RESOLVED FIXED". The coding style is completely inconsistent with the GDK one; please, take five minutes to read the coding style document and update the patch. The preferred way for submitting patches is using git format-patch, or git bz; please, read: https://wiki.gnome.org/Newcomers/SubmittingPatches ::: gdk/gdkkeyuni.c @@ +882,3 @@ return keyval & 0x00ffffff; +#if defined(G_OS_WIN32) Not really how platform detection works. GDK has the GDK_WINDOWING_WIN32 symbol that can be used at compile time, but GDK can be compiled with multiple backends in the shared library, which means you also need to check for the currently running backend. @@ +883,3 @@ +#if defined(G_OS_WIN32) + extern guint32 gdk_win32_keymap_get_decimal_mark(void); This is not how we use symbols. You should include the header file that declares this symbol instead. @@ +885,3 @@ + extern guint32 gdk_win32_keymap_get_decimal_mark(void); + if (keyval == 0xffae) + return (guint32) gdk_win32_keymap_get_decimal_mark (); Coding style: too many leading spaces. Indentation is 2 spaces. ::: gdk/win32/gdkkeys-win32.c @@ +332,3 @@ +/* return current decimal mark as unicode character - bug id 756751 */ +guint32 gdk_win32_keymap_get_decimal_mark(void) Coding style: * newline after the return type * space between function name and open parenthesis @@ +334,3 @@ +guint32 gdk_win32_keymap_get_decimal_mark(void) +{ +#if 0 Please, drop the ifdeffed out debugging g_print calls. @@ +335,3 @@ +{ +#if 0 + g_print("gdk_win32_keymap_get_decimal_mark(): %04x\n", decimal_mark); Coding style: indentation. @@ +337,3 @@ + g_print("gdk_win32_keymap_get_decimal_mark(): %04x\n", decimal_mark); +#endif + if (decimal_mark) return (decimal_mark); Coding style: newline after the condition @@ +408,3 @@ wcs[0], wcs[1]); #endif + if (vk == VK_DECIMAL) Coding style: indentation. @@ +415,3 @@ + wcs[0], wcs[1]); +#endif + if ((k == 1) && (shift == 0)) decimal_mark = wcs[0]; Coding style: newline after the condition.
Thank you for the comments, I will read coding style and submit a updated patch next weekend.
Created attachment 319208 [details] [review] GDK Win32 keyboard decimal mark patch Reworked patch, @ebassi - some questions about your comments: - After setting up GTK CODING-STYLE in SlickEdit, it looks like formatting is already defect in the function gdk/win32/gdkkeys-win32.c :: update_keymap() What's to do now ? Reformat the whole function body ? Or only part of it ? The patch would be difficult to read then. - Where to put the function definition for extern guint32 gdk_win32_keymap_get_decimal_mark(void); the implementation must stay in gdk/win32/gdkkeys-win32.c because it uses local static vars. today it will be called from gdk/gdkkeyuni.c for win32 platform only but might get similar implementations in other backends in the future - Can you really multiple GDK_WINDOWING_WIN32 backends at the same time ? I found only one single keymap in the win32 driver.
The macro GDK_IS_WIN32_DISPLAY() were introduced with Gtk3. The patch shlould also be needed for Gtk2. Would this be ok: #if defined(GDK_WINDOWING_WIN32) #if defined(GDK_IS_WIN32_DISPLAY) if (GDK_IS_WIN32_DISPLAY (gdk_display_manager_get_default_display())) { if (keyval == 0xffae) return (guint32) gdk_win32_keymap_get_decimal_mark (); } #else if (keyval == 0xffae) return (guint32) gdk_win32_keymap_get_decimal_mark (); #endif #endif
(In reply to Matthias Clasen from comment #35) > I've reverted the change. Not worth the amount of controversy. Sad for > Fredy, I guess. Hi Matthias, it looks like you only revert the patch in master and not in gtk-3-18 nor gtk-2-24 branches.
Could you do that ?
revert done on gtk-2-24 branch: 1ac15824c73629528a7e33d0d1d588a31643302b revert done on gtk-3-18 branch: 5d218939a3bedbe624a315ba52c882147f1b128c
Created attachment 321727 [details] [review] GDK Win32 keypad decimal mark patch This patch sets the keypad decimal mark character to the appropriate character for the active national keyboard layout. When switching keyboard layouts during an application session, the decimal mark character changes as expected. It can be used for gtk 2.24 and 3.18 branches. It was tested on Win7/64-Bit with the current git branch 2.24 --- Please review and apply the patch.
Patch contains unneeded whitespace change (you've converted some tab characters into spaces; well, most likely your editor did that for you, but that's not the point). This prevents me (or anyone) from correctly reviewing changes in the fourth hunk.
Created attachment 321729 [details] [review] GDK Win32 keypad decimal mark patch removed unwanted noise from patch rebuild gtk2 again with gtk2.24.28 distribution tested again on Win7/64-Bit - works please review & apply
Review of attachment 321729 [details] [review]: ::: gdk/win32/gdkkeys-win32.c @@ +418,3 @@ *ksymp = gdk_unicode_to_keyval (wcs[0]); else if (k == -1) { You've removed spurious whitespace changes (for lines that only had whitespace change), but whitespace for the actual changes is still wrong (spaces instead of tabls). This will have to be fixed before anything is committed. Okay, so we hijack keysym_tab calculation for VK_DECIMAL and force the function to call ToUnicodeEx(), then, if it is successful and shift is 0, cache the unicode char for decimal point in a static variable for later use. I have two concerns about this: 1) keysym_tab for VK_DECIMAL is still calculated as it was before (handle_special() sets it). I've skimmed the code, and i see that keysym_tab is used in other functions: gdk_win32_keymap_get_entries_for_keyval() will continue to return VK_DECIMAL when given GDK_KEY_KP_Decimal. That feels right, but still bears some consideration. gdk_win32_keymap_get_entries_for_keycode() will continue to return GDK_KEY_KP_Decimal when given VK_DECIMAL, which doesn't feel right (i don't know which programs or GDK/GTK functions call gdk_win32_keymap_get_entries_for_keycode(), and what they expect to get). Accordingly, when gdk_win32_keymap_lookup_key() is called and given the result of gdk_win32_keymap_get_entries_for_keyval(), it will return GDK_KEY_KP_Decimal, even though we probably would like it to return decimal_mark. Same for gdk_win32_keymap_translate_keyboard_state(). Granted, we never really considered this particular angle before, as previous patches only changed gdkkeyuni.c. So while the result of the patch as-is might be technically incorrect in some cases, it will not be more incorrect than what we have now, so lack of surety about the behaviour of these extra functions should not be considered an obstacle for pushing this patch as-is (except for whitespaces). IMO. 2) keysym_tab generator for each keycode runs 4 times, with different shift values. Yet you always sidetrack it, always call ToUnicodeEx() on VK_DECIMAL, with different shift values, but only store decimal_mark for shift==0. Luckily, decimal key seems to be unaffected by modifier keys (handle_special() returns GDK_KEY_KP_Decimal regardless of the value of shift) and is always handled by handle_special(), so this code performs correctly in practice. Still, it might be appropriate to make vk == VK_DECIMAL more specific by adding && shift == 0.
Agree about 2) but i will have to do further investigations about modifier behaviour on win32, because we actually do have '.' and 'Delete' on the samy physical key. Thanks for the input, i'll be back soon
I've checked out the effect of some modifier combinations on the keypad into GtkEntry on Win7/64-Bit (patch applied): - Layout: de_CH Numlock ON + KP_Decimal --> '.' Numlock ON + KP_Decimal + Shift --> Delete Numlock OFF + KP_Decimal --> Delete Numlock OFF + KP_Decimal + Shift -> NOOP - Switched to Layout: de_DE --> same behaviour with ',' I found that "Numlock OFF + KP_Decimal + Shift" has no effect when there is no text selection. If there is a text selection, it will delete the selection. While there is slight inconsistency in the event processing of GtkEntry, debug output shows that the underlying event gets delivered correctly with GDK_KEYPRESS Delete+SHIFT_L to Gtk. Summary The Delete key is still working as before with the patch applied. I do not want to start another discussion about GtkEntry behaviour.
Created attachment 321769 [details] [review] GDK Win32 keypad decimal mark patch - fixed TAB/SPACE usage in patch - added (shift == 0) to (vk == VK_DECIMAL) condition to be more specific - verified patch is working with a complete rebuild of Gtk+ 2.24.28 on Win7/64-Bit success please review & apply
Review of attachment 321769 [details] [review]: Patch seems OK, here are some nitpicks. I'll also ask nacho to look at it, he's better at nitpicking than i am. ::: gdk/gdkkeyuni.c @@ -884,0 +884,5 @@ +#if defined(GDK_WINDOWING_WIN32) + /* keypad decimal mark depends on active keyboard layout */ + ... 2 more ... Space between gdk_display_manager_get_default_display and () ::: gdk/win32/gdkkeys-win32.c @@ -332,0 +333,10 @@ +/* return current decimal mark as unicode character - bug id 756751 + */ + ... 7 more ... Space between (guint32) and '.' ::: gdk/win32/gdkprivate-win32.h @@ -501,2 +501,4 @@ MSG* msg); +/* keypad decimal mark depends on active keyboard layout */ +guint32 gdk_win32_keymap_get_decimal_mark(void); Space between gdk_win32_keymap_get_decimal_mark and (void)
Created attachment 321770 [details] [review] GDK Win32 keypad decimal mark patch nitpicks fixed :-)
Review of attachment 321770 [details] [review]: See my nitpicks :) ::: gdk/gdkkeyuni.c @@ +883,3 @@ +#if defined(GDK_WINDOWING_WIN32) + /* keypad decimal mark depends on active keyboard layout */ you added this comment also on the prototype @@ +892,3 @@ + } +#else + if (keyval == 0xffae) /* gtk2 */ why this gtk2 comment? I'd say remove it @@ +895,3 @@ + return (guint32) gdk_win32_keymap_get_decimal_mark (); +#endif + no need for this empty line before the endif ::: gdk/win32/gdkkeys-win32.c @@ +49,2 @@ static guint *keysym_tab = NULL; +static wchar_t decimal_mark = 0; /* bug id 756751 */ no need to specify this bug id comment, there is the git history for that @@ +331,3 @@ } +/* return current decimal mark as unicode character - bug id 756751 If you need to add this comment to specify what the method does then the method is not clear enough ::: gdk/win32/gdkprivate-win32.h @@ +501,3 @@ MSG* msg); +/* keypad decimal mark depends on active keyboard layout */ this is a comment that I would expect on the C file
Created attachment 321775 [details] [review] GDK Win32 keypad decimal mark patch comments fixed
Review of attachment 321775 [details] [review]: See more nitpicks ::: gdk/gdkkeyuni.c @@ +882,3 @@ return keyval & 0x00ffffff; +#if defined(GDK_WINDOWING_WIN32) still not sure why we need all these ifdefs, I would expect it that if win32 do something, instead there are 2 checks. Is this because you want to support gtk2 on the same patch? if this is the reason I do not like it. you are targetting gtk3, and you should make a different patch for gtk2. ::: gdk/win32/gdkkeys-win32.c @@ +334,3 @@ + return current decimal mark as unicode character + */ + comments like this should be right above the method so remove the empty line. also it should look like this: /* * */
Created attachment 321786 [details] [review] GDK Win32 keypad decimal mark patch (gtk3) here's the patch for gtk3
Created attachment 321787 [details] [review] GDK Win32 keypad decimal mark patch (gtk2) Here's the patch for gtk2
I think Fredy deserves a big vote of thanks for persevering with this. Well done!
I'm starting to understand how proper code management is done. Ups, forgot the middle star in what the comment should look like.
Review of attachment 321786 [details] [review]: I am a bit worried about the fact that it was not even compiled this patch... LRN did you test this? ::: gdk/gdkkeyuni.c @@ +883,3 @@ +#if defined(GDK_WINDOWING_WIN32) + if (GDK_IS_WIN32_DISPLAY (gdk_display_manager_get_default_display ())) this will actually not compile.... you should probably use gdk_display_get_default. ::: gdk/win32/gdkkeys-win32.c @@ +332,3 @@ +/* keypad decimal mark depends on active keyboard layout + return current decimal mark as unicode character this comment is still not properly using the style: /* * */
All my tests were Gtk2 only. I would have to set up a test environment for Gtk3. I could do that next weekend.
Review of attachment 321787 [details] [review]: Apart from fixing the comment this looks good to go. ::: gdk/win32/gdkkeys-win32.c @@ +332,3 @@ +/* keypad decimal mark depends on active keyboard layout + return current decimal mark as unicode character /* * */
>I am a bit worried about the fact that it was not even compiled this patch... > LRN did you test this? No, i did not. And you're right, gdk_display_manager_get_default_display() takes one argument, so calling it without one is obviously wrong. I think someone should ask Company about that, he's the one who's been messing around with displays (or was it screens?) recently. Since OP has no GTK3 at hand, i could try the patch (and fix it to compile) myself, i'm doing some work on GDK anyway. And since we're nitpicking: >- if (*ksymp == 0) >+ if ((*ksymp == 0) || ((vk == VK_DECIMAL) && (shift == 0))) Again, wrong indentation (was 1 tab + 6 spaces, now it's 2 tabs). You should set up your editor to give 1 tab the length of 8 spaces. (Personally, i hate tabs exactly for this reason).
According to the doc, there is one display manager, so it should be > gdk_display_manager_get_default_display (gdk_display_manager_get ()); -- While i already succeded in compiling Gtk3 on Linux, i'm not quite sure to bring up my whole test environment for Gtk3 on Win32 during next weekend.
It should be a matter of calling: gdk_display_get_default ()
Created attachment 321858 [details] [review] GDK Win32 keypad decimal mark patch (gtk3) (fixed) Called gdk_display_get_default () instead of gdk_display_manager_get_default_display (). Renamed gdk_win32_keymap_get_decimal_mark -> _gdk_win32_keymap_get_decimal_mark. Lined up the * in the comment before _gdk_win32_keymap_get_decimal_mark(). Fixed indentation for if ((*ksymp == 0) || ((vk == VK_DECIMAL) && (shift == 0))). This patch applies to gtk+-3.0-git-f7ec9c9 and seems to work OK (i did also notice that the behaviour of the DEL key is somewhat strange, but it's not related to or caused by this patch, so i agree that it shouldn't be an obstacle).
Review of attachment 321858 [details] [review]: What is the .orig2 in the patch? ::: gdk/gdkkeyuni.c @@ +883,3 @@ +#if defined(GDK_WINDOWING_WIN32) + if (GDK_IS_WIN32_DISPLAY (gdk_display_get_default ())) do we really need to check this if we have the ifdef right before?
(In reply to Ignacio Casal Quinteiro (nacho) from comment #75) > What is the .orig2 in the patch? Sorry, been doing it by hand. > ::: gdk/gdkkeyuni.c > @@ +883,3 @@ > > +#if defined(GDK_WINDOWING_WIN32) > + if (GDK_IS_WIN32_DISPLAY (gdk_display_get_default ())) > > do we really need to check this if we have the ifdef right before? No idea. Ask mclasen.
The #if is a build time check - you can't use GDK_IS_WIN32_DISPLAY if your compiler can't see the win32 gdk headers. The type check is a runtime check, to confirm that you are actually dealing with the win32 backend, and not, e.g. Broadway. So yes, they have different purposes and are both needed.
Created attachment 322481 [details] [review] W32: Fix for commit 1f74f12d9, re-enabling decimal separator key Correctly bugzilled version of the same patch
Comment on attachment 322481 [details] [review] W32: Fix for commit 1f74f12d9, re-enabling decimal separator key Attachment 322481 [details] pushed as 0a6ee5e - W32: Fix for commit 1f74f12d9, re-enabling decimal separator key Should we resolve this bug as "fixed" now?
Did you push to the Gtk3 branch ? If yes, i would like to verify correct operation after setting up my build environment for Win32. --- Functionality was already checked with Gtk2 branch last week (fresh pull).
Attachment 321787 [details] pushed (with cosmetic changes) into branch gtk-2-24 as 4bf5290 - W32: Fix for commit 1f74f12d9, re-enabling decimal separator key
@LRN - I'm giving up for today, because cross compilation of Gtk 3.18.8 for MinGW64 is broken. > configure: error: cairo_win32_surface_create_with_format is not found in cairo library This function is missing in Cairo 1.14.6, but Cairo 1.15.2 disappeared from the download folder while ChangeLog.cairo-1.15.2 is still there. http://permalink.gmane.org/gmane.comp.lib.cairo/26122 --- Pse set this bug to fixed/resolved I will verify the fix as soon as i get cross compilation to work. Many thanks for your efforts.
(In reply to Fredy Paquet from comment #82) > @LRN - I'm giving up for today, because cross compilation of Gtk 3.18.8 for > MinGW64 is broken. > > > configure: error: cairo_win32_surface_create_with_format is not found in cairo library > > This function is missing in Cairo 1.14.6, but Cairo 1.15.2 disappeared from > the download folder while ChangeLog.cairo-1.15.2 is still there. > > http://permalink.gmane.org/gmane.comp.lib.cairo/26122 Yeah, that one is unfortunate. You can use 1.14.6, applying this[1] patch. > > --- > Pse set this bug to fixed/resolved I will, once you confirm that it works for you. [1] https://cgit.freedesktop.org/cairo/commit/?id=16898ba11b4d6e9e2e64bb2d02d0fb5adbe266e2
Thank you for the tip, I will give it another try next weekend.
Good news After several days of tweaking library versions and fighting with the gnu build system i was able to cross compile Gtk3 for MinGW 64-Bit with a fresh pull from https://github.com/GNOME/gtk.git Used gtk3-widget-factory.exe for tests. Test with Branch 3.19.10 - hitting keypad separator with Swiss or German keyboard layout always renders a dot '.' switching keyboard layouts has no effect Test with Branch 3.19.11 - hitting keypad separator with Swiss keyboard layout renders a dot '.' with German keyboard layout renders a comma ',' switching keyboard layouts while running the app works as expected So the patch is working fine. Many thanks for your help. This issue is SOLVED. -- Cross-Compilation notes: - you should add introspection.m4 to the m4 folder of the distribution configure scripts will terminate with an error if introspection is not available
Marking this as fixed (even though it was only fixed for Windows, for any possible meanings of "fixed"). -- Cross-Compilation notes: - introspection.m4 is made available by gobject-introspection package. You didn't install or build it, i guess.
*** Bug 761145 has been marked as a duplicate of this bug. ***