-
-
Notifications
You must be signed in to change notification settings - Fork 388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Toggle preserve state #2478
base: master
Are you sure you want to change the base?
Toggle preserve state #2478
Conversation
This PR is experimental:
Please test and give feedback |
I haven't had a chance to test this yet, but it looks like a very nice addition. EDIT: I don't know how, but I completely missed the |
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. |
- it helps to match displayed tags and their names in scripts
4268120
to
861419e
Compare
And possibly a highlight colour for the preserved tag name? |
But does this override Preserved state? I mean if a tag is marked as preserved, we don't expect it to be modified right? |
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:
|
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.
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:
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:
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. |
@phw, @rdswift I would like to keep this as is if:
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:
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). |
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 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. |
Summary
Problem
Solution
Action
Additional actions required: