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

Fixed Grobid Preference Dialog Logic, Removed Checkbox #12034

Merged
merged 13 commits into from
Oct 27, 2024
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -101,6 +101,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
- We fixed an issue where recently opened files were not displayed in the main menu properly. [#9042](https://github.com/JabRef/jabref/issues/9042)
- We fixed an issue where the DOI lookup would show an error when a DOI was found for an entry. [#11850](https://github.com/JabRef/jabref/issues/11850)
- We fixed an issue where <kbd>Tab</kbd> cannot be used to jump to next field in some single-line fields. [#11785](https://github.com/JabRef/jabref/issues/11785)
- We fixed an issue where the "Do not ask again" checkbox was not working, when asking for permission to use Grobid [koppor#556](https://github.com/koppor/jabref/issues/566).
- We fixed an issue where we display warning message for moving attached open files. [#10121](https://github.com/JabRef/jabref/issues/10121)
- We fixed an issue where it was not possible to select selecting content of other user's comments.[#11106](https://github.com/JabRef/jabref/issues/11106)
- We fixed an issue where web search preferences "Custom API key" table modifications not discarded. [#11925](https://github.com/JabRef/jabref/issues/11925)
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/entryeditor/EntryEditor.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
import org.jabref.gui.entryeditor.fileannotationtab.FulltextSearchResultsTab;
import org.jabref.gui.externalfiles.ExternalFilesEntryLinker;
import org.jabref.gui.help.HelpAction;
import org.jabref.gui.importer.GrobidOptInDialogHelper;
import org.jabref.gui.importer.GrobidUseDialogHelper;
import org.jabref.gui.keyboard.KeyBinding;
import org.jabref.gui.keyboard.KeyBindingRepository;
import org.jabref.gui.menus.ChangeEntryTypeMenu;
Expand Down Expand Up @@ -423,7 +423,7 @@ private void setupToolBar() {
if (fetcher instanceof PdfMergeMetadataImporter.EntryBasedFetcherWrapper) {
// Handle Grobid Opt-In in case of the PdfMergeMetadataImporter
fetcherMenuItem.setOnAction(event -> {
GrobidOptInDialogHelper.showAndWaitIfUserIsUndecided(dialogService, preferences.getGrobidPreferences());
GrobidUseDialogHelper.showAndWaitIfUserIsUndecided(dialogService, preferences.getGrobidPreferences());
PdfMergeMetadataImporter.EntryBasedFetcherWrapper pdfMergeMetadataImporter =
new PdfMergeMetadataImporter.EntryBasedFetcherWrapper(
preferences.getImportFormatPreferences(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
import org.jabref.gui.copyfiles.CopySingleFileAction;
import org.jabref.gui.icon.IconTheme;
import org.jabref.gui.icon.JabRefIconView;
import org.jabref.gui.importer.GrobidOptInDialogHelper;
import org.jabref.gui.importer.GrobidUseDialogHelper;
import org.jabref.gui.keyboard.KeyBinding;
import org.jabref.gui.linkedfile.DeleteFileAction;
import org.jabref.gui.linkedfile.LinkedFileEditDialog;
Expand Down Expand Up @@ -236,7 +236,7 @@ private Node createFileDisplay(LinkedFileViewModel linkedFile) {
parsePdfMetadata.setTooltip(new Tooltip(Localization.lang("Parse Metadata from PDF.")));
parsePdfMetadata.visibleProperty().bind(linkedFile.isOfflinePdfProperty());
parsePdfMetadata.setOnAction(event -> {
GrobidOptInDialogHelper.showAndWaitIfUserIsUndecided(dialogService, preferences.getGrobidPreferences());
GrobidUseDialogHelper.showAndWaitIfUserIsUndecided(dialogService, preferences.getGrobidPreferences());
linkedFile.parsePdfMetadataAndShowMergeDialog();
});
parsePdfMetadata.getStyleClass().setAll("icon-button");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,31 @@
/**
* Metadata extraction from PDFs and plaintext works very well using Grobid, but we do not want to enable it by default
* due to data privacy concerns.
* To make users aware of the feature, we ask each time before querying Grobid, giving the option to opt-out.
* To make users aware of the feature, we ask before querying for the first time Grobid, saving the users' preference
*/
public class GrobidOptInDialogHelper {
public class GrobidUseDialogHelper {

/**
* If Grobid is not enabled but the user has not explicitly opted-out of Grobid, we ask for permission to send data
* to Grobid using a dialog and giving an opt-out option.
* If the user has not explicitly opted-in/out of Grobid, we ask for permission to send data to Grobid by using
* a dialog. The users' preference is saved.
*
* @param dialogService the DialogService to use
* @return if the user enabled Grobid, either in the past or after being asked by the dialog.
*/
public static boolean showAndWaitIfUserIsUndecided(DialogService dialogService, GrobidPreferences preferences) {
if (preferences.isGrobidUseAsked()) {
return preferences.isGrobidEnabled();
}
if (preferences.isGrobidEnabled()) {
preferences.setGrobidUseAsked(true);
return true;
}
if (preferences.isGrobidOptOut()) {
return false;
}
boolean grobidEnabled = dialogService.showConfirmationDialogWithOptOutAndWait(
boolean grobidEnabled = dialogService.showConfirmationDialogAndWait(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The dialog should not be displayed if preferences are enabled without it. Here's an example:

  1. Reset preferences.
  2. Restart.
  3. Open preferences, then go to Web Search and allow Grobid.
  4. Import a file.
  5. The dialog will prompt to enable Grobid, even though it is already enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @LoayGhreeb, thanks for the suggestion. I have made changes such that if a user has manually enabled Grobid, they will not be prompted in any case (even if they manually disable Grobid in the future) as required.

Localization.lang("Remote services"),
Localization.lang("Allow sending PDF files and raw citation strings to a JabRef online service (Grobid) to determine Metadata. This produces better results."),
Localization.lang("Do not ask again"),
optOut -> preferences.setGrobidOptOut(optOut));
Localization.lang("Send to Grobid"),
Localization.lang("Do not send"));
preferences.setGrobidUseAsked(true);
preferences.setGrobidEnabled(grobidEnabled);
return grobidEnabled;
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/importer/ImportCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ private ParserResult doImport(List<Path> files, Importer importFormat) throws IO
if (importer.isEmpty()) {
// Unknown format
UiTaskExecutor.runAndWaitInJavaFXThread(() -> {
if (FileUtil.isPDFFile(filename) && GrobidOptInDialogHelper.showAndWaitIfUserIsUndecided(dialogService, preferences.getGrobidPreferences())) {
if (FileUtil.isPDFFile(filename) && GrobidUseDialogHelper.showAndWaitIfUserIsUndecided(dialogService, preferences.getGrobidPreferences())) {
importFormatReader.reset();
}
dialogService.notify(Localization.lang("Importing file %0 as unknown format", filename.getFileName().toString()));
Expand All @@ -155,7 +155,7 @@ private ParserResult doImport(List<Path> files, Importer importFormat) throws IO
UiTaskExecutor.runAndWaitInJavaFXThread(() -> {
if (((importer.get() instanceof PdfGrobidImporter)
|| (importer.get() instanceof PdfMergeMetadataImporter))
&& GrobidOptInDialogHelper.showAndWaitIfUserIsUndecided(dialogService, preferences.getGrobidPreferences())) {
&& GrobidUseDialogHelper.showAndWaitIfUserIsUndecided(dialogService, preferences.getGrobidPreferences())) {
importFormatReader.reset();
}
dialogService.notify(Localization.lang("Importing in %0 format", importer.get().getName()) + "...");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public void storeSettings() {
filePreferences.setKeepDownloadUrl(shouldkeepDownloadUrl.getValue());
importerPreferences.setDefaultPlainCitationParser(defaultPlainCitationParser.getValue());
grobidPreferences.setGrobidEnabled(grobidEnabledProperty.getValue());
grobidPreferences.setGrobidOptOut(grobidPreferences.isGrobidOptOut());
grobidPreferences.setGrobidUseAsked(grobidPreferences.isGrobidUseAsked());
grobidPreferences.setGrobidURL(grobidURLProperty.getValue());
doiPreferences.setUseCustom(useCustomDOIProperty.get());
doiPreferences.setDefaultBaseURI(useCustomDOINameProperty.getValue().trim());
Expand Down
22 changes: 11 additions & 11 deletions src/main/java/org/jabref/logic/importer/util/GrobidPreferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@

public class GrobidPreferences {
private final BooleanProperty grobidEnabled;
private final BooleanProperty grobidOptOut;
private final BooleanProperty grobidUseAsked;
private final StringProperty grobidURL;

public GrobidPreferences(boolean grobidEnabled,
boolean grobidOptOut,
boolean grobidUseAsked,
String grobidURL) {
this.grobidEnabled = new SimpleBooleanProperty(grobidEnabled);
this.grobidOptOut = new SimpleBooleanProperty(grobidOptOut);
this.grobidUseAsked = new SimpleBooleanProperty(grobidUseAsked);
this.grobidURL = new SimpleStringProperty(grobidURL);
}

Expand All @@ -30,19 +30,19 @@ public void setGrobidEnabled(boolean grobidEnabled) {
this.grobidEnabled.set(grobidEnabled);
}

// region: optout; models "Do not ask again" option
public boolean isGrobidOptOut() {
return grobidOptOut.get();
// region: GrobidUseAsked;
public boolean isGrobidUseAsked() {
return grobidUseAsked.get();
}

public BooleanProperty grobidOptOutProperty() {
return grobidOptOut;
public BooleanProperty grobidUseAskedProperty() {
return grobidUseAsked;
}

public void setGrobidOptOut(boolean grobidOptOut) {
this.grobidOptOut.set(grobidOptOut);
public void setGrobidUseAsked(boolean grobidUseAsked) {
this.grobidUseAsked.set(grobidUseAsked);
}
// endregion: optout
// endregion: GrobidUseAsked

public String getGrobidURL() {
return grobidURL.get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ public class JabRefCliPreferences implements CliPreferences {
public static final String IMPORTERS_ENABLED = "importersEnabled";
public static final String GENERATE_KEY_ON_IMPORT = "generateKeyOnImport";
public static final String GROBID_ENABLED = "grobidEnabled";
public static final String GROBID_OPT_OUT = "grobidOptOut";
public static final String GROBID_PREFERENCE = "grobidPreference";
public static final String GROBID_URL = "grobidURL";

public static final String DEFAULT_CITATION_KEY_PATTERN = "defaultBibtexKeyPattern";
Expand Down Expand Up @@ -455,7 +455,7 @@ protected JabRefCliPreferences() {

// region: Grobid
defaults.put(GROBID_ENABLED, Boolean.FALSE);
defaults.put(GROBID_OPT_OUT, Boolean.FALSE);
defaults.put(GROBID_PREFERENCE, Boolean.FALSE);
defaults.put(GROBID_URL, "http://grobid.jabref.org:8070");
// endregion

Expand Down Expand Up @@ -2181,11 +2181,11 @@ public GrobidPreferences getGrobidPreferences() {

grobidPreferences = new GrobidPreferences(
getBoolean(GROBID_ENABLED),
getBoolean(GROBID_OPT_OUT),
getBoolean(GROBID_PREFERENCE),
get(GROBID_URL));

EasyBind.listen(grobidPreferences.grobidEnabledProperty(), (obs, oldValue, newValue) -> putBoolean(GROBID_ENABLED, newValue));
EasyBind.listen(grobidPreferences.grobidOptOutProperty(), (obs, oldValue, newValue) -> putBoolean(GROBID_OPT_OUT, newValue));
EasyBind.listen(grobidPreferences.grobidUseAskedProperty(), (obs, oldValue, newValue) -> putBoolean(GROBID_PREFERENCE, newValue));
EasyBind.listen(grobidPreferences.grobidURLProperty(), (obs, oldValue, newValue) -> put(GROBID_URL, newValue));

return grobidPreferences;
Expand Down
3 changes: 3 additions & 0 deletions src/main/resources/l10n/JabRef_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -2804,3 +2804,6 @@ Citation\ Entry=Citation Entry

File\ Move\ Errors=File Move Errors
Could\ not\ move\ file\ %0.\ Please\ close\ this\ file\ and\ retry.=Could not move file %0. Please close this file and retry.

Send\ to\ Grobid=Send to Grobid
Do\ not\ send=Do not send
Loading