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 ability to use DOI directly in Web Search #9969

Merged
merged 5 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We added the option to disable the automatic linking of files in the entry editor [#5105](https://github.com/JabRef/jabref/issues/5105)
- We added the link icon for ISBNs in linked identifiers column. [#9819](https://github.com/JabRef/jabref/issues/9819)
- We added support for parsing MathML in the Medline importer. [#4273](https://github.com/JabRef/jabref/issues/4273)
- We added the ability to search for a DOI directly from 'Web Search'. [#9674](https://github.com/JabRef/jabref/issues/9674)

### Changed

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package org.jabref.gui.importer.fetcher;

import java.util.Map;
import java.util.Optional;
import java.util.SortedSet;
import java.util.concurrent.Callable;

import javafx.beans.property.ListProperty;
import javafx.beans.property.ObjectProperty;
Expand All @@ -20,8 +22,11 @@
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.importer.SearchBasedFetcher;
import org.jabref.logic.importer.WebFetchers;
import org.jabref.logic.importer.fetcher.DoiFetcher;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.entry.identifier.DOI;
import org.jabref.model.strings.StringUtil;
import org.jabref.model.util.OptionalUtil;
import org.jabref.preferences.PreferencesService;
import org.jabref.preferences.SidePanePreferences;

Expand All @@ -43,6 +48,7 @@ public class WebSearchPaneViewModel {
private final ListProperty<SearchBasedFetcher> fetchers = new SimpleListProperty<>(FXCollections.observableArrayList());
private final StringProperty query = new SimpleStringProperty();
private final DialogService dialogService;
private final PreferencesService preferencesService;
private final StateManager stateManager;

private final Validator searchQueryValidator;
Expand All @@ -51,6 +57,7 @@ public class WebSearchPaneViewModel {
public WebSearchPaneViewModel(PreferencesService preferencesService, DialogService dialogService, StateManager stateManager) {
this.dialogService = dialogService;
this.stateManager = stateManager;
this.preferencesService = preferencesService;

SortedSet<SearchBasedFetcher> allFetchers = WebFetchers.getSearchBasedFetchers(
preferencesService.getImportFormatPreferences(),
Expand All @@ -77,6 +84,13 @@ public WebSearchPaneViewModel(PreferencesService preferencesService, DialogServi
// in case user did not enter something, it is treated as valid (to avoid UI WTFs)
return null;
}

// check if DOI is present
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 the code the next line. Please remove this comment.

Add a comment in the line before "null". It should be similar than line 84 "in case user did not enter something, it is treated as valid (to avoid UI WTFs)"

Optional<DOI> doi = DOI.findInText(queryText);
if (doi.isPresent()) {
return null;
}

try {
parser.parse(queryText, NO_EXPLICIT_FIELD);
return null;
Expand Down Expand Up @@ -130,16 +144,27 @@ public void search() {
}

SearchBasedFetcher activeFetcher = getSelectedFetcher();
BackgroundTask<ParserResult> task;
Copy link
Member

Choose a reason for hiding this comment

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

This variable can be introuced in line 162, can't it? In JabRef, we introduce the variable at the latest point possible - not somewhere inbetween.

Callable<ParserResult> parserResultCallable = () -> new ParserResult(activeFetcher.performSearch(query));
String fetcherName = activeFetcher.getName();

Optional<DOI> doi = DOI.findInText(query);
if (doi.isPresent()) {
DoiFetcher doiFetcher = new DoiFetcher(preferencesService.getImportFormatPreferences());
parserResultCallable = () -> new ParserResult(OptionalUtil.toList(doiFetcher.performSearchById(doi.get().getDOI())));
fetcherName = doiFetcher.getName();
}

String finalFetcherName = fetcherName;
Copy link
Member

Choose a reason for hiding this comment

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

This variable is not ncessary - just using fetcherName is enough.

Copy link
Contributor Author

@aqurilla aqurilla Jun 4, 2023

Choose a reason for hiding this comment

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

Thanks for your feedback!

When I try to use fetcherName in the client.trackEvent() call, an error is raised that local variables referenced from a lambda expression must be final or effectively final. Is there a way to pass the value to the lambda without creating the effectively final finalFetcherName variable? A possible solution is to make it an instance variable but I am not sure that's a good idea

Copy link
Member

Choose a reason for hiding this comment

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

No, then keep it as is. I already though that it's probably used in a lambda

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Then I would also state that in the code:

Suggested change
String finalFetcherName = fetcherName;
final String finalFetcherName = fetcherName;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I've updated the PR with these changes

Globals.getTelemetryClient().ifPresent(client ->
client.trackEvent("search", Map.of("fetcher", activeFetcher.getName()), Map.of()));
client.trackEvent("search", Map.of("fetcher", finalFetcherName), Map.of()));

BackgroundTask<ParserResult> task;
task = BackgroundTask.wrap(() -> new ParserResult(activeFetcher.performSearch(query)))
task = BackgroundTask.wrap(parserResultCallable)
.withInitialMessage(Localization.lang("Processing %0", query));
task.onFailure(dialogService::showErrorDialogAndWait);

ImportEntriesDialog dialog = new ImportEntriesDialog(stateManager.getActiveDatabase().get(), task);
dialog.setTitle(activeFetcher.getName());
dialog.setTitle(fetcherName);
Copy link
Member

Choose a reason for hiding this comment

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

The "new" variable should also be used - otherwise, it is confusing IMHO.

Suggested change
dialog.setTitle(fetcherName);
dialog.setTitle(finalFetcherName);

dialogService.showCustomDialogAndWait(dialog);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,22 @@ void notCorrectQueryValidationStatus() {
viewModel.queryProperty().setValue("Miami AND Beach OR Houston AND Texas");
assertFalse(viewModel.queryValidationStatus().validProperty().not().getValue());
}

@Test
void queryConsistingOfDOIIsValid() {
viewModel.queryProperty().setValue("10.1007/JHEP02(2023)082");
assertTrue(viewModel.queryValidationStatus().validProperty().getValue());
}

@Test
void canExtractDOIFromQueryText() {
viewModel.queryProperty().setValue("this is the DOI: 10.1007/JHEP02(2023)082, other text");
assertTrue(viewModel.queryValidationStatus().validProperty().getValue());
}

@Test
void queryConsistingOfInvalidDOIIsInvalid() {
viewModel.queryProperty().setValue("101.1007/JHEP02(2023)082");
assertFalse(viewModel.queryValidationStatus().validProperty().getValue());
}
}