-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: main
Are you sure you want to change the base?
feat: support open reference at google scholar #13153
Conversation
|
Update following are wrong comments; the feature is about opening using the web browser
|
jabgui/src/main/java/org/jabref/gui/maintable/SearchGoogleScholarAction.java
Show resolved
Hide resolved
jablib/src/test/java/org/jabref/logic/util/ExternalLinkCreatorTest.java
Outdated
Show resolved
Hide resolved
|
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. |
jabgui/src/main/java/org/jabref/gui/maintable/SearchGoogleScholarAction.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/maintable/SearchShortScienceAction.java
Show resolved
Hide resolved
koppor
left a comment
There was a problem hiding this 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)
|
|
||
| @Test | ||
| void getShortScienceSearchURLEncodesSpecialCharacters() { | ||
| ImporterPreferences stubPreferences = new StubImporterPreferences(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| fx:id="catalogEnabledColumn" | ||
| text="%Enabled" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@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. |
1b9bacd to
550796e
Compare
550796e to
eade1cf
Compare
Thank you for your patience. I just resolved the merge conflicts and the tests to use mocks. |
| <TableColumn minWidth="120" | ||
| fx:id="customApiKey" | ||
| text="Key"/> | ||
| fx:id="customApiKey" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
|
@trag-bot didn't find any issues in the code! ✅✨ |
|
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. |
Yubo-Cao
left a comment
There was a problem hiding this 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.
|
|
||
| // 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 -> { |
There was a problem hiding this comment.
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.
| this.citationMergeMode = citationMerge; | ||
| } | ||
|
|
||
| private void updatePlaceholder(VBox placeholderBox) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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?
| return uriBuilder.toString(); | ||
|
|
||
| String urlWithTitle = baseUrl.replace("{title}", title); | ||
| return author.map(a -> urlWithTitle.replace("{author}", a)).orElse(urlWithTitle); |
There was a problem hiding this comment.
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.`
|
I like the idea of preferences; therefore, this PR should be continued. |
Thank you for your feedback and for supporting the preferences feature! At this time, I won’t be able to continue working on this PR due to other commitments. I hope what I’ve done so far can serve as a useful starting point for further development. I appreciate the opportunity to contribute. Thank you! |
|
Hey @Yubo-Cao , are you maybe interested in finalizing this PR? This is maybe a low hanging fruit. Would be great for the students who have worked on this to see, that this gets into the code. |


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.

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
authoror strings.The following shows the link the user is directed to after clicking "Search Google Scholar".

Closes #12268.
Bullet Pointed List of Changes:
Collaborators
@lydia-yan @yoasaaa @brandon-lau0 @FlyJoanne
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)