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

Implement icon extraction from windows ico files #380

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

graag
Copy link

@graag graag commented Oct 3, 2021

The PR is marked as a Draft as I'm not sure what would be the best way to handle required dependencies.

The idea is to extract a png icon for games that provide only goggame-*.ico, a png version would be useful for .desktop files.
Current PR does not introduce any new mandatory dependencies and tries two approaches to extracting the icon:

  • use python pillow module if available as default extractor,
  • use imagemagick as a fallback.

There are two issues I would like to discuss:

  • The code could be simplified a lot if we would add pillow as mandatory dependency to requirements.txt, is that acceptable?
  • How to handle error messages? Which should remain as prints to the console and which should be displayed in a popup dialog? What about translations?

- Use python pillow if available as default extractor
- Use imagemagick as a fallback
@sharkwouter
Copy link
Owner

Adding Pillow would be acceptable if it's needed. Do Gnome, KDE and other desktop environments not support using ico files as icons in .desktop files?

@sharkwouter
Copy link
Owner

In this case I would show an error message in the terminal, but not as a pop-up. Error messages in the terminal should not be translatable.

Adding Pillow as a dependency would make it so Imagemagick would not be needed, right?

@graag
Copy link
Author

graag commented Oct 6, 2021

Adding Pillow would be acceptable if it's needed. Do Gnome, KDE and other desktop environments not support using ico files as icons in .desktop files?

Actually I did not test that lately - most of this is from my install script I've been using with lgogdownloader.
The whole mess probably originated from problems with importing .desktop files to Steam. The icon is not set properly and the dialog window suggest only .png and .tga files.

I just tested with KDE and Steam and they both support .ico files.
For KDE it looks like it selects the lowest resolution from the file - which we could improve by extracting a png.

@graag
Copy link
Author

graag commented Oct 6, 2021

In this case I would show an error message in the terminal, but not as a pop-up. Error messages in the terminal should not be translatable.

OK

Adding Pillow as a dependency would make it so Imagemagick would not be needed, right?

Yes. With Pillow as a dependency the part with imagemagick could be dropped.

However it looks that it would be enough to point to proper .ico file - although KDE could select a better resolution.

@sharkwouter
Copy link
Owner

Oh that would be good. Much simpler too.

@sharkwouter
Copy link
Owner

@graag do you think this could be completed before the 7th of November? It would be nice to include it in the next release.

@sharkwouter sharkwouter added this to the 1.2.0 milestone Nov 8, 2021
@sharkwouter sharkwouter modified the milestones: 1.2.0, 2.0.0 Sep 25, 2022
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

3 participants