Skip to content
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

Collection links (#91) #103

Merged
merged 1 commit into from
Aug 24, 2019
Merged

Collection links (#91) #103

merged 1 commit into from
Aug 24, 2019

Conversation

retorquere
Copy link
Contributor

@retorquere retorquere commented Aug 4, 2019

This implements #91. It builds on #102 -- if so desired, I can rebase onto #102 after that has been merged.

@retorquere
Copy link
Contributor Author

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

@retorquere retorquere mentioned this pull request Aug 5, 2019
@bjohas
Copy link
Contributor

bjohas commented Aug 5, 2019

Thank you - looks great!

A couple of requests:

@bjohas
Copy link
Contributor

bjohas commented Aug 5, 2019

This is not directly related, but in case it's of interest: https://forums.zotero.org/discussion/78550/getting-zotero-to-work-under-ubuntu-linux

@retorquere
Copy link
Contributor Author

It isn't related, but if you installed from my debs, you can file an issue there. If you installed from the tarball, they don't work out of the box.

@retorquere
Copy link
Contributor Author

From that forum post, I don't understand how this would be an old url? What's the difference between the two? It wouldn't be automatically fixed, no.

@bjohas
Copy link
Contributor

bjohas commented Aug 5, 2019

Ah yes - sorry, not very clear at all. I meant that in zotero://select/groups/5/collections/3ESW8QB9, the '5' refers to a property of my zotero client (being the 5th library perhaps). This was the case with some outdated zotero:// links as well (I think).

Let me start again: For example, in one of my libraries, I have these URIs:

I was expecting zotero://select/groups/2339240/collections/3ESW8QB9 for the 2nd. This is analogous to

  • https://www.zotero.org/groups/2339240/items/L6HPNDMZ
  • zotero://select/items/5_L6HPNDMZ vs. zotero://select/groups/2339240/items/L6HPNDMZ (see dstillman's comment in the above forum link - not sure where I got the 5568133 link from, and sorry for that confusing matters).

@retorquere
Copy link
Contributor Author

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

@retorquere
Copy link
Contributor Author

* Could it also generate the “[https://zotero.org/…”](https://zotero.org/%E2%80%A6%E2%80%9D) links from collections ("Copy Zotero URI")?

Also as in put them on the clipboard together? Or as in add a new menu option?

* I believe the format e.g. zotero://select/groups/5/collections/JBQWA3FR is an 'old' link, c.f. here: #97 (and https://forums.zotero.org/discussion/78312/zotero-uri-vs-select-item). It might be that once that issue makes it into the release, it will be automatically fixed here. Or maybe not? Would be good to check!

I think that's fixed in zOSl.xpi

@retorquere
Copy link
Contributor Author

Oh I see what you mean with the URI. https://0x0.st/zOSR.xpi

@bjohas
Copy link
Contributor

bjohas commented Aug 5, 2019

Perfect - that's fixed it! And I installed from your repo, so the zotero:// links work too!

Will you be able to add https://www.zotero.org/groups/2339240/edtechhub/items/collectionKey/3ESW8QB9 - style links as well? (No rush - just checking it's on the radar)

@retorquere
Copy link
Contributor Author

Doesn't zOSR already add that?

@bjohas
Copy link
Contributor

bjohas commented Aug 5, 2019

AH YES! It does - as long as I remember to turn it on the in the preferences!!

THANK YOU!!!!

@retorquere
Copy link
Contributor Author

The PR has been rebased

@retorquere
Copy link
Contributor Author

Shouldn't make any difference, but to make sure: https://0x0.st/zVTC.xpi

@bjohas
Copy link
Contributor

bjohas commented Aug 22, 2019

@willsALMANJ - just a tiny friendly and most courteous prod on this PR :)
Thanks!!

@bjohas
Copy link
Contributor

bjohas commented Aug 22, 2019

@retorquere I think the collection links may be wrong.

When I copy the URI to a collection
https://www.zotero.org/groups/2292090/activating_edtech_1_resources/collections/XGMUY62W?
However, that link just goes to the main library.

When I actually click on the lhe link, I get
https://www.zotero.org/groups/2292090/activating_edtech_1_resources/items/collectionKey/XGMUY62W
which shows the items. From then on, when I hover, the correct links come up. It may be a slight inconsistency in the web library system (which is changing anyway).

I realise that this is my mistake, as I suggested the wrong link - sorry. Can you adjust the code to use the alternative link instead?

@retorquere
Copy link
Contributor Author

That's pretty strange because I just request the collection URI from Zotero using Zotero.URI.getCollectionURI

@wshanks
Copy link
Owner

wshanks commented Aug 23, 2019

@bjohas thanks -- I haven't forgotten. I am trying to get to this as soon as I can.

@wshanks
Copy link
Owner

wshanks commented Aug 23, 2019

I reviewed this and it looked good to me (thanks for incidentally improving the code as you add things to it).

One thing I noticed is that I get this message in the error console when clicking on an item in the collection context menu:

Timestamp: 08/23/2019 12:31:23 AM
Error: TypeError: o is undefined
Source File: chrome://zotero/content/zoteroPane.js
Line: 2372

referencing this line. It seems that Zotero is trying to capture the click event and handle it in JS instead of using the built-in XUL system for tying commands to menu items. The copy functions still work despite the error. I am not sure there is an easy to prevent Zotero from trying to handle the click, so we might just have to leave it the way it is. What do you think?

Besides that, we just need to resolve the question about the URI format, and I can copy the strings to the other locales and merge. I haven't read the clean up PR yet, but I don't anticipate that being a problem to merge after I get a chance to read it. We should add something to the documentation for these functions as well (can be done separately from this PR).

@retorquere
Copy link
Contributor Author

I've asked about Zotero.URI.getCollectionURI on the Zotero dev forums. When I get more info, I'll amend the copy action.

It's going to be hard to get rid of the o is undefined error, as that function searches a private array. I could monkey-patch onCollectionContextMenuSelect to only pass on events when they don't match IDs known to Zutilo, but monkey-patching has it's own risks, and since onCollectionContextMenuSelect is only called here, the error disappears harmlessly into the void.

@retorquere
Copy link
Contributor Author

@retorquere
Copy link
Contributor Author

So it looks like it's something that's going to be fixed at their end.

@retorquere
Copy link
Contributor Author

https://0x0.st/zW3g.xpi disables the "copy link" option if the user isn't synced.

The cleanup PR can wait until we've settled this one. I'll rebase to master after this is merged and then re-run the linter.

@wshanks
Copy link
Owner

wshanks commented Aug 23, 2019

Okay, so then we are good to merge this other than the benign error. I posted a comment in-line about a way to avoid the error. I am curious what you think.

@retorquere
Copy link
Contributor Author

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

@wshanks wshanks merged commit 5722b95 into wshanks:master Aug 24, 2019
@retorquere retorquere deleted the collection-links branch August 24, 2019 07:12
@bjohas
Copy link
Contributor

bjohas commented Aug 24, 2019

Thank you both, that's amazing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants