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

Fixes Web icon also for ISBN #9879

Closed
wants to merge 0 commits into from
Closed

Fixes Web icon also for ISBN #9879

wants to merge 0 commits into from

Conversation

mkumar09
Copy link

@mkumar09 mkumar09 commented May 13, 2023

Fixes #9819

Description : The ISBNs were not having a URL standard field in the BiBEntry fields. So made the changes to add the URL field in the BiBEntry retrieved from the fetcher.

Issue_fix#9819

Compulsory checks

Preview Give feedback

@koppor
Copy link
Member

koppor commented May 14, 2023

Thank you for working on this. Please rethink the solution: JabRef knows the URL based on the precense of an ISBN. The link can be changed in future. Maybe one wants to open books on Amazon or the University Library. Then one does not want to change all bib files. If you opt for Amazon and I for university, the Bib file would change back and forth.

Thus, it is an UI issue. Check the main table how the URL is filled. I think, we have a field property ISBN, which can be used.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

The implemention should work on the view not on the Bib data.

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label May 14, 2023
@calixtus
Copy link
Member

calixtus commented May 14, 2023

You can take a look at BibEntryTableViewModel::createLinkedIdentifiersBinding .
I believe this would be a good starting point for a fix.

@mkumar09
Copy link
Author

mkumar09 commented May 16, 2023

I checked BibEntryTableViewModel::createLinkedIdentifiersBinding.
@koppor @calixtus Does this fix work ?

private static EasyBinding<Map<Field, String>> createLinkedIdentifiersBinding(BibEntry entry) { return EasyBind.combine( entry.getFieldBinding(StandardField.URL), entry.getFieldBinding(StandardField.DOI), entry.getFieldBinding(StandardField.URI), entry.getFieldBinding(StandardField.EPRINT), entry.getFieldBinding(StandardField.ISBN), (url, doi, uri, eprint,isbn) -> { Map<Field, String> identifiers = new HashMap<>(); isbn.ifPresent(value -> identifiers.put(StandardField.URL,"https://openlibrary.org/isbn/"+value)); url.ifPresent(value -> identifiers.put(StandardField.URL, value)); doi.ifPresent(value -> identifiers.put(StandardField.DOI, value)); uri.ifPresent(value -> identifiers.put(StandardField.URI, value)); eprint.ifPresent(value -> identifiers.put(StandardField.EPRINT, value)); return identifiers; }); }

@koppor
Copy link
Member

koppor commented May 16, 2023

I can't see a diff. Would be good to do a commit. Then, one can also try out locally.

@koppor
Copy link
Member

koppor commented May 16, 2023

@mkumar09 Did you try your solution?

Two things to check:

A) does the web column display the icon if the ISBN is present?

image

B) Does a click on that icon open the page?


I think, your code snippet misses the first requirement.

@mkumar09
Copy link
Author

mkumar09 commented May 19, 2023

@mkumar09 Did you try your solution?

Two things to check:

A) does the web column display the icon if the ISBN is present?

image

B) Does a click on that icon open the page?

I think, your code snippet misses the first requirement.

Hey @koppor. I checked the solution with this code snippet. It fulfills the first requirement,i.e, to show the link icon but for some reason, clicking on it doesn't open the page( on hovering over the link icon, it shows the correct URL but clicking doesn't work somehow)

@Siedlerchr
Copy link
Member

Best is to push your code so we can have a chance to see all changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web icon also for ISBN
4 participants