Fixed Grobid Preference Dialog Logic, Removed Checkbox by arshchawla21 · Pull Request #12034 · JabRef/jabref

arshchawla21 · 2024-10-20T00:33:41Z

This issue revolves around the Grobid preference dialog logic. More specifically, it was found the "Do not ask again" button would not function as expected in certain cases, when asking the user for permission to use Grobid when uploading a PDF. Below demonstrates a logic diagram for the previous (broken) functionality. For context, the previous implentation (in GrobidPreferences.java), contained two boolean key variables, grobidEnabled and grobidOptOut.

From the diagram it is clear that in certain cases (highlighted red), the logic does not work as expected. A potential fix revolves around removing the checkbox altogether, only displaying the dialog once, and saving the users' preference for the future. This approach has been suggested by the following comments: #9801 (comment) and #9802 (comment). The new logic would operate as per the following diagram:

My implementation follows the above logic, with grobidOptOut being redefined to grobidPreference. Below is the dialog a user will receive if they have not used Grobid before:

While this will work as expected for new users, one potential flaw is that some users (who have used Grobid in the past) will be also receive this dialog. Specifically users who had Grobid enabled and users who had Grobid disabled, but did not specifically opt-out (did not press "Do not ask again").

I believe the implementation is robust, but I'm always open to suggestions and would greatly appreciate any feedback on potential improvements!

Fixes JabRef#566.

Mandatory checks

I own the copyright of the code submitted and I licence it under the MIT license
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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

…D usage preference. "Do not ask" checkbox replaced with "Save Preference", and GROBID_OPTOUT replaced with GROBID_PREFERENCE to reduce ambiguity.

…only be asked about Grobid permissions once, with the preference being stored for the future. Updated CHANGELOG.md

Siedlerchr · 2024-10-20T12:34:36Z

                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("Yes"),


Yes no only dialogs should be avoided, as the buttons are not self explanatory, better wording:

Send to Grobid

Do not send

We have this requirement at https://devdocs.jabref.org/code-howtos/ui-recommendations.html#name-your-buttons-for-the-actions, but need more text for explanation.

„yes“ is not an action.

koppor · 2024-10-20T14:45:15Z

Note this removes the possibility to ask everytime. #9291 (comment) – This is OK for me now!

koppor · 2024-10-20T14:48:38Z

The new logic would operate as per the following diagram:

I like diagrams. Thank you!

I do not get what „enabled“ means.

The diagram does not show what happens after restart of JabRef. The dialog should only appear lf user did Not decide yet.

…prove clarity.

…ix-for-koppor-issue-566

koppor · 2024-10-26T22:05:29Z

    public static boolean showAndWaitIfUserIsUndecided(DialogService dialogService, GrobidPreferences preferences) {
-        if (preferences.isGrobidEnabled()) {
-            return true;
+        if (preferences.isGrobidPreference()) {


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).

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

arshchawla21 · 2024-10-26T22:09:33Z

Yes no only dialogs should be avoided, as the buttons are not self explanatory, better wording:

Send to Grobid

Do not send

I have updated the dialog as per this suggestion, improving clarity.

The new logic would operate as per the following diagram:

I like diagrams. Thank you!

I do not get what „enabled“ means.

The diagram does not show what happens after restart of JabRef. The dialog should only appear lf user did Not decide yet.

Enabled is referring to the grobidEnabled variable. To confirm, the dialog only appears if the user has not decided. This is due to the groidPreference variable being updated and stored after the user has made their decision for the first time.

…ity. Renamed GrobidPreferenceDialogHelper.java to GrobidUseDialogHelper.java.

koppor · 2024-10-26T23:11:25Z

@arshchawla21 I review using https://github.com/JabRef/jabref/pull/12034/files. Maybe, you should open this, too. - You will see that Christoph made a suggestion to the CHANGELOG.md

koppor · 2024-10-26T23:12:00Z

I resolved the merge conflicts in CHANGELOG.md to have the CI working.

Co-authored-by: Christoph <siedlerkiller@gmail.com>

koppor · 2024-10-27T00:29:15Z

I added it to the merge queue. Should be in main in 30 minutes. Thank you for your work.

arshchawla21 · 2024-10-27T00:39:55Z

Thank you so much for your guidance @koppor, is there anything else I need to do? The unit test check appears to be failing.

koppor · 2024-10-27T00:49:33Z

Thank you so much for your guidance @koppor, is there anything else I need to do? The unit test check appears to be failing.

Fortunately, we have the merge queue doing the final checks properly.

Please check and fix:

 org.jabref.logic.l10n.LocalizationConsistencyTest

  Test findMissingLocalizationKeys() FAILED

  org.opentest4j.AssertionFailedError: 
  DETECTED LANGUAGE KEYS WHICH ARE NOT IN THE ENGLISH LANGUAGE FILE
  PASTE THESE INTO THE ENGLISH LANGUAGE FILE "JabRef_en.properties"
  
  Do not send=Do not send
  Send to Grobid=Send to Grobid
  
   ==> expected: <[]> but was: <[Do not send (src/main/java/org/jabref/gui/importer/GrobidUseDialogHelper.java LANG), Send to Grobid (src/main/java/org/jabref/gui/importer/GrobidUseDialogHelper.java LANG)]>

…d" and "Do not send".

arshchawla21 · 2024-10-27T02:18:31Z

Hi @koppor, I've made the required changes to "JabRef_en.properties", and the checks appear to be passing. I believe this PR is ready to merge whenever you are ready!

LoayGhreeb · 2024-10-27T16:39:15Z

-            return false;
-        }
-        boolean grobidEnabled = dialogService.showConfirmationDialogWithOptOutAndWait(
+        boolean grobidEnabled = dialogService.showConfirmationDialogAndWait(


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

Reset preferences.

Restart.

Open preferences, then go to Web Search and allow Grobid.

Import a file.

The dialog will prompt to enable Grobid, even though it is already enabled.

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.

…le Grobid on first use, if Grobid has already been manually enabled.

Co-authored-by: Loay Ghreeb <loayahmed655@gmail.com>

ThiloteE · 2024-11-15T07:26:55Z

@arshchawla21 If I may ask, what app did you use to make those diagrams?

arshchawla21 · 2024-11-16T04:32:10Z

@arshchawla21 If I may ask, what app did you use to make those diagrams?

Of course, I used draw.io (available at https://app.diagrams.net/) to make the diagrams!

arshchawla21 and others added 3 commits October 16, 2024 21:45

Modified GROBID dialog logic, allowing users to explicitly save GROBI…

e945666

…D usage preference. "Do not ask" checkbox replaced with "Save Preference", and GROBID_OPTOUT replaced with GROBID_PREFERENCE to reduce ambiguity.

Removed checkbox entirely, dialog logic updated such that users will …

0e0ec95

…only be asked about Grobid permissions once, with the preference being stored for the future. Updated CHANGELOG.md

Merge branch 'main' into fix-for-koppor-issue-566

2caa27d

Siedlerchr reviewed Oct 20, 2024

View reviewed changes

Comment thread CHANGELOG.md Outdated

Siedlerchr reviewed Oct 20, 2024

View reviewed changes

koppor added the status: changes-required Pull requests that are not yet complete label Oct 23, 2024

arshchawla21 added 2 commits October 27, 2024 08:56

Updated dialog wording to be 'Send to Grobid' and 'Do not send' to im…

e254b75

…prove clarity.

Merge remote-tracking branch 'origin/fix-for-koppor-issue-566' into f…

158e2e8

…ix-for-koppor-issue-566

koppor requested changes Oct 26, 2024

View reviewed changes

Refactored grobidPreference boolean to grobidUseAsked to improve clar…

1160359

…ity. Renamed GrobidPreferenceDialogHelper.java to GrobidUseDialogHelper.java.

Merge branch 'main' into fix-for-koppor-issue-566

5a19cf5

Update CHANGELOG.md

123449d

Co-authored-by: Christoph <siedlerkiller@gmail.com>

koppor previously approved these changes Oct 27, 2024

View reviewed changes

koppor enabled auto-merge October 27, 2024 00:15

Merge branch 'main' into fix-for-koppor-issue-566

ab4325a

Updated JabRef_en.properties to reflect language keys: "Send to Grobi…

3edfb38

…d" and "Do not send".

auto-merge was automatically disabled October 27, 2024 01:33
Head branch was pushed to by a user without write access

arshchawla21 dismissed koppor’s stale review via 3edfb38 October 27, 2024 01:33

koppor previously approved these changes Oct 27, 2024

View reviewed changes

Merge branch 'main' into fix-for-koppor-issue-566

e9bc98a

koppor added this pull request to the merge queue Oct 27, 2024

LoayGhreeb requested changes Oct 27, 2024

View reviewed changes

LoayGhreeb removed this pull request from the merge queue due to a manual request Oct 27, 2024

Modified Grobid dialog logic such that users are not prompted to enab…

420f940

…le Grobid on first use, if Grobid has already been manually enabled.

arshchawla21 dismissed koppor’s stale review via 420f940 October 27, 2024 20:14

LoayGhreeb reviewed Oct 27, 2024

View reviewed changes

Comment thread src/main/java/org/jabref/gui/importer/GrobidUseDialogHelper.java Outdated

LoayGhreeb previously approved these changes Oct 27, 2024

View reviewed changes

LoayGhreeb removed the status: changes-required Pull requests that are not yet complete label Oct 27, 2024

Update src/main/java/org/jabref/gui/importer/GrobidUseDialogHelper.java

76173e5

Co-authored-by: Loay Ghreeb <loayahmed655@gmail.com>

arshchawla21 dismissed LoayGhreeb’s stale review via 76173e5 October 27, 2024 20:48

LoayGhreeb approved these changes Oct 27, 2024

View reviewed changes

LoayGhreeb enabled auto-merge October 27, 2024 20:51

LoayGhreeb added this pull request to the merge queue Oct 27, 2024

Merged via the queue into JabRef:main with commit ce27eb3 Oct 27, 2024

ThiloteE mentioned this pull request Nov 15, 2024

Unchecked "Do not ask again" does not work JabRef/jabref-koppor#566

Closed

mulla028 mentioned this pull request Nov 28, 2024

Click on entry at "Check integrity" focus bug fixed #12022

Merged

7 tasks

Uh oh!

Conversation

arshchawla21 commented Oct 20, 2024

Mandatory checks

Uh oh!

Uh oh!

Siedlerchr Oct 20, 2024

Choose a reason for hiding this comment

Uh oh!

koppor Oct 20, 2024

Choose a reason for hiding this comment

Uh oh!

koppor commented Oct 20, 2024

Uh oh!

koppor commented Oct 20, 2024

Uh oh!

koppor Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

arshchawla21 Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

arshchawla21 commented Oct 26, 2024

Uh oh!

koppor commented Oct 26, 2024

Uh oh!

koppor commented Oct 26, 2024

Uh oh!

koppor commented Oct 27, 2024

Uh oh!

arshchawla21 commented Oct 27, 2024

Uh oh!

koppor commented Oct 27, 2024

Uh oh!

arshchawla21 commented Oct 27, 2024

Uh oh!

LoayGhreeb Oct 27, 2024

Choose a reason for hiding this comment

Uh oh!

arshchawla21 Oct 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ThiloteE commented Nov 15, 2024

Uh oh!

arshchawla21 commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arshchawla21 commented Nov 16, 2024 •

edited

Loading