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 5 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 @@ -98,6 +98,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.
arshchawla21 marked this conversation as resolved.
Show resolved Hide resolved
- 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.GrobidPreferenceDialogHelper;
import org.jabref.gui.keyboard.KeyBinding;
import org.jabref.gui.keyboard.KeyBindingRepository;
import org.jabref.gui.menus.ChangeEntryTypeMenu;
Expand Down Expand Up @@ -432,7 +432,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());
GrobidPreferenceDialogHelper.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.GrobidPreferenceDialogHelper;
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());
GrobidPreferenceDialogHelper.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,27 @@
/**
* 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 Grobid, saving the users' preference for the future.
*/
public class GrobidOptInDialogHelper {
public class GrobidPreferenceDialogHelper {

/**
* 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.isGrobidEnabled()) {
return true;
if (preferences.isGrobidPreference()) {
Copy link
Member

Choose a reason for hiding this comment

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

We should use another word. This is NOT a JabRef setting (AKA preference), but a kind of "to prefer something". Moreover, this is asked at the first time only. Thus, I would rename it to isGrobidUseAsked. This is to strengthen that this is a one-time thing (in contrast to being asked at every run).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the observation, this would certainly make the logic more clear. I have made the changes as required.

return preferences.isGrobidEnabled();
}
if (preferences.isGrobidOptOut()) {
return false;
}
boolean grobidEnabled = dialogService.showConfirmationDialogWithOptOutAndWait(
boolean grobidEnabled = dialogService.showConfirmationDialogAndWait(
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.setGrobidPreference(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) && GrobidPreferenceDialogHelper.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())) {
&& GrobidPreferenceDialogHelper.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.setGrobidPreference(grobidPreferences.isGrobidPreference());
grobidPreferences.setGrobidURL(grobidURLProperty.getValue());
doiPreferences.setUseCustom(useCustomDOIProperty.get());
doiPreferences.setDefaultBaseURI(useCustomDOINameProperty.getValue().trim());
Expand Down
20 changes: 10 additions & 10 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 grobidPreference;
private final StringProperty grobidURL;

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

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

// region: optout; models "Do not ask again" option
public boolean isGrobidOptOut() {
return grobidOptOut.get();
// region: preference; models "Save Preference" option
public boolean isGrobidPreference() {
return grobidPreference.get();
}

public BooleanProperty grobidOptOutProperty() {
return grobidOptOut;
public BooleanProperty grobidPreferenceProperty() {
return grobidPreference;
}

public void setGrobidOptOut(boolean grobidOptOut) {
this.grobidOptOut.set(grobidOptOut);
public void setGrobidPreference(boolean grobidPreference) {
this.grobidPreference.set(grobidPreference);
}
// endregion: optout

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,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 @@ -451,7 +451,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 @@ -2174,11 +2174,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.grobidPreferenceProperty(), (obs, oldValue, newValue) -> putBoolean(GROBID_PREFERENCE, newValue));
EasyBind.listen(grobidPreferences.grobidURLProperty(), (obs, oldValue, newValue) -> put(GROBID_URL, newValue));

return grobidPreferences;
Expand Down