Skin customization changes

[TIV73]

Hi,
amazing job on the latest less preference funcionality! Not only is saving the preferences orders of magnitude faster than in the previous release, the new declarative method of defining options is also increadibly easy while it probably still covers the majority of all common use cases.

A couple of things I came across:
Adding customization options in info.js is not recognized as proper addon configuration in the addon list. What I mean is that adding a config.js file gave the skin a little config button in the list of installed addons that opened the customization options screen in addition to adding the settings to the options screen. Not a big deal but it seems inconsistent.


Modified options are persisted as n_[type] (like 3_color, 2_radio, etc.) in persistent.json, which could lead to inconsistent values if the order of elements is changed after their value has been set. I wanted to highlight it since it happened to me when I set up the options, and it could lead to issues and inconsistend behavior down the road if addon authors change the layout customization settings in future updates.


I would love to see an option to either pick up the default values directly from a less file, or make it possible to remove a stored value if the user clicks the reset changes button instead of assigning a static default value.

Consider the following use case: A user tries out different skin color settings, but decides that they ultimately like the default value best, so they click the reset changes button.
Some time later, the addon author finds out that the default value is hard to read in some places and pushes an update that changes the default in the their info.js and less files.
The user installs the update, but since the reset changes button sets a static value instead of removing the option alltogether they will not get the new default.

[drakinite]

Thanks a ton for the feedback! It especially feels great when we're able to sneak in a big performance boost. (Previously, when customLESS changed, it was recompiled on every sub-window; but now it's only compiled on the main window, and the sub-windows just reload the CSS)

1) I forgot about that! Tracked as #1 in https://www.ventismedia.com/mantis/view ... 449#c65583
2) That's a good point. However, I couldn't really think of any other way to procedurally create those keys in a way that was reliable. I considered including the option "title" in there, but the title can easily be changed. and that would break it. Do you have any other ideas? Perhaps an optional "id" attribute? Personally, I don't think requiring a user-set "id" is the best option, because the goal of this feature is to help skin developers who don't have much programming knowledge.
3) Picking up default values from a LESS file wouldn't be feasible. I suppose I could make the "reset changes" button clear the customLESS value for dropdown and color picker controls, because those are hard-associated with only a single variable. But since radiogroup options are significantly more complex, I don't think it would be feasible to force all of their associated variables to be clear. (This is because different radiogroup options can technically set different variables, even though it's not a good idea to do so.)
Perhaps an alternative could be that skin-specific customLESS attributes be saved with an associated addon version, and they get reset when a new version of the addon is installed? After all, after #1 is implemented, the user will be prompted with the skin options next time they update the addon.

[TIV73]

2) Dropdown and color types already have something like an ID. They always target a specific, single variable. It's not a unique ID, strictly speaking, since I can target the same variable multiple times in different fields, but the configuration scheme is not complex enough to allow for inter-dependent field setups, so I cannot think of a use case where having multiple fields that target a single variable would be feasible. The options screen could be expanded with a check to make sure that each variable is only targeted by one field.
radio fields don't fit the scheme, but this could be worked around by giving them a variable field that is then used to store their value.

3) The option to null custom values would maybe just be needed for color and dropdown types. I think I read somewhere that passing an empty string to the setLess function makes it remove that preference, so you can already do that for radio fields.


I guess forcing the user to update their settings each time they install a new skin version would effectively solve 2 and 3, but I'm not sure how I feel about it UX wise.
It would force the user to either write down the color codes for all their settings or have them start from scratch each time they update a skin, which could ultimately drive users away from ever updating skins and, by extension, addons in general since they are afraid of losing settings.

[drakinite]

radio fields don't fit the scheme, but this could be worked around by giving them a variable field that is then used to store their value.

I don't follow. Radio fields are designed to support multiple variables. Could you clarify what you mean by that suggestion?

I guess forcing the user to update their settings each time they install a new skin version would effectively solve 2 and 3, but I'm not sure how I feel about it UX wise.
It would force the user to either write down the color codes for all their settings or have them start from scratch each time they update a skin, which could ultimately drive users away from ever updating skins and, by extension, addons in general since they are afraid of losing settings.

No, no - It would be very stupid to do that, you're right. That's not what I meant. When a new addon version is installed, the config panel can still be populated with the previous settings. But in the case where the config options are changed, the panel can do its best to repopulate the user's preferences. And when they click OK, the customLESS will be updated, eliminating problem #3.

[TIV73]

I don't follow. Radio fields are designed to support multiple variables. Could you clarify what you mean by that suggestion?

Sure, sorry. I meant a variable that saves the selection made by the user. To hold the value that's currently saved as x_radio in the [skin]_skinOptions section in persistent.json.

[drakinite]

Fyi, the next beta build (Not the one that was posted tonight) will include the new fixes: https://www.ventismedia.com/mantis/view ... 449#c65792

[TIV73]

Amazing, thanks a lot!

[drakinite]

No prob!