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

[StatusIndicator] Add method to get indicator icon screen rect. #89275

Merged
merged 1 commit into from
May 2, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Mar 8, 2024

Add status_indicator_get_rect method to get indicator rectangle in the screen coordinates. Usefully of positioning popups (existing methods only include mouse click coordinates).

@Mickeon
Copy link
Contributor

Mickeon commented Mar 8, 2024

I don't quite understand what this status indicator is, could you provide an example?

@bruvzg
Copy link
Member Author

bruvzg commented Mar 8, 2024

I don't quite understand what this status indicator is

System tray icons (StatusIndicator class):

image

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Aah, roger that.

doc/classes/DisplayServer.xml Outdated Show resolved Hide resolved
doc/classes/StatusIndicator.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Docs are just fine now, can't speak on implementation

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Code looks good.

@Riteo
Copy link
Contributor

Riteo commented Mar 8, 2024

Is there another usecase, outside of a popup? I wonder if we could perhaps add just an open_popup method of sorts, as I don't see this playing well with Wayland (when/if it will be implemented) at least.

@Mickeon
Copy link
Contributor

Mickeon commented Mar 8, 2024

Can't think of any on top of my head. If not for a popup it could be used for other visual flair (e.g. item inside the main program window moving towards the taskbar). Very niche! A popup sounds more common.

@bruvzg
Copy link
Member Author

bruvzg commented Mar 12, 2024

Is there another usecase, outside of a popup? I wonder if we could perhaps add just an open_popup method of sorts

The only info org.kde.StatusNotifierItem provide is position hint (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), and existing StatusIndicator callback already include click coordinates (currently it's absolute mouse position, but probably it would make sense to switch it to the same position hint instead for consistency, but I'm not sure what's point it should return exactly). But ideally you should know both tray icon rect and popup rect to position it perfectly, I do not think there's any workaround.

as I don't see this playing well with Wayland (when/if it will be implemented) at least.

Until full multi-window support is implemented, it's going to be completely useless for Wayland anyway. But I plan to implement org.kde.StatusNotifierItem (since it will work with X11 as well).

@Riteo
Copy link
Contributor

Riteo commented Mar 12, 2024

@bruvzg

Until full multi-window support is implemented, it's going to be completely useless for Wayland anyway.

Not necessarily, if there's an API for making "native" menus.

That said, I don't know the current platform APIs for this kind of stuff (if there's any) and probably we're going to make some non-screen-based ones anyways.

I'm only slightly concerned about the amount of duplicated/deprecated API surface we might have if we ever switch to (or just add) a window-centric API, which is what Wayland needs. We should really set up a meeting or something one day, as it's not a trivial matter IMO.

@bruvzg
Copy link
Member Author

bruvzg commented Mar 13, 2024

if there's an API for making "native" menus.

Most apps (at least on macOS and Windows) aren't using menus as popup (2/3 of indicators I currently have on macOS are fully custom popups). Not sure if there's any kind of native menu.

I'm only slightly concerned about the amount of duplicated/deprecated API surface we might have if we ever switch to (or just add) a window-centric API, which is what Wayland needs.

org.freedesktop.StatusNotifierItem use screen coordinates, so I do not think we have a choice. Also, how it can be window-centric when there's no window.

Edit: Other UI toolkits seems to handle it in the same way, e.g, https://doc.qt.io/qt-6/qsystemtrayicon.html#geometry

@Riteo
Copy link
Contributor

Riteo commented Mar 15, 2024

@bruvgz

Most apps (at least on macOS and Windows) aren't using menus as popup (2/3 of indicators I currently have on macOS are fully custom popups). Not sure if there's any kind of native menu.

org.freedesktop.StatusNotifierItem use screen coordinates, so I do not think we have a choice. Also, how it can be window-centric when there's no window.

Mh... Not sure about other platforms, but it looks like there's actually a native menu API for FreeDesktop in the indicator logic: org.freedesktop.StatusNotifierItem.Menu, which is described as OBJECT PATH: DBus path to an object which should implement the com.canonical.dbusmenu interface . This weirdly named interface is defined here: https://github.com/gnustep/libs-dbuskit/blob/4dc9b56216e46e0e385b976b0605b965509ebbbd/Bundles/DBusMenu/com.canonical.dbusmenu.xml

This stuff is ancient though, I'm not sure how much it's actually implemented but sway has a todo comment for that, so there's still some modern interest. We could investigate on whether KDE and GNOME implement it.

Edit: Other UI toolkits seems to handle it in the same way, e.g, https://doc.qt.io/qt-6/qsystemtrayicon.html#geometry

Note that Qt has also a QSystemTrayIcon::setContextMenu(QMenu *menu) method. Which makes it possible to do so without having direct access to the screen from the application, AFAICT. Note that it uses "custom" popups so they definitely don't use this DBus API.

I would really like to avoid adding another screen-based API, but if it becomes too complicated we could let this one in too and offset the problem to when we'll have to manage the other ones anyways.

@akien-mga
Copy link
Member

akien-mga commented Apr 29, 2024

So this got stale, can we reach a consensus?

It seems to me like we don't know yet whether FreeDesktop allows more than just screen coordinates for this API, so I feel like it might be best to merge this for now, and if we figure out another API would be better in the future, we can implement that and deprecate this one? Or have both options, which seems to be what FreeDesktop may offer?

@Riteo
Copy link
Contributor

Riteo commented May 2, 2024

@akien-mga

I'm pretty sure FreeDesktop indeed offers the APIs discussed above, as QT (or at least, telegram-desktop) tries to use it (by looking at dbus-monitor). I haven't tested this, but I suppose that KDE would show a native-looking menu.

That said, the only usecase provided here is making menus, which has already been addressed in #89588. It offers a new generic interface which allows platforms to handle menus however they please, without exposing platform-specific stuff to scripting.

In fact, by looking at the code it looks like the Windows backend is fetching the icon rect just fine through this new interface.

In any case, if there's still further need for this specific API, I'm not against merging this, as my main concern (generic menu handling) has been resolved.

@bruvzg
Copy link
Member Author

bruvzg commented May 2, 2024

The primary purpose of this PR is positioning and displaying custom non-menu popups (which is not fully supported by DBus, it only provides click coordinates, not icon rect).

You might also want to display some sort of popup without user clicking the tray icon (which seems to be impossible with DBus, but can be done on macOS and Windows, and will require icon rect).

@Riteo
Copy link
Contributor

Riteo commented May 2, 2024

@bruvzg you're right, DBus does not expose that information.

That said, as there are still usecases for other platforms outside of menus, so I'm all for it :D

@akien-mga akien-mga merged commit e249df4 into godotengine:master May 2, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants