make caret more visible

helge17 / tuxguitar

Improve TuxGuitar and provide builds

Other

435 stars 35 forks source link

make caret more visible #363

Closed guiv42 closed 5 months ago

guiv42 commented 5 months ago

Make caret more visible, and change configuration parameters so modification is effective also for users upgrading TuxGuitar version.

@helge17 I would appreciate if you can review:

the graphical rendering (always subjective)
could you please check if this is OK also in macOS configuration?
I would appreciate your opinion about how to depreciate configuration parameters. Since configuration folder is preserved when TuxGuitar is updated, we had already added the possibility to create new parameters in new version. This case, I would like to replace old configuration parameters with new ones. So I have created a list of "obsolete key names", and updated the configuration managers to delete these old parameters. Does it seem reasonable to you?

helge17 commented 5 months ago

The new caret looks great, thank you!

As far as I understand it, all properties present in the configuration file are read in first. Then (only) the keys from the list OBSOLETE_KEYS are deleted. Wouldn't it be cleaner to delete all keys not present in TGConfigKeys (or only read in these valid keys)? So if a invalid user-edited or obsolete key is still present in the configuration file, it will always be removed. In this way, a list of obsolete keys would only be needed for documentation purposes, perhaps as a comment in the source code.

guiv42 commented 5 months ago

Thank you very much for the review. I get your point about obsolete parameters: replace blacklist by whitelist? I think having the obsolete parameters list in the code and not only comments could make it more robust. I'll try to update this PR with a combined approach whitelist+blacklist. Just need to check all modules using configuration parameters.

guiv42 commented 5 months ago

Unfortunately it appears significantly more complex than expected. @helge17 : is it OK for you to keep the "old key names blacklist" strategy proposed in this PR unchanged?

helge17 commented 5 months ago

is it OK for you to keep the "old key names blacklist" strategy proposed in this PR unchanged?

Of course, my comment was just a suggestion. I'll leave it up to you to merge the PR in case there's anything you want to change.

guiv42 commented 5 months ago

OK, thanks. Your suggestion is fully relevant. But I'm a bit lost in these configManagers, propertiesManagers, propertiesHandlers, propertiesReaders, etc. So, as it is this PR may not be the cleanest solution, but in my opinion it's the safest.