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

Ask for enable indexing when clicking fulltext search #11854

Merged
merged 7 commits into from
Sep 29, 2024
Merged

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Sep 29, 2024

fixes #9491 (comment)

grafik

Mandatory checks

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

@Siedlerchr Siedlerchr requested a review from koppor September 29, 2024 19:00
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 29, 2024
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.

Small comment/question inside.

Can you look into following, too?

image

If I disable fulltext search in the prefernces (1), it should also be disabled in the search bar (2).

boolean enableFulltextSearch = dialogService.showConfirmationDialogAndWait(Localization.lang("Fulltext search"), Localization.lang("Fulltext search requires the setting 'Automatically index all linked files for fulltext search' to be enabled. Do you want to enable indexing now?"), Localization.lang("Enable indexing"), Localization.lang("Keep disabled"));

LibraryTab libraryTab = tabContainer.getCurrentLibraryTab();
if (libraryTab != null && enableFulltextSearch) {
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 try to disable the button if no library is active. Refs #11837.

Follow-up I would say.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we disable it, the new dialog would never come up

Copy link
Member Author

Choose a reason for hiding this comment

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

Search is disabled if libray is not active
grafik

Copy link
Member

Choose a reason for hiding this comment

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

Then why this null check? 😅

Copy link
Member Author

@Siedlerchr Siedlerchr Sep 29, 2024

Choose a reason for hiding this comment

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

Intellij suggested that because library tab is nullable.. (reduce warnings in the code=.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we disable it, the new dialog would never come up

The button should be disabled when preferences are off. The dialog will appear if the preferences are disabled and clicks to enable the button. If accepted to enable indexing, the button and preferences should both be enabled; otherwise, both should remain disabled.

Copy link
Member

Choose a reason for hiding this comment

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

If we disable it, the new dialog would never come up

The button should be disabled when preferences are off. The dialog will appear if the preferences are disabled and clicks to enable the button. If accepted to enable indexing, the button and preferences should both be enabled; otherwise, both should remain disabled.

I understand "clicks to enable the button" as a "click" int he preference.

The thing Christoph implemented was the intention of the whole thing. If we did as you described, we would not implement #9491 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Intellij suggested that because library tab is nullable.. (reduce warnings in the code=.

CC @calixtus. Annotation introduced at #10578. Follow-up: Document when this thing can be null. We should try not to pass null.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand "clicks to enable the button" as a "click" int he preference.

The toggle button in the search bar.

Copy link
Member Author

Choose a reason for hiding this comment

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

please test the latest changes should be okay now

github-actions[bot]

This comment was marked as outdated.

@koppor
Copy link
Member

koppor commented Sep 29, 2024

I am so sorry, but if I press "Keep disabled", the popup appears again - and again - and again.

@Siedlerchr Siedlerchr added this pull request to the merge queue Sep 29, 2024
Copy link
Contributor

github-actions bot commented Sep 29, 2024

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

Merged via the queue into main with commit a50cad7 Sep 29, 2024
26 checks passed
@Siedlerchr Siedlerchr deleted the fulltextsearch branch September 29, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deactivate fulltextsearch as default
3 participants