mviereck / x11docker

Run GUI applications and desktops in docker and podman containers. Focus on security.
MIT License
5.62k stars 378 forks source link

nxagent - clone existing keyboard options #208

Closed seandilda closed 2 years ago

seandilda commented 4 years ago

I'm having an issue with nxagent where keyboard options (ie ctrl:nocaps) are being ignored.

In digging into this, x11docker is setting a keyboard= option for nxagent based on 'setxkbmap -query', but isn't applying any option.

When I modify x11docker to pass 'keyboard=clone' instead, nxagent properly detects my options and applies them.

I'm happy to submit a pull request if it would be helpful.

mviereck commented 4 years ago

Interesting, I was not aware of keyboard=clone.

I'm happy to submit a pull request if it would be helpful.

That would be nice! Also, you would appear in list of contributors. Doing it myself would be less work for you. Choose what you like more.

mviereck commented 4 years ago

I've pushed a commit with this change. It is really better than the previous solution. Thank you!

One detail: The option must be passend with quotes: keyboard='clone', otherwise it fails.

mviereck commented 4 years ago

I've reverted the change because keyboard='clone' has issues with AltGr key. I cannot type some special chars like @ anymore. Instead, x11docker now allows --keymap=clone for --nxagent to fit your needs.

If you have an idea how to fix the AltGr issue, I am happy to implement it and to default to keyboard='clone' again.

xavierog commented 2 years ago

Hello,

One detail: The option must be passend with quotes: keyboard='clone', otherwise it fails.

Are you sure about this? nxagent seems to exhibit the opposite behaviour:

$ echo "keyboard='clone'" > /tmp/nxagent.options
$ nxagent -options /tmp/nxagent.options :1
Warning: Option file doesn't contain a port specification.

NXAGENT - Version 3.5.99.26

[copyright - abridged]

Info: Agent running with pid '2775395'.
Session: Starting session at 'Sun May  8 19:15:05 2022'.
Info: Using alpha channel in render extension.
Info: Not using local device configuration changes.
Warning: Cannot read keystroke file '/home/xavier/.nx/config/keystrokes.cfg'.
Info: using keystrokes file '/etc/nxagent/keystrokes.cfg'
Currently known keystrokes:
  [keystrokes - abridged]
Warning: Wrong keyboard type: ''clone''. <========================= HERE
Session: Session started at 'Sun May  8 19:15:05 2022'.
Info: Screen [0] resized to geometry [2556x1080] fullscreen [0].

I fixed this on my side with this trivial patch:

diff --git a/x11docker b/x11docker
index 80c01d2..42f7450 100755
--- a/x11docker
+++ b/x11docker
@@ -3797,7 +3797,7 @@ mode=$Screensize
         ;;
         *) # --keymap
           case "$Xkblayout" in
-            clone) Nxagentoptions="$Nxagentoptions,keyboard='clone'" ;;
+            clone) Nxagentoptions="$Nxagentoptions,keyboard=clone" ;;
             *)     Nxagentoptions="$Nxagentoptions,keyboard='evdev/$Xkblayout'" ;;
           esac
         ;;

However, I believe the next line does not make sense either:

            *)     Nxagentoptions="$Nxagentoptions,keyboard='evdev/$Xkblayout'" ;;

Indeed, according to the nxagent(1) manpage, nxagent does not seem to accept any keyboard string starting with "evdev" (which is a kind of "rules", not a "model"):

       keyboard=<string> or kbtype=<string>

               query|clone|<model>/<layout>|rmlvo/<rules>#<model>#<layout>#<variant>#<options>

I would advise to keep things simple by acting as a mere passthrough:

Nxagentoptions="$Nxagentoptions,keyboard=$Xkblayout"
mviereck commented 2 years ago

Thank you for pointing on this!

Are you sure about this? nxagent seems to exhibit the opposite behaviour:

I have checked it and found that keyboard=clone must not have '' quotes now. I assume this has changed meanwhile. I did not notice because nxagent also sets a fitting keyboard layout by default now. It seems some development has been done in this area.

I have set keyboard=clone as default now. It no longer has issues with AltGr that I've encountered once.

I would advise to keep things simple by acting as a mere passthrough: Nxagentoptions="$Nxagentoptions,keyboard=$Xkblayout"

This just does not work, while keyboard=evdev/$Xkblayout works. However, this one also must not have '' quotes now.

xavierog commented 2 years ago

Thank you for pointing on this!

And thank you for writing x11docker in the first place.

I have checked it and found that keyboard=clone must not have '' quotes now. I assume this has changed meanwhile.

Quite possibly. I am admittedly too lazy to check nxagent's development history.

I did not notice because nxagent also sets a fitting keyboard layout by default now. It seems some development has been done in this area.

Does it? The manpage says:

If keyboard is omitted the internal defaults of nxagent will be used (rules: base, layout: us, model: pc102, empty variant and options).

I have set keyboard=clone as default now.

Oh, that's great! The manpage also says that For compatibility reasons [clone] is not the default., but it is a shame, and it does make sense to have it as default in x11docker.

It no longer has issues with AltGr that I've encountered once. This just does not work, while keyboard=evdev/$Xkblayout works.

For reference, the code that parses the keyboard option is here: https://code.x2go.org/gitweb?p=nx-libs.git;a=blob;f=nx-X11/programs/Xserver/hw/nxagent/Keyboard.c#l551 It confirms that passing evdev/something will end up with model="evdev" and layout="something". I think it only makes sense because there is another test in that same file that also compares the model against "evdev": https://code.x2go.org/gitweb?p=nx-libs.git;a=blob;f=nx-X11/programs/Xserver/hw/nxagent/Keyboard.c#l1538

Anyway, my suggestion aimed at ensuring that x11docker does not prevent users from passing the rmlvo options of their dream to nxagent: it is easier for those who need it to pass evdev/something than it is to remove it for those who do not need it (especially since evdev can be passed as part of a rmlvo string).

However, this one also must not have '' quotes now.

Yes, I confirm.

Anyway, thanks for your work :)

mviereck commented 2 years ago

Anyway, my suggestion aimed at ensuring that x11docker does not prevent users from passing the rmlvo options of their dream to nxagent: it is easier for those who need it to pass evdev/something than it is to remove it for those who do not need it (especially since evdev can be passed as part of a rmlvo string).

To add rmlvo support to all X servers I would have to check the keyboard setup possibilities for each of them. In several configurations I just provide e.g. de for german keyboard without evdev or rmlvo or anything else like that. I am not sure if this likely rarely needed feature is worth the effort.

However, I could implement an inofficial feature just for nxagent. x11docker could check the provided string for /. If yes, the string is forwarded unchanged to nxagent. If no, x11docker adds evdev/.

xavierog commented 2 years ago

To add rmlvo support to all X servers [...] I am not sure if this likely rarely needed feature is worth the effort.

Definitely not. The general point here is that end users sometimes need to pass custom arguments to the tools that x11docker runs (though this can be achieved by playing with PATH and wrapper scripts). There is a mechanism to pass arbitrary docker/podman options but mechanisms to better control X servers would be nice too. On the other hand, that makes x11docker more complex.

x11docker could check the provided string for /. If yes, the string is forwarded unchanged to nxagent. If no, x11docker adds evdev/.

Yes, that would be nice: a small condition for you, more flexibility for end users.

mviereck commented 2 years ago

There is a mechanism to pass arbitrary docker/podman options but mechanisms to better control X servers would be nice too.

x11docker has an undocumented/experimental option --xopt="ARGS" to pass additional X server options. This might be useful in some cases. It would not help in this case because the keyboard of nxagent is configured in a different way specific to nxagent only.

x11docker could check the provided string for /. If yes, the string is forwarded unchanged to nxagent. If no, x11docker adds evdev/.

Yes, that would be nice: a small condition for you, more flexibility for end users.

I have added this.

xavierog commented 2 years ago

Thank you!