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

Set default value for fulltext indexing to false #9516

Closed
wants to merge 5 commits into from

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented Jan 2, 2023

fixes #9491

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

@koppor
Copy link
Member

koppor commented Jan 2, 2023

This PR refs #9491

@koppor koppor modified the milestones: v6.0, v5.9 Jan 2, 2023
@@ -692,7 +692,7 @@ private JabRefPreferences() {
// Download files by default
defaults.put(DOWNLOAD_LINKED_FILES, true);
// Create Fulltext-Index by default
defaults.put(FULLTEXT_INDEX_LINKED_FILES, true);
defaults.put(FULLTEXT_INDEX_LINKED_FILES, false);
Copy link
Member

Choose a reason for hiding this comment

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

Actually I would leave this as true and only the search button deactivated for the moment

Copy link
Member Author

Choose a reason for hiding this comment

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

It's what we literally discussed yesterday and decided againstindexing fulltext by default.

Copy link
Member

Choose a reason for hiding this comment

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

I personally changed my mind after the meeting.

If indexing is disabled, users will try to use file search, but it will not work. Users will not know that they first have to enable indexing for file search to work. They will report that file search is broken. Disable indexing only, once users are somehow notified that file search can only be used, if indexing is enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes we discussed also that this is not to be implemented as the lucene search pr of @btut changes so many things with the search that there should not be made any major changes in behaviour, GUI or logic, since all this work would be thrown away when or have to be undone before merging the lucene PR.
Decisions were made for a reason yesterday. Don't start rediscussing what we already decided yesterday.

Copy link
Member

Choose a reason for hiding this comment

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

Yesterdays discussion notes: #9491 (comment)

koppor
koppor previously approved these changes Jan 3, 2023
@calixtus calixtus added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers ui labels Jan 3, 2023
@@ -224,7 +223,7 @@ private void initSearchModifierButtons() {
fulltextButton.setSelected(searchPreferences.isFulltext());
fulltextButton.setTooltip(new Tooltip(Localization.lang("Fulltext search")));
initSearchModifierButton(fulltextButton);
EasyBind.subscribe(preferencesService.getFilePreferences().fulltextIndexLinkedFilesProperty(), value -> fulltextButton.setDisable(!value));
fulltextButton.disableProperty().bind(preferencesService.getFilePreferences().fulltextIndexLinkedFilesProperty().not());
Copy link
Member

Choose a reason for hiding this comment

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

Still Needs to be bound to the UI button state and maybe we can bind managed (completely removed from the UI) to the fulltextIndexLinkedFilesProperty property

@Siedlerchr Siedlerchr removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 3, 2023
fulltextButton.setTooltip(new Tooltip(Localization.lang("Fulltext search")));
initSearchModifierButton(fulltextButton);
EasyBind.subscribe(preferencesService.getFilePreferences().fulltextIndexLinkedFilesProperty(), value -> fulltextButton.setDisable(!value));
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice idea! Additionally, it should set the button state to "false" if the fulltext search is deactivated (and only then)

@Siedlerchr Siedlerchr deleted the fulltext_default_false branch January 4, 2023 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deactivate fulltextsearch as default
4 participants