Fixed an xlet-settings issue causing invalid callback/signal triggers with some setting types

[NotSirius-A]

Hello, this PR closes #13151. I fixed a problem described in the linked issue. I've also found that this problem comes up with the list setting type.

Issue was caused by an improper condition checking whether the setting was changed or not. More specifically, for three setting types: datechooser, timechooser and list it would always evalute them as changed, because the condition if (value == oldValue) continue; is always false for them. This causes these setting types to always fire their respective callbacks every time any other setting is changed.

My solution implements a new function _isValueChanged(), which checks whether the setting was actually changed. The implementation seems a bit too complex for what it does, but I don't think there's a different way. I'm pretty sure JS doesn't have a good, robust, builtin way of checking whether any two object have the same keys/values, so I think it's necessary to implement such functionality separately for every unique setting type.

[fredcw]

I think the method name should be changed to _isValueUnchanged or it's return value negated bc _isValueChanged sounds like it should return true if it has changed.

I'm curious about a couple of things. Why does type need to be checked (although I guess it can't do any harm):

if(oldValueType === valueType

Also, would JSON.stringify not work?

JSON.stringify(obj1) !== JSON.stringify(obj2)

[NotSirius-A]

Thanks for these suggestions.
Yes you're right the name should be changed. It was originally _isValueEqual and I just changed the name without thinking about it.

Checking if the old type is the same as new type is probably not necessary, its just a precaution, maybe too cautious. I figured its theoretically possible to manually change the type in the .json in between updates, which could result in potentially weird behavior.

JSON.stringify() would probably work and be a lot cleaner, but I was worried about the performance. I think that stringifying potentially large objects can be a bit slow. I am no JS expert, maybe its fine, but in my experience parsing is usually demanding. Also, JSON.stringify() method would fail if properties were ordered differently, which I think could happen with manual editing.

[fredcw]

I agree, stringify would likely be much slower.

If oldValueType !== valueType then the comparison is done anyway right? :

} else {
    equal = (value === oldValue);
}

Maybe put:

if (oldValueType !== valueType) return false;

at the beginning?

Also, maybe my idea of _isValueUnchanged wasn't so good bc it's a double negative (unchanged ... return false) which can be confusing xD. I prefer your original _isValueEqual

[NotSirius-A]

I've changed the function name to: _hasSettingChanged(), I think it's better now. The reason why I didn't like _isValueEqual() is because to me it sounds a bit too general. This is not a general function it's just for one specific purpose. Also, _isValueEqual() suggests that it's comparing two unrelated values, but it's not. One is a value and the other previous oldValue it clearly suggest a time relationship between them, so I think the name should reflect that, hence the has prefix in the name. What do you think?

I did put if (oldValueType !== valueType) return true; at the beginning. You're right it makes more sense.

[fredcw]

Nice. Now that I think about it some more, I don't think that checking the type is necessary. I think only the xlet developer would need to change the type and they would do it in the settings-schema.json file in the xlet directory. In which case the current user settings file would be updated automatically in _ensureSettingsFiles() when the applet is restarted.

[NotSirius-A]

You're probably right, I didn't consider _ensureSettingsFiles() before. I don't think there's much harm in doing some extra checks though. I guess I will delete the if (oldValueType !== valueType) return true; line, so that future contributors won't wrongly think it's important.

[fredcw]

Omg, I feel like I'm a bad influence on you now. 😂

if (value.length !== oldValue.length) return true;

undefined === undefined xD

[NotSirius-A]

It's fine 😂. I feel like I've coded this poorly, you've given many good suggestions.

undefined === undefined xD

Isn't this fine? I'm not sure what's wrong here? We've established that value and oldValue type is the same, so if we're dealing with for example a Number or other type that doesn't implement .length both are guaranteed to be undefined. In such case (value.length !== oldValue.length) will always evaluate to false, therefore the function will not return true and will proceed normally.

I guess using undefined as a valid logic input does seem weird, but this is JS after all.

[fredcw]

You're absolutely right, it does work. I guess the only issue is that for the reader, it relies on their knowledge of the finer points of JS. In python or C for instance, trying to access an undefined property will throw an error. So although it works, it's perhaps less readable for those less familiar with JS's more subtle behaviours.

[NotSirius-A]

That's fair. I've changed it to only check list types where length is definitely defined.

[fredcw]

looks good.