-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Changes from 4 commits
df7a657
1f807b7
8616f77
79a71fd
3046361
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||||||
|
@@ -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; | ||||||
|
||||||
|
@@ -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; | ||||||
|
@@ -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(), | ||||||
|
@@ -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; | ||||||
} | ||||||
|
||||||
Optional<DOI> doi = DOI.findInText(queryText); | ||||||
if (doi.isPresent()) { | ||||||
// in case the query contains a DOI, it is treated as valid | ||||||
return null; | ||||||
} | ||||||
|
||||||
try { | ||||||
parser.parse(queryText, NO_EXPLICIT_FIELD); | ||||||
return null; | ||||||
|
@@ -130,16 +144,26 @@ public void search() { | |||||
} | ||||||
|
||||||
SearchBasedFetcher activeFetcher = getSelectedFetcher(); | ||||||
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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then I would also state that in the code:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))) | ||||||
BackgroundTask<ParserResult> 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); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
dialogService.showCustomDialogAndWait(dialog); | ||||||
} | ||||||
|
||||||
|
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 variable is not ncessary - just using
fetcherName
is enough.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.
Thanks for your feedback!
When I try to use
fetcherName
in theclient.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 finalfinalFetcherName
variable? A possible solution is to make it an instance variable but I am not sure that's a good ideaThere 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.
No, then keep it as is. I already though that it's probably used in a lambda
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.
Got it, thanks!