Conditional menu, copy-paste items

[retorquere]

PR for #78

[retorquere]

That error is now fixed, this PR is ready for review for my part

[wshanks]

Great. I looked over it quickly and it looked good. I will try to look over it more closely this weekend and see if I have any questions.

I'll mention @bjohas here as well to see if he has any feedback since he created #78.

[retorquere]

This currently only adds copy, paste and paste empty -- I'm not sure what copy non-empty is supposed to accomplish. The paste only uses fields from the copy that are non-empty, ie it doesn't clear out fields in the target in any case currently.

[bjohas]

Regarding getting down to one copy / one paste: As the right click options can be enabled / disabled in the Zutilo menu, it might be helpful to have the variants as shown above. Users can then chose what they want?

The four options were (renamed):

1. Copy all all fields to clipboard: Copies the item metadata to clipboard (right panel only, not notes).
2. Copy non-empty fields to clipboard: Copies the item metadata to clipboard (right panel only, not notes), omitting empty fields.
3. Paste if empty all fields (without overwriting): Pastes all fields that are on the clipboard, but without overwriting existing data. Merges author fields if there is one in the pasted data.
4. Paste over fields (and overwrite): Pastes all fields that are on the clipboard, overwriting existing data. Replaces authors if there is an author field in the pasted data.

The reasons to have copy-non-empty is as follows: I can prepare a new item, that has specific fields that I want to be overwritten. So e.g. I put in the correct journal name, which is the only entry. I now do a copy-non-empty, and the clipboard now only has the journal name. I then do a paste-over for 10 items, which corrects the journal name (whether there's a journal name or not).

In a sense, options 1/3 and 2/4 (as shown above) are pairs. 1/3 is a 'complete/augment', while 2/4 is a 'conform certain fields'. The combination 1/4 is just a kind of 'duplicate' function, so redundant. The combinations, 1/3 and 2/3 are identical.

There may be an argument to order the options like this:

1. Copy all all fields to clipboard: Copies the item metadata to clipboard (right panel only, not notes).
2. Paste if empty all fields (without overwriting): Pastes all fields that are on the clipboard, but without overwriting existing data. Merges author fields if there is one in the pasted data.
3. Copy non-empty fields to clipboard: Copies the item metadata to clipboard (right panel only, not notes), omitting empty fields.
4. Paste over fields (and overwrite): Pastes all fields that are on the clipboard, overwriting existing data. Replaces authors if there is an author field in the pasted data.

as it makes the (new ordering) 1/2 and 3/4 pairing clearer. This makes the 3/2 combination a bit odd, but it's equivalent to 1/2, and I guess still adjacent to 2.

[bjohas]

@willsALMANJ "What happens if you paste onto an item of a different type? Would it paste in the common fields and ignore the ones that do not fit?"

I think so - that seems like the right behaviour. E.g. I may want to copy a set of authors onto a journal paper, a book, and a blog post or similar.

At the moment, does the paste-all change the item type? The correct behaviour (I think) would be to not change the item type (as that would also mean having to deal with disappearing fields). The functions were meant to be able to completement/selectively replace metadata, rather than changing item type.

[retorquere]

The reasons to have copy-non-empty is as follows: I can prepare a new item, that has specific fields that I want to be overwritten. So e.g. I put in the correct journal name, which is the only entry. I now do a copy-non-empty, and the clipboard now only has the journal name. I then do a paste-over for 10 items, which corrects the journal name (whether there's a journal name or not).

This only makes sense if you want empty fields in the source item clear out filled fields in the target item if you perform 4. In Javascript, there's very little difference between "not present" and "empty" -- it would still be mostly the paste logic which decides what happens with non-present/empty fields. I really don't see any difference between having

{ "itemType": "journalArticle", "title": "some title", "pages": "" }

and

{ "itemType": "journalArticle", "title": "some title" }

on the clipboard. In both instances it just means "pages is not set". Unless you want to be able to clear out the target on paste if it has pages set. In that case, there would be a difference between the two, but that use-case hasn't been discussed yet. I don't see any other use-case that would make the distinction relevant. Currently, paste does not clear out fields where the target field is set but the source field is not, regardless of which paste option you choose.

In a sense, options 1/3 and 2/4 (as shown above) are pairs. 1/3 is a 'complete/augment', while 2/4 is a 'conform certain fields'. The combination 1/4 is just a kind of 'duplicate' function, so redundant. The combinations, 1/3 and 2/3 are identical.

This still depends on whether you mean "conform" to include "clear out". If not, the behavior is entirely determined by whether you choose 3 or 4 and not whether you choose 1 or 2.

@willsALMANJ "What happens if you paste onto an item of a different type? Would it paste in the common fields and ignore the ones that do not fit?"

I think so - that seems like the right behaviour. E.g. I may want to copy a set of authors onto a journal paper, a book, and a blog post or similar.

At the moment, does the paste-all change the item type? The correct behaviour (I think) would be to not change the item type (as that would also mean having to deal with disappearing fields). The functions were meant to be able to completement/selectively replace metadata, rather than changing item type.

current paste does not change the itemType -- changing the itemtype would make things more involved still. Paste currently skips over any fields that are not valid for the target item (and which those fields are may thus vary per selected item pasted over).

[bjohas]

current paste does not change the itemType -- changing the itemtype would make things more involved still. Paste currently skips over any fields that are not valid for the target item (and which those fields are may thus vary per selected item pasted over).

ok great!

Copy all all fields to clipboard: Copies the item metadata to clipboard (right panel only, not notes). Paste if empty all fields (without overwriting): Pastes all fields that are on the clipboard, but without overwriting existing data. Merges author fields if there is one in the pasted data. Copy non-empty fields to clipboard: Copies the item metadata to clipboard (right panel only, not notes), omitting empty fields. Paste over fields (and overwrite): Pastes all fields that are on the clipboard, overwriting existing data. Replaces authors if there is an author field in the pasted data.

Yes, that was the idea!

I think the two types of copy that you combine with two types of paste is poor design though, as it's not that striaght forward.

How about this:

1. Copy all source fields to clipboard: Copies the item metadata to clipboard (right panel only, not notes).
2. Paste+Merge. (paste where source has value and target has none/empty. Merges author fields if there are some in the pasted data.)
3. Paste+Clobber. (paste where source has value, Replaces authors if there is an author field in the pasted data.)
4. Paste-Clear (paste all fields from source, even if they are empty).

Somewhere in the documentation, we then write: If you are using copy + paste-clear, in sequence and onto the same item type, you are setting all source fields to the target (in a sense, duplicating the item). The use case for this are:

1. You have an item with correct metadata in one library, but with incorrect metadata in another lbirary. The copy+paste-clear saves you from dragging the item to the new library and merging.
2. You have imported a PDF, no metadata was recognised, so you created a parent item with incomplete, possibly wrong, data. You then realise that this is part of a series, where you have metadata, which you want to copy (clearing all your previous metadata). Instead of duplicating an item from the series, and moving the PDF across, you just copy the metadata from the source onto the target.
3. You have an item that has been corrected (e.g. a book chapter) that you now want to copy onto another set of chapters in the same book. In this process, you have to clear some fields. You'd copy the item, edit the clipboard to only retain new fields and fields to be cleared, and then use Paste-Clear.

Does that make sense? I think it's simpler than having two copies and two pastes.

[retorquere]

3. Paste+Clobber. (paste where source has value, Replaces authors if there is an author field in the pasted data.)

But only if there are authors in the copied data? That means author (or creators to be more precise) get special treatment. Otherwise, conceptually, it would be empty, and clobber would imply clearing out existing creators in the paste target.

Does that make sense? I think it's simpler than having two copies and two pastes.

It seems a lot clearer to me at least.

[bjohas]

But only if there are authors in the copied data? That means author (or creators to be more precise) get special treatment. Otherwise, conceptually, it would be empty, and clobber would imply clearing out existing creators in the paste target.

For paste+clobber, the authors get replaced (if the source has authors). For paste+clear, the authors get removed if there is an empty authors field (in the source).

Btw. - what about tags? Zutilo already has copy tags / remove / paste tags, so we should be able to ignore tags for the purposes of this PR? (I.e. the new functions ignore tags)

[retorquere]

And creators get replaced if present when clear is chosen I take it? Clear just means "overwrite anything, even if the source was empty", correct?

I'd propose to use the same semantics for creators and tags, or ignore tags if you don't expect to usually want to copy them.

[retorquere]

The new copy semantics (except tags) are now implemented. I'm leaving the docs and the final decision on what stuff is named in the UI to you guys.

[bjohas]

Thank you! @retorquere - Could you also produce an XPI? I dont have the dev version installed and am on a new laptop without my usual Terminal tools for zipping. I'll also add some notes on the new functionality into the documentation pages.

[retorquere]

https://0x0.st/zfKG.xpi

So what do we do about the tags?

[bjohas]

No need to do anything about tags - that functionality already exists in zutilo, and it's good to keep it seoarate. (Imho)

[bjohas]

Thanks for the plugin as XPI! I have a couple more requests:

(1) Could the JSON be "formatted", i.e. indents etc. Or is that problematic? It would be helpful for amending it, or checking what's there.

(2) When the JSON Is copied to the clipboard, we need all fields, including empty fields. This has two reasons (A) It allows people to see what fields there are, and then amend manually. (B) the empty fields are needed for paste-clear.

Does that sound feasible?

[retorquere]

(1) and (2) added

edit: forget the previous XPI url -- get https://0x0.st/zfcp.xpi

[wshanks]

This looks good to me. I hope the SQL query doesn't break because I only half understand it.

I am a little concerned about how things like attachments, notes, and libraryID have to be filtered out explicitly. Is there no way to filter out "the stuff that shows up in the item info box" from other item properties? I think it works fine now, but I am just wondering about what happens if a new item property like attachments or notes gets added in the future.

One oddity that I noticed -- when I pasted with overwrite or clear, the selected target item was updated visually. When I pasted with merge, the selected target item was not updated. I had to select another item and then select the target item again to see the new entries show up.

What is the use case for the merge paste? I was thinking it was for when you had several similar items missing metadata and you added the data to one item and then copied and merge pasted it into the others. Since only the missing fields are written, the other item information is kept in tact. But in this case, I wouldn't want usually want the creators from the first item merged in I think. Still I wonder if this is useful enough vs. just expecting users to copy an item, edit it down to the fields they want, and then do an overwrite paste.

For me, the use cases for the overwrite and clear paste are similar. There is some common metadata I definitely want to be present in a set of items. I think it is unlikely I would want all the fields to be the same (otherwise I would just duplicate the item), so I would probably copy an item to the clipboard, paste into an external text editor, edit the copied item externally to cut down to just the fields I want to set, and then copy and paste into the items I wanted to update. Since in this use case I am already editing by hand, I could just remove the empty fields if I didn't want them cleared. So for me, just having the clear paste and not the overwrite paste would be enough. The one nice thing I see about the overwrite paste is that if I don't care about clearing out fields I could duplicate one item to a dummy item, clear out any fields I didn't want to overwrite, then copy it, and overwrite paste it -- effectively using this duplicated dummy item instead of editing the JSON in an external editor.

Other follow up items to take care of:

1. Finalize strings and then update the other locales.
2. Add the corresponding keyboard shortcuts.
3. Handle the CopyItems const in some way. Right now, when Zutilo is uninstalled and reinstalled, there is an error about redefining the const and loading fails.

[bjohas]

Regarding the three paste options: Let me go through the use-cases and write them down, and see what's really needed and how it can be explained well. Will take me a day or so...

[retorquere]

This looks good to me. I hope the SQL query doesn't break because I only half understand it.

I've removed the SQL query. You still have to know a fair but about the internals of Zotero for this, but at least this is in the regular API in itemFields.js

I am a little concerned about how things like attachments, notes, and libraryID have to be filtered out explicitly. Is there no way to filter out "the stuff that shows up in the item info box" from other item properties? I think it works fine now, but I am just wondering about what happens if a new item property like attachments or notes gets added in the future.

That wasn't actually needed anymore -- isValidForType takes care of that later.

One oddity that I noticed -- when I pasted with overwrite or clear, the selected target item was updated visually. When I pasted with merge, the selected target item was not updated. I had to select another item and then select the target item again to see the new entries show up.

I don't honestly know what could cause that. One function handles both, only different thing is what fields are affected. The save is literally exactly the same.

What is the use case for the merge paste? I was thinking it was for when you had several similar items missing metadata and you added the data to one item and then copied and merge pasted it into the others. Since only the missing fields are written, the other item information is kept in tact. But in this case, I wouldn't want usually want the creators from the first item merged in I think. Still I wonder if this is useful enough vs. just expecting users to copy an item, edit it down to the fields they want, and then do an overwrite paste.

I'll leave that for @bjohas and @willsALMANJ, but I'd prefer to have some convergence on this soon -- I would preferably clear this item before I return to work in a few days.

Other follow up items to take care of:

1. Finalize strings and then update the other locales.

Can we do this outside this PR? @bjohas or you or anyone else could work on this, it doesn't really involve any work that I need to do.

2. Add the corresponding keyboard shortcuts.

I'd also prefer to keep that outside this PR, but I'm open to do it if it is clear what keys to attach and how to technically attach them

3. Handle the CopyItems `const` in some way. Right now, when Zutilo is uninstalled and reinstalled, there is an error about redefining the `const` and loading fails.

I've fixed that in the latest push (I think).

[retorquere]

https://0x0.st/zf_D.xpi

[bjohas]

Many thanks!

@willsALMANJ - I've started writing some notes here: https://docs.google.com/document/d/1O4wZhnIGC5TPWx2JFDpqihHTl6CJ8xFjHmhW4F0MHnA/edit - not quite complete yet, but have added two Copy+Paste-Clear examples. See what you think!

[bjohas]

Have complete those notes now.

[retorquere]

Any changes required in the implementation?

[bjohas]

No - not at all.

However, I have tested this:

One oddity that I noticed -- when I pasted with overwrite or clear, the selected target item was updated visually. When I pasted with merge, the selected target item was not updated. I had to select another item and then select the target item again to see the new entries show up.

I don't honestly know what could cause that. One function handles both, only different thing is what fields are affected. The save is literally exactly the same.

For me, the paste+merge works, but it takes say 3-5 seconds before the item visually updates. FOr the other two pastes, it's instantaneous.

[retorquere]

I have no explanation for that as it literally is the same function, with just different selectivity on which fields it will update.

https://0x0.st/zfJK.xpi adds more logging -- if you also install the latest edtechhub, restart with debugging enable from the help menu, replicate the problem, then choose Send EdTech hub debug log from the help menu and paste the ID you get in the popup here. That will give me the debug log, although I don't see how it could tell me anything.

[retorquere]

I'm getting "this is undefined on 1126" in zoteroOverlay.js when I click items in the Zutilo prefs. Is the refreshZoteroItemPopup called somewhere as a function rather than as a method?

[wshanks]

@retorquere Yes, refreshZoteroItemPopup is called as function from an observer in zutilo.jsm that watches for changes to preference values: https://github.com/willsALMANJ/Zutilo/blob/master/addon/chrome/content/zutilo/zutilo.jsm#L179

[wshanks]

@bjohas The use cases seem fine to me. As I don't often need these features, I just wanted to push back because I don't want more complexity than necessary (for me, just paste-clear and editing json by hand would be enough).

There was one thing I didn't quite follow in your write up was the creator merging feature. Why is merging the creators the best choice here? I can see users thinking that just empty fields will get populated and getting confused when they notice the extra authors.

[wshanks]

@retorquere I don't know if I understood your comment about clearing this item. Is there a benefit to you to having this merged soon or if we say that the code is fine is it okay for @bjohas and myself to finish adding the docs/strings before merging?

[retorquere]

@retorquere Yes, refreshZoteroItemPopup is called as function from an observer in zutilo.jsm that watches for changes to preference values: https://github.com/willsALMANJ/Zutilo/blob/master/addon/chrome/content/zutilo/zutilo.jsm#L179

Got it, fixed that.

@retorquere I don't know if I understood your comment about clearing this item. Is there a benefit to you to having this merged soon or if we say that the code is fine is it okay for @bjohas and myself to finish adding the docs/strings before merging?

I don't know whether you or @bjohas can add things onto this PR (doesn't it come from my repo?) but it seems preferable to me that if you or @bjohas want to make changes that that doesn't need to go through me.

The other benefit is just that I prefer to not have things sitting too long with my name on it, or I'd be spreading myself too thing across too many tasks.

[retorquere]

Also, for #91, I'd want to build on the CheckVisibility infrastructure to make sure that the menu item only shows for true collections, not faux-collections like search, duplicates, etc (which do not have select links or URLs into the online library)

[bjohas]

@willsALMANJ - have added a few more things about authors to the notes: https://docs.google.com/document/d/1O4wZhnIGC5TPWx2JFDpqihHTl6CJ8xFjHmhW4F0MHnA/edit. When the PR has been done, I'll do another PR to amend the documentation accordingly.

@retorquere, if @willsALMANJ agrees, could we use

• Fields: Copy
• Fields: Paste-Keep
• Fields: Paste-Over
• Fields: Paste-Clear as labels? I think these are closest and good wording. If they need changing later, @willsALMANJ and I can do that.

[retorquere]

I can easily change those, I have no strong opinions on how it's worded int the UI.

[wshanks]

The other Zutilo items are phrased as commands. How about:

• Copy item fields
• Paste into empty item fields
• Paste non-empty item fields
• Paste all item fields

I think we have no other outstanding issues in the PR? We still have to do the docs and locales before making a release with these features, but it seems like we have the doc text in @bjohas's Google doc and I can propagate the strings to the other locales.

[bjohas]

That sounds good to me!

Let me do separate PR for the documentation.

[retorquere]

Those strings have now been committed.

For translations I can heartily recommend Crowdin. They deal with propagation and will also suggest translations for new items. Works really well for my own plugin.

[retorquere]

https://0x0.st/zfI7.xpi

[bjohas]

@willsALMANJ - I've now updated the documentation, see PR here: https://github.com/willsALMANJ/Zutilo/pull/102

Do you think we're set for accepting the PRs? Or is there something else we should do?

We'll look at https://github.com/willsALMANJ/Zutilo/issues/91 next, and it would be good to have this one accepted first :)

[retorquere]

It's currently just the one. I can start on the second when this is merged, as I intend to build on this.

[retorquere]

Oh wait, you mean the PR for the docs, sorry.

[wshanks]

I think we are all set with the PR's. I will integrate them in when I have a little more time than I have right now.

[wshanks]

Sorry, I am still seeing a problem when I try to reload the extension. With Zutilo from this PR installed, if I go to the addons manager and choose to install the xpi again, I see SyntaxError: redeclaration of let CopyItems in the error console and the extension does not finish loading. Researching a little bit, I found that declaring a class with class creates a let variable and let variables can not be deleted from their scope.

Do you know if there is a way to declare the classes in a scope that Zutilo can unload? Or could the classes just be turned into object properties inside of ZutiloChrome?

[retorquere]

So you just install the XPI again from the gear menu?

[retorquere]

I don't have let CopyItems anywhere in the sourcecode though. Does it say what line number? The classes should not be the problem, and it looks like it's specifically complaining about a let statement somewhere.

[retorquere]

Ah hold on sorry

[retorquere]

Humm... it's possible to move it into the Zutilo scope. Bit gnarly though.

[retorquere]

I haven't been able to replicate the problem. What exactly do you do to trigger this?

[retorquere]

I'd prefer it if I could replicate it, but the new change I just pushed should get rid of it.

[retorquere]

Sorry about all the noise but I haven't worked with restartless extensions before. Classic extensions don't have this behavior.

[wshanks]

Sorry for the delay. I reviewed this latest version and it looks good to me. I will merge it and #101 this weekend. I will see if I can get to the other PR's as well. I haven't had a chance to look at them too closely yet.