Preferences for Grobid by btut · Pull Request #8002 · JabRef/jabref

btut · 2021-08-19T10:27:48Z

Implements two new preferences in ImportSettingsPreferences:

grobidEnabled
grobidURL

The preferences are used in the Fetchers and Importers that use Grobid. Creating a GrobidService while grobidEnabled is false will result in an UnsupportedOperationException.

Fixes #8000

[] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
[] Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

btut · 2021-08-19T10:31:48Z

There are two things left to do:

In Add preferences + popup for usage of GROBID #8000 we discussed showing a popup dialogue when a user tries to import using Grobid for the first time. How/where/when should we do that? Do we need to introduce one more preference to store if it is actually the first time?
This also affects the GrobidCitationFetcher that is already released. The problem here (in comparison to the GrobidPdfImporter) is that we do not have any alternatives in place. Shall we deactivate the option to import plaintext altogether if Grobid is disabled?

Siedlerchr · 2021-08-19T10:38:20Z

You can use public boolean showConfirmationDialogWithOptOutAndWait(String title, String content
You need to store the decision in the prefs and obbiously check if they are true before showing the dialog

Example:

              boolean continueImport = dialogService.showConfirmationDialogWithOptOutAndWait(Localization.lang("Duplicates found"),
                            Localization.lang("There are possible duplicates that haven't been resolved. Continue?"),
                            Localization.lang("Continue with import"),
                            Localization.lang("Cancel import"),
                            Localization.lang("Do not ask again"),
                            optOut -> preferences.setShouldWarnAboutDuplicatesForImport(!optOut));

btut · 2021-08-19T12:40:20Z

Looks promising, but I am having trouble deciding when to show the dialogue.

We can show it either when JabRef opens. This is the easiest option to implement but a little intrusive.

The second option is to show it when a user triggers an action that requires Grobid.

There are multiple actions that trigger Grobid:

import plain text
import pdf
(future - in Implement an interface to import PDF metadata from multiple sources (XMP, Grobid, ...) #7929) a button near a linked file
One of the WebFetchers in the EntryEditor

I started implementing the second option, but there is a lot of code to change and it is not pretty IMHO.
I implemented it for two of the points above in e5222ce so you can understand what I mean. The plain-text one does not even work yet - I guess it's because of nested dialogues?

What do you think? Shall I continue implementing it that way or do we want to go the first route, asking the users when JabRef launches? They can immediately opt out and never see the dialogue again.

koppor · 2021-08-19T18:30:08Z

The second option is to show it when a user triggers an action that requires Grobid.

This is the behavior, I would expect.

Later, we can add some welcome screen enabling well-used default preferences (enable all online services, ...). Refs https://github.com/koppor/jabref/issues/96.

I started implementing the second option, but there is a lot of code to change and it is not pretty IMHO.

Yeah. I would still keep the number of questions at the start low.

I implemented it for two of the points above in e5222ce so you can understand what I mean. The plain-text one does not even work yet - I guess it's because of nested dialogues?

OMG - nested dialogues. A work around could be to "cheat" and show the confirmation before the other. However, this is not a very good approach ^^.

calixtus

Some nitpicks for codestyle and readability.
Did not expect to grow introduction of prefs for grobid so large, but seems reasonable.
Use of Globals is not nice, but in the current state of JabRefPreferences necessary, @DominikVoigt and me are diving into this issue these days.

calixtus · 2021-08-21T07:06:02Z

+
+        grobidEnabled.selectedProperty().bindBidirectional(viewModel.grobidEnabledProperty());
+        grobidURL.textProperty().bindBidirectional(viewModel.grobidURLProperty());
+        grobidURL.disableProperty().bind(grobidEnabled.selectedProperty().not());


Could be done in directly in FXML. (disabled=${grobidEnabled.selected}) or similar. Lot's of examples elsewhere...

I don't think that's possible here because you can't do the negation in fxml. It needs to be disabled if grobidEnabled is NOT selected.

calixtus · 2021-08-21T07:14:44Z


+    @Test
+    public void failsWhenGrobidDisabled() {
+        assertThrows(UnsupportedOperationException.class, () -> new GrobidService(new ImportSettingsPreferences(false, false, false, "http://grobid.jabref.org:8070")));


Maybe extract the ImportSettingsPreferences (I hate that name) to a separate variable, following the typical test scheme:

// given ... // when ... // then ...

See https://blog.codecentric.de/en/2017/09/given-when-then-in-junit-tests/ for example

Extracting is easy and I agree it is more readable. For the 'when' clause, since I expect an exception, something like this would be necessary. In my opition expectThrows is easier to read though. Is it ok to violate given-when-then in this case?

This is not a hard law, but a suggestion. Should make reading and understanding tests easier. If in this case it's not, then ok.

calixtus · 2021-08-21T07:32:42Z

Changelog necessary?

btut · 2021-08-21T11:42:43Z

Changelog necessary?

You are right, since this also affects the plaintext-import that was already released, a Changelog entry is necessary.

btut added 3 commits August 19, 2021 11:37

Settings for grobid

31b62a0

Merge branch 'main' of github.com:JabRef/jabref into useGrobidPreference

cdc9fb1

Use settings

fcdb5a4

btut added 2 commits August 19, 2021 14:37

Store opt-out preference

3440b32

Partial implementation of opt-in/out dialogue

e5222ce

Siedlerchr reviewed Aug 19, 2021

View reviewed changes

Comment thread src/main/java/org/jabref/gui/bibtexextractor/ExtractBibtexDialog.java Outdated

Siedlerchr reviewed Aug 19, 2021

View reviewed changes

Comment thread src/main/java/org/jabref/gui/entryeditor/EntryEditor.java Outdated

btut added 3 commits August 20, 2021 10:19

Show dialog before all Grobid actions

ef5444a

Static code checks

ffaeec5

Merge branch 'main' of github.com:JabRef/jabref into useGrobidPreference

4f4f398

btut self-assigned this Aug 20, 2021

btut added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 20, 2021

btut requested review from calixtus and koppor August 20, 2021 12:56

btut added the project: gsoc label Aug 20, 2021

btut marked this pull request as ready for review August 20, 2021 14:04

btut added 2 commits August 20, 2021 16:14

Merge branch 'main' of github.com:JabRef/jabref into useGrobidPreference

70a2e3e

Fixed missing import (introduced by merge)

61b3b5b

Siedlerchr approved these changes Aug 20, 2021

View reviewed changes

calixtus requested changes Aug 21, 2021

View reviewed changes

calixtus mentioned this pull request Aug 21, 2021

Implement an interface to import PDF metadata from multiple sources (XMP, Grobid, ...) #7929

Merged

5 tasks

btut added 3 commits August 21, 2021 13:36

Extract given-clause in test

69af125

Improved readability

f676003

Changelog

1e97104

Changelog update

55a4653

Siedlerchr approved these changes Aug 21, 2021

View reviewed changes

calixtus approved these changes Aug 21, 2021

View reviewed changes

calixtus merged commit 07712ce into JabRef:main Aug 21, 2021

calixtus mentioned this pull request Aug 23, 2021

Add preferences + popup for usage of GROBID #8000

Closed

Uh oh!

Conversation

btut commented Aug 19, 2021 • edited by calixtus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

btut commented Aug 19, 2021

Uh oh!

Siedlerchr commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

btut commented Aug 19, 2021

Uh oh!

Uh oh!

Uh oh!

koppor commented Aug 19, 2021

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

calixtus Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

btut Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

calixtus Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

btut Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

calixtus Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calixtus commented Aug 21, 2021

Uh oh!

btut commented Aug 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

btut commented Aug 19, 2021 •

edited by calixtus

Loading

Siedlerchr commented Aug 19, 2021 •

edited

Loading