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

Importing pdfs will now always ask for permission to send file to Grobid #9291

Closed
wants to merge 8 commits into from

Conversation

DunkDoge
Copy link

@DunkDoge DunkDoge commented Oct 23, 2022

This fix makes it so that the dialogue box below appears everytime you import a pdf if it has been enabled in preferences. Previously it only asked the first time. However, since this involves privacy it's important to ask everytime.

image

Previously the code would check whether Grobid was enabled. If it was, it would return true before showing the dialogue box. However now it checks whether it is not enabled and returns false if it isn't.

Fixes koppor#566

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

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution. Very much appreciated!

I don't think its very user friendly to ask every time.

The confusing part seems to be the "Do not ask again" checkbox, since by design this dialog is only a single-instance confirmation. So I would propose to:

  • remove the "do not ask again" checkbox
  • disable grobid by default (this is probably already the default)
  • add a short privacy statement in the prefs under "enable grobid"

@DunkDoge
Copy link
Author

@tobiasdiez Hi I've modified the dialogue box according to your dot points. Before I push, I wanted to make sure the "enable grobid" in prefs your talking about is the checkbox in Import and Export (in preferences) where it says "Allow sending PDF files and raw citation strings to a JabRef online service (Grobid)...."

@DunkDoge DunkDoge marked this pull request as draft October 29, 2022 05:19
@tobiasdiez
Copy link
Member

Yes, that's the correct one. Thanks!

@DunkDoge DunkDoge marked this pull request as ready for review October 29, 2022 09:35
@@ -23,14 +23,15 @@ public static boolean showAndWaitIfUserIsUndecided(DialogService dialogService,
return true;
}
if (preferences.isGrobidOptOut()) {
return false;
return preferences.isGrobidEnabled();
Copy link
Member

Choose a reason for hiding this comment

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

Can we not completely remove the isGrobidOptOut check and everything else related to the opt-out?

Copy link
Author

Choose a reason for hiding this comment

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

Hi I've removed all the opt out stuff from the dialoghelper. Could you have another look? The only thing I'm confused about though is that the dialog is meant to be single instance. Wouldn't removing opt out cause it to show the box everytime import if Grobid is disabled in prefs?

@DunkDoge DunkDoge marked this pull request as draft October 30, 2022 05:22
@DunkDoge DunkDoge marked this pull request as ready for review October 30, 2022 05:45
@calixtus
Copy link
Member

We should define a general rule how to deal with do-not-ask-agains to avoid inconsistencies in the UI. Let's talk about this in the next devcall.

@tobiasdiez
Copy link
Member

I agree, a general strategy would be good. But please also keep in mind that the Grobid confirmation is special (at least if I understood it correctly) in that you have a global preference and the dialog in itself is a confirmation that you want to send the information even though the preference is not yet enabled.

@calixtus
Copy link
Member

Yes of course grobid is special (at least when it works from time to time). But I believe this is a question concerning just the UI.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

The answer to "Do not ask again" should be

  • stored in the preferences
  • be customizable in the preference dialog (so that the user can choose whether he wants to be asked again)
  • handled in this "section" of the code

That way, one gains:

  • Users are enabled that JabRef always asks
  • Users are enabled to skip that JabRef asks
  • Users can re-enable that JabRef always asks.

Thus, please rewrite your code and add new code.

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"),
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this!

@koppor koppor added status: devcall status: changes required Pull requests that are not yet complete and removed status: devcall labels Nov 7, 2022
@tobiasdiez
Copy link
Member

@koppor as a general strategy, I agree with this. But how is this helping in this special setting where there is already a preference that is controlling if the feature is enabled or not? Having two preference options "Enabling Grobid" and "Ask again for confirmation for Grobid" is not user friendly in my opinion and only confusing.

What I could imagine is to change the global preference to a radio button with the options: enable grobid, disable grobid, always asks for confirm. But to be honest, I don't see the point of "always ask for confirm". Either a user trusts us or not.

@Siedlerchr
Copy link
Member

The Do not ask again is synced to the preferences so that you can disable this behavior again without resetting all your prefs

@koppor
Copy link
Member

koppor commented Nov 7, 2022

@tobiasdiez I like your three-state preference. Then, we still keep the "Do not ask again" setting, which then sets the three-state preference to one of the first two (according to the user's choice). Then, "Do not ask again" is persisted "somehow". -- The default preference is "always ask" so that the dialog starts at the first run.

@tobiasdiez
Copy link
Member

But what are you "asking again" in this situation?
Should choosing "Yes" in the dialog enable the grobid globally? If yes, what's the point of "asking again"?

There are other confusing scenarios:
For example, if you select "don't ask again" in the dialog and click on "No". Later you enable Grobid in the preferences. Should the dialog be displayed again if you invoke the feature? The same if the user selects first "Yes" and then later disables grobid globally. Should the dialog be displayed at all if you enable grobid in the preferences?

I think it is less complex without the "ask again":

  • grobid enabled globally: no dialog shown
  • grobid not enabled globally: show dialog

@koppor
Copy link
Member

koppor commented Nov 9, 2022

But what are you "asking again" in this situation? Should choosing "Yes" in the dialog enable the grobid globally? If yes, what's the point of "asking again"?

  • DoNotAskAgain(yes) + Grobid(yes): Preference enable grobid
  • DoNotAskAgain(no) + Grobid(yes): Preference "always ask for confirm"
  • DoNotAskAgain(yes) + Grobid(no): Preference disable grobid
  • DoNotAskAgain(no) + Grobid(no): Preference "always ask for confirm"

I see that for case 2 and 4, there is the same setting stored. This has only impact to the button default (which I would say can be ignored) --> the "Yes" button is always the default.

When checking that the dialog is called:

  • Preference enable grobid --> do not show dialog, use Grobid
  • Preference disable grobid --> do not show dialog, do not use Grobid
  • Preference "always ask" --> show dialog

There are other confusing scenarios: For example, if you select "don't ask again" in the dialog and click on "No". Later you enable Grobid in the preferences. Should the dialog be displayed again if you invoke the feature?

No, because the user chose "Enable grobid" and not "always ask"

The same if the user selects first "Yes" and then later disables grobid globally.

No, because the user chose "Disable grobid" and not "always ask"

Should the dialog be displayed at all if you enable grobid in the preferences?

No, because the user chose "Enable grobid" and not "always ask"

I think it is less complex without the "ask again":

The "aks again" has IMHO the benefit that the user does not need to open the preferences. JabRef should be usable "knob-less". This is always a trade-off. We should, however, avoid to send the users ever to the preferences. With "always ask", they are not send to the preferences.

@tobiasdiez
Copy link
Member

The "aks again" has IMHO the benefit that the user does not need to open the preferences.

That you could also have without the "always ask" option. Upon the first use of the feature (if grobid is not explicitly enabled already), ask the user. Depending on the answer, set the preference to "enable" or "disabled".

@koppor
Copy link
Member

koppor commented Nov 10, 2022

That you could also have without the "always ask" option. Upon the first use of the feature (if grobid is not explicitly enabled already), ask the user. Depending on the answer, set the preference to "enable" or "disabled".

What about the second usage? If I want to be asked every time? How can I do that? If JabRef stores the preference (use or don't use), it does not ask again.

@tobiasdiez
Copy link
Member

That wouldn't be possible of course. But as I said above, I don't see the real use case for this. Why would a user want to be asked every time?

@koppor
Copy link
Member

koppor commented Jan 3, 2023

Closing this issue due to inactivity 💤

Please ping us if you intend to resume work on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
import preferences status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unchecked "Do not ask again" does not work
6 participants