Switch colors of search icon for the two search modes #3871

lenhard · 2018-03-21T18:31:54Z

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?)

koppor · 2018-03-21T18:47:20Z

Grammar-based search is the advanced search. So, the magnifier should be cyan, when following search string is active:

author=kolb and author=kopp

Taking examples from the documentation (https://help.jabref.org/en/Search#search-modes)

All of these are advanced, but they are not recognized as such:

title|keywords = "image processing"

(author = miller or title|keywords = "image processing") and not author = brown

Following is normal mode:

progress “marine aquaculture”progress “marine aquaculture”

This is now with cyan. This is wrong.

Following is normal mode:

progress

Correctly ~~recognized~~ displayed as normal mode with this patch.

koppor

This fix is not in line with the intention of basic and advanced mode. basic is non-grammar-based, advanced is grammar-based

stefan-kolb · 2018-03-21T20:28:51Z

If so, the whole detection is wrong?!

koppor · 2018-03-22T06:39:37Z

Only the beginning of the detection is somehow wrong. For complex terms it is right, but for single words, it is wrong. See the examples above. Maybe, we changed something when going from JabRef 3.6 to 3.7.

lenhard · 2018-03-22T18:53:14Z

Or maybe the detection never was entirely correct. If I have a lot of time on my hands, I might have a look at the detection algorithm. Otherwise we can close this PR and / or remove the coloring of the the search icon again. After all, what is the point of changing its color if the circumstances are incorrect?

tobiasdiez · 2018-03-22T19:30:30Z

@lenhard If you indeed have time to look at the search algorithm, please replace it completely ;-) #1975

koppor · 2018-03-22T21:10:34Z

It worked in 3.6 and no one noticed that it broke at 3.7. So, the point is to keep the idea and to fix it... The idea in other words: make users aware of the feature and that the search might not work if the grammar based search was intended, but not matched (typo in query) Am 22.03.2018 19:53 schrieb "Jörg Lenhard" <notifications@github.com>: Or maybe the detection never was entirely correct. If I have a lot of time on my hands, I might have a look at the detection algorithm. Otherwise we can close this PR and / or remove the coloring of the the search icon again. After all, what is the point of changing its color if the circumstances are incorrect? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3871 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTafiYxOvx2TOF6vCt2ytpyzzRUWwEvks5tg_McgaJpZM4S1MHL> .

stefan-kolb · 2018-03-22T21:37:45Z

Actually there should be tests for it then, as this is not a UI feature but the detection is logic based...

lenhard · 2018-03-27T15:18:23Z

Did we update the antlr stuff after v3.6? Because the query "progress" actually is a valid grammar-based query that compiles with our antlr code. That's the reason for the "wrong" coloring.

I have now added code that checks if a query consists only of word / whitespace characters and/or digits and uses a normal contains-based query for that. Thus, grammar-based searches are only executed when special symbols are used ALTHOUGH our grammar is more powerfull than that. The terms mentioned by @koppor above are now colored as expected. @koppor: I hope this limitation is what is wanted. Please do some testing to see if this matches your expectations.

koppor

Minor comments. In general, it resolves the issue!

koppor · 2018-03-30T16:56:39Z

src/main/java/org/jabref/logic/search/SearchQuery.java

@@ -85,7 +85,7 @@ private String getLocalizedRegularExpressionDescription() {
 }

 public boolean isGrammarBasedSearch() {
- return rule instanceof GrammarBasedSearchRule;
+ return rule instanceof GrammarBasedSearchRule || rule instanceof RegexBasedSearchRule;


Should regex really be treated as grammar-based search? I would not treat it like that, because regex search is orthogonal to grammar-based search. One can activate it in addition.

At least in the documentation: https://help.jabref.org/en/Search#search-modes they are listed under the heading of "advanced search". But I can easily change that of course.

I updated the help accordingly to also reflect that regular expressions are orthogonal. 😇

Could you additionally add a comment there that this is the "Advanced Search" described in the help? I know, this contradicts DDD, but I think, it is OK in this place.

Ok, I adapted the code accordingly.

koppor · 2018-03-30T16:57:16Z

src/main/java/org/jabref/model/search/rules/SearchRules.java

 /**
 * Returns the appropriate search rule that fits best to the given parameter.
 */
 public static SearchRule getSearchRuleByQuery(String query, boolean caseSensitive, boolean regex) {
- if (StringUtil.isBlank(query)) {
+ if (isSimpleQuery(query)) {


Reads good!

koppor · 2018-03-30T16:58:06Z

src/main/java/org/jabref/model/search/rules/SearchRules.java

@@ -25,6 +29,10 @@ public static SearchRule getSearchRuleByQuery(String query, boolean caseSensitiv
 }
 }

+ private static boolean isSimpleQuery(String query) {
+ return StringUtil.isBlank(query) || simpleExpression.matcher(query).matches();


Do you really need both? The current regex has a star (*) at the end, which means zero (0) ore more matches. So, the empty string should match, too.

Good suggestion, that sounds reasonable. I removed the StringUtil dependency and as far as I can see there is no difference in the search.

koppor · 2018-04-02T19:46:37Z

In general good. Minor issues - not sure whether they should be fixed separately.

j - OK:

jö - not OK:

Minor issue:

When I clear the field using Ctrl+A and then Delete (after an advanced search), the marker is still advanced:

Otherwise: LGTM :)

lenhard · 2018-04-06T20:05:33Z

Thanks for the comments @koppor

I fixed all the glitches you described and this PR should now be ready.

Conflicts: CHANGELOG.md

* upstream/master: Pdf exporter - delete xmp actions in the menu bar and the cli option (#3947) Improve search performance (#3950) New Crowdin translations (#3949) Update dependencies for junit, mockito and checkstyle (#3951) Add XMP Exporter (#3895) Switch colors of search icon for the two search modes (#3871) # Conflicts: # src/main/java/org/jabref/gui/search/GlobalSearchBar.java

Switch colors of search icon for the two search modes

183679c

lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 21, 2018

lenhard mentioned this pull request Mar 21, 2018

Magnifier changes colors wrong #3870

Closed

Siedlerchr approved these changes Mar 21, 2018

View reviewed changes

koppor requested changes Mar 21, 2018

View reviewed changes

Merge branch 'master' into switch-search-mode-color

57fe659

lenhard added 3 commits March 27, 2018 16:26

Switch back mode detection

4c45545

Try to see if a query uses simple systems only

cb56ae4

Fix checkstyle errors

f616cb5

koppor requested changes Mar 30, 2018

View reviewed changes

koppor added this to the v4.2 milestone Apr 3, 2018

lenhard added 5 commits April 6, 2018 22:41

Remove Regex search from advanced mode

0b8f8b3

Change pattern to scan for punctuation characters

47efd62

Merge branch 'master' into switch-search-mode-color

0efe1f8

Conflicts: CHANGELOG.md

Reset icon color if query is removed

49099af

Remove unnecessary StringUtil dependency

b9e6111

koppor merged commit be91964 into master Apr 12, 2018

koppor deleted the switch-search-mode-color branch April 12, 2018 11:53

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 12, 2018

koppor mentioned this pull request Oct 27, 2019

Search triggers too soon when entering regex #5349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch colors of search icon for the two search modes #3871

Switch colors of search icon for the two search modes #3871

lenhard commented Mar 21, 2018

koppor commented Mar 21, 2018 •

edited

Loading

koppor left a comment

stefan-kolb commented Mar 21, 2018

koppor commented Mar 22, 2018

lenhard commented Mar 22, 2018

tobiasdiez commented Mar 22, 2018

koppor commented Mar 22, 2018 via email

stefan-kolb commented Mar 22, 2018

lenhard commented Mar 27, 2018

koppor left a comment

koppor Mar 30, 2018

lenhard Apr 1, 2018

koppor Apr 2, 2018

lenhard Apr 6, 2018

koppor Mar 30, 2018

koppor Mar 30, 2018

lenhard Apr 6, 2018

koppor commented Apr 2, 2018 •

edited

Loading

lenhard commented Apr 6, 2018

Switch colors of search icon for the two search modes #3871

Switch colors of search icon for the two search modes #3871

Conversation

lenhard commented Mar 21, 2018

koppor commented Mar 21, 2018 • edited Loading

koppor left a comment

Choose a reason for hiding this comment

stefan-kolb commented Mar 21, 2018

koppor commented Mar 22, 2018

lenhard commented Mar 22, 2018

tobiasdiez commented Mar 22, 2018

koppor commented Mar 22, 2018 via email

stefan-kolb commented Mar 22, 2018

lenhard commented Mar 27, 2018

koppor left a comment

Choose a reason for hiding this comment

koppor Mar 30, 2018

Choose a reason for hiding this comment

lenhard Apr 1, 2018

Choose a reason for hiding this comment

koppor Apr 2, 2018

Choose a reason for hiding this comment

lenhard Apr 6, 2018

Choose a reason for hiding this comment

koppor Mar 30, 2018

Choose a reason for hiding this comment

koppor Mar 30, 2018

Choose a reason for hiding this comment

lenhard Apr 6, 2018

Choose a reason for hiding this comment

koppor commented Apr 2, 2018 • edited Loading

lenhard commented Apr 6, 2018

koppor commented Mar 21, 2018 •

edited

Loading

koppor commented Apr 2, 2018 •

edited

Loading