Toggle preserve state

[zas]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:

Problem

• JIRA ticket (optional): PICARD-XXX

Solution

Action

Additional actions required:

• Update Picard documentation (please include a reference to this PR)
• Other (please specify below)

[zas]

This PR is experimental:

• switch to toggle Preserve state, rather than having Add & Remove
• one can select multiple tags and toggle them at once (before only one could be switched at once)
• add a visual marker (normal text + [P], translatable, an icon could be used too but that's more complicated) when the tag is in preserve list
• disable edit/remove for preserved tags (of course, toggling will let user edit/remove again)
• unrelated, but add a tooltip over tag names showing the internal tag name (as it appears in scripts and various lists)

Please test and give feedback

[rdswift]

I haven't had a chance to test this yet, but it looks like a very nice addition. I wonder if it might also help if there was a way to clearly show which tags are being preserved? I actually wonder if it would make sense to have a checkbox on each line (to the left of the tag name to indicate the status (and could also be used to toggle the status for that tag).

EDIT: I don't know how, but I completely missed the [p] indicator. That addresses my comment. Sorry for the confusion.

[phw]

I haven't run the code, but I'm unsure about disabling manual editing of preserved columns. Is this really something we want to do? I always understood the preserved setting as something that prevents automatic change, but manual editing is unrestricted and the user can change the tags of individual tags as they wish.

Maybe we should rather add some info while editing that this is a preserved tag? Partially this is already the case with the [P] specified, but maybe we could also add some note to the edit tag dialog.

[rdswift]

Maybe we should rather add some info while editing that this is a preserved tag? Partially this is already the case with the [P] specified, but maybe we could also add some note to the edit tag dialog.

And possibly a highlight colour for the preserved tag name?

[zas]

manual editing is unrestricted and the user can change the tags of individual tags as they wish.

But does this override Preserved state? I mean if a tag is marked as preserved, we don't expect it to be modified right?

[zas]

And possibly a highlight colour for the preserved tag name?

I changed the tag name style from bold to normal in this case too. But I didn't touch the New Value column yet, we need to clarify expected behaviors in various cases:

• what happens when one sets Preserved to a tag but the value was already modified (but not saved ofc)?
• should we make it clear the new value will not be written, or just display the original value instead
• perhaps I don't understand well what preserved or not actually does

[rdswift]

I changed the tag name style from bold to normal in this case too.

I saw that, but it really didn't make it obvious to me. The color idea was just a suggestion, and I can certainly live without it.

But I didn't touch the New Value column yet, we need to clarify expected behaviors in various cases:

• what happens when one sets Preserved to a tag but the value was already modified (but not saved ofc)?
• should we make it clear the new value will not be written, or just display the original value instead
• perhaps I don't understand well what preserved or not actually does

I'm not sure I understand the intent fully either, but what I think would make sense to me (from a user's perspective) is:

1. If a tag is marked as preserved at the time the release is loaded, it is shown as being preserved ([p], not bolded, coloured, or whatever is decided) and the new value should be shown as the current value (if there is a current value set) or the value from MB (if there is no current value set).

2. If a tag is not marked as preserved at the time the release is loaded, the new value should be shown as the value from MB (if there is no current value set). This could result in a current value being set and the new value being blank, in which case I expect that the current value would be kept unless the "Clear existing tags" option was enabled. Perhaps the new value could be shown as something like "[blank - current value will be kept]" to clarify what will happen.

3. If a tag is toggled from "not preserved" to "preserved" then the line should be revised as per Item 1 above.

4. If a tag is toggled from "preserved" to "not preserved" then any preserved indication should be removed from the line, and the new value should be updated to the value from MB if it still available. If it is not still available, the new value should remain the same as the before the tag was toggled, and the user would have to perform a reload to update the values for any tags changed to "not preserved" (and we would need to somehow make that clear to the user).

5. Any manual changes to a tag should always be honoured. In other words, the new values shown should always be the ones written, with the possible exception of Item 2 above.

Alternately, the new value could always show the value from MB, and we could rely on the "preserved" indicator to inform the user that the tag will not be updated to the new value (unless the current value is empty). Any manual changes made to either "preserved" or "not preserved" tags should be honoured, in which case any manual changes should be somehow shown as having been overridden (different colour highlight, bolded, or whatever).

The clearest solution (although not practical due to space constraints) would be to have three columns:

• Original value (with preserved status indicator)
• New value (from MB)
• Value to save (which can be manually overridden)

In this case, the "Value to save" could be set to "Original value" (or "New value" if there is no "Original value") if the tag is "preserved", or "New value" (or "Original value" if there is no "New value") if the tag is "not preserved", and the value can be updated for a tag automatically whenever the "preserved" state for the tag is toggled.

[zas]

@phw, @rdswift I would like to keep this as is if:

• it works for you and you think that's a good move
• you don't find any bug

The discussion above seems to suggest improvements over this PR, so I'd rather merge this and work on improvements later.

@rdswift : I avoided to use color change (and added [P] visual indicator) for this because colors are always a problem for accessibility. As improvement we could just use a small icon instead of [P]

To me this PR is an improvement on what we have now:

• visual indicator for tags in Preserved list
• easy toggling of one or more tags (using multiple selection)
• one label in context menu instead of two (Add + Remove vs Toggle)

About editing a preserved tag you didn't clearly answer (or I misunderstood), should we protect a Preserved tag from edition or not? Why (or not)?

I can easily left this part of changes out but I still fail to understand what the point of editing a tag that's marked as preserved (note: it doesn't prevent to edit it totally, toggle it to not preserved and edition is possible).

[zas]

Transcript of a small discussion we had about this on IRC:

outsidecontext> do tristate checkboxes work as part of menu actions? if so we could make it to show the mixed state. but toggling would then still toggle all in one direction or another
zas> hmmm, isn't that even more confusing?
zas> well, dunno, we can give it a try
outsidecontext> I don't think so. that's how such operations most often work when selecting multiple things
zas> also the [P] should perhaps be an icon in the New tag column instead
outsidecontext> I'm also still unsure about completely disable editing. the preserved tags in the past only prevented the automatic overwriting, but users have always be free to change the tags.
outsidecontext> it could work maybe if we rename the option to read-only tags, though
outsidecontext> would also make the toggle more easily understandable. something like "[ ] Read only"
zas> I suggest we keep this PR around for a while and come back to it when we have clearer idea about what to do. I guess the whole purpose of Preserve tags is ... to ensure those aren't overwritten
zas> so read only could work
zas> so the value in New value column should be the same as previous column when the tag is marked "preserved"
zas> but we don't want to erase previous changes so we need to store previously modified value (if it was)
zas> basically edit tag value -> new value displayed, set as preserved, original value displayed, unset preserved, previous new value displayed again
zas> plus a visual indicator to mark tags which are set as read-only
zas> and tristate checkbox to toggle states
outsidecontext> sounds good. but we probably would need to also change where this gets applied
zas> yes
outsidecontext> preserved tags currently get applied on data loading. afterwards you are free to change it as you wish. I'm not sure, but this might even apply to plugins. but definitely to manual changes, including running scripts manually
zas> we should instead check for it at save time
outsidecontext> so proper way would likely be to apply this before saving as well to ensure original values get used
zas> ^^this
zas> it would be a much more predictable behavior
outsidecontext> then we would also be free to store any modification in the new metadata, but have e.g. the metadatabox display the original value if read-only

[phw]

@zas Thinking about our discussion from yesterday there is more that this would change. As I understand the code in this PR it prevents manual editing of the tag, no matter if the value was present in the original file or not. If we keep this behavior and rename the option to "readonly" it would be a rather large deviation from the current preserved tags.

The preserved tags prevent that Picard replaces existing tags with data from MB on loading. It will still set the data if it is not present in the file before. Likewise there is no limitation of later editing it.

I think the new implementation should at least keep the first part and allow modification of the tag if it is not yet present in the file. But actually I think readonly does not capture this concept very good, and "preserved" might indeed stay the better wording.

But then I'm still not sure about preventing the manual editing completely. I would still be in favor of warning the user instead.

What we probably should ensure is that preserved tags are also save from modification by plugins. I have the suspicion that plugins currently can change those tags.