GNOME Bugzilla – Bug 790453
Groups not assigned in order while setting user layouts
Last modified: 2018-05-21 14:39:14 UTC
Created attachment 363850 [details] [review] Patch to assign groups to layouts in order. When layouts are set in gf_keyboard_manager_set_user_layouts, the LayoutInfo instances are first inserted in the hash table and then the table elements are iterated on and groups are assigned. This results in groups being assigned regardless of the order of user layouts, but rather in the order their names happen to be hashed. One case where this becomes a problem is with SDL which assumes the layout in Group1 is the main one, in order to generate keycodes. The attached patch fixes this by assigning groups while inserting the layouts in the hash table, with no need to iterate on the table entries afterwards.
Review of attachment 363850 [details] [review]: I tested this patch and I can confirm that my configuration (English and Russian keyboard layouts, switch using Super+Space, fast switch using CapsLock) works fine with it.
Review of attachment 363850 [details] [review]: Please update patch to not use tabs!
Tested the patch and it is crashing for me immediately with a segmentation fault. (gdb) backtrace
+ Trace 238625
(gdb) p *info $21 = {id = 0x56054f2243e0 "de", layout = 0x56054f12ee30 "de", variant = 0x56054f3fdce0 "", group = 0x56054f08bd70, index = 1} (gdb) p *info->group $22 = (LayoutInfo *) 0x56054f3cd850 (gdb) p *info->group[0] $23 = {id = 0x0, layout = 0x21 <error: Cannot access memory at address 0x21>, variant = 0x56054f3ce510 " ", group = 0x56054f08bd70, index = 0} (gdb) p *info->group[1] $24 = {id = 0x56054f08bd60 "", layout = 0x56054f40cad0 "\360\336?O\005V", variant = 0x0, group = 0x21, index = 1329585664} (gdb) p *info->group[2] $25 = {id = 0x56054f3fdcc0 "us", layout = 0x56054f3fde70 "us", variant = 0x56054f08bf90 "", group = 0x56054f08bd70, index = 2} I currently have the following layouts configured: 1. German 2. English (US) 3. Chinese (Intelligent Pinyin) 4. Chinese (SunPinyin) I cannot get it to crash with the released version.
Created attachment 372280 [details] [review] Patch to assign groups to layouts in order.
Okay, the crash was triggered by setups that happened to have twice the same layout in the set during gf_keyboard_manager_set_user_layouts. The new attached patch addresses that and uses spaces instead of tabs.
Did you attach old patch?
Also that crash is not caused by your patch, if you have fix then please as separate patch.
Created attachment 372281 [details] [review] Patch to assign groups to layouts in order.
I just uploaded the right patch. Sorry for the mess. The crash *was* caused by my previous patch because it did not exactly do what it said: if a layout is added twice, the second instance replaces the first in the hash table, and thus assigning layouts to groups as they are inserted will result in a layout in the group pointing to unallocated memory (the first instance that has been freed by the insertion of the second one). Without the previous patch, the layouts are assigned to groups by iterating over the entries in the hash table after all layouts have been inserted/replaced, so even if they are in incorrect order, no layout in the group points to unallocated memory. The new patch simply first checks whether the layout currently being added is already in the hash table. If so, only the layout name and variant name are replaced, (if, for whatever reason, they are different from the previous instance). Otherwise, it is assigned to the group and inserted in the hash table.
(In reply to Ignacy Gawędzki from comment #9) > I just uploaded the right patch. Sorry for the mess. > > The crash *was* caused by my previous patch because it did not exactly do > what it said: if a layout is added twice, the second instance replaces the > first in the hash table, and thus assigning layouts to groups as they are > inserted will result in a layout in the group pointing to unallocated memory > (the first instance that has been freed by the insertion of the second one). > Without the previous patch, the layouts are assigned to groups by iterating > over the entries in the hash table after all layouts have been > inserted/replaced, so even if they are in incorrect order, no layout in the > group points to unallocated memory. Right, but still it is not problem with your patch. :) It just helped to find bug. > The new patch simply first checks whether the layout currently being added > is already in the hash table. If so, only the layout name and variant name > are replaced, (if, for whatever reason, they are different from the previous > instance). Otherwise, it is assigned to the group and inserted in the hash > table. No, please patch without that. First GNOME Control Center does not add duplicates so I dont think we should worry about that. Even if we want then I think we should just skip next duplicate. if (info != NULL) continue; but then again it would just hide bug again... The problem here is that mentioned input sources are not duplicates and they all should work, nothing should be removed.
Review of attachment 372281 [details] [review]: ::: gnome-flashback/libinput-sources/gf-keyboard-manager.c @@ +566,3 @@ + info = (LayoutInfo *) g_hash_table_lookup (manager->layout_infos, + ids[i]); + if (info != NULL) empty space before this line. @@ +572,3 @@ + g_free (info->variant); + info->variant = g_strdup (variant); + continue; I think it enough to just use continue.For same id you will get same layout and variant so there is no need to free & g_strdup.
(In reply to Alberts Muktupāvels from comment #10) > (In reply to Ignacy Gawędzki from comment #9) > > I just uploaded the right patch. Sorry for the mess. > > > > The crash *was* caused by my previous patch because it did not exactly do > > what it said: if a layout is added twice, the second instance replaces the > > first in the hash table, and thus assigning layouts to groups as they are > > inserted will result in a layout in the group pointing to unallocated memory > > (the first instance that has been freed by the insertion of the second one). > > Without the previous patch, the layouts are assigned to groups by iterating > > over the entries in the hash table after all layouts have been > > inserted/replaced, so even if they are in incorrect order, no layout in the > > group points to unallocated memory. > > Right, but still it is not problem with your patch. :) It just helped to > find bug. Nevertheless, with that bug present, the first patch introduced a behavior that leads to a crash in some situations, which is obviously not desirable. My goal is to fix one aspect of the software that annoys me without breaking it for others. > > The new patch simply first checks whether the layout currently being added > > is already in the hash table. If so, only the layout name and variant name > > are replaced, (if, for whatever reason, they are different from the previous > > instance). Otherwise, it is assigned to the group and inserted in the hash > > table. > > No, please patch without that. First GNOME Control Center does not add > duplicates so I dont think we should worry about that. Even if we want then > I think we should just skip next duplicate. if (info != NULL) continue; but > then again it would just hide bug again... > > The problem here is that mentioned input sources are not duplicates and they > all should work, nothing should be removed. Fine then, I can simply skip duplicates.
Created attachment 372290 [details] [review] Patch to assign groups to layouts in order.
Review of attachment 372290 [details] [review]: Again wrong patch? ::: gnome-flashback/libinput-sources/gf-keyboard-manager.c @@ +573,3 @@ + info->variant = g_strdup (variant); + continue; + } if (g_hash_table_contains (manager->layout_infos, ids[i])) continue;
Created attachment 372291 [details] [review] Original Patch with Formatting fixes and contains check
(In reply to Alberts Muktupāvels from comment #15) > Review of attachment 372290 [details] [review] [review]: > > Again wrong patch? > > ::: gnome-flashback/libinput-sources/gf-keyboard-manager.c > @@ +573,3 @@ > + info->variant = g_strdup (variant); > + continue; > + } > > if (g_hash_table_contains (manager->layout_infos, ids[i])) > continue; Yeah, well... I notoriously forget to add -p to git amend. :/
Review of attachment 372291 [details] [review]: Look fine to me. Thanks for the correction.
Thanks! Sorry that this took so long...