gui: Disable Save button in modals when invalid or no changes

[tomasz1986]

gui: Disable Save button in modals when invalid or no changes

Currently, the modals have their Save buttons enabled regardless if
anything has been modified or the input values are valid or not. This
makes it so that the buttons become active only when the user has
either edited any of the values and the input values are valid.

For this to work in the Advanced Configuration modal, Folders in
advancedFolders, Devices in advancedDevices, and Default Folder, Device
and Ignore Patterns in advancedDefaults need not use separate forms for
each repeated element but rather they must be wrapped into a single,
larger form.

Signed-off-by: Tomasz Wilczyński twilczynski@naver.com

[acolomb]

Nice idea. I have only glanced at the code so far, hope to take a closer look and do some testing soon.

[tomasz1986]

No problem, take your time 🙂.

For the record, this is just a crude method to detect whether the user has edited anything. The proper way would be to also check whether the modified values actually differ from their initial state (see https://stackoverflow.com/questions/40485315/reset-form-as-pristine-if-user-changed-input-values-to-their-initial-value), but I think that this shall be a much more serious undertaking for a different PR.

On side note, the GitHub's diff is really not great as it shows seemingly a lot of code changes while in the Advanced Configuration modal it's really just a few form tag modifications plus a lot of code alignment fixes with no actual changes.

[imsodin]

On side note, the GitHub's diff is really not great as it shows seemingly a lot of code changes while in the Advanced Configuration modal it's really just a few form tag modifications plus a lot of code alignment fixes with no actual changes.

You can hide whitespace only changes, very useful for cases like this (?w=1 query param does it - there has to be a UI element too but I don't know that).

Change looks sensible to me.

[tomasz1986]

You can hide whitespace only changes, very useful for cases like this (?w=1 query param does it - there has to be a UI element too but I don't know that).

Thank you! I've managed to find the button 😅. So much better now!

[acolomb]

Some small indentation fixes required. Maybe teach your editor to fix that?

On a side note, Even My Aunt Crashes the System by invoking C-Home C-Space C-End M-x indent-region RET. But I wouldn't seriously expect anyone to start learning such stuff today... 😉

Seriously, there is a logic error if you change something in a modal, then cancel and open it again, the Save button is still enabled. Probably need to reset the dirty flag on load?

[tomasz1986]

Seriously, there is a logic error if you change something in a modal, then cancel and open it again, the Save button is still enabled. Probably need to reset the dirty flag on load?

Fixed. I've followed the same logic as already used for folderEditor and deviceEditor.

[er-pa]

Even though I understand the idea behind it and do like it overall, from an end-user's perspective it's not too great at this point, in my opinion.

1.) It can be triggered by more than one condition while it's unclear which of them triggers it. When everything works as expected this is probably not an issue, but some odd things happen and you're just scratching your head. This, for example, is also the case for the fav-icon (exclamation-mark)...there's little as frustrating as not knowing why something is the way it is without it being very clear. Even more so if it actually has consequences for what we can or cannot do.
2.) The cursor becomes a 'blocked'-icon, which implies an error or faulty condition of some sort while this is not the case.
3.) The close-button next to it somewhat makes this confusing. An x-mark usually is quite neutral. But when a check-mark is besides it you get more of a good vs bad feeling. You're inclined to want to press the check-mark but now you can't and should press the X-mark - counterintuitive. Which is fine if there actually is an error, but that's no longer the case.

And, I think that this was already mentioned, the behaviour overall should be a bit more consistent imo; properly tracking changes. A space -> backspace enables the button and keeps it enabled, that kind of negates half of the point of this feature?