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

Handle Wayland screens by using their names #2226

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Handle Wayland screens by using their names #2226

merged 2 commits into from
Jan 23, 2025

Conversation

tsujan
Copy link
Member

@tsujan tsujan commented Jan 20, 2025

The main reason behind this change is that screen setting by using numbers may not work with all Wayland compositors.

With this patch, and on Wayland,

  • The screen names are used for determining where the panels should be shown;
  • A panel is not moved to another screen when its screen is disconnected (unlike on X11);
  • If the screen assigned to a panel is not connected on startup, that panel won't be created, but it will be created as soon as its corresponding screen is connected.

IMPORTANT NOTE: Since I couldn't test this patch, I made it based on some assumptions. For example, I don't know if Qt or the compositor hides a panel, automatically moves it to another screen, or just causes a crash when its screen is unplugged. Here, I've assumed that it hides the panel. So, the patch should be tested thoroughly, and it might need changes.

@tsujan tsujan requested a review from stefonarch January 20, 2025 16:39
@marcusbritanicus
Copy link
Contributor

I have not tested this patch, but I did test the latest release of lxqt-panel with two screens. When a screen which has lxqt-panel closes, it is not moved to the other screen. It is simply hidden. When both screens are closed, both the bars are hidden, and the panel stays alive.

IMPORTANT NOTE: Since I couldn't test this patch, I made it based on some assumptions.

I am assuming you'd be using labwc, or wayfire. In either case, you can run a nested session with multiple screens: WLR_WL_OUTPUTS=2 labwc give you two screens. Same for wayfire. To start an app inside this session, use the command: WAYLAND_DISPLAY=wayland-X <app_name>. You'll have to do a little bit of trial and error to figure out X.
If you're running wayfire as the host session, and labwc nested session, X will be 0. If you're running labwx as host and wayfire or labwc as nested, then X will generally be 1.
If both host and the nested sessions are wayfire, then X will be 2.

One disadvantage is that once closed, you cannot re-open the screens.

Also, do note: even after closing both (or all) screens, the compositor process will still stay alive. Remember to close/kill it.

@stefonarch
Copy link
Member

I have not tested this patch, but I did test the latest release of lxqt-panel with two screens. When a screen which has lxqt-panel closes, it is not moved to the other screen. It is simply hidden. When both screens are closed, both the bars are hidden, and the panel stays alive.

Sure you had the latest release? It used to jump to the internal monitor here after #2181

The main reason behind this change is that screen setting by using numbers may not work with all Wayland compositors.

With this patch, and on Wayland,

 * The screen names are used for determining where the panels should be shown;
 * A panel is not moved to another screen when its screen is disconnected (unlike on X11);
 * If the screen assigned to a panel is not connected on startup, that panel won't be created, but it will be created as soon as its corresponding screen is connected.

IMPORTANT NOTE: Since I couldn't test this patch, I made it based on some assumptions. For example, I don't know if Qt or the compositor hides a panel, automatically moves it to another screen, or just causes a crash when its screen is unplugged. Here, I've *assumed* that it hides the panel. So, the patch should be tested thoroughly, and it might need changes.
@tsujan
Copy link
Member Author

tsujan commented Jan 20, 2025

I am assuming you'd be using labwc, or wayfire.

My problem is just that I don't have access to my external monitor for now :) But even if I did, I could never test as thoroughly as @stefonarch can.

@tsujan
Copy link
Member Author

tsujan commented Jan 20, 2025

@marcusbritanicus
BTW, could you please add support for cosmic-workspace-unstable-v1 to our wlroots backend (or maybe to a new backend based on it)? That area is terra incognita to me, but your code would shed a lot of light on it for me.

@stefonarch
Copy link
Member

First tests looks ok now.
WLR_WL_OUTPUTS=2 labwc -C /tmp -S qterminal is nice, but I prefer real hardware atm with my testuser on tty2, thanks @marcusbritanicus

@tsujan you meant "ext-workspaces" as that's now the one I guess, xfce4-panel has already implemented support and it's in labwc git.

@tsujan
Copy link
Member Author

tsujan commented Jan 20, 2025

@tsujan you meant "ext-workspaces"

I'm not sure. Stable labwc doesn't support it yet, but will soon. But whatever is better — still terra incognita ;)

Copy link
Member

@stefonarch stefonarch left a comment

Choose a reason for hiding this comment

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

Couldn't find any issue with compositors and on x11 behavior is like before too. Before panels on different monitors would be inverted by sway and kwin_wayland, and by Niri randomly.

Maybe we should change "desktop 1,2,3" in the GUI to "screen" later.

@@ -215,8 +216,11 @@ LXQtPanel::LXQtPanel(const QString &configGroup, LXQt::Settings *settings, QWidg
connect(qApp, &QApplication::screenRemoved, this, [this] (QScreen* oldScreen) {
disconnect(oldScreen, &QScreen::virtualGeometryChanged, this, &LXQtPanel::ensureVisible);
disconnect(oldScreen, &QScreen::geometryChanged, this, &LXQtPanel::ensureVisible);
// wait until the screen is really removed because it may contain the panel
QTimer::singleShot(0, this, &LXQtPanel::ensureVisible);
if (QGuiApplication::platformName() != QStringLiteral("wayland"))
Copy link
Member

Choose a reason for hiding this comment

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

Could this be default but a setting to change for both x11 and wayland?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess what you mean is monitor-specific panels for X11 too. That may be possible in the same way, but

  • I really prefer not to touch X11 here (in order to avoid any chance of regression);
  • There are some codes for X11 to forcefully move the panel, and I think there should have been a good reason for including them;
  • To work on X11, I should log into an X11 session, which is so annoying ;)

Maybe later, in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

That could also be an option, but I didn't meant this.
I meant that also on wayland panels could move to the other monitor if their monitor disappears, optionally. Situations like people which sometimes work with only the laptop monitor and other times using only an external one, now they would need to recreate a panel.

Copy link
Member Author

Choose a reason for hiding this comment

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

Situations like people which sometimes work with only the laptop monitor and other times using only an external one

With this patch, they'll have no problem :) That panel(s) of each monitor will be created and shown as soon as it's connected. LXQtPanelApplication::handleWaylandScreenAdded is about that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, your comment showed me that I should remove a case of return;. Doing it… Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Not really... maybe you have 2 panels and want just them to go the big screen if you turn off your laptop screen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

maybe you have 2 panels and want just them to go the big screen if you turn off your laptop screen.

The new logic is that of screen-specific panels, which is at odds with the above. And there's no practical benefit to the latter.

@tsujan
Copy link
Member Author

tsujan commented Jan 21, 2025

Maybe we should change "desktop 1,2,3" in the GUI to "screen" later.

I think numbers are more user-friendly than names. They are what the user sees, but what is really used on Wayland is screen names, not numbers.

@stefonarch
Copy link
Member

Maybe we should change "desktop 1,2,3" in the GUI to "screen" later.

I think numbers are more user-friendly than names. They are what the user sees, but what is really used on Wayland is screen names, not numbers.

No, I meant replace the work "desktop" with "screen" or "monitor" only.

@tsujan
Copy link
Member Author

tsujan commented Jan 21, 2025

I meant replace the work "desktop" with "screen" or "monitor" only.

I completely agree. "Desktop" is misleading here. We can change it to "Screen" without touching the config key (because changing the key will be backward incompatible).

Let's do it in another PR.

@stefonarch
Copy link
Member

Let's do it in another PR.

Sure. I wonder also if we could change the blank field when the panel can't be shown where it was set by the config.

@tsujan
Copy link
Member Author

tsujan commented Jan 21, 2025

A screenshot?

The user may have assigned multiple panels to that screen.
@stefonarch
Copy link
Member

stefonarch commented Jan 21, 2025

A screenshot?

immagine

You'll get that if you have a panel at the outer right or left and then invert screen position. Could be a text, but isn't a big matter. Btw maybe on wayland a panel could be also on left of the right monitor?

@tsujan
Copy link
Member Author

tsujan commented Jan 21, 2025

You'll get that if you have a panel at the outer right or left and then invert screen position.

Understood. Yes, it could be better. It isn't specific to Wayland or X11.

Btw maybe on wayland a panel could be also on left of the right monitor?

Exactly the same problem of X11 might happen here. So, better safe than sorry.

@stefonarch
Copy link
Member

stefonarch commented Jan 22, 2025

I found now that the panels created on wayland under any compositor are then inverted (on the other screen) on x11, with any WM.
But when I assign it their monitors on X11 it will be fixed on both sides.

@tsujan
Copy link
Member Author

tsujan commented Jan 22, 2025

The code doesn't touch the screen number (mScreenNum) under Wayland. I guess it happened during you many tests because of addition/removal/moving of a panel.

Please tell me if it happens again without you doing anything other than switching between X11 and Wayland: no panel addition, removal or movement.

@stefonarch
Copy link
Member

The 2 test panels were added and edited (moved between screens) on wayland, on x11 they resulted inverted, same for a third panel added under kwin_wayland or labwc: logout and login in x11 the new panel is found on the other screen.

Panels created under x11 are were they should be under wayland (have to recheck that though).

@tsujan
Copy link
Member Author

tsujan commented Jan 22, 2025

If a panel is added/moved under Wayland, there's no guarantee that its screen is the same on X11. The reason is that the left screen isn't necessary the first one (with the index 0) on Wayland.

@tsujan
Copy link
Member Author

tsujan commented Jan 22, 2025

Panels created under x11 are were they should be under wayland (have to recheck that though).

Again, there's no guarantee, for the same reason I mentioned above. I think only a few Wayland compositors may respect that — Labwc may be among them.

@tsujan
Copy link
Member Author

tsujan commented Jan 22, 2025

I explain it in details:

Suppose that a user has two monitors and adds one panel to each on X11. His config file only has desktop=… for those panels, the desktop being only for X11. Now, if he switches to Wayland, his panels may be reversed (for the above-mentioned reason). But if he corrects them, the key screen-name will be added to his config file. From then on, his panels should be on correct screens on both X11 and Wayland, because both keys are present.

Almost the same argument holds for panels that are set up on Wayland first.

@tsujan tsujan merged commit af49de6 into master Jan 23, 2025
@tsujan tsujan deleted the wayland_screen branch January 23, 2025 07:22
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