Add ability to use DOI directly in Web Search#9969
Conversation
koppor
left a comment
There was a problem hiding this comment.
Some small comments, otherwise, it looks good
| return null; | ||
| } | ||
|
|
||
| // check if DOI is present |
There was a problem hiding this comment.
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)"
| fetcherName = doiFetcher.getName(); | ||
| } | ||
|
|
||
| String finalFetcherName = fetcherName; |
There was a problem hiding this comment.
This variable is not ncessary - just using fetcherName is enough.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
No, then keep it as is. I already though that it's probably used in a lambda
| } | ||
|
|
||
| SearchBasedFetcher activeFetcher = getSelectedFetcher(); | ||
| BackgroundTask<ParserResult> task; |
There was a problem hiding this comment.
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.
koppor
left a comment
There was a problem hiding this comment.
Reading the final variable, some suggestions.
|
|
||
| ImportEntriesDialog dialog = new ImportEntriesDialog(stateManager.getActiveDatabase().get(), task); | ||
| dialog.setTitle(activeFetcher.getName()); | ||
| dialog.setTitle(fetcherName); |
There was a problem hiding this comment.
The "new" variable should also be used - otherwise, it is confusing IMHO.
| dialog.setTitle(fetcherName); | |
| dialog.setTitle(finalFetcherName); |
| fetcherName = doiFetcher.getName(); | ||
| } | ||
|
|
||
| String finalFetcherName = fetcherName; |
There was a problem hiding this comment.
Then I would also state that in the code:
| String finalFetcherName = fetcherName; | |
| final String finalFetcherName = fetcherName; |
There was a problem hiding this comment.
Got it, I've updated the PR with these changes
This fixes #9674 by checking whether the query string contains a valid DOI. If yes, the
DOIFetcheris used instead of the selected fetcher. This is also reflected in theImportEntriesDialogtitle.Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)