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

StatusNotifierItem: announce version and status #2599

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rilian-la-te
Copy link
Contributor

Fixes #2598

According to https://bazaar.launchpad.net/~dbusmenu-team/libdbusmenu/trunk.16.10/view/head:/libdbusmenu-glib/dbus-menu.xml, there should be 4 properties, but I am implemented 2, which makes sense.

And here https://bazaar.launchpad.net/~dbusmenu-team/libdbusmenu/trunk.16.10/view/head:/libdbusmenu-glib/server.c
you can see, than libdbusmenu announces version 3.

@cschramm
Copy link
Member

cschramm commented Jan 4, 2025

Please provide some background on where you're coming from. I'm not keen to adopt a random version number from that library.

As you know, there is no actual specification, just that reference implementation if you'd like to call it that. Digging into its history makes me expect version 3 to imply support for event groups. We do not have that.

Looking at https://bazaar.launchpad.net/~dbusmenu-team/libdbusmenu/trunk.16.10/view/head:/libdbusmenu-glib/client.c#L1112, you see that all those properties are considered optional and that the version – if available – is actually used to check for >= 3 to enable event group support and nothing else.

I would not expect clients to require a version property and they definitely do not if they use libdbusmenu-glib. I'd also expect issues if we advertise version 3 to that library and thus most clients.

@rilian-la-te
Copy link
Contributor Author

rilian-la-te commented Jan 5, 2025

@cschramm you are right about version 3, and there is no support of event groups.

I need to know how to check out whether interface or functional or not, because sometimes tray clients did not announce menu at all, and it need to be checked, and check version field is best what I can think, because any of known implementations announce non-zero version, except Blueman one.

Copy link

sonarqubecloud bot commented Jan 5, 2025

@cschramm
Copy link
Member

cschramm commented Jan 7, 2025

I'm not sure I can follow. You seem to be coming from vala-panel-appmenu and as I see you already removed the version check there: https://gitlab.com/vala-panel-project/vala-panel-appmenu/-/commit/218bb4aaf1a1d9fcd89a71a14c8bf99cfd94ba1e.

If I get you right, the presence of a menu can be speculative at that point and you need to check if it holds as $something has such a menu but does not advertise it as org.freedesktop.StatusNotifierItem.Menu?

Two questions come to my mind:

  • Your importer object knows an object path. Where does that come from? Is it simply that of the StatusNotifierItem?
  • If $something does not advertise the menu, does it get picked up by other StatusNotifierHosts?

In any case, the goto solution would be introspection, assuming that $something provides it. Otherwise, you can still just try to use the interface, e.g. calling AboutToShow. libdbusmenu also just expects the interface and certain methods to be available on the provided objects and otherwise its calls will fail. Of course, it's up to the library's caller to specify the objects and it could possibly pre-check them (if they are speculative), but that's not encouraged in any way.

It also makes sense to only apply such extra checks in the speculative case and not in cases like blueman where the menu got advertised.

I don't know if there are other implementations out there that do not provide a version, but we have not had any issues with that so far – in line with libdbusmenu's behavior –, so requiring one seems risky. Also, the value appears pointless; we're neither 1 (where the property would even be version, not Version, by the way) nor 3 and there are so many releases of libdbusmenu that advertise version 2 but provide different interfaces (e.g. both version and Version variants), that I would not call that an information. 😅

@rilian-la-te
Copy link
Contributor Author

@cschramm

Yes, I am behind vala-panel and vala-panel-appmenu.

Why I added a version check in a first place? There is a cases (however, more with actual menus, not with items), when menus are advertised, but did not shown at all. So, I need to use some kind of checks about it.

Object path is taken directly from StatusNotifierItem, yes, and do not checked at all (after version check removal).

How menu is related to StatusNotifierHosts? if StatusNotifierItem exists, host will take it, but will show this item without a menu at all.

@cschramm
Copy link
Member

cschramm commented Jan 9, 2025

So it's the other way around? There are cases where org.freedesktop.StatusNotifierItem.Menu is given, but the advertised menu is not functional? I'd say you just need error handling for the method calls then; doesn't work as expected? => forget the menu. If you want to know beforehand, you can call a simple method like AboutToShow.

Getting far off-topic, but what software is an example for such a broken menu?

@rilian-la-te
Copy link
Contributor Author

There are cases where org.freedesktop.StatusNotifierItem.Menu is given, but the advertised menu is not functional?

Not org.freedesktop.StatusNotifierItem.Menu, but a menu from com.canonical.AppMenu.Registrar, because applications registered a menu, but did not build it yet.

@cschramm I encountered it with Idea, and with Firefox-Ubuntu (it advertised a dummy menu). It was 2 years ago, and I did not know is this still a case.

If you want to know beforehand, you can call a simple method like AboutToShow

AFAIK, this method did not return a value.

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.

StatusNotifierItem: announce version 3 of the DBusMenu protocol
2 participants