Various KXkb improvements #304
Merged
blu.256
merged 3 commits from fix/kxkb-fixes
into master
1 year ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'fix/kxkb-fixes'
Deleting a branch is permanent. It CANNOT be undone. Continue?
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).
Various KXkb improvementsto WIP: Various KXkb improvements 1 year ago6a422dade5
to9285b9715e
1 year agoHi Philippe,
I will test this PR in coming days and feedback as usual. Thanks for the good work.
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).
WIP: Various KXkb improvementsto Various KXkb improvements 1 year agoThe 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?
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?
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
andtde-devels
lists. Who should I let know?22667eb0d3
tofaab642fe7
1 year agoI 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.
It would be the locale one, of course.
Maybe, or may be not. For example in Italy you usually have the Italian keyboard and no US keyboard at all.
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 👍
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.
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
@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?
@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.
faab642fe7
to5ab6746f24
1 year agoI have just sent an e-mail to the users mailing list asking for opinions on TDEPersonalizer and default keyboard layouts.
Adding US layout by default as a second choice has two main problems:
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.
I tested the PR on debian bookworm. Some feedback:
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.
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".
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).
Alt+Space does not seem to work here. Not sure if it is something specific to my system or a real issue.
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.
@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.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?
Various KXkb improvementsto WIP: Various KXkb improvements 1 year agoI like the solution with the adding of the checkbox to Enable English keyboard layout.
Sounds good to me too
@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
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.
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?
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.
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?
Point 1 resolved. Xkb command now updates automatically when selecting a new value.
WIP: Various KXkb improvementsto Various KXkb improvements 1 year agoedf0e25482
tob989ea9c57
1 year agoOk, will do another test and feedback.
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:
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.
The following are randomly chosen changes of hotkey combinations.
Change hotkey option to Ctrl+Shift and that works
Change hotkey option to Ctrl+Space and that works, with Ctrl+Shift continuing to work.
Change hotkey option back to Alt+Space and that doesn't work but Ctrl+Shift and Ctrl+Space do.
Clear the setxkbmap options:
and re-apply the hotkey option to Alt+Space, and it works without logging out and back in:
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 have done a new test with the latest code. With regards to the 5 points I initially raised:
this is now ok
as discuss, this is no longer required
some key combinations seems to get active immediately, some others seems to require a logout
any key combination with 'space' does not seem to work
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.
@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.
Two points on this.
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
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 @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).
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.Various KXkb improvementsto WIP: Various KXkb improvements 1 year agoupdate: I think I know.how to make multiple hotkeys work. I'll get to work it soon.
With alt+shift I get the following. The 'FocusOut' event happens after I press Shift. The keyboard layout changes.
With alt+space I get the following and the keyboard layout does not change.
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.
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.
@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 :-)
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:
This also means that we can support multiple hotkeys, but I'll need to make it work with the combobox in the first tab.
Thanks for the update Philippe.
should I do a test now or should I wait till you finish off the combobox code that you mentioned?
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
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 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'm not totally sure about this. Somebody might be running a custom
setxkbmap
command on startup. Removing this option will overwrite their custom settings.Hi Philippe,
I did a rasonable quick test. We are getting there but there are a few new points to address.
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.
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
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
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.
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.
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.
setxkbmap -option
is the command to remove any existing options, sosetxkbmap -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.
Hi @Ray-V
thanks for your feedback and insight.
This sounds like a good idea. Maybe we can even change the text of the checkbox depending on its state:
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 😉
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.
Thanks for the feedback, everyone!
I agree with the radio button idea, so we could have the user see two options, probably like this:
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.
Near the Command line edit sounds like a good place.
@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.
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.
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 ofsetxkbmap
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.
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).
Please test the latest changes (KCMLayout UI changes, Keyboard switching hotkey "None" option).
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:
if you select "Custom" on the "Layout" tab, you now end up in the wrong tab (Indicator options instead of XKb Options)
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.
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?
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.
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
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.
This sounds critical. Could you provide some details?
/usr/share/X11/xkb/rules/
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.
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 :(
This was a bug due to incorrect initial value of the None option. Should be fixed with latest commit.
58e31148a7
to5bb462fc9f
1 year ago@blu.256
I will be away till Wednesday. I will test again after that and feedback as usual.
I agree. Currently the parent is checkable, so it would be better to either make it non-checkable or ignoring clicks on it.
Using
setxkbmap -print
gives some feedback about what the current settings are. Sure, it requires some parsing :-)@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.
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.
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? :-)
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:
user choose "overwrite options": setxkbmap should clear old options and set the new ones only. New working options should be A and C.
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. InAppend
mode, only new options should be selected.Okay, so we're targeting the advanced user here. I'll implement the relevant parts some time this week.
@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?
@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.
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:
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.
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).
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)
Thank you for the detailed write up. I'll test and try to make the needed modifications.
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.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?
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".
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.
399953c272
toeb089b14e4
1 year agoAn 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.
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:
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
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.
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:
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!
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();
wrong indentation in line 19
}
}
updateHotkeyCombo();
wrong indentation line 424
public:
KXKBApp(bool allowStyles=true, bool GUIenabled=true);
~KXKBApp();
KXKBApp(bool allowStyles=true, bool GUIenabled=true);
Few lines have wrong indentation.
void toggled();
void windowChanged(WId winId);
void layoutApply();
void slotGroupChanged(uint group);
Wrong indentation line 75
kdDebug() << "map: Next layout: " << layoutQueue.head()->layoutUnit.toPair()
<< " group: " << layoutQueue.head()->layoutUnit.defaultGroup << " for " << m_currentWinId << endl;
<< " for " << m_currentWinId << endl;
Wrong indentation line 91 and 99, based on how you aligned previous kdWarning/kdDebug statements.
But then we would lose all other options except for those currently in the dialog. Is that really what we want?
Indentation fixed :-)
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.
6b1c80d7c3
to5105e5f47c
1 year agoI 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.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:
I think the previous iteration (the one I tested yesterday) was closer to what we wanted to achieve.
I think the best solution would be as follow:
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.
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 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.
I can't seem to be able to reproduce either of this behaviour. :(
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?
Currently I'll revert my previous commit and implement the behaviour you are asking for but please do consider the point I made above. :)
I've implemented the new behaviour. It seems to work as you described.
The process is this:
m_forceGrpOverwrite
to true (by default it is false).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
m_forceGrpOverwrite
back o falseThese 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.
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:
IMO, these 2 rules are quite reasonable, but happy to hear other opinions too.
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.Thank you once again for the detailed feedback! I will look into this (hopefully last) issue tomorrow (22/03) and report back.
646d666a72
tobd75fd7f24
1 year ago@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.6e3a0405fd
toed61458697
1 year agoI 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.
Yes, the change in the lock dialog seems a good idea.
Will do a test in the morning and let you know as usual.
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):
u
in the screenshot)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 :-)
Looks good but take note of the comments about the lock screen indicator.
6797a06977
to99c1ebc96d
1 year agoWIP: Various KXkb improvementsto Various KXkb improvements 1 year ago88cfadcb68
to99c1ebc96d
1 year ago99c1ebc96d
to18e69fad6e
1 year agoI 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 :-)
18e69fad6e
toe8208c1dfb
1 year agoe8208c1dfb
into master 1 year agoPR 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.
Thanks Philippe! After building it again, I will prepare a proper 'toot' on our mastodon TDE account for this :-)
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.
Thank you both for your great cooperation and another good thing to TDE! I'm going to run builds…
Reviewers
e8208c1dfb
.