feat: support open reference at semantic scholar #13278
Conversation
brandon-lau0
changed the title
|
Your pull request needs to link an issue correctly. To ease organizational workflows, please link this pull-request to the issue with syntax as described in https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue:
Examples
|
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
| @@ -37,6 +37,9 @@ public enum StandardActions implements Action { | |||
| EXTRACT_FILE_REFERENCES_OFFLINE(Localization.lang("Extract references from file (offline)"), IconTheme.JabRefIcons.FILE_STAR), | |||
| OPEN_URL(Localization.lang("Open URL or DOI"), IconTheme.JabRefIcons.WWW, KeyBinding.OPEN_URL_OR_DOI), | |||
| SEARCH_SHORTSCIENCE(Localization.lang("Search ShortScience")), | |||
| SEARCH_GOOGLE_SCHOLAR(Localization.lang("Search Google Scholar")), | |||
There was a problem hiding this comment.
The new string 'Search Google Scholar' should be consistent with existing strings and grouped semantically with similar search actions.
| @@ -37,6 +37,9 @@ public enum StandardActions implements Action { | |||
| EXTRACT_FILE_REFERENCES_OFFLINE(Localization.lang("Extract references from file (offline)"), IconTheme.JabRefIcons.FILE_STAR), | |||
| OPEN_URL(Localization.lang("Open URL or DOI"), IconTheme.JabRefIcons.WWW, KeyBinding.OPEN_URL_OR_DOI), | |||
| SEARCH_SHORTSCIENCE(Localization.lang("Search ShortScience")), | |||
| SEARCH_GOOGLE_SCHOLAR(Localization.lang("Search Google Scholar")), | |||
| SEARCH_SEMANTIC_SCHOLAR(Localization.lang("Search Semantic Scholar")), | |||
There was a problem hiding this comment.
The new string 'Search Semantic Scholar' should be consistent with existing strings and grouped semantically with similar search actions.
| @@ -37,6 +37,9 @@ public enum StandardActions implements Action { | |||
| EXTRACT_FILE_REFERENCES_OFFLINE(Localization.lang("Extract references from file (offline)"), IconTheme.JabRefIcons.FILE_STAR), | |||
| OPEN_URL(Localization.lang("Open URL or DOI"), IconTheme.JabRefIcons.WWW, KeyBinding.OPEN_URL_OR_DOI), | |||
| SEARCH_SHORTSCIENCE(Localization.lang("Search ShortScience")), | |||
| SEARCH_GOOGLE_SCHOLAR(Localization.lang("Search Google Scholar")), | |||
| SEARCH_SEMANTIC_SCHOLAR(Localization.lang("Search Semantic Scholar")), | |||
| SEARCH(Localization.lang("Search...")), | |||
There was a problem hiding this comment.
The new string 'Search...' should be consistent with existing strings and grouped semantically with similar search actions.
| @@ -36,6 +36,7 @@ | |||
| import org.jabref.gui.specialfields.SpecialFieldMenuItemFactory; | |||
| import org.jabref.logic.citationstyle.CitationStyleOutputFormat; | |||
| import org.jabref.logic.citationstyle.CitationStylePreviewLayout; | |||
| import org.jabref.logic.importer.ImporterPreferences; | |||
There was a problem hiding this comment.
The ImporterPreferences import is added but not used in the patch. Unused imports should be removed to maintain clean code.
| final List<BibEntry> bibEntries = stateManager.getSelectedEntries(); | ||
|
|
||
| if (bibEntries.size() != 1) { | ||
| dialogService.notify(Localization.lang("This operation requires exactly one item to be selected.")); |
There was a problem hiding this comment.
The notification message should be in sentence case, not title case, to maintain consistency with other user interface texts.
| try { | ||
| NativeDesktop.openExternalViewer(databaseContext, preferences, url, StandardField.URL, dialogService, bibEntries.getFirst()); | ||
| } catch (IOException ex) { |
There was a problem hiding this comment.
The try block covers too many statements. It should be minimized to cover only the statement that might throw the IOException.
| final List<BibEntry> bibEntries = stateManager.getSelectedEntries(); | ||
|
|
||
| if (bibEntries.size() != 1) { | ||
| dialogService.notify(Localization.lang("This operation requires exactly one item to be selected.")); |
There was a problem hiding this comment.
The notification message should be in sentence case, not title case, to maintain consistency with other user interface texts.
| dialogService.notify(Localization.lang("This operation requires exactly one item to be selected.")); | ||
| return; | ||
| } | ||
| externalLinkCreator.getGoogleScholarSearchURL(bibEntries.getFirst()).ifPresent(url -> { |
There was a problem hiding this comment.
The method getGoogleScholarSearchURL should not return null. It should return an Optional to avoid null checks and improve code safety.
| try { | ||
| NativeDesktop.openExternalViewer(databaseContext, preferences, url, StandardField.URL, dialogService, bibEntries.getFirst()); | ||
| } catch (IOException ex) { |
There was a problem hiding this comment.
The try block covers too many statements. It should be minimized to only cover the statement that might throw the IOException.
|
JUnit tests of You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide. |
| @@ -41,7 +44,7 @@ public void execute() { | |||
| dialogService.notify(Localization.lang("This operation requires exactly one item to be selected.")); | |||
| return; | |||
| } | |||
| ExternalLinkCreator.getShortScienceSearchURL(bibEntries.getFirst()).ifPresent(url -> { | |||
| externalLinkCreator.getShortScienceSearchURL(bibEntries.getFirst()).ifPresent(url -> { | |||
There was a problem hiding this comment.
The try block covers too many statements. It should only cover the statement that might throw an IOException to improve readability and maintainability.
| final List<BibEntry> bibEntries = stateManager.getSelectedEntries(); | ||
|
|
||
| if (bibEntries.size() != 1) { | ||
| dialogService.notify(Localization.lang("This operation requires exactly one item to be selected.")); |
There was a problem hiding this comment.
The notification message should be in sentence case, not title case, to maintain consistency with other user interface texts.
| @@ -41,7 +44,7 @@ public void execute() { | |||
| dialogService.notify(Localization.lang("This operation requires exactly one item to be selected.")); | |||
| return; | |||
| } | |||
| ExternalLinkCreator.getShortScienceSearchURL(bibEntries.getFirst()).ifPresent(url -> { | |||
| externalLinkCreator.getShortScienceSearchURL(bibEntries.getFirst()).ifPresent(url -> { | |||
There was a problem hiding this comment.
The try block covers too many statements, which can make it harder to identify the exact source of an exception. It should cover as few statements as possible.
| @@ -196,6 +216,14 @@ public void storeSettings() { | |||
| if (apikeyPersistAvailableProperty.get()) { | |||
| preferences.getImporterPreferences().getApiKeys().addAll(apiKeys); | |||
| } | |||
|
|
|||
| // Save custom URL templates to preferences | |||
| Map<String, String> templates = searchEngines.stream() | |||
There was a problem hiding this comment.
Using Collectors.toMap without handling duplicate keys can lead to runtime exceptions. Ensure that duplicate keys are handled appropriately.
| author.ifPresent(a -> uriBuilder.addParameter("author", a)); | ||
| return uriBuilder.toString(); | ||
| } catch (URISyntaxException ex) { | ||
| throw new AssertionError("ShortScience URL is invalid.", ex); |
There was a problem hiding this comment.
Using exceptions for control flow is not recommended. Instead, handle the exception in a way that does not rely on throwing an AssertionError for normal operations.
| author.ifPresent(a -> uriBuilder.addParameter("author", a)); | ||
| return uriBuilder.toString(); | ||
| } catch (URISyntaxException ex) { | ||
| throw new AssertionError("Default Google Scholar URL is invalid.", ex); |
There was a problem hiding this comment.
Using exceptions for control flow is not recommended. Instead, handle the exception in a way that does not rely on throwing an AssertionError for normal operations.
| author.ifPresent(a -> uriBuilder.addParameter("author", a)); | ||
| return uriBuilder.toString(); | ||
| } catch (URISyntaxException ex) { | ||
| throw new AssertionError("Default Semantic Scholar URL is invalid.", ex); |
There was a problem hiding this comment.
Using exceptions for control flow is not recommended. Instead, handle the exception in a way that does not rely on throwing an AssertionError for normal operations.
| author.ifPresent(a -> uriBuilder.addParameter("author", a)); | ||
| return uriBuilder.toString(); | ||
| } catch (URISyntaxException ex) { | ||
| throw new AssertionError("ShortScience URL is invalid.", ex); |
There was a problem hiding this comment.
Using exceptions for control flow is not recommended. Exceptions should be reserved for truly exceptional conditions, not for normal control flow.
| author.ifPresent(a -> uriBuilder.addParameter("author", a)); | ||
| return uriBuilder.toString(); | ||
| } catch (URISyntaxException ex) { | ||
| throw new AssertionError("Default Google Scholar URL is invalid.", ex); |
There was a problem hiding this comment.
Using exceptions for control flow is not recommended. Exceptions should be reserved for truly exceptional conditions, not for normal control flow.
| author.ifPresent(a -> uriBuilder.addParameter("author", a)); | ||
| return uriBuilder.toString(); | ||
| } catch (URISyntaxException ex) { | ||
| throw new AssertionError("Default Semantic Scholar URL is invalid.", ex); |
There was a problem hiding this comment.
Using exceptions for control flow is not recommended. Exceptions should be reserved for truly exceptional conditions, not for normal control flow.
| @@ -1,9 +1,12 @@ | |||
| package org.jabref.logic.util; | |||
|
|
|||
| import java.net.MalformedURLException; | |||
| import java.util.Collections; | |||
There was a problem hiding this comment.
The import statement for Collections is unnecessary as the code should use List.of() for creating empty lists instead of Collections.emptyList().
| @@ -1,9 +1,12 @@ | |||
| package org.jabref.logic.util; | |||
|
|
|||
| import java.net.MalformedURLException; | |||
| import java.util.Collections; | |||
| import java.util.List; | |||
There was a problem hiding this comment.
The import statement for List is unnecessary as the code should use List.of() for creating empty lists instead of Collections.emptyList().
|
Follow up at #13153 @brandon-lau0 please, just push to your branch and do not create a new PR. New PRs cost time on our side and reduce time to give feedback. |
This PR adds new "Search Semantic Scholar" option to the list of available search engines to allow users to directly link to within Jabref.
The URL is also configurable within the "Search Engine URL Templates" section in the "Web search" tab in Preferences. For example, we add the
{author}parameter to search via title and author.Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)