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.

Sign up for GitHub

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

Jump to bottom

Update workspaces on screen change #1058

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Update workspaces on screen change #1058

wants to merge 10 commits into from

Conversation

Eskaan
Copy link
Contributor

@Eskaan Eskaan commented Apr 23, 2023

Description

Update workspace configuration on XRRCrtcChangeNotifyEvents from Xrandr (When anything of the monitor configuration gets changed).

State of draft

  • Read XRRCrtcChangeNotifyEvents from xlib Events
  • Update workspace(s) corresponding to the Crtc

Details about XRRCrtcChangeNotifyEvent

From my research, a crtc is a virtual output that gets rendered by X11, whilst an output is a reference to the actual monitor (or similar, I am not really sure about that). The screen is the entire rendered area, i.e. everything covered by the DISPLAY env variable.

An XRRCrtcChangeNotifyEvent gets emitted, when one of said crtc's is changed. So for example, when a output is disabled, is moved, or the resolution changes. It also gets emitted for crtc that automatically move in place for the disabled crtc/output (beacause one crtc must always be at x:0, y:0).

Type of change

  • Development change (no change visible to user)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation only update (no change to the factual codebase)
  • This change requires a documentation update

Checklist:

  • Ran make test-full locally with no errors or warnings reported
    Note: To fully reproduce CI checks, you will need to run make test-full-nix. Usually, this is not neccesary.
  • Manual page has been updated accordingly
  • Wiki pages have been updated accordingly (to perform after merge)

@VuiMuich
Copy link
Member

Nice find!
Is this intended to work without SoftReload, that would be tremendously awesome. Then we would just need a smart way to get the additional bars started/killed.

@Eskaan
Copy link
Contributor Author

Eskaan commented Apr 24, 2023

Is this intended to work without SoftReload

Yes.

Then we would just need a smart way to get the additional bars started/killed.

Something I haven't thought about yet, but yes, this is important.

For some time now, I thought about maybe doing some hooks for after-initial-startup and similar for quite some time now.
I always have to run my xrandr script manually because I need to SoftReload after it and putting that in up causes a boot loop whilst in .xinitrc it dosen't work (I think X11 isn't fully initialized yet at this point).

@VuiMuich
Copy link
Member

I just had a bunch of crazy ideas, but I haven't had any looks at the code at all, so no idea, if those are even remotely feasible:

  • Make the SoftReload able to take a bool no-up-script and/or Option<String> with a path to a script to run instead
  • this might be easier to implement, if leftwm-command would have actual subcommands instead of taking a single String argument. But ofc this would be quite a big refactor, albeit a quite nice one as well. Real bummer with this one: we would break theme compatibility again.

@Eskaan
Emit new display events, move stuff around (where it belongs) and add…
34858b8 
… a test:

New events: ScreenUpdate and ScreenDelete are now being emited when the layout changes (update includes create)
Renamed initial_events to event_queue to avoid confusion
Moved workspace-determining code from initial_events() to screen_crate_handler(), this is core; not display-server territory
Added test for current workspace-id assignment scheme to verify the new code
Eskaan
Eskaan commented Apr 25, 2023
View reviewed changes
display-servers/xlib-display-server/src/event_translate.rs Outdated
Comment on lines 256 to 258
r#"Leftwm does not support more than one output per crtc (if that is even possible to have).
LeftWM will only apply changes to the first output.
If you are seing this error, please create an issue on our GitHub page and it will be resolved."#
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VuiMuich @hertg what do you think about that?

I have tried some things, but I haven't goten xrandr yet to put both of my monitors (outputs) on one crtc. It is possible to return a Vec<Screen> here, but I don't think this kind of exotic configuration even exists (if possible). I've added this error to indicate an issue. This is not fatal at all, you will just have to SoftReload like before to reset the configuration.

Copy link
Member

Choose a reason for hiding this comment

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

I have to admit that I'm a bit too out of the loop to actually understand what you mean by outputs and crtc. But I like your thinking ahead and very verbose log message. I fully approve of, and appreciate your approach.

Copy link
Contributor Author

@Eskaan Eskaan Apr 25, 2023 •
edited

Choose a reason for hiding this comment

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

image
...I am still trying to understand that myself. I haven't found no real documentation on the internal XRandR API whatsoever. I just try to get together what I can.

I think there are the output, the crtc and the screen. The screen is everything covered by the DISPLAY env variable, whilst the output corresponds to a hardware monitor and the crtc is basically the virtual representation of the output.

EDIT: e.g. So the output sets the resolution and the crtc the position on the screen

@Eskaan
Copy link
Contributor Author

Eskaan commented Apr 28, 2023

I have this feature at a working state now, but of course there is still some stuff to do, as most themes will not work with this. For now this is working:tm:, you can check it out if you want and make a few tests yourself.

It's now time to discuss how to implement the theme update hooks (breaking or not!).

I'll be waiting 'til after #1000 anyways to not make merge conflicts for that beast.

Btw, rerun the test ci please, that didn't fail because of me.

@hertg
Copy link
Member

hertg commented Apr 28, 2023

triggered a re-run, it went through now

@Eskaan Eskaan mentioned this pull request Feb 6, 2024
Open
@Eskaan
Copy link
Contributor Author

Eskaan commented Feb 7, 2024

I just updated this branch in case anyone wants to test it (I didn't yet and don't exactly expect this to behave after a 3.5k line diff merge).

It's still not finished but should work for now by calling the up script on screen change, which should in term reload the theme.

@Eskaan
Copy link
Contributor Author

Eskaan commented Feb 7, 2024

Update: After short testing I noticed two issues:

  • We aren't deduplicating screen updates, so the up scripts are called multiple times in a short time (asynchronously!), I don't think that's good for most scripts and, for example, is (presumably) resulting in a core dump from picom for me (the process gets killed before it can boot correctly).
  • Some focus issues (?) after creating a new workspace. It's just one tag on two workspaces, but it's fixing itself after calling a different tag to one of them. I'm unsure if this problem is related to the pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hertg hertg hertg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Eskaan @VuiMuich @hertg