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

sni: fix passing relative coordinates to dbus methods #2417

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

Cherser-s
Copy link
Contributor

Fixes passing relative coordinates to ContextMenu, Activate, SecondaryActivate DBus methods, while handling mouse click events. Resolves #2165.

@alebastr
Copy link
Contributor

the x and y parameters are in screen coordinates and is to be considered an hint to the item about where to show the context menu.

This doesn't really do what you wanted: Wayland windows have no access to the global coordinate space, and root_x/root_y are calculated in a fake coordinate space originated from (0,0) of a toplevel surface (as explained in details in gdk/wayland/gdkwindow-wayland.c). So the change just replaces one set of wrong coordinates with another.

It might be possible to translate toplevel-based coordinates into global using the layer-surface size/margins/anchors and xdg-output data, but I don't think it worth the effort. No wayland-native apps will be able to use these values anyways.

@Cherser-s
Copy link
Contributor Author

Ok I implemented the coordinate translation, though I don't check the case if both bar size (width or height) and margins are being set for now.

src/modules/sni/item.cpp Outdated Show resolved Hide resolved
@alebastr
Copy link
Contributor

alebastr commented Aug 21, 2023 via email

@Cherser-s
Copy link
Contributor Author

Nope, the configure event just isn't triggered if window has sufficient size to begin with. I worked it around with also configuring offsets within the map event.

Copy link
Contributor

@alebastr alebastr left a comment

Choose a reason for hiding this comment

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

Please, clang-format your changes. It's enforced by CI, but apparently Github CI runs should be approved by repo owner for first-time contributions :(

For monitor geometry changes, consider following snippet

void onOutputGeometryChanged() { configureGlobalOffset(window.get_width(), window.get_height()); }

// Bar::Bar
  output->monitor->property_geometry().signal_changed().connect(
      sigc_mem_fun(*this, &Bar::onOutputGeometryChanged));

This should fix the global coordinates after scale factor change even if the surface size wasn't modified.

src/bar.cpp Outdated Show resolved Hide resolved
include/bar.hpp Outdated Show resolved Hide resolved
@Cherser-s Cherser-s force-pushed the sni-click-coordinate-fix branch 2 times, most recently from 834fd04 to b5dfe43 Compare August 22, 2023 07:02
Copy link
Contributor

@alebastr alebastr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing my comments.

@Alexays can you please approve the CI run for this PR?

@Cherser-s
Copy link
Contributor Author

Can output's geometry change without creating a new Bar object beforehand?

@alebastr
Copy link
Contributor

alebastr commented Aug 25, 2023 via email

@Cherser-s
Copy link
Contributor Author

Cherser-s commented Aug 25, 2023

Bar won't be recreated on that kind of change

Yup, if both width and height are set, then monitor geometry change handling needed

Doesn't correctly handle the case with both margin and width/height being set at the same time.
@Alexays
Copy link
Owner

Alexays commented Sep 1, 2023

LGTM, It's ok to be merged?

@Cherser-s
Copy link
Contributor Author

I would say yes, I didn't plan on adding anything else.

@Alexays Alexays merged commit 2d27e48 into Alexays:master Sep 4, 2023
8 checks passed
@Alexays
Copy link
Owner

Alexays commented Sep 4, 2023

Thanks!

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.

SNI tray icons don't receive screen coordinates
3 participants