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

Improving slightly the appindicators. #4814

Closed
wants to merge 16 commits into from
Closed

Improving slightly the appindicators. #4814

wants to merge 16 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 10, 2015

  • Resize the status indicator and the appindicator icons in the same way.
  • Do not redisplay status icons in a panel change (just resize it).
  • Dynamically enable and disable the appindicators using the value of the global.settings.
  • Dynamically enable and disable the appindicator when an applet with a same role will do the opposite action. This also will unregistered the appindicator Dbus iface.
  • Added shortcut support.
  • Encapsulate the blacklist functionality inside the manager.
  • Reorganize the position of starting, to allow the access to the systray in the initialization.
  • Can be decided the behavior for the indicators menu (right click or left click).
  • Added support for label (to the right side of the icon) if somehow is possible.
  • Added support for tooltip, showing the appindicator "title" property as a tooltip. The indicator "tooltip" property is an array and could be in html format, so will require a webkit to be properly displayed. Anyway, the support for the html format, need to be placed inside the Cinnamon Tooltip implementation (not inside the appindicator).

- Resize the status indicator and the app indicator icons in the same way.
- Do not redisplay status icons in a panel change (just resize it).
…inside the api, to remove the GDbus interface of a blacklisted appindicator.
@ghost ghost changed the title Resize the icons in the same way. Improving slightly the appindicators. Nov 11, 2015
@clefebvre clefebvre changed the title Improving slightly the appindicators. [3.0] Improving slightly the appindicators. Nov 11, 2015
…e the position of starting, to allow the access to the systray in the initialization.
@ghost
Copy link
Author

ghost commented Nov 12, 2015

Now is possible merge the systray manager and the indicator manager inside only one class easy (as a decision).

Please note the difference between the blacklist of the the indicator manager and the action of "black list" of one icon (really do not add the icon). Please see, the indicator manager can be used by several applets (or instances) at the same time. The black list prevented the creation of any icons with the specific appindicator id and don't add an icon is only just a local decision in the current applet instance.

@ghost
Copy link
Author

ghost commented Nov 12, 2015

As was commented in this issue:
#4741

Please note is possible open the main windows assigned to an indicator using this function:
https://github.com/linuxmint/Cinnamon/blob/master/js/ui/indicatorManager.js#L410

Of course will be necessary define a compatible behaviour to be possible then use this characteristic.

…eft click). Add support for label (to the right side of the icon) if somehow is possible. Add support for tooltip, showing the indicator tooltip property or otherwise if is not possible will be displayed the title property as a tooltip.
@ghost
Copy link
Author

ghost commented Nov 15, 2015

indicator

@ghost
Copy link
Author

ghost commented Nov 15, 2015

psensor

@Givrix
Copy link

Givrix commented Nov 18, 2015

AppIndicators surely offer a slicker integration with the theme.
However I can't help regretting the click efficiency of the old systray. (Show/hide with left click, menu with right click, and functions with middle click if relevant)
That was for me a major reason to quit unity and use cinnamon instead.
So I found the option to disable indicators in Settings/General, but some icons seem unavailable now like dropbox and the update notifier.

Would there be a way to select which mode is the default instead of enabling/disabling AppIndicators?

@ghost
Copy link
Author

ghost commented Nov 18, 2015

@Givrix internally there are a switch. The code observed if there are an interface of appindicator available. If there are one then will be used, if not then will not be used and will be used the old behavior. If there are a problem with the old behavior i think is related with other things and not with appindicators. What you want is what currently occurs and in this pull request this occurs dynamically, without need to restart cinnamon when you disable indicators in Settings/General.

Another thing is that i added an option to use the appindicators with the right click instead of the left one as a user decision. I don't know if this option will be added in the general settings, or will be a hidden option, but is an option. Maybe this is what you are looking for.

In any case i need to agree with you about enable indicator by default. When you introduce a new feature is better make it as an option, then if the users want the new option you can set it as the default.

Regards.

…tml format. Fix an invalid code and return St.Icon instead of Clutter.Actor in the pixmap construction to be possible apply the style-class property.
@brownsr
Copy link
Member

brownsr commented Jan 1, 2016

Hi lestcape. Would it be possible to have the indicators managed as a set in the systray along with the icons ? At the moment I can get inconsistent spacing in the production version because the app icons are managed as a set with a controlling layout manager, but then the indicators are added afterwards. Minor, true, but it would make the icons in the systray homogeneous in appearance, as they used to be in Cinnamon 2.6. Example below, the icons for hexchat and the update manager are followed by two indicators for blueman and tomboy notes, and you can see the spacing difference.
systray
I have another reason unrelated to the production code, which is that I am working on a PR for vertical panels, and as things currently stand the systray does not behave as one visual unit when dropped onto a vertical panel. Example below, you can see how the app icons are doing what I have told them to, but the indicators are behaving differently.
systray2

I really like what the indicators do by the way, having them pick up on the theme styling is great, and I hope I have understood the situation correctly in what I have written above.

Regards

@ghost
Copy link
Author

ghost commented Jan 1, 2016

Hi @brownsr thanks for that.

Can you please tell me what you consider better?
The status icons are handled by a layout manager here: https://github.com/linuxmint/Cinnamon/blob/master/files/usr/share/cinnamon/applets/systray%40cinnamon.org/applet.js#L62

The indicator icons are added to the main actor here:
https://github.com/linuxmint/Cinnamon/blob/master/files/usr/share/cinnamon/applets/systray%40cinnamon.org/applet.js#L129

Then, we can have a new manager for indicator, or we can merge the indicators in the same layout manager of the status icons. Also indicators can be set to the right or to the left.

The question is, what will be better to you?

Thanks for the vertical panel, if you required any help. Here I'm.

Regards.

@brownsr
Copy link
Member

brownsr commented Jan 1, 2016

Thanks for the reply lestcape, I tried using the layout manager of the status icons at the point the indicators were set up in iconActor in the indicatorManager, but the type of objects used for the indicators did not accept a layout manager. I did manage to get things to work from an alignment point of view by assigning to manager_container rather than to the main actor at the second point you linked to. I got almost immediate hangs through, presumably because the status icon logic does loops through the children of the manager_container clearing and resetting them, and I don't imagine that reacts too well if some of the entries are actually to indicators :-) It ought to be possible to rewrite this logic to filter out array entries processed in this way to just those that belong to status icons [though there is a test at one point that assumes 'not found' is the start of the array, which was taxing my mind as to how to handle it], but at that point I saw your earlier comments about merging the objects, and thought it might be better to keep anything of that sort in your PR to avoid confusion, if you were already thinking about something similar. I also thought an array with two sub-types might be rather messy unless there are neat ways of doing this. I can have a go at working it out though, and send it on via PR to you or just cut and paste here, if you want.

Thanks for the offer on the vertical panels. I need to get a 'work in progress' PR in soon, as there are a few things I keep bouncing off without success [redisplay of the systray and launchers after dragging to a new orientation, for example], so I need to get some fresh eyes on it.

@ghost
Copy link
Author

ghost commented Jan 2, 2016

@brownsr i think you are stuck with the position parameter. Anyway it's not currently used and how i see, is not well implemented also.

Whatever there are an insertion, the position is always -1:

https://github.com/linuxmint/Cinnamon/blob/master/files/usr/share/cinnamon/applets/systray%40cinnamon.org/applet.js#L227
https://github.com/linuxmint/Cinnamon/blob/master/files/usr/share/cinnamon/applets/systray%40cinnamon.org/applet.js#L234
https://github.com/linuxmint/Cinnamon/blob/master/files/usr/share/cinnamon/applets/systray%40cinnamon.org/applet.js#L241

This mean this condition will never satisfy:
https://github.com/linuxmint/Cinnamon/blob/master/files/usr/share/cinnamon/applets/systray%40cinnamon.org/applet.js#L277

So, the status icons always will be inserted at the begin of the list.
https://github.com/linuxmint/Cinnamon/blob/master/files/usr/share/cinnamon/applets/systray%40cinnamon.org/applet.js#L284

I can insert always the indicator icons at the end of the list and in this way we can virtually separate the icons using the type (status, indicator). What you think?

@ghost
Copy link
Author

ghost commented Jan 2, 2016

Also we can filter the list of status icon, and apply the position parameter to the filter list only, to not affected the original idea, because i don't know why this exist, maybe for something that will be added later.

@ghost
Copy link
Author

ghost commented Jan 2, 2016

I see the whole scenario. There are an error that can cause a cinnamon freeze here: https://github.com/linuxmint/Cinnamon/blob/master/js/ui/indicatorManager.js#L898

Need to be:
actorDragable = actorDragable.get_parent();

The Indicators are not homogeneous. I added a label and we can have indicators like the indicator-multiload: http://3.bp.blogspot.com/-beOxEYiPYwg/T7l6T-nIRLI/AAAAAAAAED8/O1Ei8c3DS8E/s1600/indicator-multiload.png

So, can not be added in a box like that.
Also there are another problem, if i add an indicator inside a different actor. The main systray actor are added inside the panel in a way that fill the y coordinate. So, the menu can be orientate properly, but if i add the indicator in other actor, that not fill the y coordinate, the menu will be displayed inside the panel.

@brownsr
Copy link
Member

brownsr commented Jan 2, 2016

Wow, you have been busy. Thanks. Perhaps I should just have a go at coding it up using your ideas and see how it behaves.

…ter than one with the same id in our internal cache. Merge the status icons and indicator icon in the same Clutter.BoxLayout, for handle the space in a better way. The icons do not allow a theme directly now, as St.Icon not apply the style_class when we set the content for a Clutter.Image, but use gio is worse. Check the status inside the initialization, and prevent show the indicators in a passive state. Do not think that all indicators icons have the height = width, so get the better width for the required height.
@ghost
Copy link
Author

ghost commented Jan 2, 2016

@brownsr i think all is solved now.

@ghost
Copy link
Author

ghost commented Jan 2, 2016

@brownsr see that indicators can have a label, you need to deside what you want to do with the label. One option is hide it and another could be set the actor vertical:

        let children = this.manager_container.get_children().filter(function(child) {
            return (child.toString().indexOf("CinnamonTrayIcon") == -1);
        });
        for (let i = 0; i < children.length; i++) {
            children[i].set_vertical(current_panel.get_vertical());
        }

@brownsr
Copy link
Member

brownsr commented Jan 2, 2016

lestcape, that's great, it works very well. I think the visual appearance is better with the homogeneous line left in though, for my test case anyway.

@ghost
Copy link
Author

ghost commented Jan 2, 2016

install the last version of psensor, and indicator-multiload and you will see why can not be set homogeneous.

@ghost
Copy link
Author

ghost commented Jan 2, 2016

Do you think that this three indicators are homogeneous? I think you understand now.
captura de pantalla de 2016-01-02 06 45 00

@brownsr
Copy link
Member

brownsr commented Jan 2, 2016

Got it :-)

@ghost
Copy link
Author

ghost commented Jan 3, 2016

I have some memory leaks in my system. Probably because of the last commit for indicator (that is also a fix for indicator-multiload). Apparently, is pretty hard for cinnamon handled the indicator-multiload, as what is doing is change his image several time per seconds. I review the code and all object that i use don't need an explicit destruction. Will be good have the opinion of others, because could be in my case another extension.

@clefebvre
Copy link
Member

Hi @lestcape

I'd like us to chat about this. It's not urgent at all, but it's important.

  • I've got cold-feet on modifications of the statusIcon support (namely because the design is a complete mess and we fixed so many rendering issues on this already, I'm afraid to create new regressions).
  • As you guys mentioned, indicators give us a much saner icon object to manipulate but it lacks features. We've been contemplating a libindicator fork and/or extension for a while. Apps using that new tech could benefit from the pros of indicator, along with tooltip and proper click support. If the fallback switch is in libindicator, we might even be able to fallback in a smarter way (i.e. to only use indicators where wanted without seeing all supported apps switch automatically).

Don't hesitate to get in touch on the IRC. It's a bit complicated because if we come up with a new lib, even if it's not Mint-specific, it won't be present in many distros... switching to that, adding support to it, this triggers both Mint-specific and Cinnamon-specific discussions.

@clefebvre clefebvre changed the title [3.0] Improving slightly the appindicators. [3.2] Improving slightly the appindicators. Apr 20, 2016
@ghost
Copy link
Author

ghost commented Apr 20, 2016

Yes @clefebvre I also agree with you.

@ghost
Copy link
Author

ghost commented Apr 21, 2016

Just to mention, on pure Dbus indicator support, the click depend of the action that can be implemented or not inside the client. So, a missing or wrong action, can be taking as a client bug and reported again the client dev.

There are two types of click signals and the client will need to implemented both. One is recommended for right click or left click and another for middle click or left click.

About icons, the problem is that Gio have an internal cache for icons and several clients replace the icons but with the same name. This is in my personal opinion the reason of the current problems with the icons. In this commit, this problem is resolved using an internal cache and rebuilding the cache checking the tv.time while we don' t use Gio to get the icons any more.

What is certainly true, is that there are several buggy clients, that do not followed the specification.

In another hands, I recommended NOT used libindicator, not because the libindicator can not be adapted and ported to all distros. I think this could be possible, but will not be possible apply all canonical fixed for the apps, to properly use libindicator in all distros. So, this will be ending in a problem without solution, specially for Fedora users.

There are another way to live with this, and is not hard-code the appindicator inside Cinnamon. Create an applet with it and implement only the Dbus functions inside cinnamon, to be one unique DBus iface to provide the information.

Another option could be just implemented a custom actions for the click and use muffin to handled the click action instead of send a signal to the client.

@ghost
Copy link
Author

ghost commented Apr 21, 2016

Also @mtwebster apply a patch for the same thing with the texture cache right now. Anyway, this commit 1bef2e4 avoid this problem.

@clefebvre clefebvre changed the title [3.2] Improving slightly the appindicators. Improving slightly the appindicators. Sep 12, 2016
@ghost
Copy link
Author

ghost commented Oct 8, 2016

Let me know if i can remove this. The pull request is broken again, if will not be merged, please tell me to remove the pull, for me it's difficult have several branch at the same time... Some time ago i remove a pull, because i think that was not useful, but was merged.

@ghost
Copy link
Author

ghost commented Oct 8, 2016

For those who want this as an a separate applet, with much more improvements, will probably be added to spice website in the next week, when will also support vertical panels...
https://github.com/lestcape/Systray-Indicator

@clefebvre
Copy link
Member

@lestcape, shall we close it and start new ones instead?

I'm not sure it needs to be outside of Cinnamon (i.e. as a spice). But maybe we can take things step by step, for instance improve the indicators without modifying the statusicons. Isolate changes done to one and the other.

Let me know what you prefer.

@ghost
Copy link
Author

ghost commented Oct 11, 2016

Hi @clefebvre what i prefer is try to improve this as possible to be better. I don't have any problem to follow the rule that you considered important. Also i think will be important the help of @JosephMcc, there are some visual improve where he sure have a better idea than me of how do it.

Thanks, i will try to create a new pull this weekend.

@clefebvre
Copy link
Member

I feel the same way about @JosephMcc. Don't tell him too much though :)

I'll close this one so, in the meantime.

@clefebvre clefebvre closed this Oct 12, 2016
@JosephMcc
Copy link
Contributor

I feel like I need to frame this :)

@clefebvre
Copy link
Member

Damn... where's the delete button!!!

@ghost ghost deleted the lestcape-patch-1 branch October 13, 2016 06:48
@ghost
Copy link
Author

ghost commented Oct 13, 2016

Was deleted and a new pull was created: #5790

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.

None yet

5 participants