Skip to content

feat: support open reference at google scholar #13153

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

brandon-lau0
Copy link
Contributor

@brandon-lau0 brandon-lau0 commented May 20, 2025

Summary of Changes

This PR adds a new "Search Google Scholar" feature to JabRef, similar to the existing "Search ShortScience" functionality. The feature allows users to quickly search for a selected entry's title in Google Scholar directly from the main table's context menu. The search engines are moved under the "Search..." tab in the right click menu for better organization.
Screenshot 2025-05-30 at 4 01 57 PM

In addition, we make Google Scholar and Short Science links configurable in the Preferences tab, under a new section named "Search Engine URL Templates". One of the comments mentioned, in the issue, that "the URL should be configurable. Reason: We cannot maintain JabRef for all site URLs; and the list of sites might be oppionated." This allows users to customize their URL templates, such as adding additional query parameters like author or strings.

Screenshot 2025-05-30 at 3 54 02 PM

The following shows the link the user is directed to after clicking "Search Google Scholar".
Screenshot 2025-05-30 at 4 15 12 PM

Closes #12268.

Bullet Pointed List of Changes:

  • Added new search Google Scholar option
  • Moved search engines to a "Search" sub-menu in right click menu
  • Allow "Search Google Scholar" and "Search Short Science" URLs to be configurable in Preferences under the "Search Engine URL Templates" section

Collaborators

@lydia-yan @yoasaaa @brandon-lau0 @FlyJoanne

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • [/] Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (if change is visible to the user)
  • 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.

@koppor
Copy link
Member

koppor commented May 20, 2025

Update following are wrong comments; the feature is about opening using the web browser

Please investigate org.jabref.logic.importer.fetcher.GoogleScholar and read on at https://devdocs.jabref.org/code-howtos/fetchers.html.

Also look at #7075

You can try to work on JabRef#695.

@koppor koppor changed the title feat: support searching via google scholar feat: support open reference at google scholar May 21, 2025
@jabref-machine
Copy link
Collaborator

Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Checkstyle (pull_request)" and click on it.

In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues, commit, and push.

@brandon-lau0 brandon-lau0 marked this pull request as ready for review May 30, 2025 23:48
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.

Small comment on the tests

(I currently don't have much time to dive into this PR, but wanted to give a little pice of work to move this PR forward)

@@ -29,25 +50,34 @@ private boolean urlIsValid(String url) {

@Test
void getShortScienceSearchURLEncodesSpecialCharacters() {
ImporterPreferences stubPreferences = new StubImporterPreferences();
Copy link
Member

Choose a reason for hiding this comment

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

Typically, this is done with mock and when. See other classes. Can you try this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I removed the StubImporterPreferences and replaced it with mock.

Comment on lines +46 to +51
fx:id="catalogEnabledColumn"
text="%Enabled"
Copy link
Member

Choose a reason for hiding this comment

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

this is no off lines 50/51.

Why the additoinal indent / reformat?

It is hard to review code which makes formatting changes.

Copy link
Contributor Author

@brandon-lau0 brandon-lau0 Jun 8, 2025

Choose a reason for hiding this comment

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

I'm not too sure why it shows up as a diff in GitHub. Locally, I didn't make any changes to Lines 50 and 51, but it shows up as an indent formatting change here. I've tried a couple times to resync with the original, but it didn't seem to work. Do you have any ideas for why this may be the case?

Screenshot 2025-06-07 at 6 30 21 PM

@koppor
Copy link
Member

koppor commented Jun 5, 2025

@brandon-lau0 May I ask if you are going to continue to work on this? - There are merge conflicts to resolve and tests made properly. If not, it is allright for us; we can then ask another student to continue.

@brandon-lau0 brandon-lau0 force-pushed the configurable_external_search_features branch from 1b9bacd to 550796e Compare June 8, 2025 00:33
@brandon-lau0 brandon-lau0 force-pushed the configurable_external_search_features branch from 550796e to eade1cf Compare June 8, 2025 01:12
author.ifPresent(a -> uriBuilder.addParameter("author", a));
return uriBuilder.toString();
} catch (URISyntaxException ex) {
throw new AssertionError("ShortScience URL is invalid.", ex);
Copy link

Choose a reason for hiding this comment

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

Using exceptions for control flow is not recommended. Exceptions should be used for exceptional states, not normal control flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would never happen. Note that this URL is hardcoded. It's only used since Java enforced checked exceptions.

author.ifPresent(a -> uriBuilder.addParameter("author", a));
return uriBuilder.toString();
} catch (URISyntaxException ex) {
throw new AssertionError("Default Google Scholar URL is invalid.", ex);
Copy link

Choose a reason for hiding this comment

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

Using exceptions for control flow is not recommended. Exceptions should be used for exceptional states, not normal control flow.

@brandon-lau0
Copy link
Contributor Author

@brandon-lau0 May I ask if you are going to continue to work on this? - There are merge conflicts to resolve and tests made properly. If not, it is allright for us; we can then ask another student to continue.

Thank you for your patience. I just resolved the merge conflicts and the tests to use mocks.

@@ -78,12 +101,12 @@
text="%Catalog"
/>
<TableColumn minWidth="120"
fx:id="customApiKey"
text="Key"/>
fx:id="customApiKey"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar formatting issue as previous comment. Unsure why a diff shows here. I'm not able to see the changes locally.

Screenshot 2025-06-07 at 7 09 54 PM

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, your IDE hides white spaces?

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 7, 2025
Copy link

trag-bot bot commented Jul 7, 2025

@trag-bot didn't find any issues in the code! ✅✨

@jabref-machine
Copy link
Collaborator

JUnit tests of jablib are failing. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Unit tests (pull_request)" and click on it.

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.

Copy link
Contributor

@Yubo-Cao Yubo-Cao left a comment

Choose a reason for hiding this comment

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

Great work! I have tried out your PR, and basically everything works.

@@ -180,10 +181,10 @@ public MainTable(MainTableDataModel model,
// force match category column to be the first sort order, (match_category column is always the first column)
this.getSortOrder().addFirst(getColumns().getFirst());
this.getSortOrder().addListener((ListChangeListener<TableColumn<BibEntryTableViewModel, ?>>) change -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not create white space changes unless necessary.

@@ -576,13 +577,13 @@ public void setCitationMergeMode(boolean citationMerge) {
}

private void updatePlaceholder(VBox placeholderBox) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The same comments go to the rest of the changes in this file. Do not create whitespace changes unless necessary.

stateManager.getActiveDatabase().ifPresent(databaseContext -> {
final List<BibEntry> bibEntries = stateManager.getSelectedEntries();

if (bibEntries.size() != 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The check is unnecessary, since the bibEntries.size() != 1 would not possibly occur in the context of where this action is used. Note that if, more than one entry is selected, the menu button would automatically turn to disabled mode.

<columnResizePolicy>
<TableView
fx:constant="CONSTRAINED_RESIZE_POLICY"/>
</columnResizePolicy>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think they are in the JabRef_en.properties?

author.ifPresent(a -> uriBuilder.addParameter("author", a));
return uriBuilder.toString();
} catch (URISyntaxException ex) {
throw new AssertionError("ShortScience URL is invalid.", ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

This would never happen. Note that this URL is hardcoded. It's only used since Java enforced checked exceptions.

return uriBuilder.toString();

String urlWithTitle = baseUrl.replace("{title}", title);
return author.map(a -> urlWithTitle.replace("{author}", a)).orElse(urlWithTitle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you want to escape the URL for the variable replacement. In the most typical use cases, where the user declares the {title} as a part of the query parameters, it would make more sense to have it URL encoded.

Furthermore, we don't actually check if a URL is invalid here. This led to the issue, where the user could plausibly have the URL template filled in with mailto:{title}@xxxx.com or arbitrary URL, plausibly creating an injection risk. Alternatively, the lack of error message for invalid URL maybe is less ideal. On Linux platform, the xdg-desktop-portal would handle this exception and say something on the line of, "Cannot find application to open XXXX URL", rather than pointing out that the preference is misconfigured.`

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.

"Open in Google Scholar" / "Semantic Scholar"
4 participants