Differential D4220

Add a basic SNI for keyboard layout
ClosedPublic
Actions

Authored by graesslin on Jan 20 2017, 6:23 AM.

Details

Reviewers
davidedmundson
Group Reviewers
Plasma on Wayland
KWin
Commits
R108:fe561c5c7def: Add a basic SNI for keyboard layout
Summary

On X11 the SNI for keyboard layout is provided by the keyboard kded.
On Wayland that kded has no real access to the layouts and cannot
properly implement switching. Given that it's better to integrate the
SNI directly in KWin.

The implementation of the SNI is largly based on the existing SNI from
plasma-desktop/kcms/keyboard. The implementation so far supports:

  • Switching to next layout on toggle
  • Presenting all layouts in a context menu
  • Switching to a specific layout through the context menu
  • Opening the keyboard layout configuration module

Not yet supported are:

  • config option whether to show the SNI
  • scroll on SNI
  • flags and/or short text for the layouts

The last point needs more explanation. On X11 the layout name is
something like "de" or "us". This can be directly mapped to a flag and
can be added as a short note.

Xkbcommon does not provide this information directly. Instead it provides
us the full name of the layout, e.g. "German" or "English (us)". There is
no way in the API to go from "German" to "de".

Instead we need to parse the evdev.xml file to gather all information
about layouts. This is already done in the keyboard kcm to configure
layouts. The implementation needs to be split out into a small helper
library.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
graesslin updated this revision to Diff 10382.Jan 20 2017, 6:23 AM
graesslin retitled this revision from to Add a basic SNI for keyboard layout.
graesslin updated this object.
graesslin edited the test plan for this revision. (Show Details)
graesslin added reviewers: KWin, Plasma on Wayland.
Restricted Application added projects: Plasma on Wayland, KWin. · View Herald TranscriptJan 20 2017, 6:23 AM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript
luebking added a subscriber: luebking.Jan 20 2017, 8:13 AM

"On Wayland that kded has no real access to the layouts and cannot
properly implement switching. Given that it's better to integrate the
SNI directly in KWin."

I'll raise a fundamental question: is wayland prone to end up being
PID 0: systemd
PID 1: kwin
?

Since kwin is the new display server, it may need an interface in such occasions, signalling the change, exposing the layout and allow clients to alter the layout.
Suppose someone would want to have a virtual keyboard with a layout indicator/switcher, that should not be dragged into kwin? Latter is now an absolutely crucial process (notably as long as handing over wayland clients isn't supported) and dragging in functionality drags in complexity drags in crashes …

/2¢

davidedmundson added a subscriber: davidedmundson.Jan 20 2017, 11:20 AM
davidedmundson added inline comments.
keyboard_layout.cpp
201

so KToolInvocation::startServiceByName()?

218

You probably don't want to be doing this.

SNI has two two code paths for menus.

addAction() which works the way you'd expect creating a DBus menu and sending that

setContextMenu is a more legacy version that gets a signal from Plasma to show a menu, then kwin's process does the actual showing of it. (and generally speaking doing that won't work on wayland..it might be an exception here)

Given you want a flat list, use m_notifierItem->addAction(..) even for the separator.

graesslin added inline comments.Jan 20 2017, 1:47 PM
keyboard_layout.cpp
201

yes, except that we used to use KToolInvocation and ported away from it. Something - in the case of KWin - doesn't work for it. The standard case of KWin is special. You can see it with the line below. We create a Process, not a QProcess. It's a specialized class to do KWin specific adjustments. Sometimes KWin is meh.

218

uh nice, didn't know that and followed the existing code too blindly.

graesslin added a comment.Jan 20 2017, 1:50 PM

I'll raise a fundamental question: is wayland prone to end up being

Yes, we are renaming kwin to kwind :-)

it may need an interface in such occasions, signalling the change, exposing the layout and allow clients to alter the layout.

Yes, that's built into the Wayland protocol.

This change is about the opposite: supporting *changing* the layout. Not about notifying the layout change, that is something we already support for at least a year.

graesslin added inline comments.Jan 20 2017, 2:19 PM
keyboard_layout.cpp
218

This doesn't work :-( I added the actions, but the menu doesn't get shown at all.

luebking added a comment.Jan 20 2017, 3:02 PM
In D4220#78878, @graesslin wrote:

This change is about the opposite: supporting *changing* the layout. Not about notifying the layout change, that is something we already support for at least a year.

Yes.
Afaiu presently kwinD (;-P) is the only authority to alter the layout and the only one to provide a GUI for that as well?
Clients can't do that - be it a virtual keyboard or a multilingual editor, be it im- or explicitly (text language recognition or a drop down menu)?

What I meant to concern (and dared to be slightly OT in it) is that too much (interactive) features (and code) are dragged into a highly crucial process which becomes an increasingly monolithic-by-design GUI shell.

graesslin added a comment.Jan 20 2017, 3:11 PM
In D4220#78887, @luebking wrote:
In D4220#78878, @graesslin wrote:

This change is about the opposite: supporting *changing* the layout. Not about notifying the layout change, that is something we already support for at least a year.

Yes.
Afaiu presently kwinD (;-P) is the only authority to alter the layout and the only one to provide a GUI for that as well?
Clients can't do that - be it a virtual keyboard or a multilingual editor, be it im- or explicitly (text language recognition or a drop down menu)?

they can. Any application can modify the keyboard layout as they wish. They just cannot modify the global layout.

What I meant to concern (and dared to be slightly OT in it) is that too much (interactive) features (and code) are dragged into a highly crucial process which becomes an increasingly monolithic-by-design GUI shell.

That's why we use an SNI instead of doing the UI ourselves.

If I understand you correctly you would like to see a dbus or Wayland interface provided by KWin to allow setting the layout. That's exactly what an SNI is.

graesslin updated this revision to Diff 10391.Jan 20 2017, 3:30 PM

Scroll support

graesslin updated this revision to Diff 10395.Jan 20 2017, 7:33 PM

Support config option for show/hiding the sni

graesslin updated this revision to Diff 10427.Jan 22 2017, 10:42 AM

Translate layout names

davidedmundson added a comment.Jan 24 2017, 9:40 PM

One comment, but otherwise ship it.

That contextMenu thing will come back to haunt us in the future, but hopefully SNI will be fixed by then.

keyboard_layout.cpp
142

You only want this in ::reconfigure() before resetLayout and not here.

You're calling this method from reconfigure and in processKeymapChange, from keyboard_input - that won't affect whether you're showing or hiding it.

graesslin added inline comments.Jan 25 2017, 9:36 AM
keyboard_layout.cpp
142

that won't affect whether you're showing or hiding it.

Yes it does. processKeymapChange is called when a parent Wayland server informs this nested Wayland server about a new keymap. This means we have a new number of keyboard layouts and that can affect whether the notifier is shown or not.

davidedmundson accepted this revision.Jan 25 2017, 9:43 AM
davidedmundson added a reviewer: davidedmundson.
This revision is now accepted and ready to land.Jan 25 2017, 9:43 AM
Closed by commit R108:fe561c5c7def: Add a basic SNI for keyboard layout (authored by graesslin). · Explain WhyJan 25 2017, 1:01 PM
This revision was automatically updated to reflect the committed changes.
bam added a subscriber: bam.Dec 18 2020, 12:35 PM

We are going to revert this in favor of https://invent.kde.org/plasma/plasma-desktop/-/merge_requests/181:
it supports all the existing functional here plus missing one (problem with short layout names was solved):

  • flags and/or short text for the layouts

If any objections, please tell.

bam added a comment.Dec 18 2020, 8:09 PM

Not a revert but rather rollback of SNI part:
https://invent.kde.org/plasma/kwin/-/merge_requests/560

Revision Contents
Changeset List

PathPackages
Mkeyboard_input.h (6 lines)
Mkeyboard_input.cpp (48 lines)
Mkeyboard_layout.h (15 lines)
Mkeyboard_layout.cpp (127 lines)

Diff 10539

View Options

keyboard_input.h

Loading...
View Options

keyboard_input.cpp

Loading...
View Options

keyboard_layout.h

Loading...
View Options

keyboard_layout.cpp

Loading...