-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Initial notification icon support #710
Conversation
As icon locations are platform dependent, iterate through paths & add to notifications
Doesn't resources/128x128.png always exist regardless of system? |
In my testing a relative path to This could be rewritten to search for the icon - though I find that to likely be less efficient than a handful of hardcoded locations. Sometimes it's upscayl.png, others 128x128.png, sometimes can use relative path, other times can't.... so in addition to efficiency I think reliability also reduced performing a lookup for it. edit: more info |
Maybe it has to be a ./? I currently don't have access to my Arch laptop so I can't test it yet |
You mean as below?
As much as i'd love that simplicity to be true, confirming this doesn't work for AppImage (and I suspect others). |
Hmm, maybe it's only an AppImage thing? AppImage/AppImageKit#1035 I'd suspect the |
There very well may be an appimage variable or some such to the icon however confirming that Can confirm the deb/rpm work simply with Windows I believe does use So this justifies my thought process in hardcoding a handful of paths, while not my favorite bit of code it oddly seems the simplest and can't imagine these paths changing anytime soon. The flatpak and mac builds i'm curious about, would appreciate feedback from someone who can test these. |
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you are still working on this, please reply to indicate so. |
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you are still working on this, please reply to indicate so. |
So would appreciate some input on this; I've confirmed this PR works on majority of builds - even if it didn't work, for eg., on the mac build the notification would still show same as without the PR. |
The PR was in draft, I thought you were still working on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We just need to update one thing :)
Just hoped someone could test it for mac/flatpak :) |
Continuing discussion in b2a3e54 |
As icon locations are platform dependent, iterate through paths & add to notification util.
Confirmed working well for linux(deb/rpm), AppImage & Windows platforms.
Todo:
@aaronliu0130 appreciate if you're able to test.
Before and after shots:
Linux notifications:
![upscayl-notif-linux](https://private-user-images.githubusercontent.com/127086564/304077871-4f98257f-0a1c-4ce1-a346-914b1e46fd0c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ4MDAxMjcsIm5iZiI6MTcyNDc5OTgyNywicGF0aCI6Ii8xMjcwODY1NjQvMzA0MDc3ODcxLTRmOTgyNTdmLTBhMWMtNGNlMS1hMzQ2LTkxNGIxZTQ2ZmQwYy5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwODI3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDgyN1QyMzAzNDdaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT01MWZhZjk5YWQ5NTAyMjAzZWM2M2ZkODUxYjI1NjI3YzI0M2UzYzU3M2U1ZjU2YjllYTI3YWEzNDhiMGY4NDFlJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.rMPl59msEC4qb241xf5x3E4Q_oKBxCVSVSu5OV_VIxU)
(bottom before, top after)
Windows:
![upscayl-win-notif](https://private-user-images.githubusercontent.com/127086564/304078106-1aa5ffb6-c907-4493-97de-ee1b99a627e9.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ4MDAxMjcsIm5iZiI6MTcyNDc5OTgyNywicGF0aCI6Ii8xMjcwODY1NjQvMzA0MDc4MTA2LTFhYTVmZmI2LWM5MDctNDQ5My05N2RlLWVlMWI5OWE2MjdlOS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwODI3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDgyN1QyMzAzNDdaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1hM2MyYTMwNTFhNWViMWQ3YzA5NzVjOGNhMDNmMTNlNmViZmUxODg0ODdjY2U1NWIzNzk1ZGFhNzY1YjUyMDhhJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.VSVgWTl-mmjSfSgVJAl84d1h1qvRJzigj9-wZZ-cTt0)
Win.