Subject: Bug#744: user-local keystrokes.cfg does not work

From: Horst Schirmeier <[email-masked]>
Date: Thu, 15 Jan 2015 16:10:46 +0100

Package: x2goagent
Version: 2:3.5.0.29-0x2go1~git20150113.557+wheezy.heuler.1

A user-local ~/.nx/config/keystrokes.cfg (as described on
<http://wiki.x2go.org/doku.php/wiki:advanced:nx-keyboard-shortcuts>,
which, btw, still talks about the singular file name "keystroke.cfg") is
not read by nxagent, incapacitating users from overriding system-wide
defaults.

The core reason is the way x2goagent communicates the custom
/etc/x2go/keystrokes.cfg path to nxagent:

NXAGENT_KEYSTROKEFILE=/etc/x2go/keystrokes.cfg
export NXAGENT_KEYSTROKEFILE

In nxagent's keystroke-file search order, this environment variable is
tested for before ~/.nx/config/keystrokes.cfg is looked at.  As a simple
remedy, I suggest imitating nxagent's search order in the x2goagent
wrapper script:

---
 debian/wrappers/x2goagent | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/debian/wrappers/x2goagent b/debian/wrappers/x2goagent
index 129c7ca..750c651 100644
--- a/debian/wrappers/x2goagent
+++ b/debian/wrappers/x2goagent
@@ -29,7 +29,15 @@ test -x $NX_LIBS/../x2go/bin/$NXAPP && export NX_LIBS=$NX_LIBS
 
 export LD_LIBRARY_PATH
 
-NXAGENT_KEYSTROKEFILE=/etc/x2go/keystrokes.cfg
-export NXAGENT_KEYSTROKEFILE
+for CONFIG in ~/.x2go/config/keystrokes.cfg ~/.nx/config/keystrokes.cfg \
+              /etc/x2go/keystrokes.cfg /etc/nxagent/keystrokes.cfg
+do
+	if [ -r "$CONFIG" ]
+	then
+		NXAGENT_KEYSTROKEFILE=$CONFIG
+		export NXAGENT_KEYSTROKEFILE
+		break
+	fi
+done
 
 exec $NX_LIBS/../x2go/bin/$NXAPP "$@"

-- 
PGP-Key 0xD40E0E7A

---

From: Mihai Moldovan <[email-masked]>
Date: Thu, 15 Jan 2015 21:57:27 +0100

> +              /etc/x2go/keystrokes.cfg /etc/nxagent/keystrokes.cfg

Do we actually WANT to make ~/.nx/foo override /etc/x2go/foo *for
x2goagent*? Or do we rather want to get rid of ~/.nx and /etc/nxagent
entirely within x2go components (which, really, would make some sort of
sense, especially if $SOMETHING created ~/.nx/config/keystrokes.cfg and
users wonder why /etc/x2go/keystrokes.cfg does not take any effect.)

Actually, I see another problem there. Wouldn't it be smarter to
consider both ~/.x2go/foo and /etc/x2go/keystrokes.cfg (if existent),
with values in ~/.x2go/foo overriding those of the global configuration
file? A priority-based merge would really be the thing we're looking
for. I've got something like that lying around.

It's not exactly small, though. And would benefit from being shared
code, as it could (and also would) be used in both x2goagent and x2goserver.

Mihai

---

From: Horst Schirmeier <[email-masked]>
Date: Thu, 15 Jan 2015 22:27:01 +0100

> users wonder why /etc/x2go/keystrokes.cfg does not take any effect.)

I have no objection removing ~/.nx and /etc/nxagent from the file list
in my patch, keeping ~/.x2go/config/keystrokes.cfg and
/etc/x2go/keystrokes.cfg.  As ~/.nx does not work at all at the moment,
this wouldn't even break existing setups (but would require updating the
documentation in the wiki).

> code, as it could (and also would) be used in both x2goagent and x2goserver.

This may or may not be a good idea.  For example, IMHO it'd require
introducing a way to completely remove/disable /etc/x2go/keystrokes.cfg
definitions via directives in user-local files, instead of only
overriding key definitions.  Also, it has the potential of not being
compatible to existing setups.  Nevertheless, I suggest opening a new
bug for this, as #744 needs to be fixed either way.

Horst

-- 
PGP-Key 0xD40E0E7A

---

From: Mike Gabriel <[email-masked]>
Date: Thu, 15 Jan 2015 22:56:49 +0000

Control: tag -1 patch

Hi Horst, hi Mihai,

>> +              /etc/x2go/keystrokes.cfg /etc/nxagent/keystrokes.cfg

Thanks for your patch, Horst.

> Do we actually WANT to make ~/.nx/foo override /etc/x2go/foo *for

First: We want to allow users to override system-wide settings by user  
settings.

> users wonder why /etc/x2go/keystrokes.cfg does not take any effect.)

This indeed is a bit of a drama, I agree.

1. We want to provide NX-X11 to people who still use NX (e.g. FreeNX).

To provide this, we have to think generically here. Paths like ~/.nx  
or /etc/nxagent/ are hard-coded in NX-X11 and nxagent and I think that  
is ok. All X2Go'ish paths should be overridden via env vars or  
otherwise.

In nxagent, there is some code that checks ARGV[0] (== nxagent? ==  
x2goagent?) and triggers the branding of start-up screens of desktop  
sessions (the gray X2GO logo).

Maybe for setting paths (esp. ~/.x2go/keystrokes.cfg), some similar  
mechanism should be used?

> for. I've got something like that lying around.

It would be smarter, indeed. But one step at a time.

Let's get this issue solved first cleanly (it obviously is a namespace  
issue for NX configuration files). Once that is sorted out, you should  
bring your merging code into the game. I'd suggest you file it as a  
wishlist bug + patch (or without patch) for now...

> code, as it could (and also would) be used in both x2goagent and x2goserver.

Ok.... So we would bring another dependency into the game that no  
already existing library or whatsoever could already cover in  
functionality? (Mike is scared of to many upstream projects having to  
be maintained inside X2Go).

Mike

-- 

DAS-NETZWERKTEAM
mike gabriel, herweg 7, 24357 fleckeby
fon: +49 (1520) 1976 148

GnuPG Key ID 0x25771B31
mail: [email-masked], http://das-netzwerkteam.de

freeBusy:
https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xfb

---

From: Horst Schirmeier <[email-masked]>
Date: Fri, 16 Jan 2015 15:11:05 +0100

On Thu, 15 Jan 2015, Mike Gabriel wrote:
> First: We want to allow users to override system-wide settings by
> user settings.
> 
> or /etc/nxagent/ are hard-coded in NX-X11 and nxagent and I think
> that is ok. All X2Go'ish paths should be overridden via env vars or
> otherwise.
> 

How about this variant?

patch for x2go-specific keystroke configuration files, enable user-local
keystrokes.cfg
---
 ...agent_x2go-specific-keystroke-config.full.patch | 22 ++++++++++++++++++++++
 debian/patches/series                              |  1 +
 debian/wrappers/x2goagent                          |  3 ---
 3 files changed, 23 insertions(+), 3 deletions(-)
 create mode 100644 debian/patches/321_nxagent_x2go-specific-keystroke-config.full.patch

diff --git a/debian/patches/321_nxagent_x2go-specific-keystroke-config.full.patch b/debian/patches/321_nxagent_x2go-specific-keystroke-config.full.patch
new file mode 100644
index 0000000..8ab4f93
--- /dev/null
+++ b/debian/patches/321_nxagent_x2go-specific-keystroke-config.full.patch
@@ -0,0 +1,22 @@
+--- a/nx-X11/programs/Xserver/hw/nxagent/Keystroke.c
++++ b/nx-X11/programs/Xserver/hw/nxagent/Keystroke.c
+@@ -27,6 +27,7 @@
+ #include "Options.h"
+ #include "Keystroke.h"
+ #include "Drawable.h"
++#include "Init.h" /* extern int nxagentX2go */
+ 
+ #include <unistd.h>
+ 
+@@ -261,6 +262,11 @@ static void parse_keystroke_file(void)
+   char *homefile = "/.nx/config/keystrokes.cfg";
+   char *etcfile = "/etc/nxagent/keystrokes.cfg";
+ 
++  if (nxagentX2go) {
++    homefile = "/.x2go/config/keystrokes.cfg";
++    etcfile = "/etc/x2go/keystrokes.cfg";
++  }
++
+   if (nxagentKeystrokeFile != NULL && access(nxagentKeystrokeFile, R_OK) == 0)
+   {
+     filename = strdup(nxagentKeystrokeFile);
diff --git a/debian/patches/series b/debian/patches/series
index 98d3e5e..c65f645 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -61,6 +61,7 @@
 301_nx-X11_use-shared-libs.full.patch
 302_nx-X11_xkbbasedir-detection.full.patch
 320_nxagent_configurable-keystrokes.full.patch
+321_nxagent_x2go-specific-keystroke-config.full.patch
 400_nxcomp-version.full+lite.patch
 #401_nxcomp_bigrequests-and-genericevent-extensions.full+lite.patch
 600_nx-X11+nxcompext+nxcompshad_unique-libnames.full.patch
diff --git a/debian/wrappers/x2goagent b/debian/wrappers/x2goagent
index 129c7ca..ba07a3f 100644
--- a/debian/wrappers/x2goagent
+++ b/debian/wrappers/x2goagent
@@ -29,7 +29,4 @@ test -x $NX_LIBS/../x2go/bin/$NXAPP && export NX_LIBS=$NX_LIBS
 
 export LD_LIBRARY_PATH
 
-

-- 
PGP-Key 0xD40E0E7A

---

From: Mihai Moldovan <[email-masked]>
Date: Fri, 16 Jan 2015 15:27:17 +0100

> Thanks for your patch, Horst.

I totally forgot about that. Thanks.

> First: We want to allow users to override system-wide settings by

Yes, my point was explicitly about ~/.nx vs. /etc/x2go. ~/.x2go
overriding /etc/x2go is naturally OK.

>> x2goagent*? Or do we rather want to get rid of ~/.nx and
>> /etc/nxagent entirely within x2go components (which, really, would
>> make some sort of sense, especially if $SOMETHING created
>> ~/.nx/config/keystrokes.cfg and users wonder why
>> /etc/x2go/keystrokes.cfg does not take any effect.)
>

And that's fine. nxagent (itself) should rely on ~/.nx and /etc/nxagent.

I'm just saying that for x2goagent, it would be smarter to ignore ~/.nx
and /etc/nxagent, thus only taking ~/.x2go and /etc/x2go into consideration.

> sessions (the gray X2GO logo).

Well, if we have that anyway, we can expand on that. Horst's new patch
sounds great by making use of exactly that.

> mechanism should be used?

Wait... there's ~/.x2go/config/keystrokes.cfg and
~/.x2go/keystrokes.cfg? What's the difference between those?

>> consider both ~/.x2go/foo and /etc/x2go/keystrokes.cfg (if
>> existent), with values in ~/.x2go/foo overriding those of the
>> global configuration file? A priority-based merge would really be
>> the thing we're looking for. I've got something like that lying
>> around.
>

On 15.01.2015 10:27 PM, Horst Schirmeier wrote:
> For example, IMHO it'd require introducing a way to completely
> remove/disable /etc/x2go/keystrokes.cfg definitions via directives in
> user-local files, instead of only overriding key definitions.

I'm not so sure anymore. As pointed out, merging means that you cannot
(easily) delete keys in the global config file. I didn't think of that one.

Mihai

---

From: Mihai Moldovan <[email-masked]>
Date: Fri, 16 Jan 2015 15:29:04 +0100

> +++ b/debian/patches/321_nxagent_x2go-specific-keystroke-config.full.patch

LGTM. Thank you!

Mihai

---

From: Horst Schirmeier <[email-masked]>
Date: Fri, 16 Jan 2015 15:32:45 +0100

> ~/.x2go/keystrokes.cfg? What's the difference between those?

Currently, there "is" neither.  That's why I filed the bug report in the
first place. ;-)  We still can decide what path it should be.  I favor
~/.x2go/config/keystrokes.cfg for being closest to the original
~/.nx/config/keystrokes.cfg.

Horst

-- 
PGP-Key 0xD40E0E7A

---

From: Mike Gabriel <[email-masked]>
Date: Mon, 19 Jan 2015 08:35:49 +0000

Control: tag -1 patch

Hi Horst,

On  Fr 16 Jan 2015 15:11:05 CET, Horst Schirmeier wrote:

>  create mode 100644  
> debian/patches/321_nxagent_x2go-specific-keystroke-config.full.patch
>
> diff --git  
> a/debian/patches/321_nxagent_x2go-specific-keystroke-config.full.patch  
> b/debian/patches/321_nxagent_x2go-specific-keystroke-config.full.patch
> new file mode 100644
> index 0000000..8ab4f93
> --- /dev/null

> [...]

The patch looks ok. However, two things...

(1) can you re-send that patch as an attachment (not as an inline  
text)? (I have been struggling with extracting inline patches from  
mail bodies in the past with a lot of hassle on my side, so I am  
prophylactically asking without even having tried to apply your  
sent-in (inline-text) patch).

(2) I am tempted to merge that patch into patch 320. Would that be ok  
with you?

Mike

-- 

DAS-NETZWERKTEAM

GnuPG Key ID 0x25771B31

freeBusy:

---

From: Horst Schirmeier <[email-masked]>
Date: Mon, 19 Jan 2015 09:51:15 +0100

Hi Mike,

> sent-in (inline-text) patch).

Sure.

> (2) I am tempted to merge that patch into patch 320. Would that be
> ok with you?

320 is generally useful for nxagent and could be merged into its
codebase one day.  Mine is x2go specific.  This objection aside, feel
free to merge the patches, sure! :)

Horst

-- 
PGP-Key 0xD40E0E7A
From fccaa20d36e401d17f958ad1028682c837f2be16 Mon Sep 17 00:00:00 2001
From: Horst Schirmeier <[email-masked]>
Date: Fri, 16 Jan 2015 14:00:09 +0100
Subject: [PATCH] patch for x2go-specific keystroke configuration files, enable
 user-local keystrokes.cfg

---

 export LD_LIBRARY_PATH
 
2.2.1.62.g3f15098