Add tray_icon_path to config to allow overriding tray icon #396
Conversation
github-actions
bot
added
the
Z-Community-PR
|
3nprob
force-pushed
the
allow-custom-tray-icon
branch
from
f237216
to
0f385e7
Compare
3nprob
force-pushed
the
allow-custom-tray-icon
branch
from
0f385e7
to
1d23623
Compare
|
OT but since there is no issue tracker on this repo to file a meta issue: the |
3nprob
force-pushed
the
allow-custom-tray-icon
branch
from
1d23623
to
f4aca16
Compare
|
Tested that it works fine to use a path from the local filesystem by editing |
|
The tray icon gets overridden by the app favicon when you get a notification, so this would cause your icon to flicker between custom and element, no? |
|
@3nprob what OS did you test on? We only don't use the favicon override on macOS, where the OS has stable notification bubble count support so we can skip it |
|
@t3chguy Linux(Wayland). No flicker noticed yet. |
That probably depends on your OS, Electron docs don't specify other than a recommendation to use |
3nprob
force-pushed
the
allow-custom-tray-icon
branch
from
f4aca16
to
2845c46
Compare
3nprob
force-pushed
the
allow-custom-tray-icon
branch
3 times, most recently
from
33fe2f1
to
849d894
Compare
|
Hmm, with the new changes forcing filetype per platform no, svgs won't wok anymore even if supported. Is the platform-detection functionality for manual icons that vital @turt2live ? |
|
Well, it needs to work on all the platforms. Windows wants an ico, so we should be giving it an ico. |
src/electron-main.ts
Outdated
| // It's important to call `path.join` for the bundled assets so we don't end up with the packaged asar in the final path. | ||
| const icon_path = process.platform === 'win32' | ||
| ? (global.vectorConfig?.tray_icons.ico || path.join(resPath, 'img', 'element.ico')) | ||
| : (global.vectorConfig?.tray_icons.png || path.join(resPath, 'img', 'element.png')); |
There was a problem hiding this comment.
After thinking about it a bit more, I don't think this is the right approach. This is a user config-- I don't think it makes sense to constrain it to particular file types.
In your first version, that just had a single path, @t3chguy said
As per Electron docs, ICO is recommended on Windows, this configuration model doesn't support this.
How does a simple path not support ico? The user can just provide an ico path if they're configuring this on windows. The config option can be documented to inform the user what kind of icon they should use for their system. Why would a user, who's using element-desktop on a particular system, provide configs for multiple system tray file formats?
I'd recommend reverting to just specifying a single path, not different formats, if @t3chguy agrees with my assessment.
Or am I misunderstanding that this is an end user config? Is this intended for packagers to be able to provide custom icons? I just got element set up the other day so I'm quite possibly missing context!
There was a problem hiding this comment.
also tagging @turt2live since they've commented on providing an ico as well.
There was a problem hiding this comment.
The config here is a build config, not a user config, ftr.
I'm not concerned with the API shape in all honesty: it just needs to work and not look awful ;)
There was a problem hiding this comment.
Ah, thank you, I completely revoke my comment! Makes sense.
How would people feel about me adding a user config in a separate PR to change the tray icon? I'm using a custom icon pack on Linux, and I'd like to have the tray icon match.
There was a problem hiding this comment.
That would require a product decision, though I don't foresee it being a successful change proposal if I'm being honest ;)
Rationale being that things like brand config are meant to be build-level things and not customizable by the user, even if technically possible.
There was a problem hiding this comment.
config.json is is still exposed and documented as user run-time configuration and I really hope that this will continue being supported, regardless how commonly it might be used by those keeping telemetry enabled today.
There was a problem hiding this comment.
it's supported as a user config as well, but primarily this feature would be used as part of a build pipeline - we would be unlikely to support it as part of a user-supplied config (if an issue were to be opened as such).
element.io/nightly/config.json
Outdated
| @@ -49,5 +49,6 @@ | |||
| "feature_spotlight": true, | |||
| "feature_video_rooms": true | |||
| }, | |||
| "map_style_url": "https://api.maptiler.com/maps/streets/style.json?key=fU3vlMsMn4Jb6dnEIFsx" | |||
| "map_style_url": "https://api.maptiler.com/maps/streets/style.json?key=fU3vlMsMn4Jb6dnEIFsx", | |||
| "tray_icons": {} | |||
There was a problem hiding this comment.
we shouldn't be supplying default values in the config. This is because the code needs to handle a case where the value isn't supplied, and we pick sensible defaults for everything. Whenever we can, we force ourselves to test this by not specifying them in the production configs.
There was a problem hiding this comment.
Am I reading this right if with this change, you now consider this resolved?
There was a problem hiding this comment.
well, it's resolved when the config.json files for nightly and release aren't modified :p
src/electron-main.ts
Outdated
| // It's important to call `path.join` for the bundled assets so we don't end up with the packaged asar in the final path. | ||
| const icon_path = process.platform === 'win32' | ||
| ? (global.vectorConfig?.tray_icons.ico || path.join(resPath, 'img', 'element.ico')) | ||
| : (global.vectorConfig?.tray_icons.png || path.join(resPath, 'img', 'element.png')); |
There was a problem hiding this comment.
it's supported as a user config as well, but primarily this feature would be used as part of a build pipeline - we would be unlikely to support it as part of a user-supplied config (if an issue were to be opened as such).
|
We also need to consider @t3chguy's comment about the icon changing when the user gets a notification. I don't really foresee that being an easy fix, however. |
turt2live
requested review from
a team
and removed request for
justjanne and
t3chguy
|
Product team: Do we want to support this feature? (assuming it can be made to work when the user receives a notification) |
Could this be addressed in a separate issue perhaps? I can't seem to reproduce this yet (ie for me, the icon is always shown to the user-configured one, notifications or not, no flickering). Been running this in 3 concurrent instances for the past week fwiw. |
|
It would need solving for us to feel comfortable landing this PR, so I don't think it can be safely broken out to another issue, sorry. Some platforms might magically work but it would need extensive testing on all platforms we provide direct support for. |
|
@turt2live All right, let's hope someone else can help with this. I'm surprised this isn't considered prioritized considered element-hq/element-web#2320 is still highly relevant (most commented and upvoted issue ever and it's not getting better over time). The fix in this PR takes the one proposed workaround (profiles) from a painful hack (which it becomes when combined with the fact that frequent client freezes from other issues forces force-closes, which makes it a russian roulette on which tray icon to close each time) to actually usable. This one issue is enough of a pain-point that it's hurting the organizational buy-in on the whole Matrix adaptation. In lack of another suitable client and support for this we'll probably have to start maintaining a fork, which I don't look forward to. |
|
Multiple accounts is indeed something we'd like to support one day. In the meantime, we require that features be complete in order to reduce maintenance burden of the app. |
|
So @t3chguy is saying "We only don't use the favicon override on macOS, where the OS has stable notification bubble count support so we can skip it". Can we confirm which scenerios/platforms trigger the flickering and what changes are needed? I'd at least like to figure out if this is a Windows-only issue or if there is any way for me to reproduce it |
3nprob
force-pushed
the
allow-custom-tray-icon
branch
from
cd3c47e
to
3aa5416
Compare
3nprob
force-pushed
the
allow-custom-tray-icon
branch
from
3aa5416
to
1f51e59
Compare
|
There is now a check that disabled the webview-sourced icon override (that is, making non-mac platforms behave like mac) in case the tray icon has been explicitly configured. With the default, the previous behavior is retained. |
There was a problem hiding this comment.
Please avoid force pushes after initial review has been given - it makes it much harder to review the code.
I would also suggest holding off on writing too much code before the Product team has a look. There's a chance that they will not approve the feature (which is why we're asking for their review), and we'd rather not have to throw away too much code.
Hm, I thought I had done my prework here but I guess I must have misinterpreted something. #774 is Triaged (which I take to indicate that it's at least not been closed as "irrelevant"/"wontmerge"), recently enough that I think it's reasonable to assume it's not a remnant of past dreams. element-hq/element-web#4745 is accepted and AFAICT all parts of it but this has already been implemented and exposed in user-accessible ways. Where can one read up to better understand the decision-making process when deciding what features can be considered for merging into the |
Checklist
Adds new config entry
tray_icon_path, allowing the user to override the tray icon with their own.This resolves #774
Use-cases:
Also minor fix in removing redundant global variable
iconPath.Type: enhancementHere's what your changelog entry will look like:
✨ Features
Type: enhancement