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

convert search worker to javafx #4897

Merged
merged 35 commits into from
Jun 6, 2019
Merged

convert search worker to javafx #4897

merged 35 commits into from
Jun 6, 2019

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Apr 18, 2019

One of the few things which was still using swing.

Search Counter is implemented again
Search Highlighting is also implemented


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 18, 2019
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.

The search is also done here:

entriesFiltered = new FilteredList<>(entriesViewModel);
entriesFiltered.predicateProperty().bind(
Bindings.createObjectBinding(() -> this::isMatched,
Globals.stateManager.activeGroupProperty(), Globals.stateManager.activeSearchQueryProperty())
);

which makes the SearchWorker kind of obsolete

* upstream/master:
  Fix Some Codacy Code Convention Issues (#4904)
  Bump ridl from 6.2.2 to 6.2.3 (#4903)
  Bump juh from 6.2.2 to 6.2.3 (#4902)
  Bump jurt from 6.2.2 to 6.2.3 (#4901)
  Bump unoil from 6.2.2 to 6.2.3 (#4900)
  Bump richtextfx from 0.9.3 to 0.10.0 (#4899)
  Store column widths as integer (#4896)
  Improve full text search for several files (#4855)
@Siedlerchr
Copy link
Member Author

OKay, thanks for the hint. now removed the search worker completely.

}
}

private void updateUIWithSearchResult(List<BibEntry> matchedEntries) {
Copy link
Member

Choose a reason for hiding this comment

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

What about this functionality? I think both the search result message as well as the highlighting are nice-to-have features.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, yes I will look if I can integrate this somehow

@tobiasdiez tobiasdiez changed the title convert search worker to javafx [WIP] convert search worker to javafx Apr 24, 2019
@tobiasdiez tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 24, 2019
* upstream/master:
  Fix for the issue #4912 (#4916)
  Refactorings, move functionality of CustomEntryTypesManager to Preferences Use ObservableList instead of LIstProperty
  Toggle enable status of menu items (#4872)
  Fixes throwing an exception when 'id' field is present in bib file (#4918)
  Quick fix for error when opening preferences (#4917)
  Revert "Create new layout for preferences regarding columns"
  Create new layout for preferences regarding columns
  Rearrange the padding of the "Append library" dialog (#4914)
  Make Group dialog resizable (#4910)
  Adjust save exception to inlcude orgininal stack traces
  remove dialogService parameter
  remove dialog service argument
  rework dialog, create fxml etc fix l10n Remove obsolete code
  rework threading stuff simplify code
  Rework PosteOpenActions to javafx (custom entry type import)
  Rework PosteOpenActions to javafx (custom entry type import)
@Siedlerchr
Copy link
Member Author

I actually found a way to add the search result message and the highlighter, but I am not if this behaviour in the entry editor is really correct, and this creates sometimes really odd issues #4472

However, in the entry editor itself there seems no highlighting be done?

void addSearchListener(SearchQueryHighlightListener listener) {
// TODO: Highlight search text in entry editors
searchListeners.add(listener);
panel.frame().getGlobalSearchBar().getSearchQueryHighlightObservable().addSearchListener(listener);
}

And I noticed that when the entry editor is open, and I search a term which is not present, the entry stays open at the last selected entry:

grafik

@Siedlerchr Siedlerchr changed the title [WIP] convert search worker to javafx convert search worker to javafx Apr 27, 2019
* upstream/master:
  Throw BibEntryNotFound exception in case entry is no longer present  (#4935)
  Improve author parsing (#4931)
  Ui preferences global modifications (#4926)
  Bump checkstyle from 8.19 to 8.20 (#4928)
  Bump mysql-connector-java from 8.0.15 to 8.0.16 (#4924)
  UI Preferences->advanced tab optimization : separators and text modification (#4922)

# Conflicts:
#	src/main/java/org/jabref/gui/search/SearchWorker.java
@Siedlerchr
Copy link
Member Author

Got it wo work
grafik

@Siedlerchr
Copy link
Member Author

Siedlerchr commented May 1, 2019

TODO: The highlighting needs to be recalculated on tab switch

TODO: Highlighting for WebView

Maybe moving the register of the search listener fo the bind to entry?

@Siedlerchr Siedlerchr changed the title convert search worker to javafx [WIP] convert search worker to javafx May 1, 2019
@Siedlerchr Siedlerchr mentioned this pull request May 2, 2019
* upstream/master:
  Remove UI ThreadList for maintable as it prevents sorting in maintable (#4964)
  fix map collision (#4962)
  Added a link to the current open issues with tags 'good first issue' to the beginning of CONTRIBUTING.md to help new contributors find a way to get started. (#4960)
  Fixed the tab order in the database login dialog (#4955)
@Siedlerchr Siedlerchr changed the title [WIP] convert search worker to javafx #convert search worker to javafx May 12, 2019
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 12, 2019
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.

Generally looks fine to me.

src/main/java/org/jabref/gui/StateManager.java Outdated Show resolved Hide resolved
@@ -403,7 +403,7 @@ private void fetchAndMerge(EntryBasedFetcher fetcher) {
void addSearchListener(SearchQueryHighlightListener listener) {
// TODO: Highlight search text in entry editors
searchListeners.add(listener);
panel.frame().getGlobalSearchBar().getSearchQueryHighlightObservable().addSearchListener(listener);

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line

@@ -69,6 +72,17 @@ public SourceTab(BibDatabaseContext bibDatabaseContext, CountingUndoManager undo
this.fileMonitor = fileMonitor;
this.dialogService = dialogService;

Globals.stateManager.addSearchQueryHighlightListener(highlightPattern -> {
Copy link
Member

Choose a reason for hiding this comment

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

Please pass stateManager as external dependency.

@@ -0,0 +1,7 @@
#bibtexSourceCodeArea .search {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to move this code to the entry editor css?

);
entriesFiltered.addListener((ListChangeListener<BibEntryTableViewModel>) c -> {
Copy link
Member

Choose a reason for hiding this comment

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

Use bindings: Bindings.size(entriesFiltered) ?

// TODO: Remove search worker as this is doing the work twice now
searchWorker = new SearchWorker(currentBasePanel, searchQuery, searchDisplayMode);
searchWorker.execute();
updateResults(Globals.stateManager.searchResultSizeProperty().get(),
Copy link
Member

Choose a reason for hiding this comment

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

Is this really working as desired? I would have expected that the resultSize is not yet updated correctly (for large databases).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thing. I'm trying to bind it to the results property but turns out to be a bit more complicated as the illegal search query msg is also listed in the label

* upstream/master:
  Fix downloading pdf produces html as extension (#4965)
  Too big error message #4827 (#4966)
  Reformat Understanding the basics section of CONTRIBUTING.md (#4978)
  Revised README.md (#4974)
  Switch Jsoup's StringUtil for JabRef's StringUtil (#4970)
move css for highlighting
binding for searchResultSize
Use SearchQuery directly

TODO: fix updating of search results
* upstream/master:
  Resize different fonts changing entry type (#4980)
  Extended Hints - Alternative to #4971 (#4975)
  Fix command line help text (#4979)

# Conflicts:
#	src/main/java/org/jabref/gui/search/GlobalSearchBar.java
* upstream/master:
  Fix background color of dialogs in dark mode (#4994)
  NPE-fix for Preferences/Ext-Prog/Settings for X/Browse  (#4983)
  Bump richtextfx from 0.10.0 to 0.10.1 (#4989)
  Bump tika-core from 1.20 to 1.21 (#4984)
* upstream/master:
  Fix icon size - the second (#4993)

# Conflicts:
#	src/main/java/org/jabref/gui/entryeditor/EntryEditor.java
@Siedlerchr
Copy link
Member Author

@tobiasdiez I am currently having trouble with the updating of the search result count. I tried around with different ways but I always end up with the problem that the count is either 1 or 0 , although the number of found/matched entries in the Main table is obviously higher.
I have no freaking idea why. Maybe you have an idea

@davidemdot
Copy link
Member

davidemdot commented May 26, 2019

@Siedlerchr:

I am currently having trouble with the updating of the search result count. I tried around with different ways but I always end up with the problem that the count is either 1 or 0 , although the number of found/matched entries in the Main table is obviously higher.

I am looking this branch and trying to replicate your error, but the search result counter is OK for me. Maybe I did not understand the point properly, so could you explain in detail what is going bad?

Here you can find three screenshots, from JabRef running in "convertSearchWorker" branch:

1  ette
2  ettesadsda
3  torres

And here you can see the output from my terminal (I did more than three searches):

4  console

@Siedlerchr
Copy link
Member Author

Glad that it works for you. I was running it from Eclipse. Will double check my Java version again

@Siedlerchr
Copy link
Member Author

@davidemdot Okay, I found out that it only works with one open library. If you have at least two libraries open and switch the tabs than you encounter this behaviour.

@tobiasdiez seems like I somehow have to include the databasecontext in the search result binding. Any idea how to achieve this?

* upstream/master:
  Update mac icon and install4j to 7.0.11 (#5005)
  Bump checkstyle from 8.20 to 8.21 (#5001)
  fix IEE fetcher (#4999)
  Fix the 'Attach file' dialog for starting on the user's main directory (#4996)
  Readme TOC (#4986)
@Siedlerchr Siedlerchr changed the title #convert search worker to javafx convert search worker to javafx May 27, 2019
@Siedlerchr
Copy link
Member Author

As the search is now working fine with multiple databases open I am going to merge this.

@Siedlerchr Siedlerchr merged commit dd4f0a0 into master Jun 6, 2019
@Siedlerchr Siedlerchr deleted the convertSearchWorker branch June 6, 2019 08:08
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.

None yet

3 participants