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

Add option to show application icon #942

Closed
wants to merge 1 commit into from

Conversation

weisJ
Copy link
Contributor

@weisJ weisJ commented Nov 4, 2024

Add option to show app icon in the history list (see #912). The settings toggle is missing translations to languages other than english.
The app icon is also shown in the popover.

Icons are kept for 24 hours (so they are updated if the application icon changes).

I tried to match the icon size to the visual height of the text next to it (for both the history list and the popover).

image image

@weisJ weisJ marked this pull request as draft November 5, 2024 16:57
@weisJ
Copy link
Contributor Author

weisJ commented Nov 5, 2024

Changed this to draft as I have a better idea for how to do icon invalidation.

@weisJ weisJ force-pushed the applicationIcons branch 2 times, most recently from 277aeee to 38c742e Compare November 7, 2024 19:06
@weisJ weisJ marked this pull request as ready for review November 7, 2024 19:09
@weisJ
Copy link
Contributor Author

weisJ commented Nov 7, 2024

I have changed invalidation to attach a file system event handler to listen for changes to the app file. If it is modified the icon is updated. If it is deleted a fallback icon will be shown and every hours a new attempt at retrieving the icon is made.

@weisJ
Copy link
Contributor Author

weisJ commented Nov 11, 2024

Fixed the SwiftLint issues.

Related to this feature: As I have been using it enabled the last two weeks I noticed that due to a lot of copies coming from the web the icons didn't add much context for me in this case. I have now a sort of good working mechanism which shows me the favicon of the webpage the copy originated from instead of the app icon.
Crucially this of course requires making requests to those webpages, which broadens the sandbox requirements for Maccy. Would this feature be something worth while preparing a PR for? (I totally understand if the privacy concerns here outweigh the potential benefits)

@Romachamp
Copy link

I think just app icons would be enough. Website icons aren’t all that good, and I still use a lot of other apps too. So, everything, which is done currently is pretty good and suits my needs.

@p0deje
Copy link
Owner

p0deje commented Nov 22, 2024

Merged in 9f96adc with some smaller changes (refactoring, vertically aligning icon for image copies, extra leading padding).

Thanks again @weisJ, you rock!

@p0deje p0deje closed this Nov 22, 2024
@weisJ
Copy link
Contributor Author

weisJ commented Nov 24, 2024

Just noticed: If you want the icon to be vertically centred then you could have simply removed the VStack. I put it there as I thought having the icon top aligned would look more natural.

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.

3 participants