ubuntu / gnome-shell-extension-appindicator

Adds KStatusNotifierItem support to the Shell
https://extensions.gnome.org/extension/615/appindicator-support/
GNU General Public License v2.0
1.21k stars 163 forks source link

Some things appear to be inconsistent #198

Open lestcape opened 5 years ago

lestcape commented 5 years ago

Is supposed that, this code, will control the indicator, but is never executed, because the indicator was implemented using a shell PanelMenu.Button and this component in a button press will forced the menu to be toggled not matter what.

On the other hand, probably is good that this happens, because where is the Activate signal in the libAppIndicator implementation of Ubuntu? I only can see the SecondaryActive signal. It need to be something like this patch on Arch.

JasonLG1979 commented 4 years ago

Even if they enabled the activate signal it still would not work as intended. They return nothing which is the equivalent to returning Clutter.EVENT_PROPAGATE so the event is propagated to the next handler or the vfunc_event virtual function in this case. At any point in the function that they want to stop the menu from being toggled they should return Clutter.EVENT_STOP to stop the event from being propagated.

To answer this code comment:

   //HACK: event should be a ClutterButtonEvent but we get only a ClutterEvent (why?)
   //      because we can't access click_count, we'll create our own double click detector.

You're not getting button events because despite it's name PanelMenu.Button is not a button, it's an St.Widget.

JasonLG1979 commented 4 years ago

Something like this should fix the situation:

    _boxClicked(actor, event) {
        let button = event.get_button();
        if (button == Clutter.BUTTON_MIDDLE || button == Clutter.BUTTON_SECONDARY) {
            // activeMenu can be null, don't call close on null...
            let activeMenu = Main.panel.menuManager.activeMenu;
            if (activeMenu) {
                Main.panel.menuManager._closeMenu(true, activeMenu);
            }
            if (button == Clutter.BUTTON_MIDDLE) {
                this._indicator.secondaryActivate();
            } else {
                this._indicator.open();
            }
            return Clutter.EVENT_STOP;
        }
        return Clutter.EVENT_PROPAGATE;
    }

This also puts a stop to the double click nonsense. In this Clutter.BUTTON_MIDDLE is secondary activate, Clutter.BUTTON_SECONDARY (right click for most people) is activate and every other bottom press falls though and toggles the menu.

JasonLG1979 commented 4 years ago

As far as libAppIndicator missing the Activate method that's pretty easy to work around.


function activate(appId) {
    let desktopId = `${appId}.desktop`;
    let app = Shell.AppSystem.get_default().lookup_app(desktopId);

    if (app && !app.is_window_backed() && !app.get_busy()) {
        app.activate();
    }
}
lestcape commented 4 years ago

@JasonLG1979 thanks. You have right, but I think none of us can answer why the Activate signal is missing in the Ubuntu implementation. So, my point is what is the rationale behind it? After have an answer to that question, I think that we can think in some possible solution to what happens. But I suspect that we will not get an answer to that thing in first place because some how is an intended behaviour in Ubuntu.

Unfortunately, I think it is a shame that each distro does the implementation of the indicators in his own way, by modifying what in principle should be common to all. I think that the reason of a protocol is precisely to define how it should be for everyone and in consequence make all developers in agreement. When i see a variation like that, i simple truly disagree that this should exist. That is why i opened the issue in first place.

JasonLG1979 commented 4 years ago

You have right, but I think none of us can answer why the Activate signal is missing in the Ubuntu implementation.

That's really a moot point (or question I guess?) libAppIndicator was just a poorly written abstraction layer over the org.kde.StatusNotifierItem spec and now it's unmaintained. It should just be left to die.

I think it is a shame that each distro does the implementation of the indicators in his own way.

They really don't have to. org.kde.StatusNotifierItem is still a thing.

lestcape commented 4 years ago

libAppIndicator was just a poorly written abstraction layer over the org.kde.StatusNotifierItem spec

To me seem to be more like an unfinished implementation of the org.kde.StatusNotifierItem protocol than a layer over it.

They really don't have to. org.kde.StatusNotifierItem is still a thing.

org.kde.StatusNotifierItem is just a protocol and then it should have a usable implementation. So, if you have the plan of implement the protocol fully, i don't think you can just be left to die the libAppIndicator implementation. I think that write it from scratch seem to be less practically. Instead i think is better to improved the libAppIndicator to satisfy the protocol specifications.

Anyway, it's just not only Ubuntu, everybody out there have his own implementation or want to create a new one that really-really will solved the problem of the system-tray with his own vision. That not help if you are a simple developer of an application and you want to add an icon to the system-tray in Linux, not matter in what environment or distribution you are.

JasonLG1979 commented 4 years ago

I think that the reason of a protocol is precisely to define how it should be for everyone and in consequence make all developers in agreement. When i see a variation like that, i simple truly disagree that this should exist. That is why i opened the issue in first place.

No one will ever totally agree and you can't force them. A good interface separates information from presentation. (libAppIndicator failed spectacularly at that separation) The interface simply exposes well defined properties, signals and methods and it is left up to the consumer of the interface to decide how to present the information to the user, if they do at all, in a manner that is consistent from app to app. By consistent I don't mean everything looks or even behaves the same across different desktops. What I mean is that on that particular desktop all tray icons behave the same as decided by the devs of that desktop not by the spec and not by the apps. Just look at MPRIS and the notification spec as examples of that.

Instead of one giant convoluted tray icon spec that tries to do everything a few small purpose built specs would do much better. Really what to we need? What apps actually need a tray icon? I'm going to go out on a limb and say that most apps don't really need a tray icon, and that probability at least 1/2 of the ones that do could accomplish the same thing by just sending a notification if they want the user to know something.

lestcape commented 4 years ago

No one will ever totally agree and you can't force them.

In the case that not one can be fully convinced, i prefer have nothing than instead have something that only will work in some specific place. So, to me If it is not a global effort, it is just lost energy. And if it will have a consensus, some one will take the first step, rigth?

In my opinion, this type of things can not be designed as a desktop feature, instead it should be see as an application feature that the desktop will implement. Then, if it designed for a multiplatform applications and is not a cross platform solution, it should not exist because instead of resolve something to the developer, what this will create is one possibility more to worried about.

Of course, this is my opinion. To me is wrong contribute to something that have not the intention to provide a secured API to the Linux application developers. In fact I always thought that libAppindicator tried to unite the world of KDE with the GNOME one. Looking at things again, I think it seems that there is now a third world different from the other two.

JasonLG1979 commented 4 years ago

To me seem to be more like an unfinished implementation of the org.kde.StatusNotifierItem protocol than a layer over it.

I think you're confused. Appindicator is not an implementation of org.kde.StatusNotifierItem it's clearly an abstraction layer. It's a dumbed down shim between an app and org.kde.StatusNotifierItem because naked org.kde.StatusNotifierItem is somewhat convoluted.

org.kde.StatusNotifierItem is just a protocol and then it should have a usable implementation.

Again you are confused. org.kde.StatusNotifierItem is a dbus interface spec it is not a thing. It's just a spec that defines how clients (desktops) and servers (apps) talk to each other. It does not decide how desktops are supposed to present anything.

i don't think you can just be left to die the libAppIndicator implementation. I think that write it from scratch seem to be less practically. Instead i think is better to improved the libAppIndicator to satisfy the protocol specifications.

It's already dead it died with Unity.

That not help if you are a simple developer of an application and you want to add an icon to the system-tray in Linux.

I would think long and hard if you really need a tray icon. More than likely you don't. Aside from maybe chat or cloud sync apps I can't really think of apps that need a tray icon.

i prefer have nothing

So do I. I'm not really a fan of the panel dumpster.

In my opinion, this type of things can not be designed as a desktop feature, instead it should be see as an application feature that the desktop will implement. Then, if it designed for a multiplatform applications and is not a cross platform solution, it should not exist because instead of resolve something to the developer, what this will create is one possibility more to worried about.

You can't force anyone. People have different ideas of what makes for good design.

lestcape commented 4 years ago

I think you're confused. Appindicator is not an implementation of org.kde.StatusNotifierItem it's clearly an abstraction layer.

Well, in my opinion you can not construct an abstraction layer without to have an implementation, because to add a layer over something, you need to have this "something" first. And in the case of LibAppindicator, there are not this "something" anywhere. So, to me is not a layer, but this is conceptually speaking. The StatusNotifierItem by his own is an specification and then need an implementation (like this one) to then have a layer later. Anyway, you can said that LibAppindicator is a different protocol than StatusNotifierItem and unfortunate you probably have right.

JasonLG1979 commented 4 years ago

You're still confused. Clients (desktops or whatever wants to expose things to the user) and servers (apps) implement their respective sides of the interface. LibAppindicator does neither of those things. LibAppindicator is a middle man. It translates. It is not an implementation. This extension is an example of a client side implementation. The code to expose the interface in an app is the server side implementation. Saying that LibAppindicator is an implementation of StatusNotifierItem is like saying C is an implementation of X86 assembly because the C compiler can speak X86 assembly.

JasonLG1979 commented 4 years ago

There is a distinct difference between an abstraction layer and an implementation.

JasonLG1979 commented 4 years ago

And in the case of LibAppindicator, there are not this "something" anywhere.

Look at the dang source code. LibAppindicator is using the StatusNotifierItem interfaces behind the scenes. There is nothing you can do in LibAppindicator that you can't do in StatusNotifierItem...

JasonLG1979 commented 4 years ago

LibAppindicator was garbage and widely regarded as such. It's right up there with libNotify. Just give it up. If really want a tray icon learn how to use StatusNotifierItem.

bluppfisk commented 4 years ago

I would think long and hard if you really need a tray icon. More than likely you don't. Aside from maybe chat or cloud sync apps I can't really think of apps that need a tray icon.

I agree that status icons are overused. But would you really rather have an empty bar taking up valuable vertical screen real estate while you could display valuable information there? You can disagree about the aesthetics but I like having a spot where I can monitor information that is important to me (e.g. a stock ticker).

Just leaving the bar empty with only the time and a wifi icon is a waste imho.