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

Feat: Customization of folders and files #92

Merged
merged 7 commits into from
May 30, 2023
Merged

Feat: Customization of folders and files #92

merged 7 commits into from
May 30, 2023

Conversation

wladimirgrf
Copy link
Contributor

This pull request adds a new feature to the project, specifically the ability to customize the icons of folders and files using the settings.json.

Solving this issue: Add the possibility to set folders/files associations #73

Example:

config workspace
Screenshot 2023-05-21 at 15 40 02 Screenshot 2023-05-21 at 15 39 53

@miguelsolorio miguelsolorio self-assigned this May 22, 2023
Copy link
Owner

@miguelsolorio miguelsolorio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wladimirgrf thanks for working on this big feature for the theme! As I was testing this out, it seems like the syncing stopped working when adding new icons. A good test is to duplicate one of the files and add a new entry. Usually, the sync logic would pick this up and prompt for a reload and in your PR it appears to no longer prompt for sync. Mind taking a look to see if that can be fixed?

Besides that, it's working great 👏🍻

@wladimirgrf
Copy link
Contributor Author

@wladimirgrf thanks for working on this big feature for the theme! As I was testing this out, it seems like the syncing stopped working when adding new icons. A good test is to duplicate one of the files and add a new entry. Usually, the sync logic would pick this up and prompt for a reload and in your PR it appears to no longer prompt for sync. Mind taking a look to see if that can be fixed?

Besides that, it's working great 👏🍻

@miguelsolorio sure, I'll take a look.

@wladimirgrf
Copy link
Contributor Author

@miguelsolorio I did an update. There was an error with removing the settings. Icons were not updated. Changing the updateConfig to use the sorce file resolves this issue.

About the main problem. I would like your opinion on syncOriginal. Since we are changing the theme file and not the source. The files will always be different if the user has a custom configuration, and sync will force this update every time.

Which approach do you think is better? Did we remove the file compare from this function or do you have a better idea how to handle this?

@miguelsolorio
Copy link
Owner

I'd like to keep the original source theme file and then generate a modified version that has whatever customizations the user has added, I believe that was the original intention of the sync and it would check for new icons that were added and update the modified version with them. I do see now in the updated PR that you get prompted to reload each time because of the conflict you mentioned (files are always different).

If you can find a way to make it work with the new settings you've added, be it another method or the same, that would work for me. As long as users can customize their settings and the theme reflects that, that works for me.

@wladimirgrf
Copy link
Contributor Author

I'd like to keep the original source theme file and then generate a modified version that has whatever customizations the user has added, I believe that was the original intention of the sync and it would check for new icons that were added and update the modified version with them. I do see now in the updated PR that you get prompted to reload each time because of the conflict you mentioned (files are always different).

If you can find a way to make it work with the new settings you've added, be it another method or the same, that would work for me. As long as users can customize their settings and the theme reflects that, that works for me.

@miguelsolorio I created a "backup" logic.
Now we have the modified.json and bkp.json file.

All customization and current theme will be registered in the modified and the bkp has the purpose to synchronize new icons or basically any update in symbol-icon-theme.json.

See if it needs any more changes or if it's OK to merge.

@miguelsolorio
Copy link
Owner

This is getting closer! As I was testing out the other setting, symbols.hidesExplorerArrows, I noticed that there were a lot of inconsistencies in making it work. This setting just appends "hidesExplorerArrows": false, to the theme, so maybe there is a bug in the logic for the backup? Seems to only happen on reload.

CleanShot.2023-05-30.at.10.21.41.mp4

@wladimirgrf
Copy link
Contributor Author

This is getting closer! As I was testing out the other setting, symbols.hidesExplorerArrows, I noticed that there were a lot of inconsistencies in making it work. This setting just appends "hidesExplorerArrows": false, to the theme, so maybe there is a bug in the logic for the backup? Seems to only happen on reload.

CleanShot.2023-05-30.at.10.21.41.mp4

@miguelsolorio Found the problem.
The update action occur on window state change, either by focus or window reload.

This forces the workspace settings to be compared with the project settings.

As the object has no default value, it triggers the update function that considers only the "new" information, thus filling hidesExplorerArrows with the default value (false), since we overwrite the json completely.

I've removed key verification, this way we keep what's in the user's workspace.

Let me know if you find any more issues.

Copy link
Owner

@miguelsolorio miguelsolorio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 This is now working as expected, thanks so much for fixing the various bugs that came up! Will merge this and ship a new release shortly.

@miguelsolorio miguelsolorio merged commit 9f2d95d into miguelsolorio:main May 30, 2023
@miguelsolorio miguelsolorio added this to the 0.0.11 milestone May 30, 2023
@wladimirgrf wladimirgrf deleted the feature/folders-associations branch June 1, 2023 22:44
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.

2 participants