nxagent: Pass down if window manager has been detected #830
Conversation
| EventSelectForWindow(register WindowPtr pWin, register ClientPtr client, Mask mask) | ||
| #endif |
There was a problem hiding this comment.
I don't like this one. EventSelectForWindow is called from within ChangeWindowAttributes (in dix/window.c). ChangeWindowAttribute is a method set in the pScreen struct. We do define pScreen->ChangeWindowAttributes in Screen.c and assign nxagentChangeWindowAttributes (Window.c) to it. In nxagentChangeWindowAttributes we call XChangeWindowAttributes after doing some nxagent magic.
So, on what code path is the above EventSelectForWindow function really called insider nxagent? I feel that we should handle this a bit more object oriented, that is: define an nxagentEventSelectForWindow and have that called from whereever. Also, I'd prefer to see dix/events.c patched in a way, that ChangeWindowAttributes calls nxagentEventSelectForWindow if NXAGENT_SERVER is defined, EventSelectForWindow (from X.Org), if not defined.
| extern Bool nxagentWMIsRunning; | ||
|
|
||
| int | ||
| EventSelectForWindow(register WindowPtr pWin, register ClientPtr client, Mask mask) |
There was a problem hiding this comment.
please rename to nxagentEventSelectForWindow and let it be called from whereever needed.
|
It is called from the dispatcher (dix/dispatch.c or NXdispatch) once a
client calls InputSelect.
Would you like to see a backtrace?
Mike Gabriel <notifications@github.com> schrieb am Sa., 10. Aug. 2019,
13:55:
… ***@***.**** requested changes on this pull request.
I don't like this approach of hacking nxagent code paths into the the DIX.
See my inline comments.
Please get back to me, if you have any questions about my reasoning.
Thanks,
Mike
------------------------------
In nx-X11/programs/Xserver/dix/events.c
<#830 (comment)>
:
> EventSelectForWindow(register WindowPtr pWin, register ClientPtr client, Mask mask)
+#endif
I don't like this one. EventSelectForWindow is called from within
ChangeWindowAttributes (in dix/window.c). ChangeWindowAttribute is a method
set in the pScreen struct. We do define pScreen->ChangeWindowAttributes in
Screen.c and assign nxagentChangeWindowAttributes (Window.c) to it. In
nxagentChangeWindowAttributes we call XChangeWindowAttributes after doing
some nxagent magic.
So, on what code path is the above EventSelectForWindow function really
called insider nxagent? I feel that we should handle this a bit more object
oriented, that is: define an nxagentEventSelectForWindow and have that
called from whereever. Also, I'd prefer to see dix/events.c patched in a
way, that ChangeWindowAttributes calls nxagentEventSelectForWindow if
NXAGENT_SERVER is defined, EventSelectForWindow (from X.Org), if not
defined.
------------------------------
In nx-X11/programs/Xserver/hw/nxagent/NXevents.c
<#830 (comment)>
:
> @@ -520,6 +520,37 @@ DefineInitialRootWindow(register WindowPtr win)
}
}
+#ifdef NXAGENT_SERVER
+extern Bool nxagentWMIsRunning;
+
+int
+EventSelectForWindow(register WindowPtr pWin, register ClientPtr client, Mask mask)
please rename to nxagentEventSelectForWindow and let it be called from
whereever needed.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#830?email_source=notifications&email_token=ABQHBZHG42C53DGPSXBY5GDQD2UE3A5CNFSM4IKSJTVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBGBVCY#pullrequestreview-273422987>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHBZBNOEFW6STC7DTQ4F3QD2UE3ANCNFSM4IKSJTVA>
.
|
|
Here's the according backtrace: (gdb) bt Note that nxagentChangeWindowAttributes is not called at all! dix's ChangeWindowAttributes calls nxagentChangeWindowAttributes at the end, but EventSelectForWindow is called before. This is all internal functionality so we need to intercept that somehow. It is just the same as most other call interceptions we do. Uli Update: pasted the correct backtrace... |
|
After having another look: while we could check all this nxagentChangeWindowAttributes we could not return a result because ChangeWindowAttributes calls it without any rc check. So we have these alternatives:
|
|
Hi,
On Sa 10 Aug 2019 15:18:20 CEST, Ulrich Sibiller wrote:
After having another look: while we could check all this
nxagentChangeWindowAttributes we could _not_ return a result because
ChangeWindowAttributes calls it without any rc check. So we have
these alternatives:
Ok...
1. patch dix/window.c:ChangeWindowAttributes (we don't want that)
Why don't we want that?
```
#ifdef NXAGENT_SERVER
... nxagentEventSelectForWindow(...)
#else
... EventSelectForWindow(...)
#endif
```
license wise, this is unproblematic. It only means that we need to get
this ifdef into X.Org upstream one day.
2. replace ChangeWindowAttributes by an own function
This is too much code to have replaced.
3. patch EventSelectForWindow (that's what I did). It is only called
from ChangeWindowAttributes.
You replace the whole function, I'd prefer seeing the function calls
to EventSelectForWindow conditionally replaced, as this change on
X.Org is much more likely to find acceptance on the X.Org upstream
side, I presume (only guessing here, though).
Mike
…--
DAS-NETZWERKTEAM
c\o Technik- und Ökologiezentrum Eckernförde
Mike Gabriel, Marienthaler str. 17, 24340 Eckernförde
mobile: +49 (1520) 1976 148
landline: +49 (4351) 850 8940
GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31
mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
|
|
On Sat, Aug 10, 2019 at 4:01 PM Mike Gabriel ***@***.***> wrote:
Hi,
On Sa 10 Aug 2019 15:18:20 CEST, Ulrich Sibiller wrote:
> After having another look: while we could check all this
> nxagentChangeWindowAttributes we could _not_ return a result because
> ChangeWindowAttributes calls it without any rc check. So we have
> these alternatives:
Ok...
> 1. patch dix/window.c:ChangeWindowAttributes (we don't want that)
Why don't we want that?
```
#ifdef NXAGENT_SERVER
... nxagentEventSelectForWindow(...)
#else
... EventSelectForWindow(...)
#endif
```
I don''t want do patch in code fragments in the middle of the code.
That is just a PITA if you try to apply backports. For me (only) the
following are acceptable approaches.
1. replace the whole function via #ifndef NXAGENT + identically named
function elsewhere. This the initial approach you took for all the
NX*.c files. Downside: we have code duplication which leads to two
places where possible backports need to be applied (and adapted to the
changed code!)
2. replace the whole function via Xorg_ rename and adding another
function named as the original on, possibly calling the renamed Xorg_
variant at some point. This is the approach I favour now as it helps
in reducing code duplication. Also we can update dix code and
automatically benefit from upstream changes without touching our own
code. Further, and most important, it greatly reduces the comparison
work that's required when backporting upstream Xorg code.
3. Use an official callback. For this approach we could suggest some
more callback hooks to Xorg upstream. But I doubt we will be able to
find appropriate callback solutions for all the places where we patch
upstream code.
license wise, this is unproblematic. It only means that we need to get
this ifdef into X.Org upstream one day.
> 2. replace ChangeWindowAttributes by an own function
This is too much code to have replaced.
exactly
> 3. patch EventSelectForWindow (that's what I did). It is only called
> from ChangeWindowAttributes.
You replace the whole function, I'd prefer seeing the function calls
to EventSelectForWindow conditionally replaced, as this change on
X.Org is much more likely to find acceptance on the X.Org upstream
side, I presume (only guessing here, though).
Yes, and that function is only called from _one_ place in Xorg. And
even if it there were more places you'd still want to have a
consistent behaviour so all calls should use the replacement function.
It is done that way on all places where I introduced the Xorg_ naming
scheme.
Uli
|
|
You have not responded to my explanations. So what are we going to do with this PR? |
I prefer variant 2., then... |
|
Currently I do not recommend to merge this. It turned out that java applications in a rootless session struggle with their window manager detection and will then fail to resize properly (window gets bigger, but not content). A workaround is to run them with _JAVA_AWT_WM_NONREPARENTING=1 I think the trick that would fix it was to propagate the root window properties _NET_SUPPORTING_WM_CHECK and _NET_WM_NAME, too. Or set them explicitly like wmname does (https://git.suckless.org/wmname/file/wmname.c.html). Update: just verifified: using wmname to set the WM to "LG3D" (wile the local side isr running KWin) makes java apps work again. But I am unsure if we can set that regardless of the local window manager. |
ok. Thanks for the heads up. |
At start a rootless nxagent checks if the real X server has a Window Manager running. It uses a standard detection routine that tries to select a special input (SubStructureRedirect). As only one client per X server is allowed to select that input one can deduce from the success of this operation if a Window Manager is running. If nxagent is run in rootless mode and has not found a Window Manager on the real X server it will grab all input (see Screen.c:nxagentOpenScreen). If any client of the nxagent runs the standard Window Manager detection routine against a rootless nxagent it will _not_ see the Window Manager. If this client happens to be a rootless nxagent again it will then grab all input which is undesired here. Other clients might do other undesired stuff in that case. To avoid all that a rootless nxagent now tries to detect if a client runs that detection routine and returns the result of its own check to the client.
uli42
force-pushed
the
pr/pass_down_wm_presence
branch
from
cb87f01
to
eeade65
Compare
At start a rootless nxagent checks if the real X server has a Window
Manager running. It uses a standard detection routine that tries to
select a special input (SubStructureRedirect). As only one client per
X server is allowed to select that input one can deduce from the
success of this operation if a Window Manager is running.
If nxagent is run in rootless mode and has not found a Window Manager
on the real X server it will grab all input (see
Screen.c:nxagentOpenScreen).
If any client of the nxagent runs the standard Window Manager
detection routine against a rootless nxagent it will not see the
Window Manager. If this client happens to be a rootless nxagent again
it will then grab all input which is undesired here. Other clients
might do other undesired stuff in that case.
To avoid all that a rootless nxagent now tries to detect if a client
runs that detection routine and returns the result of its own check to
the client.