Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pr/autograb #393

Open
wants to merge 14 commits into
base: 3.6.x
Choose a base branch
from
Open

Pr/autograb #393

wants to merge 14 commits into from

Conversation

uli42
Copy link
Member

@uli42 uli42 commented Mar 11, 2017

second version, added inputlock (force-grab) via ctrl-alt-c. Please test. Enable autograb via ctrl-alt-g.

Code definitely needs cleanup, this is just w-i-p with a lot of testing/debug code.

@sunweaver
Copy link
Member

compiler warning:

Events.c: In function ‘setWinNameSuffix’:
Events.c:714:58: warning: passing argument 3 of ‘XFetchName’ from incompatible pointer type
     XFetchName(nxagentDisplay, nxagentDefaultWindows[0], &prev);
                                                          ^
In file included from Agent.h:114:0,
                 from Events.c:44:
../../../../exports/include/nx-X11/Xlib.h:2471:15: note: expected ‘char **’ but argument is of type ‘unsigned char **’
 extern Status XFetchName(
               ^
Events.c:722:55: warning: pointer targets in passing argument 3 of ‘XStoreName’ differ in signedness [-Wpointer-sign]
  XStoreName(nxagentDisplay, nxagentDefaultWindows[0], prev);
                                                       ^
In file included from Agent.h:114:0,
                 from Events.c:44:
../../../../exports/include/nx-X11/Xlib.h:3413:12: note: expected ‘const char *’ but argument is of type ‘unsigned char *’
 extern int XStoreName(
            ^

@sunweaver
Copy link
Member

should be:

diff --git a/nx-X11/programs/Xserver/hw/nxagent/Events.c b/nx-X11/programs/Xserver/hw/nxagent/Events.c
index 30f743b..dbb079f 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Events.c
+++ b/nx-X11/programs/Xserver/hw/nxagent/Events.c
@@ -694,7 +694,7 @@ static void nxagentSwitchDeferMode(void)
 
 void setWinNameSuffix(const char *suffix)
 {
-  static unsigned char * prev = NULL;
+  static char * prev = NULL;
   char * pre = " (";
   char * post = ")";
   char * newname;

@sunweaver
Copy link
Member

Not working:

  • Ctrl + Alt + c (input lock)
  • Ctrl + Alt + f (switch to fullscreen/allscreens)
  • Ctrl + Alt + f (returned to windowed desktop session mode)

-> window title still says "... (input lock)" but the actual lock is gone.

  • Ctrl + Alt + c twice brings me back to input lock mode

@sunweaver
Copy link
Member

IRC backlog regarding this PR:

09:08 <@sunweaver> uli42: please already rebase the pr/autograb branch against 3.6.x, so it potentially is ready to merge.
09:09 <@sunweaver> (it's a trivial rebase)
09:13 < uli42> sunweaver: ?? it should be rebased, i checked it in ~1 o'clock
09:14 <@sunweaver> it is rebased by commits. But not rebased fully against the branch. It is missing the merge commits.
09:14 <@sunweaver> you need to pull 3.6.x anew before rebasing against your local 3.6.x branch.
09:15 <@sunweaver> uli42: before I write a sermon on GH, I don't get the approach you use for static unsigned char * prev = NULL;
09:16 <@sunweaver> you declare prev as a local variable and seem to get back to it in a next call to setWinNameSuffix.
09:17 <@sunweaver> Isn't it so that function-local const variables must be restricted to being handled in the one function call only. It seems like "prev" should be some global variable.
09:20 < uli42> well, we can make it global. But the purpose is to being able to reset the window title to the previous title. This is required when you use autograb and then invoke inputlock and then disable 
               inoutlock again. There's one problem with that code: prev does not get freed on exit of nxagent. I am aware of that and will fix that.
09:21 < uli42> sunweaver: regardig rebasing: I have done this after you merged #392:
09:21 < uli42> git co 3.6.x
09:21 < uli42> git pull
09:21 < uli42> git co -
09:21 < uli42> git rebase 3.6.x
09:21 < uli42> So what's wrong with that?
09:22 <@sunweaver> missing: git push gh-uli42 pr/autograb:pr/autograb
09:22 <@sunweaver> what is your latest HEAD on 3.6.x
09:23 <@sunweaver> it should be 6ac805ab4411d3045c99e3ceefe8495ac95d8e15
09:23 -!- Irssi: Pasting 25 lines to #arctica. Press Ctrl-K if you wish to do this or Ctrl-C to cancel.
09:23 <@sunweaver> Merge branch 'uli42-pr/cleanup_keystroke' into 3.6.x
09:24 <@sunweaver> it seems that prev made somehow global.
09:24 <@sunweaver> note that X2Go for example changes the window title after x2goagent startup, so we cannot grab that on session startup and reuse the grabbed name.
09:25 <@sunweaver> maybe you should even check if the suffix is still present in the window title and _only_ then reinstate the previous title.
09:25 <@sunweaver> It may happen that an external application modified it while being in autograb mode.
09:25 <@sunweaver> inputlock mode, that is.
09:29 < uli42> Ok, I cannot look at my latest commit, that one is at home... But as there are valid comments I will do some more work anyway. the unsigned was there because I had used XSetWMName before which 
               requires and unsigned char *. And x2go does something weird here: it runx nxagent with -name "X2GO-some-long-id" and then changes the window title from the outside. Using my approach will set the 
               window title to "X2Go-some-long-id", so maybe x2go shou
09:30 < uli42> Why do you think prev is global?
09:32 <@sunweaver> because you set it in one function call and get back to it in another function call.
09:32 < uli42> inputlock + fullscreen handling ist now implemented yet. I think rootless + inputlock will also break things...
09:32 < uli42> sunweaver: that's why it is static.
09:33 <@sunweaver> why not declare it globbally, handle it in the setWinNameSuffix, and then properly clean it up at session end.
09:34 <@sunweaver> e.g. handle it in AbortDDX() in Init.c or in nxagentAbortDisplay().
09:36 < uli42> sunweaver: yes, that's the way to go. But it was late yesterday and I have to get up a 6:15...
09:37 <@sunweaver> hehehe. same here, but I was in bed earlier, I guess.
09:37 < uli42> does autograb work proeprly for you?
09:42 <@sunweaver> wow!!!
09:43 <@sunweaver> I have been playing with input lock first.
09:43 <@sunweaver> and had MATE nested on top of a local i3 session.
09:43 <@sunweaver> now I tested nested i3 on top of local i3.
09:43 <@sunweaver> this is amazing.
09:43 <@sunweaver> I'd say, we should swich on autograb by default.
09:43 <@sunweaver> because it feels so much more organic.
09:47 <@sunweaver> except from my comments here and on GH, the PR looks fine.
09:47 <@sunweaver> I'll dump our chat log into the GH issue tracker

TL;DR; the prev variable in setWinNameSuffix() requires more work (make it global, manage it in setWinNameSuffix() and free it properly at session end / abortion.

Plus comments above.

The new autograb functionality is so great, I believe we should make it the default (if not now, then at least very very soon!).

@uli42
Copy link
Member Author

uli42 commented Mar 16, 2017

Things that need to be improved:

  • fix handling of FullScreen switch when autograb and/or inputlock are active
  • restrict autograb and inputlock to windowed mode
  • add keystroke to window title for inputlock mode
  • add commandline and/or options support for both autograb and inputlock
  • make autograb and inputlock variables global nxagent* variables
  • free prev from SetWinNameSuffix at program exit. So it needs to become global (or a resource, maybe?)

@sunweaver
Copy link
Member

@uli42: any news on this one? What's its status?

@uli42
Copy link
Member Author

uli42 commented Apr 3, 2017 via email

@sunweaver
Copy link
Member

sunweaver commented Apr 3, 2017 via email

@uli42
Copy link
Member Author

uli42 commented Apr 3, 2017 via email

@sunweaver
Copy link
Member

sunweaver commented Apr 3, 2017 via email

@sunweaver
Copy link
Member

@uli42: this is not working yet... As you know I use a tiling window manager (i3). I can easily escape the input lock if auto-grab is off, for example.

Also, I wonder, if we shouldn't rather introduce "flags" as opposed to the "autograb on|off" and "input lock on|off" strings in the title. E.g. "nxagent [GIX]" (grab, input lock, xinerama on) vs. "nxagent [giX]" (grab and input off, Xinerama on). That makes the agent not seem soooo English.

Furthermore, the autograb mode standalone seems to work. So I wonder, if we should separate those two features for now and get the autograb code in as is.

The input lock, I can e.g. escape from when I disable the autograb mode and then manipulate the window (e.g. minize or some fancy tiling command in i3). The window moves by keyboard event to "somewhere" else, the mouse is left outside nxagent and nxagent still shows input lock. Not true.

So for input lock we maybe need to capture mouse-in and mouse-out events (seen from nxagent's perspective). If the user manages to escape from the nxagent windows, then a new mouse-in event should trigger the input lock once more and capture the pointer.

I can take a closer look tomorrow, but maybe the above gives you a clue and things are fixed by tomorrow morning. Hehehe...

@uli42
Copy link
Member Author

uli42 commented Jun 28, 2017

Changing display of the state in the window title can be adjusted after this patch works in all situations... You write "the autograb mode standalone seems to work". Well, have you tried switch to fullscreen and then switching back? This is not working properly for me.

Also, I have not understood yet how to synthetically insert events into the queue as you suggest for the mouse stuff.

I'd appreciate any help to get this going.

Fixes ArcticaProject#384

You can now toggle between autograb mode by pressing CTRL-ALT-G
(default, can be adjusted in keystrokes.cfg).

You can toogle inputlock mode (pointer stays in the nxagent window) by
pressing CTRL-ALT-C.
@uli42
Copy link
Member Author

uli42 commented Nov 15, 2019

Update: autograb code is merged and working, but we have left out the window title handling stuff and the inputlock code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants