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

Replace ClickOverlay enterMonitor reactive actor approach with much simpler implementation #751

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

jtaala
Copy link
Collaborator

@jtaala jtaala commented Jan 14, 2024

@Lythenas, @Thesola10, I've been thinking about this one for a while:

PaperWM uses a very curious approach to implement a "one space per monitor" paradigm (see my response to a question of how PaperWM does this).

Unfortunately this approach is quite complex and exhibits many side-effects that have required many workarounds (which often causes further issues).

This PR replaces this approach with a simpler PointerWatcher implementation (a Gnome supported implementation to monitor mouse pointer movements). Moving to this approach provides the following advantages:

  • switch to a Gnome supported feature to detect when mouse pointer moves to another monitor (instead of an unsupported, custom, error-prone implementation);
  • significantly simplifies the codebase for PaperWM's multimonitor support;
  • replaces around ~164 lines of (now) unneeded code with ~31 lines;
  • removes a complex part of multimonitor support and would allow us to add simple behaviours (based on user preference) like current windows lost focus when mouse move across monitor #389;

Note: we actually already use the PointerWatcher approach for multi-monitor drag/drop support - so this PR is essentially removing the other approach and adding a "activate space" method to the PointerWatcher.

@Lythenas and @Thesola10 - could you give this PR branch a test? I'll set it as a draft atm given it's quite a large change and I'm sure there might be some side-effects from this.

@Thesola10 - I note that this may impact some of your touch signals, although, we no longer have the ClickOverlay element blocking actions so it may turn out better (just a hypothesis).

@jtaala jtaala marked this pull request as draft January 14, 2024 06:16
@jtaala jtaala changed the title Replace ClickOverlay approach with simmple pointerWatcher. Remove Replace ClickOverlay approach with much simpler implementation Jan 14, 2024
with simple `PointerWatcher` approach.  Repurpose ClickOverlay to only for for left/right stackoverlay
(left/right window previews).
@jtaala jtaala force-pushed the remove-clickoverlay-implementation branch from fe897a9 to 8b5b711 Compare January 14, 2024 06:52
@jtaala jtaala changed the title Replace ClickOverlay approach with much simpler implementation Replace ClickOverlay enterMonitor reactive actor approach with much simpler implementation Jan 14, 2024
@jtaala jtaala marked this pull request as ready for review January 15, 2024 11:05
@Thesola10
Copy link
Collaborator

Thesola10 commented Jan 16, 2024

okay, the touch workspace switching is working a lot better. No issues to report.

Does not seem to impact #735 whatsoever.

Copy link
Collaborator

@Lythenas Lythenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good. To be honest it seems so simple compared to the click overlay...

I will use this for the next couple of days to see if I notice any issues.

stackoverlay.js Show resolved Hide resolved
Copy link
Collaborator

@Lythenas Lythenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work fine. I didn't notice any problems.

@jtaala jtaala merged commit 1db9292 into develop Jan 17, 2024
@jtaala jtaala deleted the remove-clickoverlay-implementation branch January 17, 2024 20:56
@Thesola10
Copy link
Collaborator

Thesola10 commented Jan 22, 2024

I may have failed to properly test this PR. As it turns out, this seems to introduce a regression of #655.

@jtaala
Copy link
Collaborator Author

jtaala commented Jan 22, 2024

I may have failed to properly test this PR. As it turns out, this seems to introduce a regression of #655.

Thanks @Thesola10,

My guess is that we may need to add a touch-event listener on space.background, similar to what was on the enterMonitor element.

I've added this on PR #755 (see commit 21efa47).

Note this may not do anything (and I don't have a touchscreen to test this on). Can you test 21efa47 and see if it responds to touch events on another monitor (it should activate that space on touch with this change). It may help in finding out more info here (or do nothing).

jtaala added a commit that referenced this pull request Jan 26, 2024
This PR implements some fixes for a regression to touchscreen support by
#751.

It also includes some smaller fixes implemented during development of
#755. Lastly, it includes replacing deprecated methods (which are
entirely removed int Gnome 46).

@Thesola10, can you give this branch a test and let me know if touch is
working again?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants