Page MenuHomePhabricator

The gallery dialog's Apply changes action is always enabled if there are images in the gallery
Closed, ResolvedPublic

Description

The action should instead be enabled/disabled according to whether any changes have been made.

Event Timeline

Change 471272 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Improve how dialog actions are enabled

https://gerrit.wikimedia.org/r/471272

Change 471272 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Improve how dialog actions are enabled

https://gerrit.wikimedia.org/r/471272

Ryasmeen renamed this task from The gallery dialog's Apply changes action is always enabled if there are images in the gallery to .Nov 14 2018, 1:11 AM
Ryasmeen added a project: Editing QA.

@matmarex: Assuming this is not on Beta cluster yet? Since, I see the "Apply changes" button being enabled, if I just open the dialog for an existing gallery.

It should be, but maybe it wasn't yet when you tested. Or maybe you just found some case where it's broken. It works as expected for me, at a glance (testing on https://en.wikipedia.beta.wmflabs.org/wiki/Canis).

Ok, there are at-least two workflows that are broken now :(

  1. Adding a new image to an existing gallery is not enabling the "Apply changes" button, reported: T209451
  1. Sometimes I am seeing the "Apply changes" button staying enabled even if there hasn't been any changes (Haven't found the exact steps yet for this, trying to).

Change 473600 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Update "Apply" button when image added

https://gerrit.wikimedia.org/r/473600

Changing the test priority for this one, a change that has already caused a regression needs thorough testing imho.

@matmarex: another regression: T209542, possibly related with this?

  1. Sometimes I am seeing the "Apply changes" button staying enabled even if there hasn't been any changes (Haven't found the exact steps yet for this, trying to).

@Ryasmeen I can help you here! I think you should find that everything in the Options tab[1] behaves as you would expect, whereas in the Images tab, once a change has made the action stays enabled, even if you undo. This is essentially because the Options tab[1] relates to attributes on the gallery node itself, whereas the Images tab relates to descendant nodes (the gallery caption is also a descendant of the gallery).

It's not ideal, but I think it's better than what went before... I guess the task should arguably remain open until fixed properly, but having to check changes to the gallery's descendant nodes is a bit more complicated.

[1] except for the gallery caption

I assumed that disabling the "Apply" button again after changes are undone was not a requirement here, so I did not try to get it working. @Tchanders is correct that it is a bit more difficult for this dialog than for others. But I looked into it now and it actually seems feasible, so let's do it as well. I'll submit a separate patch for it (we should get the existing one merged quickly to fix the regression).

Change 473840 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Improve modification checks

https://gerrit.wikimedia.org/r/473840

After this new patch is merged, undoing the changes to an image caption or alt text, or to the gallery caption, or to the order of images, or removing a previously added image, will now disable the "Apply" button again.

The following cases will *not* disable the button again, and it is not feasible to implement them:

  • Re-adding a previously removed image with identical options
  • Changing any caption to the old value by other means than "Undo"
  • Changing image caption or alt text to the old value after switching to a different image and then back

Change 473600 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Update "Apply" button when image added

https://gerrit.wikimedia.org/r/473600

  1. Sometimes I am seeing the "Apply changes" button staying enabled even if there hasn't been any changes (Haven't found the exact steps yet for this, trying to).

@Ryasmeen I can help you here! I think you should find that everything in the Options tab[1] behaves as you would expect, whereas in the Images tab, once a change has made the action stays enabled, even if you undo. This is essentially because the Options tab[1] relates to attributes on the gallery node itself, whereas the Images tab relates to descendant nodes (the gallery caption is also a descendant of the gallery).

It's not ideal, but I think it's better than what went before... I guess the task should arguably remain open until fixed properly, but having to check changes to the gallery's descendant nodes is a bit more complicated.

[1] except for the gallery caption

@Tchanders: Yeah, I understand your point :) My comment was however about a straightforward scenario where I just re-opened the gallery dialog and saw the "Apply Changes" button being enabled, didn't do any "undo" action before that step. It's probably a very edge case therefore finding it difficult to replicate.

After this new patch is merged, undoing the changes to an image caption or alt text, or to the gallery caption, or to the order of images, or removing a previously added image, will now disable the "Apply" button again.

The following cases will *not* disable the button again, and it is not feasible to implement them:

  • Re-adding a previously removed image with identical options
  • Changing any caption to the old value by other means than "Undo"
  • Changing image caption or alt text to the old value after switching to a different image and then back

@matmarex: So if I understand correctly the patch that was supposed to do all these has been reverted because it breaks other dialogs? Is it still being worked on? If so, should we keep this task open until these scenarios are all passed?

@Ryasmeen No, nothing relevant to this task was reverted, AFAIK. These cases are unfeasible to fix due to how the dialog stores its state internally (either we don't have the "old" state to compare to, or we don't have a way of comparing that ignores internal changes that don't matter), and would require more work to resolve than I'm comfortable doing on a random task not really related to what we're working on as a team.

(I would prefer filing a separate task if we want to track that work.)

All other cases will be fixed by the latest patch I submitted (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/473840). Which, by the way, is stalled: @Esanders left a -1 review that I disagreed with and I'm waiting for a reply.

Change 473840 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Improve modification checks

https://gerrit.wikimedia.org/r/473840