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.

Sign up for GitHub

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

Jump to bottom

feat: search in icon packs for folder size #344

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: search in icon packs for folder size #344

wants to merge 2 commits into from

Conversation

kaanelloed
Copy link
Contributor

Here's my proposal to fix #336 .

First of all, I created the BlankFolderIcon class to prevent the icon from changing size, and created the searchInMultipleIconPack() function to clean up the code.

The logic is therefore that we have a default icon pack icon that refers to “discreetlauncher.folder/discreetlauncher.folder” which will be used if no icon is found for the icon pack at “discreetlauncher.folder/discreetlauncher.folder[# applications]”. If both are absent, the icon will be drawn as before.

I've also added a cache to prevent icon packs from being searched several times for the same number of applications.

@falzonv
Copy link
Owner

falzonv commented Nov 16, 2024

Hello,

Thank you for the suggestion and sorry for the delay...

Here are some things that come to my mind after a first reading:

  • The BlankFolderIcon and FolderIcon classes are very similar, there is probably a way to adapt the existing FolderIcon class to handle both cases and limit code duplication (maybe by providing "-1" as number of apps to the constructor when we should be in the "BlankFolderIcon case"?).
  • I would remove the cache to simplify the code, I don't think it is really needed because most users probably don't have more than 3-4 folders and the probability of them containing exactly the same number of apps is quite low.
  • In case there is no folder size icon in the pack but there is a default icon pack icon, the text for the number of icons should be displayed on top of it (maybe with the user-defined color) so that users do not lose this information (especially because I am not sure icon pack maintainers would really create dozens of icons just for my app - there is no limit to the number of apps in a folder -, so the default icon pack icon may be the most used one).

Could you have a look at these points?

Best regards.

@kaanelloed
Copy link
Contributor Author

I'll look into these points

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Icon pack: Folder icon always turns white
2 participants
@kaanelloed @falzonv