-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[WIP] Improvements to search #162
Conversation
# Conflicts: # build.gradle # build.xml # src/main/java/net/sf/jabref/BasePanel.java # src/main/java/net/sf/jabref/EntryEditor.java # src/main/java/net/sf/jabref/EntryEditorTab.java # src/main/java/net/sf/jabref/JabRefFrame.java # src/main/java/net/sf/jabref/JabRefPreferences.java # src/main/java/net/sf/jabref/SearchManager2.java # src/main/java/net/sf/jabref/autocompleter/AbstractAutoCompleter.java # src/main/java/net/sf/jabref/autocompleter/CrossrefAutoCompleter.java # src/main/java/net/sf/jabref/autocompleter/DefaultAutoCompleter.java # src/main/java/net/sf/jabref/autocompleter/EntireFieldAutoCompleter.java # src/main/java/net/sf/jabref/autocompleter/NameFieldAutoCompleter.java # src/main/java/net/sf/jabref/gui/AutoCompleteListener.java # src/main/java/net/sf/jabref/gui/MainTable.java # src/main/java/net/sf/jabref/gui/MainTableSelectionListener.java
This reverts commit ae50ac4.
This reverts commit 77d9875.
This reverts commit 570cac4.
Conflicts: CHANGELOG src/main/java/net/sf/jabref/Globals.java src/main/java/net/sf/jabref/JabRefPreferences.java src/main/java/net/sf/jabref/exporter/layout/LayoutEntry.java src/main/java/net/sf/jabref/gui/BasePanel.java src/main/java/net/sf/jabref/gui/IncrementalSearcher.java src/main/java/net/sf/jabref/gui/JabRefFrame.java src/main/java/net/sf/jabref/gui/PreviewPanel.java src/main/java/net/sf/jabref/gui/SearchManager.java src/main/java/net/sf/jabref/gui/entryeditor/EntryEditor.java src/main/java/net/sf/jabref/gui/entryeditor/EntryEditorTab.java src/main/java/net/sf/jabref/gui/fieldeditors/JTextAreaWithHighlighting.java src/main/java/net/sf/jabref/logic/autocompleter/AbstractAutoCompleter.java src/main/java/net/sf/jabref/logic/autocompleter/AutoCompleter.java src/main/java/net/sf/jabref/logic/autocompleter/AutoCompleterFactory.java
Conflicts: CHANGELOG src/main/java/net/sf/jabref/gui/SearchManager.java
Regarding new eclipse formatting rules: for me it is very hard to review the patch because there are too many lines that have been changed because of the change in formatting rules. This makes it very hard to determine which lines do actually have changed. I would propose that eclipse formatter changes should be done in isolation in separate pull requests. |
Run cleanup and format in eclipse on some files to make comparison in JabRef#162 easier.
I completely understand this. Now there is a second pull request #166 just containing the format/cleanup changes. Is this enough to make comparison easier or should I somehow also modify this PR? |
The thing is that when I want to know what happened in a pull request, I look at https://github.com/JabRef/jabref/pull/162/files which in this case is +2000 lines and -2000 lines at the moment. A lot of these lines were just reformatted, e.g., all the changes in the SearchManagerNoGUI. When the PR is small, formatting changes are no problem, but in this case it would help if this PR would only contain the functional changes, if this is possible without reprogramming everything. Regarding the thing with the AbstractWorker and the run/update method. This is correct as it is because in the constructor of the AbstractWorker a bit of MAGIC (it is really a lot of magic) happens so that when you invoke the run method, it will run ouside the Swing EDT and the update method will run inside the Swing EDT. Regarding the SearchRule: At the moment the search rule can be configured during its constructor and then invoked with different queries to allow reuse of the configuration done by the user in the GUI. At the moment the configuration mainly consists of [bool caseSensitive, bool regex]. But this model does not fit that well. I think it would be a good idea to just create test Predicates that receive a BibtexEntry and they can determine whether it will be included in the search or not. I think we can delete "Select matches". I do not see any usefulness in this setting, especially as it has not yet been implemented. "Autocomplete Names" does work on my machine. We should not remove it. |
We do not want to loose all the work you did. Can you rebase on master? I will try to have a look at all the changes. Another remark: why not make the live option the only one search mode and removing the explicit search button? For me, this is the typical search behavior in most of the applications I know. Even windows search. |
I will try to rebase on master at the weekend. |
Revert changes resulting from running the code formater of eclipse
The rebase run into problems since some files were moved (for example BasePanel.java). This is why I just have reverted the commit containing the code formatting changes. Now the code should be more understandable. |
@tobiasdiez It would be very, very helpful if you managed to show the diff only and if all the merge and reformat commits would have been gone. Proposal: Add a new local git worktree and copy all the files of your current branch to the new directory. Then do a new commit. I hope that this commit has clean changes and is readable. Then you can do a |
@matthiasgeiger and myself checked the new search UI and we liked it. Especially the live feature! :) We are currently discussing to simplify the search altogether in #104. Maybe we can combine a few things. May be you can have a look at this issue as well and we can work something out for integrate these ideas in your PR as well. |
@koppor I opened a second, cleaner PR #307 (I don't want to override my branch). I hope this is also ok. @simonharrer I had a look on the discussion in #104 while implementing this PR. Indeed, some of the design decision were strongly influenced by this thread. On the other hand, some of the motivations for the suggested changes in #104 do no longer apply to the search control at the top of the window (in contrast to the previous implementation as a side control). For example, the size of the dropdown showing the different search options is not very important so that is now no problem to show all the search options as a list. |
Ok, we will disucss this in the upcoming developer call. |
Please follow up at #332. |
This is a new version of my old pull request, now updated to the 2.8 version and incorporating all the suggestions from the last time. (Except the clean commit history )
The changes include:
I have a few questions:
There are quite a few changes from formatting the code in Eclipse. Is it ok if I include them?The SearchWorker implemented the AbstractWorker interface. As far as I understood, this enables one to do work asynchronously and then change the GUI later. However, the worker is simply run viaworker.getWorker().run();
worker.getCallBack().update();
In my understanding, this defeats completely the purpose of using the AbstractWorker. What is the right way to run it?
A general observation about theSearchRule
class. Every method of it takes the query as an argument. I would propose to save the query internally inSearchRule
and thus simplify the interface a little bit.The setting "Autocomplete names" is not yet implemented. In my opinion this can be completely removed.The setting "Select matches" is not yet implemented since I do not understand the purpose of this setting. Should it control whatever the first match is selected after the search is performed? Moreover, I think it was not implemented in the old search pane.Things which should be done at some point (I will open a ticket after this pull request was accepted):