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

Fixed main popup positions under Wayland #2086

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Conversation

tsujan
Copy link
Member

@tsujan tsujan commented Aug 21, 2024

… for bottom and right panels.

The problem was that some Wayland compositors returned global coordinates when anchors were applied, while some others (perhaps KWin) didn't. So, the patch uses local coordinates and changes them to the global ones in the end.

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.

I built it only with fancymenu to save time, it looks like there is a positioning issue with panels < 100% width now:

immagine

immagine

It opens 2x the px distanceof the screen border as the panel edge is.

… for bottom and right panels.

The problem was that some Wayland compositors returned global coordinates when anchors were applied, while some others (perhaps KWin) didn't. So, the patch uses local coordinates and changes them to the global ones in the end.
@tsujan
Copy link
Member Author

tsujan commented Aug 21, 2024

It opens 2x the px …

Oh, my bad! I forgot to apply a change that I'd made here. Nice catch! It's fixed now. Please try again.

@stefonarch
Copy link
Member

Looks fine now, nice work, it was worrying me quite a lot. Do we ship a point release?

@stefonarch
Copy link
Member

No need to test under x11, I forgot to ask.

@tsujan
Copy link
Member Author

tsujan commented Aug 21, 2024

Did you test it thoroughly under X11 too? I tested it only with top and bottom panels and also with auto-hiding under X11.

Do we ship a point release?

I don't think so. We didn't promise a really usable Wayland support for the panel in 2.0.0, and we don't have many Wayland users of 2.0.0 (maybe no one).

@tsujan
Copy link
Member Author

tsujan commented Aug 21, 2024

No need to test under x11

Why?!

@Bluey26
Copy link

Bluey26 commented Aug 21, 2024

Hi i installed it and restarted panel module. It was installed using:

source=("git+https://github.com/lxqt/lxqt-panel#branch=wayland_popup_position")

The popup position seems to be fixed, but the menu bar items are absent(all the opened windows). --> tested in wayland(labwc).

This happened to me too with one of the last panels i compiled, not sure why. Reverting the panel to:

source=("git+https://github.com/LXQt-Marcus-Fork/lxqt-panel#branch=wlroots_backend_patched")

seems to show my window icons again, after restarting panel module.

@stefonarch
Copy link
Member

You would need to apply the changes to @marcusbritanicus ' branch, maybe he'll include them soon.

@tsujan
Copy link
Member Author

tsujan commented Aug 21, 2024

but the menu bar items are absent

This branch doesn't contain the Wayland PRs. It's based on the master. None of the Wayland PRs is merged (because none of them is ready yet), and the master branch doesn't support task-bar on Wayland.

As I suggested in #2084 (comment), you could apply the patch to the source you used before. If that's not easy for you, you could wait until it's merged; then you could apply the patch at #2043 (comment) to the master.

EDIT: You can always extract the patch of any PR by adding .diff to its URI. For example, the patch of the current PR is available at https://github.com/lxqt/lxqt-panel/pull/2086.diff

@Bluey26
Copy link

Bluey26 commented Aug 21, 2024

Awesome! i just have managed to do it 😄 (git apply 2086.diff inside the folder)

I can verify that applying that patch to this: https://github.com/LXQt-Marcus-Fork/lxqt-panel/tree/wlroots_backend_patched
and then installing using ABS as usual seems to work. I restarted the panel module and now it works. I will try to add the exe name one to this.

Thanks a lot for the patience, i am still learning xD

@tsujan
Copy link
Member Author

tsujan commented Aug 21, 2024

i just have managed to do it 😄 (git apply 2086.diff inside the folder)

Yes, that's an effective way to do it.

Thanks for testing the PR. It'll be merged after being approved.

@marcusbritanicus
Copy link
Contributor

Tested on wlroots_backend_patched, and another local branch... Works like a charm. @tsujan Nice catch!!

@stefonarch
Copy link
Member

I applied the patch to Marcus' panel and see an inconistent behavior with right panel (probably the same with bottom panel):

  • panel configuration popup ok
  • directory menu plugin ok
  • menu + from status notifier ok

but all items inside statusnotifier have their right clicks popups at screen border.

@tsujan
Copy link
Member Author

tsujan commented Aug 22, 2024

but all items inside statusnotifier have their right clicks popups at screen border.

I intentionally used "main popup" in the title. I know about quick-launch and tray right click menus under Wayland, but they're not related to this PR.

@stefonarch
Copy link
Member

I know, thought just to mention it.

@tsujan
Copy link
Member Author

tsujan commented Aug 22, 2024

And please don't forget X11. Although I think a regression should be impossible, it needs thorough tests.

@stefonarch
Copy link
Member

Couldn't spot regressions on x11. But mainmenu is still aligned to bottom screen, but as it has bad issues anyway (search results) on wayland bottom panels we can ignore this IMO.

@tsujan
Copy link
Member Author

tsujan commented Aug 22, 2024

Couldn't spot regressions on x11.

Thanks for checking.

But mainmenu is still aligned to bottom screen

Its problem is like that of the other two cases. I'll investigate them later,. Let's keep this PR limited to LXQtPanel::calculatePopupWindowPos(), for having clean and separate commits.

@tsujan
Copy link
Member Author

tsujan commented Aug 22, 2024

I did a preliminary check and found out that those cases are all related to how QMenu::popup() currently works under the Wayland compositors that trigger the issue. I also checked and made sure that LXQtPanel::calculatePopupWindowPos() did its job correctly in their case.

@tsujan
Copy link
Member Author

tsujan commented Aug 22, 2024

For future reference:

It's getting more interesting! Actually, QMenu::popup() completely ignores the vertical positioning with a bottom panel under Wayland.

I tested it in both main-menu and status notifier by using QMenu::popup(POSITION - QPoint(100, 100)). While the horizontal position was respected, the vertical one was ignored.

So, the problem can be in QtWayland or layer-shell-qt or on a more basic level, but it isn't in our code.

@marcusbritanicus
Copy link
Contributor

If I understand correctly (I can be way off the mark on this one), on wayland, we have something called the xdg-positioner. It's duty is to position the popup. The values you give it are calculated relative to the parent. There are also options to set the anchor, gravity, size and so on.

It is quite possible that Qt is doing something wrong here. Or perhaps, Qt is moving the popup that's going "out of the screen", or something like that.

@tsujan
Copy link
Member Author

tsujan commented Aug 22, 2024

It is quite possible that Qt is doing something wrong here.

I agree. Since it doesn't happen when we show a popup explicitly, it may be in QMenu::popup() or some function that it calls. I might check it later.

@tsujan
Copy link
Member Author

tsujan commented Aug 22, 2024

I got bored and did another test, by using QMenu::setGeometry() plus QMenu::show() instead of QMenu::popup(). And it worked: the menu was shown at the correct position. It also proves that QMenu::popup() has a problem under Wayland. However, it's not a good workaround — at least not for Main Menu or Status Notifier — because it doesn't affect submenus or tooltips.

The fact that tooltips are also affected points to a deeper cause inside Qt.

@tsujan tsujan merged commit 1d287d5 into master Aug 22, 2024
@tsujan tsujan deleted the wayland_popup_position branch August 22, 2024 20:20
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.

4 participants