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

Add option to open arks in the browser from an ark identifier #9601

Merged
merged 27 commits into from
Mar 12, 2023

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Feb 6, 2023

Closes #6696
Also refactored the code a bit to allow for adding support for more identifiers in the eprint field in the future .e.g. googlebooks, jstor, PubMed, hdl, etc.

TODO

  • Depending on eprinttype to tell identifier type should be a fallback strategy
  • Validate ARK identifiers
  • Refactor tests
bandicam.2023-02-04.23-01-27-587.mp4
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@wujastyk
Copy link

wujastyk commented Feb 6, 2023

Yes! Works well. I fetched the deb file just after the build completed. Clicking the "fetch" icon next to the ARK eprint entry worked perfectly. Thank you!
It would also be great if the link icon in the "linked identifiers" field of the main display could be active (as it is for DOI and URL).
This is already a huge leap forward for the kind of work I do. I use ARKs all the time. Thank you!

jabref-ark.mp4

@HoussemNasri
Copy link
Member Author

Nice catch, looks like JabRef expected an arXiv id but got an ark one, so it generated an empty URL. In my windows machine, clicking the button opened the file manager, but I guess the behavior differs between OSs.

@wujastyk
Copy link

Is something needed to push this forward and merge the ARK features into the latest development snapshot?

@HoussemNasri
Copy link
Member Author

Just need to refractor the unit tests and implement arks validation logic. Hopefully I can spare some time to do this in the weekend.

@HoussemNasri
Copy link
Member Author

The PR is ready for review! However, there are two requirements left (ARK validation and inferring the eprint type from the eprint field .e.g. when eprint = ark:1234/5554, we know it is an ark). I'll address the missing requirements in another PR. Nevertheless, this feature is still usable without them.

@HoussemNasri HoussemNasri added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 19, 2023
Siedlerchr
Siedlerchr previously approved these changes Feb 19, 2023
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my side looks good so far, I am only unsure about the listener stuff in the bindToEntry
guess this is where carl can better judge it

}

@Override
public void bindToEntry(BibEntry entry) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really unsure about the bindToEntry method. It is called every time the tab in the entry editor the fieldeditor sits inside recieves focus (EntryEditorTab::notifyAboutFocus).
Maybe EasyBind.listen in the constructor (subscribe already runs the method on first call, listen only after the first change to the property).

Some hints at AbstractEditorViewModel

The entry editor is broken and needs to be rewritten. 😭

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bindToEntry is actually called the first time you select the tab. If you navigate away from the tab and back again, bindToEntry wouldn't be called. It would be called however if you select another entry, which makes sense.

About using subscribe instead of listen, it's necessary so that whenever the user switches to another entry, the UI state --which actions are visible on the side of that identifier editor-- is updated immediately to match the newly selected entry.

@calixtus
Copy link
Member

Thanks for checking this, so let's merge it then. As usual high quality code. Thank you.

@calixtus calixtus merged commit 7e757af into main Mar 12, 2023
@calixtus calixtus deleted the ark-resolver branch March 12, 2023 00:47
@wujastyk
Copy link

JabRef 5.10--2023-03-12--7e757af
Linux 5.15.0-67-generic amd64
Java 19.0.2
JavaFX 19+11

Downloaded and tested. It works! I'm thrilled to have this. It will help me many times every day. Thank you everyone for your hard work to make this feature work so smoothly.

Now, I just hope Archive.org survives the assault of the publishing industry! (But ARKs are widely used elsewhere too.)

Siedlerchr added a commit that referenced this pull request Mar 14, 2023
…rg.mariadb.jdbc-mariadb-java-client-3.1.0

* upstream/main: (357 commits)
  Fix syntax
  Add experimental Fetcher for Bibliotheksverbund Bayern with MarcXML parser (#9641)
  Update guidelines-for-setting-up-a-local-workspace.md
  Update guidelines-for-setting-up-a-local-workspace.md
  Bump org.tinylog:slf4j-tinylog from 2.6.0 to 2.6.1 (#9665)
  Bump apple-actions/import-codesign-certs from 1 to 2 (#9662)
  Bump com.puppycrawl.tools:checkstyle from 10.8.0 to 10.8.1 (#9661)
  Bump gittools/actions from 0.9.15 to 0.10.2 (#9663)
  Bump hmarr/auto-approve-action from 3.1.0 to 3.2.0 (#9664)
  Bump io.github.classgraph:classgraph from 4.8.156 to 4.8.157 (#9666)
  Bump org.tinylog:tinylog-api from 2.6.0 to 2.6.1 (#9667)
  Add option to open arks in the browser from an ark identifier (#9601)
  remove "jdk 19 does not work" (#9658)
  Fulltext fetcher for IACR eprints (#9651)
  Observable Preferences S (#9619)
  Issue 9646: Right-click context menu "Attach file from URL" (#9648)
  Improve the INSPIREFetcher in "Update with bibliographic information from the web" (#9645)
  Bump appleboy/ssh-action from 0.1.7 to 0.1.8 (#9653)
  Bump com.fasterxml.jackson.datatype:jackson-datatype-jsr310 (#9656)
  Bump com.puppycrawl.tools:checkstyle from 10.7.0 to 10.8.0 (#9655)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to resolve ARK references, from Archive.org etc. simliar to DOI
4 participants