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

Grobid config panel with check box #9801

Closed

Conversation

hanachaari
Copy link

@hanachaari hanachaari commented Apr 25, 2023

This fixes koppor#566, which is related to Grobid preferences.

I have changed Grobid preferences from a checkbox to a Radio-Button-Group (disable, enable, always ask).

This is the new configuration panel
image
image
image

The dialog below appears as long as the user does not check "do not ask again" or change preferences .
image

Despite the decision as mentioned here koppor#566 (comment) was to remove the "do not ask checkbox", I think it is not recommended from a UX perspective. So, I provided two versions: the branch: "gorbidConfigPanel" without a checkbox and the branch"gorbidConfigPanelWithCheckBox" with a checkbox.
The checkbox in the later branch is synchronized so when it is checked Grobid preferences will be "disabled" or "enabled" according to the user's answer(yes/no).

Compulsory checks

@hanachaari hanachaari closed this Apr 25, 2023
@hanachaari hanachaari reopened this Apr 25, 2023
@hanachaari hanachaari closed this Apr 25, 2023
@hanachaari hanachaari reopened this Apr 25, 2023
@hanachaari hanachaari closed this Apr 25, 2023
@hanachaari hanachaari reopened this Apr 25, 2023
@Siedlerchr
Copy link
Member

No need to close/open the PR everytime. Just push the new code, the PR will be updated automatically and the CI will run on every push

@Siedlerchr
Copy link
Member

And for checkstyle https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace.html#using-jabrefs-code-style

@hanachaari
Copy link
Author

I am facing a failure with the fetcher test.
image
The error message suggests the optional link to the PDF file is empty so not found. I checked the source website and I found it, so It may be a problem with the implementation of the retrieval code

@ThiloteE ThiloteE changed the title Gorbid config panel with check box Grobid config panel with check box Apr 25, 2023
@ThiloteE
Copy link
Member

To ease organizational workflows I have linked the pull-request to the issue with syntax as described in https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

Linking a pull request to an issue using a keyword

You can link a pull request to an issue by using a supported keyword in the pull request's description or in a commit message. The pull request must be on the default branch.

  • close
  • closes
  • closed
  • fix
  • fixes
  • fixed
  • resolve
  • resolves
  • resolved

If you use a keyword to reference a pull request comment in another pull request, the pull requests will be linked. Merging the referencing pull request also closes the referenced pull request.

The syntax for closing keywords depends on whether the issue is in the same repository as the pull request.

@Siedlerchr
Copy link
Member

I am facing a failure with the fetcher test. image The error message suggests the optional link to the PDF file is empty so not found. I checked the source website and I found it, so It may be a problem with the implementation of the retrieval code

If you did not touch the fetcher, you can ignore the failing tests. Sometimes they will fail on the CI but work locally

@koppor
Copy link
Member

koppor commented Apr 26, 2023

Dear @hanachaari,

Thank you for working on JabRef.

Despite the decision as mentioned here koppor#566 (comment) was to remove the "do not ask checkbox", I think it is not recommended from a UX perspective.

Maybe you overlooked following text in the isse:

A huge discussion was going on at #9291.

Link: koppor#566 (comment). We even specified the behavior with that box: #9291 (comment). It was "smashe away": #9291 (comment)

Main reason:

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?

So, please: No checkbox.

Please do Enable GROBID, Disable GROBID in the preferences.

I follow the argument of Tobias at #9291 (comment):

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

That means, the preference should have a setting if the user has already been asked. That can be done by an enum: FIRST_START, ENABLED, DISABLED. Preference storing/retrieval might be difficult. @calixtus WDYT?

koppor

This comment was marked as outdated.

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.

See above.

@calixtus
Copy link
Member

I'm somewhat impressed you implemented this checkbox as a commonfxcontrol, which was probably no easy. But... Parsing fxml needs a lot of time. If this prefs option is not used anywhere again, I would ask you to integrate it directly in the prefs tab.

About the enum, yes, sounds like a good idea. But please don't change the implementation in JabRefPreferences using the BackingStore or we'll have multiple users annoyed being asked again on start if they want or don't want to use grobid.

@hanachaari
Copy link
Author

Updates on #9802

@calixtus
Copy link
Member

Thanks for your ongoing interest in jabref.
Again, implementing this as a FX control is impressive and shows good understanding of javafx, but it does cost unnecessary computing time and resource to parse the fxml. Please integrate it directly in the tab. Also a radio switch is from the UX perrspective way too much for such a simple preference option. Please use just a checkbox titled "enable grobid" and ask for the grobid orl in the next line.

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

.

@koppor
Copy link
Member

koppor commented Apr 28, 2023

Superseeded by #9802.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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