Zotero 7-only plugin, submitted for review #59
Conversation
|
My bad, the PR was a bit hasty - I have messed up something while preparing it. I'll report again when it's fixed. |
|
False alarm, the submitted code is actually OK (as far as I can tell). Feedback is not only welcome, but needed 😅 |
There was a problem hiding this comment.
I suggest to keep chrome, chrome.manifest and install.rdf (revert commit ee11b26), but move them to the src subdirectory.
In addition either src/chrome/skin/default/zoteroocr/ubmannheim*png are required or the path for the icons has to be updated to avoid the duplicate png files. By the way, why do we have images 32 x 32 and 64 x 64, not 48 x 48 and 96 x 96 as in other add-ons?
Then the new plugin should work with Zotero 6 and with Zotero 7 (and we don't have to maintain a separate branch).
There was a problem hiding this comment.
I think that src/updates.json should be renamed to updates.json. It replaces update.rdf and is not part of the add-on. To support both Zotero 6 and Zotero 7, it needs some lines added (see https://www.zotero.org/support/dev/zotero_7_for_developers).
|
Thanks for the review! I'll take a closer look in a few days. |
|
Really looking forward to giving this a spin!! |
| @@ -1,7 +0,0 @@ | |||
| content zoteroocr chrome/content/ | |||
There was a problem hiding this comment.
Please don't remove this file. It is required for a plugin which supports both Zotero 6 and Zotero 7.
There was a problem hiding this comment.
As discussed in the issue #52 (comment) , I am not interested in supporting Zotero 6 and I prefer to submit a Z7-only version on this side branch.
| @@ -1,32 +0,0 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
Please don't remove this file. It is required for a plugin which supports both Zotero 6 and Zotero 7.
| @@ -1,61 +0,0 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
Please don't remove this file. It is required for a plugin which supports both Zotero 6 and Zotero 7.
| @@ -1,234 +0,0 @@ | |||
| // zoteroocr.js | |||
There was a problem hiding this comment.
Please don't remove this file. It is required for a plugin which supports both Zotero 6 and Zotero 7.
| @@ -1,2 +0,0 @@ | |||
| <!ENTITY ocr.menu.label "OCR selected PDF(s)"> | |||
There was a problem hiding this comment.
Please don't remove this file. It is required for a plugin which supports both Zotero 6 and Zotero 7.
| @@ -1,34 +0,0 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
Please don't remove this file. It is required for a plugin which supports both Zotero 6 and Zotero 7.
| @@ -1,30 +0,0 @@ | |||
| <?xml version="1.0" encoding="utf-8" ?> | |||
There was a problem hiding this comment.
Please don't remove this file. It is required for a smooth update from the current Zotero 6 plugin to a new one which supports both Zotero 6 and Zotero 7.
|
@aborel, would it be okay if I squash your commits with all suggested changes (perhaps with a subject line "Add support for Zotero 7"), add updates to support Zotero 6 and 7 simultaneously and then create a new pull request for that? |
pref pane working in Z7 pref pane working in Z7 pref pane working in Z7
remove stray z6 files .DS_Store removal
Yes, no problem! |
Thanks. See pull request #63 for the result. |
|
Meanwhile the changes from this pull request were committed to the master branch in commit 2f8e4ca, so they are part of the new release 0.7.0 which now supports Zotero 7, too. Thanks a lot for this great contribution! |
|
Superseded by #63 |
This PR is in response to issue #52
Functionality is essentially the same as for the master branch, but supporting Zotero 7 instead of Zotero 6 and older.
I included a minor change that might also address #46