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

Make tray icon themeable #1908

Closed
andia89 opened this issue Apr 16, 2016 · 40 comments · Fixed by #7111
Closed

Make tray icon themeable #1908

andia89 opened this issue Apr 16, 2016 · 40 comments · Fixed by #7111

Comments

@andia89
Copy link

andia89 commented Apr 16, 2016

Steps to reproduce

  1. Open telegram on Linux
  2. Enable tray icon

Expected behaviour

Icon from generic place like /usr/share/icons/hicolor is used instead of a hardcoded one, so that different icon themes can provide tray icons for telegram instead of a hardcoded png.

Actual behaviour

An icon that is hardcoded in the binary is used, with absolutely no way to change apart from building telegramDesktop from source with changed png files...

Configuration

Ubuntu 15.10 with appindicator installed

Telegram version 0.9.33

@auchri
Copy link
Contributor

auchri commented Apr 16, 2016

Shouldn't #1431 fix this issue?

@andia89
Copy link
Author

andia89 commented Apr 17, 2016

I don't think so because this is only for the app icon (the icon that shows up in the unity launcher for example), whereas the tray icon is still hardcoded.

If I made a pull request, is this something you would consider merging?

EDIT: Relevant #419

@auchri
Copy link
Contributor

auchri commented Apr 17, 2016

Duplicate #1199

If I made a pull request, is this something you would consider merging?

Yes, a PR would be great 👍

@john-preston
Copy link
Member

What will happen with the coloured unread counters that are placed upon the icon (currently red or grey, depending on the muted status of the conversations with a positive unread counter)? They will be placed that way above an arbitrary tray icon?

@andia89
Copy link
Author

andia89 commented Apr 17, 2016

I suppose that's up to the icon theme to decide. If they ship icons for all different states, they should show up

@john-preston
Copy link
Member

@wa4557 I just don't understand (and afraid a bit) how this is going to look in the pull request — the counter is added uniformly there using internal styles.

@andia89
Copy link
Author

andia89 commented Apr 17, 2016

Is it? As far as I understand the code there are a lot of different png files for all different counter values. So for me there's not really a difference if this is done internally (this will be the fallback oc), or via an icon theme

I could be wrong though, and misunderstand the code...

@DavidVentura
Copy link

Any news on this? Plasma uses the theme icon but the status bar still shows the original blue icon

teleg1
teleg2

@auchri
Copy link
Contributor

auchri commented Jun 11, 2016

@wa4557 Did you make some progress on this issue?

@andia89
Copy link
Author

andia89 commented Jun 11, 2016

Ah yeah I thought I commented on that. I coded around only to find out, that this wont work. The problem is that telegram doesn't use system Qt libraries, which consequentely means that the icon theme used is not recognized (I'm not sure how this is done by system Qt).

So this means that I was not able to use icons from hicolor or other icon themes

@DavidVentura
Copy link

Would you at least keep/distribute a directory named 'icons' with the icon? Also embed the default icon in the binary as it's done now in case the icon gets deleted. While this won't work with general themes, at least the interested users can switch the icon.

Maybe don't even ship the icon, but load it on launch if it's present.

@auchri
Copy link
Contributor

auchri commented Jun 11, 2016

@wa4557

The problem is that telegram doesn't use system Qt libraries, which consequentely means that the icon theme used is not recognized (I'm not sure how this is done by system Qt).

But it works for the app icon .. ? #1431

@andia89
Copy link
Author

andia89 commented Jun 11, 2016

Well that's a different story.

In order for telegram using system tray icons, I need the QIcon::fromTheme method, which as I said doesn't work with non system Qt libraries.

For the app icon it is as simple as changing a line in the .desktop file from Icon=/path/to/a/hardcoded/icon.png to a simple Icon=telegram the rest is done by the DE.

EDIT: I would argue, that line L524 in your linked commit could be deleted, the important thing is L1199

@DavidVentura Yeah this might work. This used to work on Ubuntu but Unity doesn't use AppIndicator anymore, and the icons are created on the fly by the program. I guess this could be changed though

@auchri auchri added the linux label Jun 11, 2016
@DavidVentura
Copy link

Would it be possible to try and load an external file if it exists, and if it doesn't just fall back to the current icon (embedded in the binary)?

@andia89
Copy link
Author

andia89 commented Jun 12, 2016

@DavidVentura This is what I'm trying to do right now. We'll see if it works

EDIT: I opened a PR to implement what @DavidVentura suggested. The code in general is a bit convoluted though, with a lot of redunand functions that are doing basically the same, for different flags. Cleaning the code is not the purpose of this PR though

andia89 added a commit to andia89/tdesktop that referenced this issue Jun 12, 2016
andia89 added a commit to andia89/tdesktop that referenced this issue Jun 12, 2016
andia89 added a commit to andia89/tdesktop that referenced this issue Jun 12, 2016
Signed-off-by: Andreas Angerer <andreas.angerer89@gmail.com> (github: wa4557)
@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Sep 1, 2016

This hack, modified for your color/icon preferences, may be useful for some of you until this is sorted.

EDIT: or this one.

@Aokromes
Copy link
Collaborator

Aokromes commented Nov 8, 2016

#1199 ?

@mrjovanovic
Copy link

@Aokromes that is for the application icon, not the tray.

@Aokromes
Copy link
Collaborator

Dunno #1353 (comment) mentions that has tray.

@AndydeCleyre
Copy link
Contributor

I had this themed with the following custom icons, and many symlinks for different message counts, in ~/.local/share/TelegramDesktop/tdata/ticons:

ico_22_1.png
icomute_22_0.png
icomute_22_1.png
icon_22_0.png
icon_22_1.png
icon_22_2.png
icon_22_3.png

But it seems with the latest update from 1.5.4 to 1.5.8 the tray icon has reverted to the blinding blue out-of-place icon. Did the necessary location or names of the custom icons change?

@john-preston
Copy link
Member

@AndydeCleyre The name changed from ico to icon.

@AndydeCleyre
Copy link
Contributor

So after #7111, in order to choose the tray icon displayed on plasma desktop, the proper way becomes to have icons in the theme named telegram-{,attention-,mute-}panel?

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Feb 4, 2020

@AndydeCleyre +

@AndydeCleyre
Copy link
Contributor

Is this in 1.9.12?

image

image

@ilya-fedin
Copy link
Contributor

Yep

@AndydeCleyre
Copy link
Contributor

How can I debug this, then?

@AndydeCleyre
Copy link
Contributor

Looks like the regression is now being discussed at #7192.

@Rnksts
Copy link

Rnksts commented May 3, 2020

No solution for Windows 10 users?

@ilya-fedin
Copy link
Contributor

No solution for Windows 10 users?

There is no such thing as "system icon theme" on Windows AFAIK

@Rnksts
Copy link

Rnksts commented May 3, 2020

True but, after seeing #3301 and #1353 closed in favor of this issue, closed too, I wanted to remind or at least confirm if we are just out of luck.

@ilya-fedin
Copy link
Contributor

True but, after seeing #3301 and #1353 closed in favor of this issue, closed too

Since things changed, I think, you can open new issue
Or ask @Aokromes to reopen one of them (they have examples of how that can be done and discussion)

@AndydeCleyre
Copy link
Contributor

Is this still supposed to work, or is there a new method? It seems broken again in 2.5.1 (see #6721).

@ilya-fedin
Copy link
Contributor

It works, with counter, but works :)

@AndydeCleyre
Copy link
Contributor

@ilya-fedin

It works, with counter, but works :)

Not entirely, anymore. Since 2.5.1, aside from the addition of an unwanted and poorly colored counter, the rest of the icon now fails to color properly when that counter is shown, instead just turning black.

Attention icon before 2.5.1:
before

Attention icon after 2.5.1:
after

So @ilya-fedin and @Aokromes, which is appropriate?

  1. re-open this issue
  2. treat the color regression issue as within scope of the TDESKTOP_DISABLE_TRAY_COUNTER regression issue ([Feature Request] Add option to disable red counter on systray icon #6721)
  3. open a new issue for the color regression alone

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Jan 27, 2021

I don't think it is a regression. No one claimed that tdesktop supports KDE's color schemes (and most likely they won't be supported due to the need to link to KDE libraries). You should use pre-colored icon theme.

#6721 is not a regression issue, it's a feature request from 2019

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Jan 27, 2021

most likely they won't be supported due to the need to link to KDE libraries

I think there may be a misunderstanding here. AFAIK no KDE libraries are currently linked, yet color schemes are indeed supported in the icon (fully before 2.5.1, broken with 2.5.1 for attention icon but still working for no-messages icon).

All that's needed is to let plasma use the theme's svg, and it will do the coloring. Here's a sample svg that calls on the theme color (telegram doesn't have to be involved, really):

Expand
<svg xmlns="http://www.w3.org/2000/svg" width="22" height="22" version="1">
 <defs>
  <style id="current-color-scheme" type="text/css">
   .ColorScheme-Text { color:#dfdfdf; } .ColorScheme-Highlight { color:#4285f4; }
  </style>
 </defs>
 <circle style="fill:currentColor" class="ColorScheme-Highlight" cx="6" cy="17" r="2"/>
 <path style="fill:currentColor" class="ColorScheme-Text" d="M 18.687469,18.303647 C 18.988656,17.713495 20.719303,7.6225537 20.966707,5.0140234 21.234358,2.4291929 19.838794,2.9098641 18.554652,3.3465233 15.404153,4.4215289 5.4432767,8.2785986 2.7167917,9.7167145 c -0.671236,0.28127 -0.961371,0.7585015 -0.472969,1.1715565 0.420514,0.350114 1.95926,0.830984 3.200218,1.075015 1.057861,0.323857 1.87956,0.01365 2.638057,-0.418712 1.822812,-1.092109 7.2543803,-4.0650435 7.5468763,-4.2586459 0.292496,-0.1936024 0.612628,0.1656936 0.448928,0.3396304 -0.163701,0.173936 -4.010423,3.6599335 -5.27339,5.2360675 -0.669101,0.866629 -0.649059,1.329311 0.0383,1.8758 1.54204,1.221933 5.662057,4.007627 6.275101,4.176004 0.781724,0.214705 1.239555,0.03683 1.569559,-0.609783 z"/>
</svg>

#6721 is not a regression issue, it's a feature request from 201

Well it is a regression from my POV, as a feature was present and is now broken (TDESKTOP_DISABLE_TRAY_COUNTER), and the new broken state also introduces additional breakage (poor accessibility and aggressive anti-user coloring, as well as simple inconsistency -- not only between this icon and the rest of the system, but between this icon and the other telegram icon states).

@ilya-fedin
Copy link
Contributor

AFAIK no KDE libraries are currently linked, yet color schemes are indeed supported in the icon (fully before 2.5.1, broken with 2.5.1 for attention icon but still working for no-messages icon).

It's because KDE colors when IconName is passed, but doesn't color IconPixmaps sadly.

All that's needed is to let plasma use the theme's svg, and it will do the coloring.

We can't do that due to the counter.

Well it is a regression from my POV, as a feature was present and is now broken (TDESKTOP_DISABLE_TRAY_COUNTER)

You can't call a feature something that was intended for temporary debug purposes. Can you complain if you used employees' entrance and someone closed it?

@AndydeCleyre
Copy link
Contributor

You are right, I am coming from the perspective of folks who use the app. I am not describing the intentions of the developers which are sometimes at odds with accessibility needs, readability, and desktop consistency.

It sounds like your answer is WONTFIX, but it looks like my best bet is to open a separate issue, so I will do so.

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Jan 27, 2021

It sounds like your answer is WONTFIX

Most likely. Currently the behavior is changed to not to add counter if "panel" icon is detected in the current beta version (2.5.6). But there are a high chance that as soon as it will land stable version, there will be complains that the counter is lost. If this will happen, the only way is #6721, but new settings couldn't be added without @john-preston permission and he doesn't want to give it.

looks like my best bet is to open a separate issue, so I will do so

It will be closed as a duplicate of #6721 most likely.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants