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

GUI: Fix iconpath iconspath Confman key discrepancy #3977

Merged
merged 2 commits into from Jun 10, 2022

Conversation

antoniou79
Copy link
Contributor

@antoniou79 antoniou79 commented Jun 8, 2022

Fixes issue with unable to download icons update (from Misc Tab) even though an icons path is set in Paths tab.

A bug ticker for this issue was opened here: https://bugs.scummvm.org/ticket/13556

An alternative approach, which would required fewer edits would be to change the Confman key to "iconspath", but since there's probably users who may have already stored a config with the original key "iconpath" it would be more seamless to change to "iconpath" as in this PR. This PR is now using the approach of keeping the "iconspath" key, and replaces the "iconpath" instances with it.

Fixes issue with unable to download icons update (from Misc Tab) even though an icons path is set in Paths tab.
@SupSuper
Copy link
Contributor

@SupSuper SupSuper commented Jun 8, 2022

Note that the wiki and docs will have to be updated as well.
The taskbar integration was already using "iconspath" so I'm not sure renaming it is the correct choice. Might be simpler to just have some kind of ConfMan["iconspath"] = ConfMan["iconpath"] somewhere in code for backwards compatibility.

@antoniou79
Copy link
Contributor Author

@antoniou79 antoniou79 commented Jun 8, 2022

Note that the wiki and docs will have to be updated as well. The taskbar integration was already using "iconspath" so I'm not sure renaming it is the correct choice. Might be simpler to just have some kind of ConfMan["iconspath"] = ConfMan["iconpath"] somewhere in code for backwards compatibility.

If "iconspath" was the older one (and it appears so), albeit not exposed to GUI but part of official releases, perhaps it would be better to adopt that one then; as mentioned the changes will be just in a couple of spots in options.cpp. And we drop "iconpath" even though it will break for a bit the config of those testing with a dev build. Until they set the path again under Paths tab that is.
I am not a fan of somehow supporting both keys to be honest. It seems too much hassle and added code complexity for little gain

@antoniou79
Copy link
Contributor Author

@antoniou79 antoniou79 commented Jun 9, 2022

I've pushed a commit to use the alternative approach of keeping the "iconspath" key, which is the oldest, documented and was included in released versions (but not exposed via the GUI), and replace the "iconpath" key instances (which were just a couple in gui/options.cpp).

The side-effect of this will be that anyone who has set the icons' path via the GUI (Paths tab), will have to set it again, since it will now appear empty, and also restart ScummVM in order to see any icons from that path in the grid view.

If this gets merged, better to do a merge-squash.

@bluegr
Copy link
Member

@bluegr bluegr commented Jun 10, 2022

This looks to be a typo that was done during development. Thanks for addressing it!
Squashing

@bluegr bluegr merged commit 1146f3d into scummvm:master Jun 10, 2022
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants