Various KXkb improvements #304

Merged
blu.256 merged 3 commits from fix/kxkb-fixes into master 1 year ago
blu.256 commented 1 year ago
Collaborator

This PR includes several improvements and fixes for the TDE Keyboard Tool.

1) Kxkb: Improve Xkb option detection

This commit introduces a new Xkb option detection mechanism based on XML files (if present). On some systems the standard Xkb rule files are less complete than XML files in terms of rules (e.g. on openSUSE most layout switching options –the grp option group– are missing). It also includes a fix for some new keys like the "LSGT" key whose names are missing from option descriptions (again based on openSUSE, might be irrelevant for other distributions but shouldn't do any harm to keep).

i.e. if both /usr/share/X11/xkb/rules/evdev.lst and /usr/share/X11/xkb/rules/evdev.xml are present, the code will read options from the XML file, as it assumes that it's more complete.

2) Kxkb: improve layout switching

This commit replaces the TQt-based layout switching hotkey of KXkb in favour of honoring the relevant Xkb options. It is based on the "grp" options group of Xkb and so enables us to use predefined X11 layout (group) switching hotkeys like "Caps Lock" or "Shift+Alt" (you can see the full list in the Options tab). The added bonus to this is that we conform
to the Xkb settings.

Also this commit introduces a convenient and user-friendly dropdown menu (combobox) into the first tab of the Keyboard Layouts configuration dialog that presents to the user the most common switching hotkeys, as well as an "Other..." item that redirects them to the "Xkb Options" tab of the dialog.

3) Improve TDE Control Centre module

This commit mostly improves the handling of Xkb options. A lot of changes are related to the previous two commits.

For details see the descriptions of the relevant commits (in the Commits tab).

This PR includes several improvements and fixes for the TDE Keyboard Tool. ## 1) Kxkb: Improve Xkb option detection This commit introduces a new Xkb option detection mechanism based on XML files (if present). On some systems the standard Xkb rule files are less complete than XML files in terms of rules (e.g. on openSUSE most layout switching options –the `grp` option group– are missing). It also includes a fix for some new keys like the "LSGT" key whose names are missing from option descriptions (again based on openSUSE, might be irrelevant for other distributions but shouldn't do any harm to keep). i.e. if both `/usr/share/X11/xkb/rules/evdev.lst` and `/usr/share/X11/xkb/rules/evdev.xml` are present, the code will read options from the XML file, as it assumes that it's more complete. ## 2) Kxkb: improve layout switching This commit replaces the TQt-based layout switching hotkey of KXkb in favour of honoring the relevant Xkb options. It is based on the "grp" options group of Xkb and so enables us to use predefined X11 layout (group) switching hotkeys like "Caps Lock" or "Shift+Alt" (you can see the full list in the Options tab). The added bonus to this is that we conform to the Xkb settings. Also this commit introduces a convenient and user-friendly dropdown menu (combobox) into the first tab of the Keyboard Layouts configuration dialog that presents to the user the most common switching hotkeys, as well as an "Other..." item that redirects them to the "Xkb Options" tab of the dialog. ## 3) Improve TDE Control Centre module This commit mostly improves the handling of Xkb options. A lot of changes are related to the previous two commits. For details see the descriptions of the relevant commits (in the Commits tab).
blu.256 added the PR/rfc PR/wip labels 1 year ago
blu.256 added this to the R14.1.0 release milestone 1 year ago
blu.256 changed title from Various KXkb improvements to WIP: Various KXkb improvements 1 year ago
blu.256 force-pushed fix/kxkb-fixes from 6a422dade5 to 9285b9715e 1 year ago
blu.256 requested review from MicheleC 1 year ago
Owner

Hi Philippe,
I will test this PR in coming days and feedback as usual. Thanks for the good work.

Hi Philippe, I will test this PR in coming days and feedback as usual. Thanks for the good work.
blu.256 commented 1 year ago
Poster
Collaborator

Commit 2 should resolve Bugzilla bug 100. A fix to that issue was long overdue :)
https://bugs.trinitydesktop.org/show_bug.cgi?id=100

Also commits 2 and 3 should improve usability of Kxkb, as per issues described here:
https://marc.info/?l=kde-usability&m=117226628817401

So that people do not have to resort to Kkbswitch anymore (which was packaged as a workaround to this exact issues).

Commit 2 should resolve Bugzilla bug 100. A fix to that issue was long overdue :) https://bugs.trinitydesktop.org/show_bug.cgi?id=100 Also commits 2 and 3 should improve usability of Kxkb, as per issues described here: https://marc.info/?l=kde-usability&m=117226628817401 So that people do not have to resort to Kkbswitch anymore (which was packaged as a workaround to this exact issues).
blu.256 changed title from WIP: Various KXkb improvements to Various KXkb improvements 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

The last piece missing is somehow adding keyboard layout configuration in TDEPersonalizer. I think embedding the whole keyboard layout module would be overkill, so here's a more elegant solution:

In the first step users choose their country and language. We could use that language option to provide a default set of layouts (us and that language) and let the user select the key combination to change between these two layouts. The user can later refine these settings in the usual way.

For most users having these two layouts by default would be very handy and the key combination would be the only major thing they would otherwise modify.

What do you think?

The last piece missing is somehow adding keyboard layout configuration in TDEPersonalizer. I think embedding the whole keyboard layout module would be overkill, so here's a more elegant solution: In the first step users choose their country and language. We could use that language option to provide a default set of layouts (`us` and that language) and let the user select the key combination to change between these two layouts. The user can later refine these settings in the usual way. For most users having these two layouts by default would be very handy and the key combination would be the only major thing they would otherwise modify. What do you think?
Owner

For most users having these two layouts by default would be very handy and the key combination would be the only major thing they would otherwise modify.

Hi Philippe,
I may be wrong, but I think for most non-English users, only one keyboard layout would be the preference.
Setting the country and language in TDEPersonalizer is good, but setting two keyboard layout by default may be confusing?
Jyst my opinion, would be good to hear from others too. Or maybe you could ask on the user mailing list and see what the user preference is?

> For most users having these two layouts by default would be very handy and the key combination would be the only major thing they would otherwise modify. Hi Philippe, I may be wrong, but I think for most non-English users, only one keyboard layout would be the preference. Setting the country and language in TDEPersonalizer is good, but setting two keyboard layout by default may be confusing? Jyst my opinion, would be good to hear from others too. Or maybe you could ask on the user mailing list and see what the user preference is?
blu.256 commented 1 year ago
Poster
Collaborator

Only one layout? Which is the us layout, or their locale's one?

The aim of employing TDEPersonalizer for layouts is to make it convenient to set up on first start according to country. This is what the big OSes have been doing by default for as long as I rememer (e.g. Windows), so it shouldn't be confusing IMO. If.e.g. you set your country to Greece, the Greek layout is automatically added without user intervention.

But really I like the idea of asking the users, so this is what I'll do.

P.S. I'm still waiting to be (re-)subscribed to both tde-users and tde-devels lists. Who should I let know?

Only one layout? Which is the us layout, or their locale's one? The aim of employing TDEPersonalizer for layouts is to make it convenient to set up on first start according to country. This is what the big OSes have been doing by default for as long as I rememer (e.g. Windows), so it shouldn't be confusing IMO. If.e.g. you set your country to Greece, the Greek layout is automatically added without user intervention. But really I like the idea of asking the users, so this is what I'll do. P.S. I'm still waiting to be (re-)subscribed to both `tde-users` and `tde-devels` lists. Who should I let know?
blu.256 force-pushed fix/kxkb-fixes from 22667eb0d3 to faab642fe7 1 year ago
SlavekB commented 1 year ago
Owner

I also believe that many users will have an English keyboard layout set in addition to their national keyboard layout. Therefore, I understand that you want to simplify such settings for users.

I also believe that many users will have an English keyboard layout set in addition to their national keyboard layout. Therefore, I understand that you want to simplify such settings for users.
Owner

Only one layout? Which is the us layout, or their locale's one?

It would be the locale one, of course.

I also believe that many users will have an English keyboard layout set in addition to their national keyboard layout

Maybe, or may be not. For example in Italy you usually have the Italian keyboard and no US keyboard at all.

But really I like the idea of asking the users, so this is what I'll do.

I favor this idea. Maybe by default we set the local keyboard layout and then we give the user a choice to add more layouts or making changes 👍

> Only one layout? Which is the us layout, or their locale's one? It would be the locale one, of course. > I also believe that many users will have an English keyboard layout set in addition to their national keyboard layout Maybe, or may be not. For example in Italy you usually have the Italian keyboard and no US keyboard at all. > But really I like the idea of asking the users, so this is what I'll do. I favor this idea. Maybe by default we set the local keyboard layout and then we give the user a choice to add more layouts or making changes 👍
SlavekB commented 1 year ago
Owner

Personally, I also do not need to have more keyboard layouts, because everything needed I can write on the national keyboard layout. However, I see in my area (especially for more experienced users) that they often have an English keyboard layout in addition to the national layout. They use this especially when working in shell.

Personally, I also do not need to have more keyboard layouts, because everything needed I can write on the national keyboard layout. However, I see in my area (especially for more experienced users) that they often have an English keyboard layout in addition to the national layout. They use this especially when working in shell.
Collaborator

Only one layout? Which is the us layout, or their locale's one?

I am victim of bug#100, because when I switch to cyrillic the Alt+Shift+k does not work as "k" is not "к".

I have always left the Locale=C in Personalizer and then manually configured the KB layout adding at least 3 layouts. Then I modify the modifier to Alt+Shift+RightArrow
This works in all layouts and keyboards.

As I've been using the system this same way since 2003, I hope that I will be able to use it the same way in the future (if possible), but LeftAlt+Shift (the windows way) is also an option.

I hope this helps

BR

> Only one layout? Which is the us layout, or their locale's one? > I am victim of [bug#100](https://bugs.trinitydesktop.org/show_bug.cgi?id=100), because when I switch to cyrillic the Alt+Shift+k does not work as "k" is not "к". I have always left the Locale=C in Personalizer and then manually configured the KB layout adding at least 3 layouts. Then I modify the modifier to Alt+Shift+RightArrow This works in all layouts and keyboards. As I've been using the system this same way since 2003, I hope that I will be able to use it the same way in the future (if possible), but LeftAlt+Shift (the windows way) is also an option. I hope this helps BR
blu.256 commented 1 year ago
Poster
Collaborator

(Michele) For example in Italy you usually have the Italian keyboard and no US keyboard at all.

(Slávek) Personally, I also do not need to have more keyboard layouts, because everything needed I can write on the national keyboard layout.

@MicheleC @SlavekB I hadn't indeed considered that a lot of national layouts can be used in place of the US one, such as Italian. I was thinking more of cyrillic, greek and other "non-latin" layouts which cannot substitute the latin layout as they are totally different.

Maybe a sane default would be to only set two layouts for such languages which don't have a layout similar to US and not for the others?

(BR) I am victim of bug#100, because when I switch to cyrillic the Alt+Shift+k does not work as "k" is not "к". [...] As I've been using the system this same way since 2003, I hope that I will be able to use it the same way in the future (if possible), but LeftAlt+Shift (the windows way) is also an option.

@deloptes I think this PR solves bug#100. It makes Kxkb use standard Xkb layout switch keyboard combinations (by listening for ISO_Next_Group) instead of re-inventing the wheel. This does mean that the Alt+Shift+k hotkey (and Alt+Shift+RightArrow too), not being a standard X11 hotkey, cannot be used anymore, but Alt+Shift and all other standard Xkb layout switching hotkeys are an option indeed. If you want, you can test this PR and see if it fixes the problem.

> (Michele) For example in Italy you usually have the Italian keyboard and no US keyboard at all. > (Slávek) Personally, I also do not need to have more keyboard layouts, because everything needed I can write on the national keyboard layout. @MicheleC @SlavekB I hadn't indeed considered that a lot of national layouts can be used in place of the US one, such as Italian. I was thinking more of cyrillic, greek and other "non-latin" layouts which cannot substitute the latin layout as they are totally different. Maybe a sane default would be to only set two layouts for such languages which don't have a layout similar to US and not for the others? > (BR) I am victim of bug#100, because when I switch to cyrillic the Alt+Shift+k does not work as "k" is not "к". \[...] As I've been using the system this same way since 2003, I hope that I will be able to use it the same way in the future (if possible), but LeftAlt+Shift (the windows way) is also an option. @deloptes I think this PR solves bug#100. It makes Kxkb use standard Xkb layout switch keyboard combinations (by listening for ISO_Next_Group) instead of re-inventing the wheel. This does mean that the Alt+Shift+k hotkey (and Alt+Shift+RightArrow too), not being a standard X11 hotkey, cannot be used anymore, but Alt+Shift and all other standard Xkb layout switching hotkeys are an option indeed. If you want, you can test this PR and see if it fixes the problem.
blu.256 force-pushed fix/kxkb-fixes from faab642fe7 to 5ab6746f24 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

I have just sent an e-mail to the users mailing list asking for opinions on TDEPersonalizer and default keyboard layouts.

I have just sent an e-mail to the users mailing list asking for opinions on TDEPersonalizer and default keyboard layouts.
Owner

Maybe a sane default would be to only set two layouts for such languages which don't have a layout similar to US and not for the others?

Adding US layout by default as a second choice has two main problems:

  1. several westerner keyboard layouts don't need it
  2. some nationality may find this annoying (or offensive). Think for example of UK: they are proud of their nation, why would they need a US layout when they have already the UK layout?

So I think a sane default is to set the local keyboard layout only.
As a useful feature, we could have a checkbox in TDEPersonalizer that the user could tick to add US layout too. Or a list showing different layouts and then the user selects what to add.

Anyway I am going to test your code and feedback. I had issues with kxkb layout switching in the past, so I see this PR as a very useful thing.

> Maybe a sane default would be to only set two layouts for such languages which don't have a layout similar to US and not for the others? Adding US layout by default as a second choice has two main problems: 1. several westerner keyboard layouts don't need it 2. some nationality may find this annoying (or offensive). Think for example of UK: they are proud of their nation, why would they need a US layout when they have already the UK layout? So I think a sane default is to set the local keyboard layout only. As a useful feature, we could have a checkbox in TDEPersonalizer that the user could tick to add US layout too. Or a list showing different layouts and then the user selects what to add. Anyway I am going to test your code and feedback. I had issues with kxkb layout switching in the past, so I see this PR as a very useful thing.
Owner

I tested the PR on debian bookworm. Some feedback:

  1. the "Key combination to switch layout" in the "Layout" tab is nice, offers a quick way to select the most common choices. Selecting a value and clicking apply, does not update the "command" string in the "XKb options" tab. Closing the configuration page and reopening, shows the new selected value in the "XKb options" tab command string.

  2. Changing the "Key combination to switch layout" in the "Layout" tab should update the entry in the list in the "XKb options" tab if "Apply" is pressed. Currently the selection in the "XKb options" tab get changed even without pressing "Apply".

  3. the first change of key combination works. If I subsequently change the combination again, it does not seem to be immediately activated. I need to logout or reboot to get the new key combination to work, although I think a couple of times the new combination started working after a while (but I may be wrong here).

  4. Alt+Space does not seem to work here. Not sure if it is something specific to my system or a real issue.

  5. Clicking the label of an option in the "XKb options" tab list, makes the entry highlighted in the list, but the corresponding radio button remains unselected. This is annoying and different from the behavior of radio buttons in TQt/TDE. Clicking the label or the radio button should result in the same outcome.

I tested the PR on debian bookworm. Some feedback: 1. the "Key combination to switch layout" in the "Layout" tab is nice, offers a quick way to select the most common choices. Selecting a value and clicking apply, does not update the "command" string in the "XKb options" tab. Closing the configuration page and reopening, shows the new selected value in the "XKb options" tab command string. 2. Changing the "Key combination to switch layout" in the "Layout" tab should update the entry in the list in the "XKb options" tab if "Apply" is pressed. Currently the selection in the "XKb options" tab get changed even without pressing "Apply". 3. the first change of key combination works. If I subsequently change the combination again, it does not seem to be immediately activated. I need to logout or reboot to get the new key combination to work, although I think a couple of times the new combination started working after a while (but I may be wrong here). 4. Alt+Space does not seem to work here. Not sure if it is something specific to my system or a real issue. 5. Clicking the label of an option in the "XKb options" tab list, makes the entry highlighted in the list, but the corresponding radio button remains unselected. This is annoying and different from the behavior of radio buttons in TQt/TDE. Clicking the label or the radio button should result in the same outcome.
blu.256 commented 1 year ago
Poster
Collaborator

@MicheleC Thank you very much for testing!

1 and 2 should be easy to fix. Note for observation 2 that this works the other way round too -- that is, the entry in the "Xkb options" tab gets updated if you change "Key combination to switch layout" in the "Layout" tab without hitting "Apply", which should also be fixed.

I'm not yet sure on what causes 3 and 4, I can't reproduce the bugs on my system. It works okay for me.

About observation 5, this seems to be the default behaviour of TQCheckListItem. Check, for example, the second and third tabs in TDE Control > Desktop > Behaviour. It's indeed annoying and should be fixed in TQt.

Adding US layout by default as a second choice has two main problems:
several westerner keyboard layouts don't need it
some nationality may find this annoying (or offensive). Think for example of UK: they are proud of their nation, why would they need a US layout when they have already the UK layout?

So, I see the following possible solution variant:
If the selected language is not English (US, UK etc.), then show a checkbox "Enable latin keyboard layout" that can be enabled or disabled by the user. What do you think?

@MicheleC Thank you very much for testing! 1 and 2 should be easy to fix. Note for observation 2 that this works the other way round too -- that is, the entry in the "Xkb options" tab gets updated if you change "Key combination to switch layout" in the "Layout" tab without hitting "Apply", which should also be fixed. I'm not yet sure on what causes 3 and 4, I can't reproduce the bugs on my system. It works okay for me. About observation 5, this seems to be the default behaviour of TQCheckListItem. Check, for example, the second and third tabs in `TDE Control > Desktop > Behaviour`. It's indeed annoying and should be fixed in TQt. > Adding US layout by default as a second choice has two main problems: > several westerner keyboard layouts don't need it > some nationality may find this annoying (or offensive). Think for example of UK: they are proud of their nation, why would they need a US layout when they have already the UK layout? So, I see the following possible solution variant: If the selected language is not English (US, UK etc.), then show a checkbox "Enable latin keyboard layout" that can be enabled or disabled by the user. What do you think?
blu.256 changed title from Various KXkb improvements to WIP: Various KXkb improvements 1 year ago
SlavekB commented 1 year ago
Owner

Adding US layout by default as a second choice has two main problems:
several westerner keyboard layouts don't need it
some nationality may find this annoying (or offensive). Think for example of UK: they are proud of their nation, why would they need a US layout when they have already the UK layout?

So, I see the following possible solution variant:
If the selected language is not English (US, UK etc.), then show a checkbox "Enable latin keyboard layout" that can be enabled or disabled by the user. What do you think?

I like the solution with the adding of the checkbox to Enable English keyboard layout.

> > Adding US layout by default as a second choice has two main problems: > > several westerner keyboard layouts don't need it > > some nationality may find this annoying (or offensive). Think for example of UK: they are proud of their nation, why would they need a US layout when they have already the UK layout? > > So, I see the following possible solution variant: > If the selected language is not English (US, UK etc.), then show a checkbox "Enable latin keyboard layout" that can be enabled or disabled by the user. What do you think? I like the solution with the adding of the checkbox to Enable English keyboard layout.
Owner

I like the solution with the adding of the checkbox to Enable English keyboard layout.

Sounds good to me too

I'm not yet sure on what causes 3 and 4

@SlavekB are you able to test on your system? My system runs inside a VM, not sure this makes any difference although it shouldn't. I could test on a old laptop during the weekend perhaps, but not earlier

About observation 5, this seems to be the default behaviour of TQCheckListItem. Check, for example, the second and third tabs in TDE Control > Desktop > Behaviour

Not really, works as expected here. If I click the label of radio buttons or check boxes, their status change accordingly. Even TDE Control > Desktop > Behaviour.

> I like the solution with the adding of the checkbox to Enable English keyboard layout. Sounds good to me too > I'm not yet sure on what causes 3 and 4 @SlavekB are you able to test on your system? My system runs inside a VM, not sure this makes any difference although it shouldn't. I could test on a old laptop during the weekend perhaps, but not earlier > About observation 5, this seems to be the default behaviour of TQCheckListItem. Check, for example, the second and third tabs in TDE Control > Desktop > Behaviour Not really, works as expected here. If I click the label of radio buttons or check boxes, their status change accordingly. Even TDE Control > Desktop > Behaviour.
Owner

About observation 5, this seems to be the default behaviour of TQCheckListItem. Check, for example, the second and third tabs in TDE Control > Desktop > Behaviour

Not really, works as expected here. If I click the label of radio buttons or check boxes, their status change accordingly. Even TDE Control > Desktop > Behaviour.

I see now what you mean. Would definitely be good to fix it too.

>> About observation 5, this seems to be the default behaviour of TQCheckListItem. Check, for example, the second and third tabs in TDE Control > Desktop > Behaviour > Not really, works as expected here. If I click the label of radio buttons or check boxes, their status change accordingly. Even TDE Control > Desktop > Behaviour. I see now what you mean. Would definitely be good to fix it too.
blu.256 commented 1 year ago
Poster
Collaborator

1 and 2 should be easy to fix. Note for observation 2 that this works the other way round too -- that is, the entry in the "Xkb options" tab gets updated if you change "Key combination to switch layout" in the "Layout" tab without hitting "Apply", which should also be fixed.

Looks like it is a little more complex than I thought: right now the GUI entries are synchronized, so if one changes, the other one changes too -- note that the actual settings are not applied at this stage. But if we postpone the synchronization of these two options until "Apply" is pressed, then which one should take precedence? What if both change before "Apply" is pressed?

> 1 and 2 should be easy to fix. Note for observation 2 that this works the other way round too -- that is, the entry in the "Xkb options" tab gets updated if you change "Key combination to switch layout" in the "Layout" tab without hitting "Apply", which should also be fixed. Looks like it is a little more complex than I thought: right now the GUI entries are synchronized, so if one changes, the other one changes too -- note that the actual settings are not applied at this stage. But if we postpone the synchronization of these two options until "Apply" is pressed, then which one should take precedence? What if both change before "Apply" is pressed?
blu.256 commented 1 year ago
Poster
Collaborator

Because the TDEPersonalizer integration feature is not a blocker and somewhat out of scope of "various Kxkb improvements" I'm going to work on it in a separate pull request.

Because the TDEPersonalizer integration feature is not a blocker and somewhat out of scope of "various Kxkb improvements" I'm going to work on it in a separate pull request.
Owner

Because the TDEPersonalizer integration feature is not a blocker and somewhat out of scope of "various Kxkb improvements" I'm going to work on it in a separate pull request.

Sounds good.

> Because the TDEPersonalizer integration feature is not a blocker and somewhat out of scope of "various Kxkb improvements" I'm going to work on it in a separate pull request. Sounds good.
Owner

right now the GUI entries are synchronized

I did some tests on other config pages which uses tabs. It seems the default behavior is that "Apply" works "across multiple tabs". That is we are allowed to change tab without saving the settings and if we press "Apply" on a different tab, then also the changes on the first tab gets save.
In light of this, I thing we can ignore point 2. Only point 1. is left to clean up.

Regarding point 3. and 4. @SlavekB are you able to do a test on your computer too?

> right now the GUI entries are synchronized I did some tests on other config pages which uses tabs. It seems the default behavior is that "Apply" works "across multiple tabs". That is we are allowed to change tab without saving the settings and if we press "Apply" on a different tab, then also the changes on the first tab gets save. In light of this, I thing we can ignore point 2. Only point 1. is left to clean up. Regarding point 3. and 4. @SlavekB are you able to do a test on your computer too?
blu.256 commented 1 year ago
Poster
Collaborator

Point 1 resolved. Xkb command now updates automatically when selecting a new value.

Point 1 resolved. Xkb command now updates automatically when selecting a new value.
blu.256 changed title from WIP: Various KXkb improvements to Various KXkb improvements 1 year ago
blu.256 force-pushed fix/kxkb-fixes from edf0e25482 to b989ea9c57 1 year ago
Owner

Ok, will do another test and feedback.

Ok, will do another test and feedback.
Ray-V commented 1 year ago
Collaborator

I get the issues 3 & 4 reported by @MicheleC.
tdebase built to commit b989ea9c57 with commit edf0e25482 added as it didn't show in the worktree although present in the b989ea9c57 diff.

With no keyboard options set:

# setxkbmap -query | grep options
options:    terminate:ctrl_alt_bksp

Enable Alt+Space as the hotkey combination:
Layout|Enable keyboard layouts|Key combination to switch layout: Alt+Space
add a second layout|Apply
launches kxkb but Alt+Space doesn't work to switch layouts

Add Xkb Options|Enable xkb options|Apply
and Alt+Space does then work without logging out and back in.

# setxkbmap -query | grep options
options:    grp:alt_space_toggle,terminate:ctrl_alt_bksp

The following are randomly chosen changes of hotkey combinations.
Change hotkey option to Ctrl+Shift and that works

# setxkbmap -query | grep options
options:    grp:ctrl_shift_toggle,grp:alt_space_toggle,terminate:ctrl_alt_bksp

Change hotkey option to Ctrl+Space and that works, with Ctrl+Shift continuing to work.

# setxkbmap -query | grep options
options:    grp:ctrl_space_toggle,grp:ctrl_shift_toggle,grp:alt_space_toggle,terminate:ctrl_alt_bksp

Change hotkey option back to Alt+Space and that doesn't work but Ctrl+Shift and Ctrl+Space do.

# setxkbmap -query | grep options
options:    grp:alt_space_toggle,grp:ctrl_space_toggle,grp:ctrl_shift_toggle,grp:alt_space_toggle,terminate:ctrl_alt_bksp

Clear the setxkbmap options:

# setxkbmap -option
# setxkbmap -query | grep options
=> null

and re-apply the hotkey option to Alt+Space, and it works without logging out and back in:

# setxkbmap -query | grep options
options:    grp:alt_space_toggle

My guess is that certain combinations of the cumulative setxkbmap options for grp: confuse the hotkey function until it finally breaks.

However, resetting the setxkbmap options is not a complete fix because the terminate:ctrl_alt_bksp option, and any that the user might have set, has been lost.

I get the issues 3 & 4 reported by @MicheleC. tdebase built to commit b989ea9c57 with commit edf0e25482 added as it didn't show in the worktree although present in the b989ea9c57 diff. With no keyboard options set: ``` # setxkbmap -query | grep options options: terminate:ctrl_alt_bksp ``` Enable Alt+Space as the hotkey combination: Layout|Enable keyboard layouts|Key combination to switch layout: Alt+Space add a second layout|Apply launches kxkb but Alt+Space doesn't work to switch layouts Add Xkb Options|Enable xkb options|Apply and Alt+Space does then work without logging out and back in. ``` # setxkbmap -query | grep options options: grp:alt_space_toggle,terminate:ctrl_alt_bksp ``` The following are randomly chosen changes of hotkey combinations. Change hotkey option to Ctrl+Shift and that works ``` # setxkbmap -query | grep options options: grp:ctrl_shift_toggle,grp:alt_space_toggle,terminate:ctrl_alt_bksp ``` Change hotkey option to Ctrl+Space and that works, with Ctrl+Shift continuing to work. ``` # setxkbmap -query | grep options options: grp:ctrl_space_toggle,grp:ctrl_shift_toggle,grp:alt_space_toggle,terminate:ctrl_alt_bksp ``` Change hotkey option back to Alt+Space and that doesn't work but Ctrl+Shift and Ctrl+Space do. ``` # setxkbmap -query | grep options options: grp:alt_space_toggle,grp:ctrl_space_toggle,grp:ctrl_shift_toggle,grp:alt_space_toggle,terminate:ctrl_alt_bksp ``` Clear the setxkbmap options: ``` # setxkbmap -option # setxkbmap -query | grep options => null ``` and re-apply the hotkey option to Alt+Space, and it works without logging out and back in: ``` # setxkbmap -query | grep options options: grp:alt_space_toggle ``` My guess is that certain combinations of the cumulative setxkbmap options for grp: confuse the hotkey function until it finally breaks. However, resetting the setxkbmap options is not a complete fix because the terminate:ctrl_alt_bksp option, and any that the user might have set, has been lost.
Owner

I have done a new test with the latest code. With regards to the 5 points I initially raised:

  1. this is now ok

  2. as discuss, this is no longer required

  3. some key combinations seems to get active immediately, some others seems to require a logout

  4. any key combination with 'space' does not seem to work

  5. this is not an issue caused by this PR, rather something to be fixed in TQt3. Not a blocker.

The PR seems good, there are no blockers. One more small improvement would be to add 'None' as a standalone entry in the key combination combobox in the 'Layout' tab. Currently it is necessary to switch to the "XKb options" tab to disable the key shortcut, having "None" in the "Layout" tab would be convenient.

I have done a new test with the latest code. With regards to the 5 points I initially raised: 1. this is now ok 2. as discuss, this is no longer required 3. some key combinations seems to get active immediately, some others seems to require a logout 4. any key combination with 'space' does not seem to work 5. this is not an issue caused by this PR, rather something to be fixed in TQt3. Not a blocker. The PR seems good, there are no blockers. One more small improvement would be to add 'None' as a standalone entry in the key combination combobox in the 'Layout' tab. Currently it is necessary to switch to the "XKb options" tab to disable the key shortcut, having "None" in the "Layout" tab would be convenient.
Owner

@Ray-V thanks for testing and confirming the same issues. Following your report and analysis, I did further testing and I can confirm that when the selection key combination is changed, it gets added to the existings combinations rather than replacing them.
Clearing the options and set a new combination does work better.

However, resetting the setxkbmap options is not a complete fix because the terminate:ctrl_alt_bksp option, and any that the user might have set, has been lost.

Two points on this.

  1. most users will probably only use one key combination to change layout. So getting rid of the older selection and set the new one when the user presses "Apply" seems reasonable to me

  2. for users who wants to have multiple key combinations, would it make sense to change from radio buttons to checkboxes in the "XKb options" tab and allow multiple selections? This way single selection can be set from the "Layout" tab while more advanced users could use the "XKb options" tab for more choices. What do you think? @everyone here :-)

@Ray-V thanks for testing and confirming the same issues. Following your report and analysis, I did further testing and I can confirm that when the selection key combination is changed, it gets added to the existings combinations rather than replacing them. Clearing the options and set a new combination does work better. > However, resetting the setxkbmap options is not a complete fix because the terminate:ctrl_alt_bksp option, and any that the user might have set, has been lost. Two points on this. 1. most users will probably only use one key combination to change layout. So getting rid of the older selection and set the new one when the user presses "Apply" seems reasonable to me 2. for users who wants to have multiple key combinations, would it make sense to change from radio buttons to checkboxes in the "XKb options" tab and allow multiple selections? This way single selection can be set from the "Layout" tab while more advanced users could use the "XKb options" tab for more choices. What do you think? @everyone here :-)
blu.256 commented 1 year ago
Poster
Collaborator

@Ray-V @MicheleC Thanks for the insight on the bug. I can confirm it when the "Reset xkb options" checkbox is turned off.

The current code supports only one hotkey, and that's why I made layout selection to be radio buttons instead of checkboxes (yes, I cheated a bit :), because I'd have to meddle with more advanced X11-fu to find all the keycodes associated with the hotkey).

So maybe we need a way to reset just the grp: option. I'll look into this (and multiple layouts also).

any key combination with 'space' does not seem to work

Can you test with xev please? You set such a combination in the settings, then launch xev and press the combination. If all goes well you should see a keyboard event marked as ISO_Next_Group.

@Ray-V @MicheleC Thanks for the insight on the bug. I can confirm it when the "Reset xkb options" checkbox is turned off. The current code supports only one hotkey, and that's why I made layout selection to be radio buttons instead of checkboxes (yes, I cheated a bit :), because I'd have to meddle with more advanced X11-fu to find all the keycodes associated with the hotkey). So maybe we need a way to reset just the grp: option. I'll look into this (and multiple layouts also). > any key combination with 'space' does not seem to work Can you test with `xev` please? You set such a combination in the settings, then launch xev and press the combination. If all goes well you should see a keyboard event marked as ISO_Next_Group.
blu.256 changed title from Various KXkb improvements to WIP: Various KXkb improvements 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

update: I think I know.how to make multiple hotkeys work. I'll get to work it soon.

update: I think I know.how to make multiple hotkeys work. I'll get to work it soon.
Owner

Can you test with xev please?

With alt+shift I get the following. The 'FocusOut' event happens after I press Shift. The keyboard layout changes.

KeyPress event, serial 47, synthetic NO, window 0x5a00001,
    root 0x537, subw 0x0, time 13448111, (677,1127), root:(755,1155),
    state 0x2010, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
    XLookupString gives 0 bytes:
    XmbLookupString gives 0 bytes:
    XFilterEvent returns: False

FocusOut event, serial 47, synthetic NO, window 0x5a00001,
    mode NotifyGrab, detail NotifyAncestor

MappingNotify event, serial 47, synthetic NO, window 0x0,
    request MappingKeyboard, first_keycode 8, count 248

MappingNotify event, serial 47, synthetic NO, window 0x0,
    request MappingKeyboard, first_keycode 8, count 248

MappingNotify event, serial 47, synthetic NO, window 0x0,
    request MappingKeyboard, first_keycode 8, count 248

MappingNotify event, serial 47, synthetic NO, window 0x0,
    request MappingKeyboard, first_keycode 8, count 248

MappingNotify event, serial 47, synthetic NO, window 0x0,
    request MappingKeyboard, first_keycode 8, count 248

MappingNotify event, serial 47, synthetic NO, window 0x0,
    request MappingKeyboard, first_keycode 8, count 248

MappingNotify event, serial 47, synthetic NO, window 0x0,
    request MappingKeyboard, first_keycode 8, count 248

MappingNotify event, serial 47, synthetic NO, window 0x0,
    request MappingKeyboard, first_keycode 8, count 248

FocusIn event, serial 55, synthetic NO, window 0x5a00001,
    mode NotifyUngrab, detail NotifyAncestor

KeymapNotify event, serial 55, synthetic NO, window 0x0,
    keys:  1   0   0   0   0   0   0   0   1   0   0   0   0   0   0   0
           0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0

KeyRelease event, serial 55, synthetic NO, window 0x5a00001,
    root 0x537, subw 0x0, time 13448882, (677,1127), root:(755,1155),
    state 0x18, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
    XLookupString gives 0 bytes:
    XFilterEvent returns: False

With alt+space I get the following and the keyboard layout does not change.

KeyPress event, serial 39, synthetic NO, window 0x5a00001,
    root 0x537, subw 0x0, time 13599281, (1174,1071), root:(1252,1099),
    state 0x10, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
    XLookupString gives 0 bytes:
    XmbLookupString gives 0 bytes:
    XFilterEvent returns: False

KeyPress event, serial 39, synthetic NO, window 0x5a00001,
    root 0x537, subw 0x0, time 13600920, (1174,1071), root:(1252,1099),
    state 0x18, keycode 65 (keysym 0xfe08, ISO_Next_Group), same_screen YES,
    XLookupString gives 0 bytes:
    XmbLookupString gives 0 bytes:
    XFilterEvent returns: False

KeyRelease event, serial 39, synthetic NO, window 0x5a00001,
    root 0x537, subw 0x0, time 13601017, (1174,1071), root:(1252,1099),
    state 0x18, keycode 65 (keysym 0xfe08, ISO_Next_Group), same_screen YES,
    XLookupString gives 0 bytes:
    XFilterEvent returns: False

KeyRelease event, serial 39, synthetic NO, window 0x5a00001,
    root 0x537, subw 0x0, time 13601962, (1174,1071), root:(1252,1099),
    state 0x18, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
    XLookupString gives 0 bytes:
    XFilterEvent returns: False
> Can you test with xev please? With alt+shift I get the following. The 'FocusOut' event happens after I press Shift. The keyboard layout changes. ``` KeyPress event, serial 47, synthetic NO, window 0x5a00001, root 0x537, subw 0x0, time 13448111, (677,1127), root:(755,1155), state 0x2010, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES, XLookupString gives 0 bytes: XmbLookupString gives 0 bytes: XFilterEvent returns: False FocusOut event, serial 47, synthetic NO, window 0x5a00001, mode NotifyGrab, detail NotifyAncestor MappingNotify event, serial 47, synthetic NO, window 0x0, request MappingKeyboard, first_keycode 8, count 248 MappingNotify event, serial 47, synthetic NO, window 0x0, request MappingKeyboard, first_keycode 8, count 248 MappingNotify event, serial 47, synthetic NO, window 0x0, request MappingKeyboard, first_keycode 8, count 248 MappingNotify event, serial 47, synthetic NO, window 0x0, request MappingKeyboard, first_keycode 8, count 248 MappingNotify event, serial 47, synthetic NO, window 0x0, request MappingKeyboard, first_keycode 8, count 248 MappingNotify event, serial 47, synthetic NO, window 0x0, request MappingKeyboard, first_keycode 8, count 248 MappingNotify event, serial 47, synthetic NO, window 0x0, request MappingKeyboard, first_keycode 8, count 248 MappingNotify event, serial 47, synthetic NO, window 0x0, request MappingKeyboard, first_keycode 8, count 248 FocusIn event, serial 55, synthetic NO, window 0x5a00001, mode NotifyUngrab, detail NotifyAncestor KeymapNotify event, serial 55, synthetic NO, window 0x0, keys: 1 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 KeyRelease event, serial 55, synthetic NO, window 0x5a00001, root 0x537, subw 0x0, time 13448882, (677,1127), root:(755,1155), state 0x18, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES, XLookupString gives 0 bytes: XFilterEvent returns: False ``` With alt+space I get the following and the keyboard layout does not change. ``` KeyPress event, serial 39, synthetic NO, window 0x5a00001, root 0x537, subw 0x0, time 13599281, (1174,1071), root:(1252,1099), state 0x10, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES, XLookupString gives 0 bytes: XmbLookupString gives 0 bytes: XFilterEvent returns: False KeyPress event, serial 39, synthetic NO, window 0x5a00001, root 0x537, subw 0x0, time 13600920, (1174,1071), root:(1252,1099), state 0x18, keycode 65 (keysym 0xfe08, ISO_Next_Group), same_screen YES, XLookupString gives 0 bytes: XmbLookupString gives 0 bytes: XFilterEvent returns: False KeyRelease event, serial 39, synthetic NO, window 0x5a00001, root 0x537, subw 0x0, time 13601017, (1174,1071), root:(1252,1099), state 0x18, keycode 65 (keysym 0xfe08, ISO_Next_Group), same_screen YES, XLookupString gives 0 bytes: XFilterEvent returns: False KeyRelease event, serial 39, synthetic NO, window 0x5a00001, root 0x537, subw 0x0, time 13601962, (1174,1071), root:(1252,1099), state 0x18, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES, XLookupString gives 0 bytes: XFilterEvent returns: False ```
Owner

I can confirm it when the "Reset xkb options" checkbox is turned off.

Ah I now see what this checkbox does. It is indeed not very clear and replacing it with a multiselection list is definitely more intuitive for the user.

Note: with the checkbox on, setting None should remove all keyboard combinations, but it does not. The last one is still there.

> I can confirm it when the "Reset xkb options" checkbox is turned off. Ah I now see what this checkbox does. It is indeed not very clear and replacing it with a multiselection list is definitely more intuitive for the user. Note: with the checkbox on, setting None should remove all keyboard combinations, but it does not. The last one is still there.
blu.256 commented 1 year ago
Poster
Collaborator

update: I think I know.how to make multiple hotkeys work. I'll get to work it soon.

About that: the code compiles, but it can't reliably grab anything with modifier keys, rendering them useless. I think I've hit into an API/design limitation in this one.

All in all, grabbing a key/key combination seems unreliable (as in the case with Alt+Space which doesn't work everywhere, as it seems).

So, probably a better solution would be to delegate the switching to Xkb itself rather than try to intercept the ISO_Next_Group keysym. This means that the way KXkb works must be brought closer to how Kkbswitch and actual KDE keyboard switching actually work.

So, we'll only call "setxkbmap" to set/update layouts/variants/options (as usual), but not to switch the actual layout. Layout switching will be handled by the X server and we'll just listen for the appropriate X event. This also means that the "Enable latin layout" checkbox will be probably gone from the first tab of Keyboard Layout TDEConfig module (which AFAIK is an old hack that is absent from modern KDE).

I will try this in about two weeks due to university exams. I think that's a cleaner approach and it should solve switching issues such as 3 and 4.

> update: I think I know.how to make multiple hotkeys work. I'll get to work it soon. About that: the code compiles, but it can't reliably grab anything with modifier keys, rendering them useless. I think I've hit into an API/design limitation in this one. All in all, grabbing a key/key combination seems unreliable (as in the case with Alt+Space which doesn't work everywhere, as it seems). So, probably a better solution would be to delegate the switching to Xkb itself rather than try to intercept the ISO_Next_Group keysym. This means that the way KXkb works must be brought closer to how Kkbswitch and actual KDE keyboard switching actually work. So, we'll only call "setxkbmap" to set/update layouts/variants/options (as usual), but not to switch the actual layout. Layout switching will be handled by the X server and we'll just listen for the appropriate X event. This also means that the "Enable latin layout" checkbox will be probably gone from the first tab of Keyboard Layout TDEConfig module (which AFAIK is an old hack that is absent from modern KDE). I will try this in about two weeks due to university exams. I think that's a cleaner approach and it should solve switching issues such as 3 and 4.
Owner

@blu.256 sounds good, will be looking forward for the updates when ready. We still have about 3 months before releasing R14.1.0, so I think we can manage to squeeze this PR in as well, it would be a nice improvement for the users :-)

@blu.256 sounds good, will be looking forward for the updates when ready. We still have about 3 months before releasing R14.1.0, so I think we can manage to squeeze this PR in as well, it would be a nice improvement for the users :-)
blu.256 commented 1 year ago
Poster
Collaborator

I made the changes I mentioned in my last comment. I had to remove a lot of legacy code because it didn't work with the new logic, but it's pre-XFree-4.2 era (XFree 4.2 was released in 2002, and last release of XFree was in 2008), so there should not be any problems.

P.S. What got removed:

  • Precompiled layouts
  • "include groups" (actually only used on old systems which did not support single group layouts)
  • "Enable Latin layout" checkbox (an old hack that's no longer needed, I presume)
  • Old layout format support for XFree
  • "Enable xkb options" checkbox (always on because we rely on it to set the layout switching hotkey)
I made the changes I mentioned in my last comment. I had to remove a lot of legacy code because it didn't work with the new logic, but it's pre-XFree-4.2 era (XFree 4.2 was released in 2002, and last release of XFree was in 2008), so there should not be any problems. P.S. What got removed: - Precompiled layouts - "include groups" (actually only used on old systems which did not support single group layouts) - "Enable Latin layout" checkbox (an old hack that's no longer needed, I presume) - Old layout format support for XFree - "Enable xkb options" checkbox (always on because we rely on it to set the layout switching hotkey)
blu.256 requested review from SlavekB 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

This also means that we can support multiple hotkeys, but I'll need to make it work with the combobox in the first tab.

This also means that we can support multiple hotkeys, but I'll need to make it work with the combobox in the first tab.
Owner

Thanks for the update Philippe.

  1. should I do a test now or should I wait till you finish off the combobox code that you mentioned?

  2. I think the stuff you removed should be fine, although I reserve to comment again after a test. The only one where I am doubtful is "Enable Latin layout", but I would have to do some research about it

  3. I think we can also remove the checkbox "Reset old options" in the "XKb options" tab (if you haven't already). If we have multiple choices in the list, that should no longer be required since the setup should always correspond to the keys selected by the user.

Thanks for the update Philippe. 1. should I do a test now or should I wait till you finish off the combobox code that you mentioned? 2. I think the stuff you removed should be fine, although I reserve to comment again after a test. The only one where I am doubtful is "Enable Latin layout", but I would have to do some research about it 3. I think we can also remove the checkbox "Reset old options" in the "XKb options" tab (if you haven't already). If we have multiple choices in the list, that should no longer be required since the setup should always correspond to the keys selected by the user.
blu.256 commented 1 year ago
Poster
Collaborator

I just finished implementation of multiple hotkeys feature. The combobox <-> Xkb options synchronization part was not that hard to do, but I had to add some basic logic to check for conflicting hotkey options (e.g. a user selects both Win+Space and Alt+Space) based on my own testing. It might not be the best approach, but it seems to work well enough™.

With that I think the PR is ready for testing yet again :-)

I think we can also remove the checkbox "Reset old options" in the "XKb options" tab (if you haven't already). If we have multiple choices in the list, that should no longer be required since the setup should always correspond to the keys selected by the user.

I'm not totally sure about this. Somebody might be running a custom setxkbmap command on startup. Removing this option will overwrite their custom settings.

I just finished implementation of multiple hotkeys feature. The combobox <-> Xkb options synchronization part was not that hard to do, but I had to add some basic logic to check for conflicting hotkey options (e.g. a user selects both Win+Space and Alt+Space) based on my own testing. It might not be the best approach, but it seems to work well enough™. With that I think the PR is ready for testing yet again :-) > I think we can also remove the checkbox "Reset old options" in the "XKb options" tab (if you haven't already). If we have multiple choices in the list, that should no longer be required since the setup should always correspond to the keys selected by the user. I'm not totally sure about this. Somebody might be running a custom `setxkbmap` command on startup. Removing this option will overwrite their custom settings.
Owner

Hi Philippe,
I did a rasonable quick test. We are getting there but there are a few new points to address.

  1. in Layout tab, it would be good to add a "None" entry in the combobox, so the user can easily choose to disable any keyboard shortcut for switching layout. The "custom" entry is good and should stay, "none" is on top of that.

  2. in Switching Options tab, I see a big space between "Indicator style" and "Switching policy" radio groups. Would be good to shift the "Switching policy" group up

  3. in Xkb option tab, try selecting a keyboard shortcut. Command at the bottom gets updated (almost) correctly (there is a double "-option" which seems wrong to me). Then untick the option: the command does not update and show the old one. Select a second option, the command will display both the first and the second selection

  4. this point goes together with 3. IMO the "Reset old options" checkbox is unnecessary, because it should be that the command follows the selection given (0, one or more choices). If a user unticks all options, he would expect to see no keyboard shortcut selected and any previous choice to be removed.

  5. The check for keyboard shortcut conflicts seems to tight. For example "Win+space" and "Alt+space" should not give any warning because they do not conflict (they are different sequences). If a choice is a full subset of another option, then a warning should be given (for example "Caps Lock" and "Alt+Caps Lock")

I will do a deeper test once we have addressed these points. But I feel we are very close to getting this ready for merging.

Hi Philippe, I did a rasonable quick test. We are getting there but there are a few new points to address. 1. in Layout tab, it would be good to add a "None" entry in the combobox, so the user can easily choose to disable any keyboard shortcut for switching layout. The "custom" entry is good and should stay, "none" is on top of that. 2. in Switching Options tab, I see a big space between "Indicator style" and "Switching policy" radio groups. Would be good to shift the "Switching policy" group up 3. in Xkb option tab, try selecting a keyboard shortcut. Command at the bottom gets updated (almost) correctly (there is a double "-option" which seems wrong to me). Then untick the option: the command does not update and show the old one. Select a second option, the command will display both the first and the second selection 4. this point goes together with 3. IMO the "Reset old options" checkbox is unnecessary, because it should be that the command follows the selection given (0, one or more choices). If a user unticks all options, he would expect to see no keyboard shortcut selected and any previous choice to be removed. 5. The check for keyboard shortcut conflicts seems to tight. For example "Win+space" and "Alt+space" should not give any warning because they do not conflict (they are different sequences). If a choice is a *full subset* of another option, then a warning should be given (for example "Caps Lock" and "Alt+Caps Lock") I will do a deeper test once we have addressed these points. But I feel we are very close to getting this ready for merging.
Ray-V commented 1 year ago
Collaborator

IMO the "Reset old options" checkbox is unnecessary, because it should be that the command follows the selection given (0, one or more choices).

I think the opposite could be true.
The default behaviour of setxkbmap is to add new options to those existing.
If a user is familiar with using setxkbmap on the command line or through a script, then they would expect any new selections to be added to the options list.

(there is a double "-option" which seems wrong to me)

setxkbmap -option is the command to remove any existing options, so setxkbmap -option -option <new options> removes existing options and sets new options in the one command line.

Maybe there's room for a compromise here.
Enable the checkbox by default and rephrase it to something like "Clear existing options".
That way, experienced users will be forewarned, and for those depending on this gui, only the options selected will be set.

A right click on "Reset old options" brings up a "What's This?" box. So there's also the option of adding a more relevant explanation of this checkbox function in that message.

>IMO the "Reset old options" checkbox is unnecessary, because it should be that the command follows the selection given (0, one or more choices). I think the opposite could be true. The default behaviour of setxkbmap is to add new options to those existing. If a user is familiar with using setxkbmap on the command line or through a script, then they would expect any new selections to be added to the options list. >(there is a double "-option" which seems wrong to me) `setxkbmap -option` is the command to remove any existing options, so `setxkbmap -option -option <new options>` removes existing options and sets new options in the one command line. Maybe there's room for a compromise here. Enable the checkbox by default and rephrase it to something like "Clear existing options". That way, experienced users will be forewarned, and for those depending on this gui, only the options selected will be set. A right click on "Reset old options" brings up a "What's This?" box. So there's also the option of adding a more relevant explanation of this checkbox function in that message.
Owner

Hi @Ray-V
thanks for your feedback and insight.

Maybe there's room for a compromise here.
Enable the checkbox by default and rephrase it to something like "Clear existing options". That way, experienced users will be forewarned, and for those depending on this gui, only the options selected will be set.

This sounds like a good idea. Maybe we can even change the text of the checkbox depending on its state:

  • if checked: "Clear existing options and set new ones"
  • if unchecked: "Append new options to existing ones"

What do you think?

Hi @Ray-V thanks for your feedback and insight. > Maybe there's room for a compromise here. Enable the checkbox by default and rephrase it to something like "Clear existing options". That way, experienced users will be forewarned, and for those depending on this gui, only the options selected will be set. This sounds like a good idea. Maybe we can even change the text of the checkbox depending on its state: - if checked: "Clear existing options and set new ones" - if unchecked: "Append new options to existing ones" What do you think?
SlavekB commented 1 year ago
Owner

Hi @Ray-V
thanks for your feedback and insight.

Maybe there's room for a compromise here.
Enable the checkbox by default and rephrase it to something like "Clear existing options". That way, experienced users will be forewarned, and for those depending on this gui, only the options selected will be set.

This sounds like a good idea. Maybe we can even change the text of the checkbox depending on its state:

  • if checked: "Clear existing options and set new ones"
  • if unchecked: "Append new options to existing ones"

What do you think?

I do not know if I understand that the name of the switch would change according to the status of the switch? It looks like very confusing. If the user would read Append new options to existing ones and want such functionality, then – logically – would turn on the switch. And the result would be exactly the opposite and the description would suddenly be the opposite… And the confused user would turn off the switch again to read the changed description again and not understand what was going on.

Either I did not understand this proposal or I am confused now 😉

> Hi @Ray-V > thanks for your feedback and insight. > > > Maybe there's room for a compromise here. > Enable the checkbox by default and rephrase it to something like "Clear existing options". That way, experienced users will be forewarned, and for those depending on this gui, only the options selected will be set. > > This sounds like a good idea. Maybe we can even change the text of the checkbox depending on its state: > - if checked: "Clear existing options and set new ones" > - if unchecked: "Append new options to existing ones" > > What do you think? I do not know if I understand that the name of the switch would change according to the status of the switch? It looks like very confusing. If the user would read _Append new options to existing ones_ and want such functionality, then – logically – would turn on the switch. And the result would be exactly the opposite and the description would suddenly be the opposite… And the confused user would turn off the switch again to read the changed description again and not understand what was going on. Either I did not understand this proposal or I am confused now 😉
Owner

I do not know if I understand that the name of the switch would change according to the status of the switch? It looks like very confusing. If the user would read Append new options to existing ones and want such functionality, then – logically – would turn on the switch. And the result would be exactly the opposite and the description would suddenly be the opposite… And the confused user would turn off the switch again to read the changed description again and not understand what was going on.

Your understanding is correct and indeed my suggestion was not that smart. Maybe we have two radio buttons then, so the user can select either one or the other. That would be much clearer.

> I do not know if I understand that the name of the switch would change according to the status of the switch? It looks like very confusing. If the user would read Append new options to existing ones and want such functionality, then – logically – would turn on the switch. And the result would be exactly the opposite and the description would suddenly be the opposite… And the confused user would turn off the switch again to read the changed description again and not understand what was going on. Your understanding is correct and indeed my suggestion was not that smart. Maybe we have two radio buttons then, so the user can select either one or the other. That would be much clearer.
SlavekB commented 1 year ago
Owner

I do not know if I understand that the name of the switch would change according to the status of the switch? It looks like very confusing. If the user would read Append new options to existing ones and want such functionality, then – logically – would turn on the switch. And the result would be exactly the opposite and the description would suddenly be the opposite… And the confused user would turn off the switch again to read the changed description again and not understand what was going on.

Your understanding is correct and indeed my suggestion was not that smart. Maybe we have two radio buttons then, so the user can select either one or the other. That would be much clearer.

Yes, radio button is a good idea.

> > I do not know if I understand that the name of the switch would change according to the status of the switch? It looks like very confusing. If the user would read Append new options to existing ones and want such functionality, then – logically – would turn on the switch. And the result would be exactly the opposite and the description would suddenly be the opposite… And the confused user would turn off the switch again to read the changed description again and not understand what was going on. > > Your understanding is correct and indeed my suggestion was not that smart. Maybe we have two radio buttons then, so the user can select either one or the other. That would be much clearer. Yes, radio button is a good idea.
blu.256 commented 1 year ago
Poster
Collaborator

Thanks for the feedback, everyone!

I agree with the radio button idea, so we could have the user see two options, probably like this:

(o) Clear existing options
( ) Append new options to existing ones

Also, I'd place this at the bottom of the dialog instead of the top, below the option list, because it's an "advanced" feature and not the first thing the user should be meant to see.

I'll address this and the other remaining issues soon.

Thanks for the feedback, everyone! I agree with the radio button idea, so we could have the user see two options, probably like this: ``` (o) Clear existing options ( ) Append new options to existing ones ``` Also, I'd place this at the bottom of the dialog instead of the top, below the option list, because it's an "advanced" feature and not the first thing the user should be meant to see. I'll address this and the other remaining issues soon.
Owner

Also, I'd place this at the bottom of the dialog instead of the top, below the option list, because it's an "advanced" feature and not the first thing the user should be meant to see.

Near the Command line edit sounds like a good place.

> Also, I'd place this at the bottom of the dialog instead of the top, below the option list, because it's an "advanced" feature and not the first thing the user should be meant to see. Near the Command line edit sounds like a good place.
blu.256 commented 1 year ago
Poster
Collaborator

The check for keyboard shortcut conflicts seems to tight. For example "Win+space" and "Alt+space" should not give any warning because they do not conflict

@MicheleC Strangely, these two do conflict on my system. Alt+Space does not work as intended when Win+Space is also enabled. Have you actually tested these two options together? Could it be a local issue for me?

The conflict checking was mostly based on options that seemed not to work on my system, therefore I've tested them thoroughly already, but I'm not ruling out the possibility that a conflict manifests itself only on some and not all systems.

> The check for keyboard shortcut conflicts seems to tight. For example "Win+space" and "Alt+space" should not give any warning because they do not conflict @MicheleC Strangely, these two do conflict on my system. Alt+Space does not work as intended when Win+Space is also enabled. Have you actually tested these two options together? Could it be a local issue for me? The conflict checking was mostly based on options that seemed not to work on my system, therefore I've tested them thoroughly already, but I'm not ruling out the possibility that a conflict manifests itself only on some and not all systems.
Owner

@MicheleC Strangely, these two do conflict on my system. Alt+Space does not work as intended when Win+Space is also enabled. Have you actually tested these two options together? Could it be a local issue for me?

Hi Philippe,
I had not tested the two options, I was assuming that being different they would work. But I have now done a test and it seems when they are both enabled, they don't work very well. Not sure this is a problem of the underlying setxkbmap program or something we set wrong.

> @MicheleC Strangely, these two do conflict on my system. Alt+Space does not work as intended when Win+Space is also enabled. Have you actually tested these two options together? Could it be a local issue for me? Hi Philippe, I had not tested the two options, I was assuming that being different they would work. But I have now done a test and it seems when they are both enabled, they don't work very well. Not sure this is a problem of the underlying setxkbmap program or something we set wrong.
blu.256 commented 1 year ago
Poster
Collaborator

Not sure this is a problem of the underlying setxkbmap program or something we set wrong.

I think it's probably a Xkb (Xorg) issue. setxkbmap is a thin wrapper around Xkb, so I don't think the problem is there. Our invocation of setxkbmap also is correct. I don't think there's anything we can do to work around this. After all, it seems quite rare for somebody to use more than one hotkey to switch keyboard layout, so maybe it's not a critical issue? It looks unlikely to me that somebody might need to have both Alt+Space and Win+Space shortcuts for changing keyboard language. :-)

I've resolved issues 1, 3 and 4 and only remains issue 2 for me to fix now.

> Not sure this is a problem of the underlying setxkbmap program or something we set wrong. I think it's probably a Xkb (Xorg) issue. `setxkbmap` is a [thin wrapper](https://github.com/freedesktop/xorg-setxkbmap/blob/master/setxkbmap.c) around Xkb, so I don't think the problem is there. Our invocation of `setxkbmap` also is correct. I don't think there's anything we can do to work around this. After all, it seems quite rare for somebody to use more than one hotkey to switch keyboard layout, so maybe it's not a critical issue? It looks unlikely to me that somebody might need to have both Alt+Space and Win+Space shortcuts for changing keyboard language. :-) I've resolved issues 1, 3 and 4 and only remains issue 2 for me to fix now.
blu.256 commented 1 year ago
Poster
Collaborator

Issue 2 resolved. I decided to go a little further and moved some options into a new tab (the ones that affect the tray icon's style).

Issue 2 resolved. I decided to go a little further and moved some options into a new tab (the ones that affect the tray icon's style).
blu.256 commented 1 year ago
Poster
Collaborator

Please test the latest changes (KCMLayout UI changes, Keyboard switching hotkey "None" option).

Please test the latest changes (KCMLayout UI changes, Keyboard switching hotkey "None" option).
blu.256 removed the PR/rfc label 1 year ago
Owner

Hi Philippe,
I have done a new test. Looks quite good and I like the new tab and radio buttons. Some (minor?) things to adjust:

  1. if you select "Custom" on the "Layout" tab, you now end up in the wrong tab (Indicator options instead of XKb Options)

  2. In the "Indicator options" tab, the checkbox in "Miscellaneous" is cut off the dialog with the default dialog size. It is necessary to resize the dialog to see it. Maybe we can move the "Miscellaneous" part to the top of the dialog, to the right of "Indicator Style".
    Also the "Label font" edit field (the one containing the font name) seems excessively wide to me, but that may be perception.

  3. in "XKb options" tab, if we select "None" and "Overwrite existing options", shoudn't the Command edit field show something like "setxkbmap -option" to reset the current settings?

  4. still in the same tab, select a keyboard shortcut (say Alt+Shift) and deselect it. "None" is automatically selected and this is fine. switching to "Layout" tab shows "None" as choice, correct.
    But try to select a keyboard shortcut (say Alt+Shift) in in "XKb options" tab, then click "Switching to another layout" twice to deselect all choices. "None" is not selected and even worse, "Layout" tab shows the original selection (Alt+Shift). I think "None" should always be selected automatically if no other combination is selected and "Overwrite existing options" is selected at the same time.

  5. in "XKb options" tab, select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift). Verify the "Layout" tab shows the correct key.
    go back to in "XKb options" tab, select "Append to existing options", deselect the first choice and select a second different choice (for example Menu). Switch to "Layout" tab: instead of the expected "Multiple..." entry, the combobox only shows the second selected entry (Menu in this case), which is not correct. Then close the dialog with "Esc" and reopen it again; the combobox shows "Multiple (Menu, None)" and the "XKb options" tab also shows the same incorrect choice (None cannot be set if another choice was selected). Even more interesting: select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift), apply it and close the dialog. Open the dialog again and you will see the "Multiple (Alt+Shift, None)" thing again.
    This "Multiple" things seems to keep showing up afterwords, even if you select "None", apply, close the dialog, open it again, select a single different key and repeat

Hi Philippe, I have done a new test. Looks quite good and I like the new tab and radio buttons. Some (minor?) things to adjust: 1) if you select "Custom" on the "Layout" tab, you now end up in the wrong tab (Indicator options instead of XKb Options) 2) In the "Indicator options" tab, the checkbox in "Miscellaneous" is cut off the dialog with the default dialog size. It is necessary to resize the dialog to see it. Maybe we can move the "Miscellaneous" part to the top of the dialog, to the right of "Indicator Style". Also the "Label font" edit field (the one containing the font name) seems excessively wide to me, but that may be perception. 3) in "XKb options" tab, if we select "None" and "Overwrite existing options", shoudn't the Command edit field show something like "setxkbmap -option" to reset the current settings? 4) still in the same tab, select a keyboard shortcut (say Alt+Shift) and deselect it. "None" is automatically selected and this is fine. switching to "Layout" tab shows "None" as choice, correct. But try to select a keyboard shortcut (say Alt+Shift) in in "XKb options" tab, then click "Switching to another layout" twice to deselect all choices. "None" is not selected and even worse, "Layout" tab shows the original selection (Alt+Shift). I think "None" should always be selected automatically if no other combination is selected and "Overwrite existing options" is selected at the same time. 5) in "XKb options" tab, select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift). Verify the "Layout" tab shows the correct key. go back to in "XKb options" tab, select "Append to existing options", deselect the first choice and select a second different choice (for example Menu). Switch to "Layout" tab: instead of the expected "Multiple..." entry, the combobox only shows the second selected entry (Menu in this case), which is not correct. Then close the dialog with "Esc" and reopen it again; the combobox shows "Multiple (Menu, None)" and the "XKb options" tab also shows the same incorrect choice (None cannot be set if another choice was selected). Even more interesting: select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift), apply it and close the dialog. Open the dialog again and you will see the "Multiple (Alt+Shift, None)" thing again. This "Multiple" things seems to keep showing up afterwords, even if you select "None", apply, close the dialog, open it again, select a single different key and repeat
Ray-V commented 1 year ago
Collaborator

I'm having problems with this for a new installation.

Running kcontrol and Keyboard Layout within the build environment is ok, but setting up a new installation in a different [not quite identical] system has issues, so the problems might be in that setup. But I thought I'd mention it in case there is a problem with this PR for a new installation which might not have been checked by anyone else.

To master [commit 2ffa44200], Control Centre ... Keyboard Layout runs ok.

For all commits from 055bc3eb1 to 3faf5adb0, within Keyboard Layout, the Layout and Options windows are blank.
For all commits from 897b0068e to 58e31148a, selecting Keyboard Layout from the Control Centre crashes.

I'm having problems with this for a new installation. Running kcontrol and Keyboard Layout within the build environment is ok, but setting up a new installation in a different [not quite identical] system has issues, so the problems might be in that setup. But I thought I'd mention it in case there ***is*** a problem with this PR for a new installation which might not have been checked by anyone else. To master [commit 2ffa44200], Control Centre ... Keyboard Layout runs ok. For all commits from 055bc3eb1 to 3faf5adb0, within Keyboard Layout, the Layout and Options windows are blank. For all commits from 897b0068e to 58e31148a, selecting Keyboard Layout from the Control Centre crashes.
blu.256 commented 1 year ago
Poster
Collaborator

I'm having problems with this for a new installation.

Running kcontrol and Keyboard Layout within the build environment is ok, but setting up a new installation in a different [not quite identical] system has issues, so the problems might be in that setup. But I thought I'd mention it in case there is a problem with this PR for a new installation which might not have been checked by anyone else.

This sounds critical. Could you provide some details?

  • The distribution (incl. version)
  • The backtrace from the crash
  • What files exist under /usr/share/X11/xkb/rules/
> I'm having problems with this for a new installation. > > Running kcontrol and Keyboard Layout within the build environment is ok, but setting up a new installation in a different [not quite identical] system has issues, so the problems might be in that setup. But I thought I'd mention it in case there ***is*** a problem with this PR for a new installation which might not have been checked by anyone else. This sounds critical. Could you provide some details? * The distribution (incl. version) * The backtrace from the crash * What files exist under `/usr/share/X11/xkb/rules/`
blu.256 commented 1 year ago
Poster
Collaborator

still in the same tab, select a keyboard shortcut (say Alt+Shift) and deselect it. "None" is automatically selected and this is fine. switching to "Layout" tab shows "None" as choice, correct.
But try to select a keyboard shortcut (say Alt+Shift) in in "XKb options" tab, then click "Switching to another layout" twice to deselect all choices. "None" is not selected and even worse, "Layout" tab shows the original selection (Alt+Shift). I think "None" should always be selected automatically if no other combination is selected and "Overwrite existing options" is selected at the same time.

I think a checkable parent item is a very bad idea because it selects all options, and many of them are incompatible with each other.

> still in the same tab, select a keyboard shortcut (say Alt+Shift) and deselect it. "None" is automatically selected and this is fine. switching to "Layout" tab shows "None" as choice, correct. But try to select a keyboard shortcut (say Alt+Shift) in in "XKb options" tab, then click "Switching to another layout" twice to deselect all choices. "None" is not selected and even worse, "Layout" tab shows the original selection (Alt+Shift). I think "None" should always be selected automatically if no other combination is selected and "Overwrite existing options" is selected at the same time. I think a checkable parent item is a very bad idea because it selects all options, and many of them are incompatible with each other.
blu.256 commented 1 year ago
Poster
Collaborator

in "XKb options" tab, select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift). Verify the "Layout" tab shows the correct key.
go back to in "XKb options" tab, select "Append to existing options", deselect the first choice and select a second different choice (for example Menu). Switch to "Layout" tab: instead of the expected "Multiple..." entry, the combobox only shows the second selected entry (Menu in this case), which is not correct.

Well, we don't have a mechanism to determine existing Xkb options in place, only those currently selected in the dialog. I'm not sure how this should be implemented, to be honest :(

> in "XKb options" tab, select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift). Verify the "Layout" tab shows the correct key. go back to in "XKb options" tab, select "Append to existing options", deselect the first choice and select a second different choice (for example Menu). Switch to "Layout" tab: instead of the expected "Multiple..." entry, the combobox only shows the second selected entry (Menu in this case), which is not correct. Well, we don't have a mechanism to determine existing Xkb options in place, only those currently selected in the dialog. I'm not sure how this should be implemented, to be honest :(
blu.256 commented 1 year ago
Poster
Collaborator

the combobox shows "Multiple (Menu, None)" and the "XKb options" tab also shows the same incorrect choice (None cannot be set if another choice was selected). Even more interesting: select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift), apply it and close the dialog. Open the dialog again and you will see the "Multiple (Alt+Shift, None)" thing again.
This "Multiple" things seems to keep showing up afterwords, even if you select "None", apply, close the dialog, open it again, select a single different key and repeat

This was a bug due to incorrect initial value of the None option. Should be fixed with latest commit.

> the combobox shows "Multiple (Menu, None)" and the "XKb options" tab also shows the same incorrect choice (None cannot be set if another choice was selected). Even more interesting: select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift), apply it and close the dialog. Open the dialog again and you will see the "Multiple (Alt+Shift, None)" thing again. This "Multiple" things seems to keep showing up afterwords, even if you select "None", apply, close the dialog, open it again, select a single different key and repeat This was a bug due to incorrect initial value of the None option. Should be fixed with latest commit.
blu.256 force-pushed fix/kxkb-fixes from 58e31148a7 to 5bb462fc9f 1 year ago
Owner

@blu.256
I will be away till Wednesday. I will test again after that and feedback as usual.

@blu.256 I will be away till Wednesday. I will test again after that and feedback as usual.
Owner

I think a checkable parent item is a very bad idea because it selects all options, and many of them are incompatible with each other.

I agree. Currently the parent is checkable, so it would be better to either make it non-checkable or ignoring clicks on it.

> I think a checkable parent item is a very bad idea because it selects all options, and many of them are incompatible with each other. I agree. Currently the parent is checkable, so it would be better to either make it non-checkable or ignoring clicks on it.
Owner

Well, we don't have a mechanism to determine existing Xkb options in place, only those currently selected in the dialog. I'm not sure how this should be implemented, to be honest :(

Using setxkbmap -print gives some feedback about what the current settings are. Sure, it requires some parsing :-)

> Well, we don't have a mechanism to determine existing Xkb options in place, only those currently selected in the dialog. I'm not sure how this should be implemented, to be honest :( Using `setxkbmap -print` gives some feedback about what the current settings are. Sure, it requires some parsing :-)
Ray-V commented 1 year ago
Collaborator

@blu.256

Commit 5bb462fc9f has fixed the crash.

Thanks for the pointer to the rules files. The blank windows I was getting were due to a corrupted evdev.lst, so it's working now.

@blu.256 Commit 5bb462fc9f has fixed the crash. Thanks for the pointer to the rules files. The blank windows I was getting were due to a corrupted evdev.lst, so it's working now.
Ray-V commented 1 year ago
Collaborator

There is a lag in the variant showing in the Command box.

Set two active layouts and highlight one.
Change Layout variant and nothing changes in the Command setxkbmap line.
Click Apply and the new variant is applied and it does show in a konsole [setxkbmap -query], but still not in the Command box.
Select the other active layout and the variant is updated in the Command box.

There is a lag in the variant showing in the Command box. Set two active layouts and highlight one. Change Layout variant and nothing changes in the Command setxkbmap line. Click Apply and the new variant is applied and it does show in a konsole [setxkbmap -query], but still not in the Command box. Select the other active layout and the variant is updated in the Command box.
blu.256 commented 1 year ago
Poster
Collaborator

There is a lag in the variant showing in the Command box.

Set two active layouts and highlight one.
Change Layout variant and nothing changes in the Command setxkbmap line.
Click Apply and the new variant is applied and it does show in a konsole [setxkbmap -query], but still not in the Command box.
Select the other active layout and the variant is updated in the Command box.

Thank you for reporting this. Should work normally now.

> There is a lag in the variant showing in the Command box. > > Set two active layouts and highlight one. > Change Layout variant and nothing changes in the Command setxkbmap line. > Click Apply and the new variant is applied and it does show in a konsole [setxkbmap -query], but still not in the Command box. > Select the other active layout and the variant is updated in the Command box. > Thank you for reporting this. Should work normally now.
blu.256 commented 1 year ago
Poster
Collaborator

Using setxkbmap -print gives some feedback about what the current settings are. Sure, it requires some parsing :-)

Thank you for pointing me to setxkbmap. It seems relatively simple to hack on the relevant code so we won't need to parse anything.

There is a drawback, though. Consider this example: The user sets additional options via setxkbmap manually. Then kcmlayout is launched. Consider that the dialog can read current Xkb settings and displays the manually set options as being on. What happens if a user deselects one of these options? setxkbmap is append-only AFAICT, so we can't disable the option without overwriting everything. And if we do, what exactly is the point of append mode if it overwrites the settings?

I've been thinking and probably there is a better way to handle this instead of overwrite/append mode: If option changes are detected, prompt the user whether they want to update Kxkb's settings according to currently set options (doing it quietly is also an option, albeit an arguable one). In this way user changes can be preserved, we get rid of any additional complexities of append-mode and the process gets more user-friendly and straightforward.

I might be overthinking things, but we have to do it right, right? :-)

> Using setxkbmap -print gives some feedback about what the current settings are. Sure, it requires some parsing :-) Thank you for pointing me to setxkbmap. It seems relatively simple to hack on the relevant code so we won't need to parse anything. There is a drawback, though. *Consider this example*: The user sets additional options via setxkbmap manually. Then kcmlayout is launched. Consider that the dialog can read current Xkb settings and displays the manually set options as being on. What happens if a user deselects one of these options? `setxkbmap` is append-only AFAICT, so we can't disable the option without overwriting everything. And if we do, what exactly is the point of append mode if it overwrites the settings? I've been thinking and probably there is a better way to handle this instead of overwrite/append mode: If option changes are detected, prompt the user whether they want to update Kxkb's settings according to currently set options (doing it quietly is also an option, albeit an arguable one). In this way user changes can be preserved, we get rid of any additional complexities of append-mode and the process gets more user-friendly and straightforward. I might be overthinking things, but we have to do it right, right? :-)
Owner

There is a drawback, though. Consider this example: The user sets additional options via setxkbmap manually. Then kcmlayout is launched. Consider that the dialog can read current Xkb settings and displays the manually set options as being on. What happens if a user deselects one of these options? setxkbmap is append-only AFAICT, so we can't disable the option without overwriting everything. And if we do, what exactly is the point of append mode if it overwrites the settings?

The dialog offers the choice to append or overwrite.
Say the initial manual settings has key shortcut options A, B and C selected. Then the user open the dialogs and deselect option B. The result should be either one of the following two:

  1. user choose "overwrite options": setxkbmap should clear old options and set the new ones only. New working options should be A and C.

  2. user choose "append options": setxkbmap should not clear old options and append the new ones only. If an option was already set, appending it won't cause any trouble.

Basically the user should know how to use setxkbmap options, if he chooses to use the "XKb options" tab. Overwrite means discard anything and set the new ones. Append means keep the existing ones and add the new ones. In Append mode, only new options should be selected.

> There is a drawback, though. Consider this example: The user sets additional options via setxkbmap manually. Then kcmlayout is launched. Consider that the dialog can read current Xkb settings and displays the manually set options as being on. What happens if a user deselects one of these options? setxkbmap is append-only AFAICT, so we can't disable the option without overwriting everything. And if we do, what exactly is the point of append mode if it overwrites the settings? The dialog offers the choice to append or overwrite. Say the initial manual settings has key shortcut options A, B and C selected. Then the user open the dialogs and deselect option B. The result should be either one of the following two: 1. user choose "overwrite options": setxkbmap should clear old options and set the new ones only. New working options should be A and C. 2. user choose "append options": setxkbmap should not clear old options and append the new ones only. If an option was already set, appending it won't cause any trouble. Basically the user should know how to use setxkbmap options, if he chooses to use the "XKb options" tab. `Overwrite` means discard anything and set the new ones. `Append` means keep the existing ones and add the new ones. In `Append` mode, only new options should be selected.
blu.256 commented 1 year ago
Poster
Collaborator

Okay, so we're targeting the advanced user here. I'll implement the relevant parts some time this week.

Okay, so we're targeting the advanced user here. I'll implement the relevant parts some time this week.
Owner

@blu.256
a reminder that R14.1.0 soft freeze is planned for March 26. It would good to include this PR in R14.1.0: are you able to complete the required work before that?

@blu.256 a reminder that R14.1.0 soft freeze is planned for March 26. It would good to include this PR in R14.1.0: are you able to complete the required work before that?
blu.256 commented 1 year ago
Poster
Collaborator

@MicheleC Thank you for the reminder. I've been busy lately, but today I was able to do some development again. I've pushed a commit which seems to do what you described: it makes the hotkey combobox in the first tab consistently shows selected hotkeys even in Append Mode. I decided to make the combobox disabled in Append mode because a combobox choice assumes overwriting previous settings, hence Overwrite mode. Xkb Append mode is confusing to represent in a GUI. If you're okay with these changes and tests are okay, then, I think, the PR can be merged.

@MicheleC Thank you for the reminder. I've been busy lately, but today I was able to do some development again. I've pushed a commit which seems to do what you described: it makes the hotkey combobox in the first tab consistently shows selected hotkeys even in Append Mode. I decided to make the combobox disabled in Append mode because a combobox choice assumes overwriting previous settings, hence Overwrite mode. Xkb Append mode is confusing to represent in a GUI. If you're okay with these changes and tests are okay, then, I think, the PR can be merged.
Owner

Hi Philippe,
I tested the new code. With reference to my previous comments in here, items 1 to 4 are now working correctly, thanks for fixing them.
Item 5 still has issues and actually with the changes you described here it is not very user friendly.
Apologies for the long post, I will try to be as precise as I can.

IMO, the combobox in the Layout tab should provide an easy and quick way to set a single keyboard shortcut choice or display the current selection. This is what most users would use.
The XKb Options tab should be for advanced users and let them play with single or multiple options and in overwrite/append mode.
IMO, making the combobox disabled when "append" is selected in the XKb Option tab is not a good idea: the general user will have no idea why the combobox is disabled and what to do to reenable it.
The way I see it, things should more or less work as follow:

  1. in Layout tab, the combobox displays the current choice (either single or multiple). The choice could have been made here or in the Xkb Options tab or in an external script.

  2. If a user changes the selection in the combobox, this should be considered as "set the new option", therefore working in "overwrite" mode all the times and overriding any previous setting (remember this is supposed to be a quick and easy way to change the settings).

  3. if an advanced user wants to have a more complex configuration, he can go to the XKb Options tab and select single/multiple shortcuts and use overwrite/append mode.

So Layout tab is for basic user (--> easy but more limited), XKb Option tab for advanced users (--> more complex but more powerful).

Here is a way to test:
a. in "XKb options" tab, select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift). Verify the "Layout" tab shows the correct key.
b. go back to in "XKb options" tab, select "Append to existing options", deselect the first choice and select a second different choice (for example Menu). Verify that the command line shows the correct append command (it does already :-) )
c. switch to "Layout" tab: the combobox should show a "Multiple(...)" entry, but currentnly only shows the second selected entry (Menu in this case), which is not correct.
d. after fixing c. (i.e. the combobox shows Multiple...), select a difference choice in the combobox (for example Ctrl+Shift). Verify that in the Xkb Option tab, only Ctrl+Shift is selected and the mode is Overwrite.
e. Close the dialog. From command line, use setxkbmap to set a different choice (possibly even a multiple one). Reopen the dialog: the combobox in the Layout tab should show the selected choice ("Multiple..." in case that was set from command line).

This way the behavior is always consistent between the tabs and CLI (and so external scripts). Also the dialog provide both a quick-n-easy and and advanced selection method, which should cater for the needs of pretty much most users, while still handling any settings done by external scripts prior to opening the dialog.

What do you think? (also extended to other users for comments)

Hi Philippe, I tested the new code. With reference to my previous comments in [here](https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/pulls/304#issuecomment-23618), items 1 to 4 are now working correctly, thanks for fixing them. Item 5 still has issues and actually with the changes you described [here](https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/pulls/304#issuecomment-23978) it is not very user friendly. Apologies for the long post, I will try to be as precise as I can. IMO, the combobox in the Layout tab should provide an easy and quick way to set a single keyboard shortcut choice or display the current selection. This is what most users would use. The XKb Options tab should be for advanced users and let them play with single or multiple options and in overwrite/append mode. IMO, making the combobox disabled when "append" is selected in the XKb Option tab is not a good idea: the general user will have no idea why the combobox is disabled and what to do to reenable it. The way I see it, things should more or less work as follow: 1. in Layout tab, the combobox displays the current choice (either single or multiple). The choice could have been made here or in the Xkb Options tab or in an external script. 2. If a user changes the selection in the combobox, this should be considered as "set the new option", therefore working in "overwrite" mode all the times and overriding any previous setting (remember this is supposed to be a quick and easy way to change the settings). 3. if an advanced user wants to have a more complex configuration, he can go to the XKb Options tab and select single/multiple shortcuts and use overwrite/append mode. So Layout tab is for basic user (--> easy but more limited), XKb Option tab for advanced users (--> more complex but more powerful). Here is a way to test: a. in "XKb options" tab, select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift). Verify the "Layout" tab shows the correct key. b. go back to in "XKb options" tab, select "Append to existing options", deselect the first choice and select a second different choice (for example Menu). Verify that the command line shows the correct append command (it does already :-) ) c. switch to "Layout" tab: the combobox should show a "Multiple(...)" entry, but currentnly only shows the second selected entry (Menu in this case), which is not correct. d. after fixing c. (i.e. the combobox shows Multiple...), select a difference choice in the combobox (for example Ctrl+Shift). Verify that in the Xkb Option tab, only Ctrl+Shift is selected and the mode is Overwrite. e. Close the dialog. From command line, use setxkbmap to set a different choice (possibly even a multiple one). Reopen the dialog: the combobox in the Layout tab should show the selected choice ("Multiple..." in case that was set from command line). This way the behavior is always consistent between the tabs and CLI (and so external scripts). Also the dialog provide both a quick-n-easy and and advanced selection method, which should cater for the needs of pretty much most users, while still handling any settings done by external scripts prior to opening the dialog. What do you think? (also extended to other users for comments)
blu.256 commented 1 year ago
Poster
Collaborator

Thank you for the detailed write up. I'll test and try to make the needed modifications.

Thank you for the detailed write up. I'll test and try to make the needed modifications.
blu.256 commented 1 year ago
Poster
Collaborator

a. in "XKb options" tab, select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift). Verify the "Layout" tab shows the correct key.
b. go back to in "XKb options" tab, select "Append to existing options", deselect the first choice and select a second different choice (for example Menu). Verify that the command line shows the correct append command (it does already :-) )
c. switch to "Layout" tab: the combobox should show a "Multiple(...)" entry, but currentnly only shows the second selected entry (Menu in this case), which is not correct.

I have followed the steps exactly as written but I can't reproduce the problem. The combobox shows "Multiple (Alt+Shift; Menu)" as expected. In the code, each time the combobox text is updated, and if we're in Append mode, we first get the X server's options and then append checkbox items to them. What this means is that probably the TQString XKBExtension::getServerOptions() function I added for that purpose does not do its job.

Please follow these steps and check the debug output for Kxkb, and especially any line that begins with "[kxkb-extension] Got server options ". The line should contain a list of currently applied options. If it is empty, then at least we know the problem is there.

> a. in "XKb options" tab, select "Overwrite existing options" and a keyboard shortcut (say Alt+Shift). Verify the "Layout" tab shows the correct key. b. go back to in "XKb options" tab, select "Append to existing options", deselect the first choice and select a second different choice (for example Menu). Verify that the command line shows the correct append command (it does already :-) ) c. switch to "Layout" tab: the combobox should show a "Multiple(...)" entry, but currentnly only shows the second selected entry (Menu in this case), which is not correct. I have followed the steps exactly as written but I can't reproduce the problem. The combobox shows "Multiple (Alt+Shift; Menu)" as expected. In the code, each time the combobox text is updated, and if we're in Append mode, we first get the X server's options and then append checkbox items to them. What this means is that probably the `TQString XKBExtension::getServerOptions()` function I added for that purpose does not do its job. Please follow these steps and check the debug output for Kxkb, and especially any line that begins with `"[kxkb-extension] Got server options "`. The line should contain a list of currently applied options. If it is empty, then at least we know the problem is there.
blu.256 commented 1 year ago
Poster
Collaborator

So Layout tab is for basic user (--> easy but more limited), XKb Option tab for advanced users (--> more complex but more powerful).

So should the combobox automatically enable Xkb Overwrite mode? Or should it just enable the relevant checkbox in the Xkb Options tab and then display a "Multiple (...)" text with all set options, like it does (or should do, at least) when a hotkey checkbox is toggled on the Xkb Options tab in Append mode?

> So Layout tab is for basic user (--> easy but more limited), XKb Option tab for advanced users (--> more complex but more powerful). So should the combobox automatically enable Xkb Overwrite mode? Or should it just enable the relevant checkbox in the Xkb Options tab and then display a "Multiple (...)" text with all set options, like it does (or should do, at least) when a hotkey checkbox is toggled on the Xkb Options tab in Append mode?
Owner

I have followed the steps exactly as written but I can't reproduce the problem. The combobox shows "Multiple (Alt+Shift; Menu)" as expected.

Interesting, this took a while to figure out! I had to do a few tests to confirm the different behavior.

I initially did all the test steps without clicking the "Apply" button on the XKb Option tab. If you do that, you see the behavior I described in my previous comment.
I then tried again but clicking "Apply" at each step and I now see the "Multiple..." selection in the combo box as you described.
After thinking well about the two different behaviours, I realized my test was wrong. Not clicking "Apply" never enforced the first shortcut, so later when changing it using "Append" I was in fact appending the second choice to nothing. Hence I didn't see the "Multiple..." entry.
I have also verified that if I have an initial choice already set, the same procedure (without Apply) bring up the correct "Multiple" choice (initial + second shortcut).
So point c. in my previous comment was already doing the right thing and my test was incorrect. Sorry for the wrong reporting.

I could not test point d. due to the fact that the combobox was grayed out due to "Append".

So should the combobox automatically enable Xkb Overwrite mode?

  1. At first, the combobox should display whatever is the current choice.
  2. If the user makes some changes in the "XKb Option" tab without clicking Apply, the combobox should display what the outcome will be and the Apply button should be enabled.
  3. If the user makes some changes in the "XKb Option" tab and clicks Apply, the combobox should display the new choice and the Apply button should be grayed out.
  4. If the user selects a different entry in the combobox, it should be consider as if the user wants to set that option, so it would be the same as an overwrite operation from the "XKb Options".

I think the current code does pretty much everything already, with the only problem that the combobox is grayed out in append mode.
So it should be a small change and we should be good to go.

> I have followed the steps exactly as written but I can't reproduce the problem. The combobox shows "Multiple (Alt+Shift; Menu)" as expected. Interesting, this took a while to figure out! I had to do a few tests to confirm the different behavior. I initially did all the test steps without clicking the "Apply" button on the XKb Option tab. If you do that, you see the behavior I described in my previous comment. I then tried again but clicking "Apply" at each step and I now see the "Multiple..." selection in the combo box as you described. After thinking well about the two different behaviours, I realized my test was wrong. Not clicking "Apply" never enforced the first shortcut, so later when changing it using "Append" I was in fact appending the second choice to nothing. Hence I didn't see the "Multiple..." entry. I have also verified that if I have an initial choice already set, the same procedure (without Apply) bring up the correct "Multiple" choice (initial + second shortcut). So point c. in my previous comment was already doing the right thing and my test was incorrect. Sorry for the wrong reporting. I could not test point d. due to the fact that the combobox was grayed out due to "Append". > So should the combobox automatically enable Xkb Overwrite mode? 1. At first, the combobox should display whatever is the current choice. 2. If the user makes some changes in the "XKb Option" tab without clicking Apply, the combobox should display what the outcome will be and the Apply button should be enabled. 3. If the user makes some changes in the "XKb Option" tab and clicks Apply, the combobox should display the new choice and the Apply button should be grayed out. 4. If the user selects a different entry in the combobox, it should be consider as if the user wants to set that option, so it would be the same as an overwrite operation from the "XKb Options". I think the current code does pretty much everything already, with the only problem that the combobox is grayed out in append mode. So it should be a small change and we should be good to go.
blu.256 force-pushed fix/kxkb-fixes from 399953c272 to eb089b14e4 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

If the user selects a different entry in the combobox, it should be consider as if the user wants to set that option, so it would be the same as an overwrite operation from the "XKb Options".

An Overwrite operation would overwrite all previously set options regardless of Append mode. All previously set options would simply get lost. At this point I'm out of ideas and questioning the whole point of Append mode.

> If the user selects a different entry in the combobox, it should be consider as if the user wants to set that option, so it would be the same as an overwrite operation from the "XKb Options". An Overwrite operation would overwrite all previously set options regardless of Append mode. All previously set options would simply get lost. At this point I'm out of ideas and questioning the whole point of Append mode.
Owner

At this point I'm out of ideas and questioning the whole point of Append mode.

I agree with you, "Append" will be barely used. Probably only some very advanced users who are familiar with the way setxkbmap works.
I think most users will fall into one of the following 2 use cases:

  1. Simple usage
    Users go to "Layout" tab, select a keyboard shortcut (or "None"), click Apply/Ok and close the dialog. His expectation at that point is that the shortcut he chose is the one working, regardless of what was there before

  2. Complex usage
    An advanced user will go to "XKb Options" tab and does some advanced setup, like multishortcuts keys, overwrite/append and so on. He has full control of what he wants to do.

I expect case 1. to be by far the most common use case. Obviously if the combobox was grayed out, that would not be possible, hence my previous comment.

An alternative to "Append" was to have the GUI to present all the choices in XKb OPtions and then handle overwrite/append internally to add/remove user choices. We briefly discussed this idea at early stages, but the "append" behavior of setxkbmap was pointed out and I think we decided to go for it rather than the "all option GUI" choice.

> At this point I'm out of ideas and questioning the whole point of Append mode. I agree with you, "Append" will be barely used. Probably only some very advanced users who are familiar with the way setxkbmap works. I think most users will fall into one of the following 2 use cases: 1. Simple usage Users go to "Layout" tab, select a keyboard shortcut (or "None"), click Apply/Ok and close the dialog. His expectation at that point is that the shortcut he chose is the one working, regardless of what was there before 2. Complex usage An advanced user will go to "XKb Options" tab and does some advanced setup, like multishortcuts keys, overwrite/append and so on. He has full control of what he wants to do. I expect case 1. to be by far the most common use case. Obviously if the combobox was grayed out, that would not be possible, hence my previous comment. An alternative to "Append" was to have the GUI to present all the choices in XKb OPtions and then handle overwrite/append internally to add/remove user choices. We briefly discussed this idea at early stages, but the "append" behavior of setxkbmap was pointed out and I think we decided to go for it rather than the "all option GUI" choice.
Owner

Just did a new test. Combobox is now always available, which is good.
There is still a small thing that needs fixing, which is pretty much point d. in one of my previous comments.

Steps to reproduce:

  1. set a single choice for keyboard shortcut in XKb Options tab and click Apply
  2. select "Append", deselect first choice, select a different choice and click Apply again. Now two options are set.
  3. close and reopen the dialog (or simply switch to Layout tab :-) )
  4. combobox displays "Multiple..." and it is enabled, which is correct
  5. select a different choice in the combobox. Expected behavior: combobox shows only newly selected choice. Actual behavior: combobox displays "Multiple..." with 3 choices (the original 2 and the new one).

Basically what is missing is this: when the users selects something different in the combobox (and only then, not before), we set overwrite mode. This will provide the behavior of use case 1. in my previous comment.

Other than this, everything else seems to work very well!

Just did a new test. Combobox is now always available, which is good. There is still a small thing that needs fixing, which is pretty much point d. in one of my previous comments. Steps to reproduce: 1. set a single choice for keyboard shortcut in XKb Options tab and click Apply 2. select "Append", deselect first choice, select a different choice and click Apply again. Now two options are set. 3. close and reopen the dialog (or simply switch to Layout tab :-) ) 4. combobox displays "Multiple..." and it is enabled, which is correct 5. select a different choice in the combobox. Expected behavior: combobox shows only newly selected choice. Actual behavior: combobox displays "Multiple..." with 3 choices (the original 2 and the new one). Basically what is missing is this: when the users selects something different in the combobox (and **only** then, not before), we set overwrite mode. This will provide the behavior of use case 1. in my previous comment. Other than this, everything else seems to work very well!
MicheleC requested changes 1 year ago
MicheleC left a comment
Owner

Minor indentation fixes required.

Minor indentation fixes required.
const TQString& layout, const TQString& variant,
const TQString& includeGroup, bool useCompiledLayouts=true);
static bool setXkbOptions(const XkbOptions options);
static TQString getServerOptions();
Owner

wrong indentation in line 19

wrong indentation in line 19
blu.256 marked this conversation as resolved
}
}
updateHotkeyCombo();
Owner

wrong indentation line 424

wrong indentation line 424
blu.256 marked this conversation as resolved
public:
KXKBApp(bool allowStyles=true, bool GUIenabled=true);
~KXKBApp();
KXKBApp(bool allowStyles=true, bool GUIenabled=true);
Owner

Few lines have wrong indentation.

Few lines have wrong indentation.
blu.256 marked this conversation as resolved
void toggled();
void windowChanged(WId winId);
void layoutApply();
void slotGroupChanged(uint group);
Owner

Wrong indentation line 75

Wrong indentation line 75
blu.256 marked this conversation as resolved
kdDebug() << "map: Next layout: " << layoutQueue.head()->layoutUnit.toPair()
<< " group: " << layoutQueue.head()->layoutUnit.defaultGroup << " for " << m_currentWinId << endl;
<< " for " << m_currentWinId << endl;
Owner

Wrong indentation line 91 and 99, based on how you aligned previous kdWarning/kdDebug statements.

Wrong indentation line 91 and 99, based on how you aligned previous kdWarning/kdDebug statements.
blu.256 marked this conversation as resolved
blu.256 commented 1 year ago
Poster
Collaborator

Basically what is missing is this: when the users selects something different in the combobox (and only then, not before), we set overwrite mode.

But then we would lose all other options except for those currently in the dialog. Is that really what we want?

> Basically what is missing is this: when the users selects something different in the combobox (and only then, not before), we set overwrite mode. But then we would lose all other options except for those currently in the dialog. Is that really what we want?
blu.256 commented 1 year ago
Poster
Collaborator

Indentation fixed :-)

Indentation fixed :-)
blu.256 commented 1 year ago
Poster
Collaborator

I'll take a look into setxkbmap and see if I can find a solution that won't reset other options except from those that we need to.

So, as I see it, we just call setxkbmap with all currently set options except the ones we have unchecked in the dialog.

~~I'll take a look into setxkbmap and see if I can find a solution that won't reset other options except from those that we need to.~~ So, as I see it, we just call setxkbmap with all currently set options except the ones we have unchecked in the dialog.
blu.256 force-pushed fix/kxkb-fixes from 6b1c80d7c3 to 5105e5f47c 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

I think I found the best possible solution. I've updated the code so that in Append mode, before the normal setxkbmap call happens, there is another one which, in effect, unsets all options that are no longer checked in Kxkb. This approach works not only with the hotkeys combobox, but with all options in the Xkb options tab, and works as a user should expect. Please do a test by following the same steps as before, and also in the Xkb Options tab by checking any option and hitting Apply and then unchecking that option and hitting Apply again.

I think I found the best possible solution. I've updated the code so that in Append mode, before the normal `setxkbmap` call happens, there is another one which, in effect, unsets all options that are no longer checked in Kxkb. This approach works not only with the hotkeys combobox, but with all options in the Xkb options tab, and works as a user should expect. Please do a test by following the same steps as before, and also in the Xkb Options tab by checking any option and hitting Apply and then unchecking that option and hitting Apply again.
blu.256 requested review from MicheleC 1 year ago
Owner

Thanks for the various comments Philippe, you raised some good points.

I have tested the latest version, but unfortunately it seems there are some regressions.

Test steps:

  1. start with a single choice selected and set from XKb Options tab in overwrite mode. The layout page shows the correct entry in the combobox (for example Alt+Shift)
  2. in Layout tab combobox, select a different choice (for example Menu) and Apply.
  3. Go to XKBOptions tab, select Append, deselect Menu (which was selected because of step 2.), select another choice (Ctrl+Shift) and Apply. Go to Layout tab: because of Append, we should see "Multiple (Menu, Ctrl+Shift) but the combobox only shows Ctrl+Shift. This seems a regression from the previous test.
  4. Go to XKbOptions tab, select overwrite, select only "None" as keyboard shortcut, Apply. Go to Layout tab, the combobox still shows "Ctrl+Shift" instead of None, even if we used the overwrite mode.

I think the previous iteration (the one I tested yesterday) was closer to what we wanted to achieve.

But then we would lose all other options except for those currently in the dialog. Is that really what we want?

I think the best solution would be as follow:

  1. in XKbOptions tab, overwrite/append whatever the user chooses. He has full control of the options. Yesterday code was doing this correctly, while today's code seems to have some regressions as explained above
  2. in Layout tab, if the user chooses a different shortcut from the combobox, set the new shortcut in overwrite mode but keep other options not related to shortcut choice. Basically it would be something like this:
    a. read all current options
    b. remove all existing shortcuts
    c. add the new shortcut (selected from the combobox) to the remaining options
    d. set all the options in overwrite mode.
    This would preserve all the non-shortcut options and set the single shortcut set by the user.
    I don't know how difficult it would be to implement this, what do you think? Would it be feasible? You may want to consider discarding the last commit since the previous iteration was doing most of the things correctly.
Thanks for the various comments Philippe, you raised some good points. I have tested the latest version, but unfortunately it seems there are some regressions. Test steps: 1. start with a single choice selected and set from XKb Options tab in overwrite mode. The layout page shows the correct entry in the combobox (for example Alt+Shift) 2. in Layout tab combobox, select a different choice (for example Menu) and Apply. 3. Go to XKBOptions tab, select Append, deselect Menu (which was selected because of step 2.), select another choice (Ctrl+Shift) and Apply. Go to Layout tab: because of Append, we should see "Multiple (Menu, Ctrl+Shift) but the combobox only shows Ctrl+Shift. This seems a regression from the previous test. 4. Go to XKbOptions tab, select overwrite, select only "None" as keyboard shortcut, Apply. Go to Layout tab, the combobox still shows "Ctrl+Shift" instead of None, even if we used the overwrite mode. I think the previous iteration (the one I tested yesterday) was closer to what we wanted to achieve. > But then we would lose all other options except for those currently in the dialog. Is that really what we want? I think the best solution would be as follow: 1. in XKbOptions tab, overwrite/append whatever the user chooses. He has full control of the options. Yesterday code was doing this correctly, while today's code seems to have some regressions as explained above 2. in Layout tab, if the user chooses a different shortcut from the combobox, set the new shortcut in overwrite mode but keep other options not related to shortcut choice. Basically it would be something like this: a. read all current options b. remove all existing shortcuts c. add the new shortcut (selected from the combobox) to the remaining options d. set all the options in overwrite mode. This would preserve all the non-shortcut options and set the single shortcut set by the user. I don't know how difficult it would be to implement this, what do you think? Would it be feasible? You may want to consider discarding the last commit since the previous iteration was doing most of the things correctly.
Owner

One more test step: go to Layout tab where some previous choice should be shown in the combobox. Select None in the combobox and Apply. The combobox changes back to the initial choice, making it impossible to select and apply None.

One more test step: go to Layout tab where some previous choice should be shown in the combobox. Select None in the combobox and Apply. The combobox changes back to the initial choice, making it impossible to select and apply None.
blu.256 commented 1 year ago
Poster
Collaborator

deselect Menu

because of Append, we should see "Multiple (Menu, Ctrl+Shift) but the combobox only shows Ctrl+Shift. This seems a regression from the previous test.

I considered this to be the expected behaviour since on next login only the last iteration of selected options gets appended to the environment's existing options (Menu was deselected so it won't be present in Kxkb's configuration and won't be applied on next login). The purpose of Append mode in Kxkb, as I see it, is to prevent overwriting options that are set externally. This shouldn't apply to options set through Kxkb because it would make things confusing. The unchecked options gets removed intentionally since they won't be set by Kxkb on next boot. This behaviour is intended to mimick the behaviour of the system after reboot.

Go to XKbOptions tab, select overwrite, select only "None" as keyboard shortcut, Apply. Go to Layout tab, the combobox still shows "Ctrl+Shift" instead of None, even if we used the overwrite mode.

One more test step: go to Layout tab where some previous choice should be shown in the combobox. Select None in the combobox and Apply. The combobox changes back to the initial choice, making it impossible to select and apply None.

I can't seem to be able to reproduce either of this behaviour. :(

I think the best solution would be as follow: in XKbOptions tab, overwrite/append whatever the user chooses. He has full control of the options.

I understand your reasoning, but I raise this point once again: what happens if the user unchecks an option in Append mode? If it is unchecked, then it doesn't get added to the Kxkb configuration, so it will not be set on the next login/boot. Is this the behaviour we want? I find it to be somewhat unintuitive from a user interface point of view. The user either a) sees an unchecked checkbox and expects the option to have been unset or b) if they see that the option has been preserved they might expect it to be persistent, that is, present on the next login. If we unset it, then we go with variant (a) and we don't make the user assume anything about the behaviour of the configuration, i.e. "what [options] you see is what you get". This can apply to all options in the Xkb options tab. Is it only I that find the alternative confusing?

I don't know how difficult it would be to implement this, what do you think? Would it be feasible? You may want to consider discarding the last commit since the previous iteration was doing most of the things correctly.

Currently I'll revert my previous commit and implement the behaviour you are asking for but please do consider the point I made above. :)

> deselect Menu > because of Append, we should see "Multiple (Menu, Ctrl+Shift) but the combobox only shows Ctrl+Shift. This seems a regression from the previous test. I considered this to be the expected behaviour since on next login only the last iteration of selected options gets appended to the environment's existing options (Menu was deselected so it won't be present in Kxkb's configuration and won't be applied on next login). The purpose of Append mode in Kxkb, as I see it, is to prevent overwriting options that are set externally. This shouldn't apply to options set through Kxkb because it would make things confusing. The unchecked options gets removed intentionally since they won't be set by Kxkb on next boot. This behaviour is intended to mimick the behaviour of the system after reboot. > Go to XKbOptions tab, select overwrite, select only "None" as keyboard shortcut, Apply. Go to Layout tab, the combobox still shows "Ctrl+Shift" instead of None, even if we used the overwrite mode. > One more test step: go to Layout tab where some previous choice should be shown in the combobox. Select None in the combobox and Apply. The combobox changes back to the initial choice, making it impossible to select and apply None. I can't seem to be able to reproduce either of this behaviour. :( > I think the best solution would be as follow: in XKbOptions tab, overwrite/append whatever the user chooses. He has full control of the options. I understand your reasoning, but I raise this point once again: what happens if the user unchecks an option in Append mode? If it is unchecked, then it doesn't get added to the Kxkb configuration, so it will **not** be set on the next login/boot. Is this the behaviour we want? I find it to be somewhat unintuitive from a user interface point of view. The user either a) sees an unchecked checkbox and expects the option to have been unset or b) if they see that the option has been preserved they might expect it to be persistent, that is, present on the next login. If we unset it, then we go with variant (a) and we don't make the user assume anything about the behaviour of the configuration, i.e. "what \[options\] you see is what you get". This can apply to all options in the Xkb options tab. Is it only I that find the alternative confusing? > I don't know how difficult it would be to implement this, what do you think? Would it be feasible? You may want to consider discarding the last commit since the previous iteration was doing most of the things correctly. Currently I'll revert my previous commit and implement the behaviour you are asking for but please do consider the point I made above. :)
blu.256 commented 1 year ago
Poster
Collaborator

I've implemented the new behaviour. It seems to work as you described.

The process is this:

  1. The user selects a hotkey from the combobox. We set the boolean variable m_forceGrpOverwrite to true (by default it is false).
  2. The user presses apply. If m_forceGrpOverwrite is true then we:
    a. read all current options from Xkb
    b. remove all existing shortcuts while leaving all other options
    c. run setxkbmap in overwrite mode
    d. leave the rest (setting the new options) to Kxkb
  3. We reset m_forceGrpOverwrite back o false
I've implemented the new behaviour. It seems to work as you described. The process is this: 1. The user selects a hotkey from the combobox. We set the boolean variable `m_forceGrpOverwrite` to **true** (by default it is **false**). 2. The user presses apply. If `m_forceGrpOverwrite` is **true** then we: a. read all current options from Xkb b. remove all existing shortcuts while leaving all other options c. run setxkbmap in overwrite mode d. leave the rest (setting the new options) to Kxkb 3. We reset `m_forceGrpOverwrite` back o **false**
Owner

I understand your reasoning...

These are very valid comments.
If we didn't have "append" mode, your comment would be 100% correct and what you described would be the expected behavior. The GUI would show all available options and when a user unselect one, he would expect it to be off after the next reboot.
But if the GUI was missing an option, this would be lost when changing any other options,hence the need for the "append" mode, as also raised by Ray and Emanoil.

I considered this to be...

It could be a matter of different opinions, but as a user if I "append" something, I think of it as to "add to the existing config" and make it the new one. After the next reboot, I would expect the last config set to be there.

Personally, I prefer a solution without "append", keeps things easier and cleaner IMO. But the observations raised by Ray and Emanoil make sense and I do understand the need for "append" mode.

My detailed feedback may feel frustrating, I know.
I am trying to test and keep things consistent, so that a user without the knowledge of this PR discussion won't be puzzled by the behavior he encounters. The basic rules are:

  1. overwrite mode: discard any previous option and set the new ones as new config
  2. append mode: add to existing options and make it the new config.

IMO, these 2 rules are quite reasonable, but happy to hear other opinions too.

> I understand your reasoning... These are very valid comments. If we didn't have "append" mode, your comment would be 100% correct and what you described would be the expected behavior. The GUI would show all available options and when a user unselect one, he would expect it to be off after the next reboot. But if the GUI was missing an option, this would be lost when changing any other options,hence the need for the "append" mode, as also raised by Ray and Emanoil. > I considered this to be... It could be a matter of different opinions, but as a user if I "append" something, I think of it as to "add to the existing config" and make it the new one. After the next reboot, I would expect the last config set to be there. Personally, I prefer a solution without "append", keeps things easier and cleaner IMO. But the observations raised by Ray and Emanoil make sense and I do understand the need for "append" mode. My detailed feedback may feel frustrating, I know. I am trying to test and keep things consistent, so that a user without the knowledge of this PR discussion won't be puzzled by the behavior he encounters. The basic rules are: 1. overwrite mode: discard any previous option and set the new ones as new config 2. append mode: add to existing options and make it the new config. IMO, these 2 rules are quite reasonable, but happy to hear other opinions too.
Owner

I tested the latest version and we are 99% there. There is still one thing to fix though.

With reference to the test steps in this comment, points 1. to 3. work perfectly. Point 4. works fine in the GUI (i.e. the GUI shows "None") but the keyboard shortcut is actually not set: setxkbmap -query still shows the selection set at point 3.
As further step 5. try this: go back to XKBOptions tab, select Append, select another unset choice and Apply. Go to Layout tab: you will see the two choices set at step 3. + the new choice set now, even though at step 4. we set None in overwrite mode.
Seems to me we are just missing the call to setxkbmap at step 4., since the GUI is indeed doing the right thing now.

With reference to the test step in this comment: selecting None from the combobox in Layout tab works fine.

I tested the latest version and we are 99% there. There is still one thing to fix though. With reference to the test steps in [this](https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/pulls/304#issuecomment-24055) comment, points 1. to 3. work perfectly. Point 4. works fine in the GUI (i.e. the GUI shows "None") but the keyboard shortcut is actually not set: `setxkbmap -query` still shows the selection set at point 3. As further step 5. try this: go back to XKBOptions tab, select Append, select another unset choice and Apply. Go to Layout tab: you will see the two choices set at step 3. + the new choice set now, even though at step 4. we set `None` in overwrite mode. Seems to me we are just missing the call to `setxkbmap` at step 4., since the GUI is indeed doing the right thing now. With reference to the test step in [this](https://mirror.git.trinitydesktop.org/gitea/TDE/tdebase/pulls/304#issuecomment-24056) comment: selecting `None` from the combobox in Layout tab works fine.
blu.256 commented 1 year ago
Poster
Collaborator

Thank you once again for the detailed feedback! I will look into this (hopefully last) issue tomorrow (22/03) and report back.

Thank you once again for the detailed feedback! I will look into this (hopefully last) issue tomorrow (22/03) and report back.
blu.256 force-pushed fix/kxkb-fixes from 646d666a72 to bd75fd7f24 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

@MicheleC The problem should be fixed now.

More info: the problem was with commit b0adca1dd4 which introduced a special check for duplicated options. I noticed that, given how append mode works, the setxkbmap string would grow long because some of the options would be repeated a lot of times. So I implemented a basic check, but got the logic somewhat wrong.

@MicheleC The problem should be fixed now. <small>More info: the problem was with commit b0adca1dd4 which introduced a special check for duplicated options. I noticed that, given how append mode works, the `setxkbmap` string would grow long because some of the options would be repeated a lot of times. So I implemented a basic check, but got the logic somewhat wrong.</small>
blu.256 force-pushed fix/kxkb-fixes from 6e3a0405fd to ed61458697 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

I have added one last feature: in the desktop's lock dialog, instead of the textual layout code, Kxkb's indicator is now displayed, which looks the same as the icon in the system tray (the tray icon's pixmap is retrieved by the lock window and then displayed instead of the label).

I have been unable to test this because of build issues (I have a mixed R14.0.13/R14.1.0 environment), but the code should work fine. Can you please test it? If you like me think this change is a good idea, then we might include it in R14.1.0.

I have added one last feature: in the desktop's lock dialog, instead of the textual layout code, Kxkb's indicator is now displayed, which looks the same as the icon in the system tray (the tray icon's pixmap is retrieved by the lock window and then displayed instead of the label). I *have been unable to test this* because of build issues (I have a mixed R14.0.13/R14.1.0 environment), but the code should work fine. Can you please test it? If you like me think this change is a good idea, then we might include it in R14.1.0.
Owner

Yes, the change in the lock dialog seems a good idea.
Will do a test in the morning and let you know as usual.

Yes, the change in the lock dialog seems a good idea. Will do a test in the morning and let you know as usual.
Owner

Hi Philippe,
I did a new round of testing. There were some minor fixes required to the last commit to get it to build correctly: I added a commit for that but please squash it later on.

The kxkb functionality seems to work fine and I think we can proceed with merging. There is probably still something to fix related to "append+reboot", but I need more testing and in any case we could do that as a bug fix for R14.1.1.

Regarding the indicator on the lock window, I like the idea but before merging that part we need to clean it up a bit. I observed three things (see screenshot):

  1. the button is now huge, which looks a bit out of place IMO (this is a matter of preference, so not really a blocker)
  2. the text (if shown) looks less nice/clean. This happens also for the indicator in the system tray. While in the systemtray it is somehow less visible, on the lock screen it is clearly in front of the user, so it would be better to polish it a bit before adding it to TDE. Again, if this can't be done by Sunday, we can do that for R14.1.1
  3. the keyboard shortcut for the indicator is no longer visible with the new version (like the underlined u in the screenshot)
Hi Philippe, I did a new round of testing. There were some minor fixes required to the last commit to get it to build correctly: I added a commit for that but please squash it later on. The kxkb functionality seems to work fine and I think we can proceed with merging. There is probably still something to fix related to "append+reboot", but I need more testing and in any case we could do that as a bug fix for R14.1.1. Regarding the indicator on the lock window, I like the idea but before merging that part we need to clean it up a bit. I observed three things (see screenshot): 1. the button is now huge, which looks a bit out of place IMO (this is a matter of preference, so not really a blocker) 2. the text (if shown) looks less nice/clean. This happens also for the indicator in the system tray. While in the systemtray it is somehow less visible, on the lock screen it is clearly in front of the user, so it would be better to polish it a bit before adding it to TDE. Again, if this can't be done by Sunday, we can do that for R14.1.1 3. the keyboard shortcut for the indicator is no longer visible with the new version (like the underlined `u` in the screenshot)
Owner

In summary, let's merge the code for the kxkb enhancement. Will leave up to you to decide what commits to squash and what to keep separate.

Please remember to rebase on top of master before merging!

For the kdesktop lock indicator, I suggest we handle that as a separate PR. If we can fix the visualization issues within Sunday, we can add it to R14.1.0, otherwise we can add it to R14.1.1.

Thanks for the good work and for the patience to entertain all my feedback for so long :-)

In summary, let's merge the code for the kxkb enhancement. Will leave up to you to decide what commits to squash and what to keep separate. **Please remember to rebase on top of master before merging!** For the kdesktop lock indicator, I suggest we handle that as a separate PR. If we can fix the visualization issues within Sunday, we can add it to R14.1.0, otherwise we can add it to R14.1.1. Thanks for the good work and for the patience to entertain all my feedback for so long :-)
MicheleC approved these changes 1 year ago
MicheleC left a comment
Owner

Looks good but take note of the comments about the lock screen indicator.

Looks good but take note of the comments about the lock screen indicator.
blu.256 force-pushed fix/kxkb-fixes from 6797a06977 to 99c1ebc96d 1 year ago
blu.256 changed title from WIP: Various KXkb improvements to Various KXkb improvements 1 year ago
blu.256 force-pushed fix/kxkb-fixes from 88cfadcb68 to 99c1ebc96d 1 year ago
blu.256 force-pushed fix/kxkb-fixes from 99c1ebc96d to 18e69fad6e 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

I have omitted the three commits regarding kdesktop_lock, and kept them on a personal branch so as to work on them once the window for R14.1.1 opens.

I have squashed the remaining 20 commits into 3 commits with some quite long descriptions.

I have rebased them on top of master (please disregard 88cfadcb68).

I think it's ready for merge :-)

I have omitted the three commits regarding kdesktop_lock, and kept them on a personal branch so as to work on them once the window for R14.1.1 opens. I have squashed the remaining 20 commits into 3 commits with some quite long descriptions. I have rebased them on top of master (please disregard 88cfadcb68). I think it's ready for merge :-)
blu.256 removed the PR/wip label 1 year ago
blu.256 force-pushed fix/kxkb-fixes from 18e69fad6e to e8208c1dfb 1 year ago
blu.256 merged commit e8208c1dfb into master 1 year ago
blu.256 deleted branch fix/kxkb-fixes 1 year ago
blu.256 commented 1 year ago
Poster
Collaborator

PR merged. Thanks everyone for proposing ideas and especially to @MicheleC for the excellent testing!

I hope that this PR solves the dreaded Bugzilla issue 100.

PR merged. Thanks everyone for proposing ideas and especially to @MicheleC for the excellent testing! I hope that this PR solves the dreaded [Bugzilla issue 100](https://bugs.trinitydesktop.org/show_bug.cgi?id=100).
Owner

PR merged. Thanks everyone for proposing ideas and especially to @MicheleC for the excellent testing!

Thanks Philippe! After building it again, I will prepare a proper 'toot' on our mastodon TDE account for this :-)

> PR merged. Thanks everyone for proposing ideas and especially to @MicheleC for the excellent testing! Thanks Philippe! After building it again, I will prepare a proper 'toot' on our mastodon TDE account for this :-)
Owner

I hope that this PR solves the dreaded Bugzilla issue 100.

I believe it does, based on all the testing I have done here. I have closed the bug with a link pointing here for info.

> I hope that this PR solves the dreaded Bugzilla issue 100. I believe it does, based on all the testing I have done here. I have closed the bug with a link pointing here for info.
SlavekB commented 1 year ago
Owner

Thank you both for your great cooperation and another good thing to TDE! I'm going to run builds…

Thank you both for your great cooperation and another good thing to TDE! I'm going to run builds…

Reviewers

SlavekB was requested for review 1 year ago
MicheleC approved these changes 1 year ago
The pull request has been merged as e8208c1dfb.
Sign in to join this conversation.
No Milestone
No Assignees
5 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdebase#304
Loading…
There is no content yet.