PICARD-2287: Copy and Paste multiple Tags

[StevilKnevil]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other

Multiple tags can be selected, and then copied and pasted between files.

PICARD-2287

Solution

When multiple tags are selected, copying them to the clipboard turns them into a JSON string with mimetype application/json;contents=picard-tags. Pasting this mimetype will iterate over all the tags in the JSON, and will apply the new_value to the current file. If new_value doesn't exist for the given tag in the clipboard then it will use original_value

Example of the JSON on the clipboard:

{
	"album": {
		"new_value": ["test"],
		"original_value": [""]
	}, 
	"title": {
		"new_value": ["My Song"], 
		"original_value": ["My Song"]
	},
	"~length": {
		"new_value": ["3:18"], 
		"original_value": ["3:18"]
	}
}

This means that Copy and Paste now need to be added to the context menu when there is more than one cell selected.

The behaviour when a single cell is selected is unchanged, i.e. copy the value as plain text.

[zas]

Apart coding style issues, please create a matching ticket, and be sure to prefix PR title with PICARD-xxxx (it creates a link between jira tickets & github PRs).
In the ticket, explain why this feature is interesting, and how it works. Title should be clear (think it will appear in Changelogs).

[rdswift]

Also, is this something that should be included / documented in the Picard User Guide? If so, please add a documentation ticket with a reference to this PR. (I noticed that you removed that part of the PR template for this PR.) Thanks.

[StevilKnevil]

Sorry - I meant to mark this as draft, while garnering feedback!

[StevilKnevil]

Also, is this something that should be included / documented in the Picard User Guide? If so, please add a documentation ticket with a reference to this PR. (I noticed that you removed that part of the PR template for this PR.) Thanks.

I did have a trawl through the documentation and didn't see anywhere that stood out as an obvious place to update the information. Perhaps a whole new section of documentation, but I'm not sure (at this stage) whether this warrants it - feedback welcomed if it's non-obvious functionality.

[Sophist-UK]

The current code (at the time of this comment) appears to be designed to meet the individual use case of @StevilKnevil, and as such is great as a proof of concept but it is not IMO generically applicable and should not be merged into Picard in its current form.

I have copied the following comments I made from the Matrix / IRC discussion with my concerns:

1. I can see why you might want to put the original value in the JSON in order to confirm that you are replacing the correct value, but that would be a very specific use case and not the general one where only the new values are needed. The JSON should be simplified to just be `{"title": "Waterloo", "artist": "ABBA"}.

2. I think you need to handle values as both arrays as in your original JSON and plain strings as in point 1.

3. This JSON is limited to a single track which makes sense for pasting into the metadata area of a single selected track. But do we want to extend the paste to copying to multiple selected tracks, and if so then A) should this be limited to album-related metadata and B) do we want to extend the JSON schema to allow for several tracks specific data to be specified using e.g. some sort of MBIDs (which IMO will be complicated and shouldn't be attempted at this point - but you should make sure now that the JSON schema can be extended later).

4. I am not sure that I agree with @outsidecontext's suggestion to use QT's special clipboard functionality - users will want to
   copy and paste between applications, so you want to have compatibility here - and that means either using a pre-defined JSON clipboard type or even thinking about implementing several common clipboard formats e.g. plain text, JSON, perhaps a spreadsheet-specific format or XML too or perhaps there is a key/value pair format.

5. Preserving Copy backwards compatibility probably should mean NOT copying JSON to the plain clipboard, but instead only by default to the JSON specific clipboard. That will also likely mean that we will need a context menu to copy JSON to the plain clipboard.

6. We will also need to preserve Paste backwards compatibility and deal with several different types of Paste contents. So you probably need to analyse what tracks are selected to be displayed in the metadata area, what metadata rows are selected and whether there is an edit box open and combine that with analysis of the possibly several types of paste data available on the clipboard before determining which data and what type of paste to do - and you probably want to warn the user if they are about to paste album data across several albums or track data across several tracks.

Addendum:

7. More and more we are using hidden metadata - do we want the capability for Copying and / or Pasting hidden metadata names that start with ~ (~length is an exception that isn't hidden) because Picard creates some of these and plugins create a massive amount more. I suspect that this would require some sort of Options... checkable option to show or hide the hidden metadata (much as in Windows or Linux file managers you can opt to show or re-hide hidden files).

[phw]

Thanks for this PR. First off this works really well, and it is a very welcomed feature. I noticed I had even already created a ticket for this a long time ago. I updated the PR to link to PICARD-2287 instead and closed the newer one as a duplicate.

I have a few coding comments. But my main concern about right now is use experience when pasting. While this is a very powerful feature, it also can lead to some unwanted surprise if a paste suddenly overwrites a large number of tags, where the user maybe expected just to paste a value. This is especially bad as Picard does not yet support any undo functionality.

I see two possible solutions:

1. Make copying the full tags an explicit option. At least then the user is aware they copied multiple values. But copying is less convenient.
2. When pasting full tags, show a warning dialog that multiple tags would be overwritten. We could have an option to not show this dialog, which would allow advanced users who use this often to skip it. Ideally the dialog would also show which tags will get overwritten or even show a preview of the changes.

Even though more complex, I think the second option would work best.

[phw]

I am not sure that I agree with @outsidecontext's suggestion to use QT's special clipboard functionality - users will want to copy and paste between applications, so you want to have compatibility here

@Sophist-UK You can store multiple representations of the data in the clipboard. So the JSON would be for internal use. The code can also fill in a plain text representation that by default would be pasted in other applications. Right now in the implementation this also contains the JSON structure, but I think it shouldn't. See my comment above.

perhaps a spreadsheet-specific format

That's actually rather easy, as the spreadsheet tools all support tab separated values. And this also makes for a nice default text representation of multiple tags, as pasting this to a text editor is also nice.

Supporting a proper plain text representation is definitely something that needs to be considered here. Other formats can of course be added, but this is definitely outside the scope of this PR. Let's focus on what this PR tries to enable: Copy and pasting multiple tags between files inside Picard.

[Sophist-UK]

@phw Philipp

Steve and I have been discussing this privately on Discord, the gist of which was very similar to your comment - except we were thinking CSV rather than TSV.

And we were thinking monospaced tabular as a default text representation rather than TSV - but your idea might work better.

[phw]

I looked a bit what other tagging tools are doing. A lot don't really support a table view like Picard and just have some text fields.

MP3Tag generally shows all files with their tags in a spreadsheet like view, but doesn't allow selecting multiple specific values. It kind of supports copying the tags from one file to another internally, but to the system clipboard it only writes a plain/text list of compact info about the files:

1. Nine Inch Nails - [Ghosts I-IV CD1/1 #02] 2 Ghosts I
2. Paradise Lost - [Symbol of Life CD1 #03] Two Worlds

Kid3 does have a spreadsheet like view for the metadata, but does not allow selecting multiple rows or fields. You can only edit a single field and copy data from there.

Puddletag is interesting, as it writes a JSON structure to the normal text clipboard. See the selection in below screenshot.

This gives the following structure:

[
    {
        "title": [
            "Hells Bells"
        ],
        "album": [
            "Back in Black"
        ],
        "track": [
            "1/10"
        ],
        "__length": "05:12"
    },
    {
        "title": [
            "Shoot to Thrill"
        ],
        "album": [
            "Back in Black"
        ],
        "track": [
            "2/10"
        ],
        "__length": "05:17"
    }
]

Very similar to ours. But I still think the approach from this PR with custom mime data and as discussed a separate text representation with spreadsheet compatibility is the better solution. App specific JSON data in plaintext clipboard is not that useful.

[StevilKnevil]

This is all really good input - thanks everyone, it's what I'd hoped to get from raising this PR. I'm going to iterate again taking the above into account. I reckon I'm about 20% done so far :)

My focus is to get copy and paste working inside Picard, and for Copy to other apps. There's definitely a follow up PR for paste from other apps.

[zas]

@StevilKnevil please fix style issues, it would ease reviews

[zas]

@StevilKnevil @phw I noticed we are updating File objects more than once when pasting multiple values. In order to avoid this I wrote StevilKnevil#1

Basically I split the code that update objects so we can delay the object.update() calls as well as ensuring we call that only once per object.
So the code that paste multiple values is modified so we get a set of objects to update at the end.

Please have a look and tell me what you think.

[StevilKnevil]

@StevilKnevil @phw I noticed we are updating File objects more than once when pasting multiple values. In order to avoid this I wrote StevilKnevil#1
Please have a look and tell me what you think.

I see (and accept) your suggestion, and have a counter proposal: StevilKnevil#2. Essentially extend the mime data handling to have both a copy to mimedata from TagDiff and a populate metadatabox from mimedata function. That way the paste function just goes through the list (in order) and the first entry in that list that can handle one of the mimedatas on the clipboard is used to paste from the clipboard

[zas]

LGTM

[phw]

@StevilKnevil This works now great and code looks good to me. I like how it also improves the general copy experience.

There is an accidentally committed bytecode file, but otherwise this is good to go.

[zas]

LGTM

[phw]

Great work, thanks a lot