Fix for issue #3381 #3443

jupf · 2017-11-18T12:32:16Z

Because the javafx Tooltip used in the GlobalSearchBar cannot render HTML, @tobiasdiez recommended in #3381 to create a TextFlow object in the SearchDescriber classes for structuring the Text.

I made the changes to return a TextFlow and its working fine so far without changing the logic that is used to build the text. I moved the search.rules.describer package like proposed from the logic package to the gui package. The existing tests were moved accordingly but still needs to be rewritten for the new TextFlow results.

There are some open questions i would like to ask:

With no Styling of the Tooltip, the readability was really bad (black text on grey background), so I changed the styling to look like the other Tooltips in the bar. Hope it was okay to do this in the context of this issue? I created a new styleclass for this in the Main.css to not change any other styled elements.
the SearchQuery class stores a description of the search. It is created with the SearchDescribers. But they are now inside the gui package and return a TextFlow. So there is a javafx dependency in the SearchQuery class which resides in the logic package at the moment. This doesnt sound right to me? Should the class be moved to the gui package too?
The GroupDialog class uses a SearchQuery object to get a search description. This class is written with Swing and is not compatible to the now returned TextFlow. If I understand it right, the project is migrating to use javaFX. So I want to propose that I write a compatibility method in the GroupDialog class to create an HTML string from the given TextFlow to be compatible while the GroupDialog is still using Swing.
Sadly the localized strings used in the SearchDescribers included the HTML tag elements.. So in many cases it is not possible to use existing translations.. just because the tags are gone. I'm not sure what to do about this. It could be possible to remove the tags manually from the translations. Not sure how long this would take..

That's how the tooltip looks with the changes:

There seems to be something wrong with the logic of the tooltip text creation, the first few words repeat itself. And it already happened prior to my changes (see the image in #3381) But I didnt had time to look very closely into this. Should I open a new issue for this problem?

I hope I didn't forget anything. I'm happy to hear your feedback!

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

Siedlerchr · 2017-11-18T12:49:22Z

First of all thanks for your contribution! Moving all logic code to gui seems like the only option here, but in general should be avoided. What about having a different approach: Keepiing all stuff string based in logic and in the GUI you have a helper method for converting the HTML tags (are only a few) from the localization into the RTF equivalent and by adding it to a textflow. I see no reason why this should not work.

If you look into the localization stuff, you see there are only a couple of <BR> and <b>/<i> tags, so nothing really advanced.
https://github.com/JabRef/jabref/blob/master/src/main/resources/l10n/JabRef_en.properties

jupf · 2017-11-18T15:18:57Z

After thinking a little longer about the package problem. I think the architecture is actually not right if the SearchDescribers generate HTML code and stay in the logic package. Because HTML code is only usable in a GUI use case and has not really anything to do with logic. So maybe they were not really in the right package to begin with if their only purpose is to create a structured text for the gui?

I think the SearchQuery objects should not hold a description stored. If it is removed, there is no further moving of classes necessary. At the places the description member of SearchQuery is used, it can be generated with the SearchDescribers class like in the SearchQuery class at the moment.

IMHO the package structure would be most honored that way

It would save the translations if the SearchDescriber creates an HTML string which can be converted later to a textflow like you said. But if the project really is moving towards javafx it seems wrong to me to keep html strings for structuring text because in javafx this is the task of a TextFlow. I would try to look into removing the HTML tags from the translations to not lose them and make a step forward using javafx

tobiasdiez · 2017-11-19T00:48:41Z

Also a big thanks from me! I don't have much time at the moment, so just a short remark concerning your point 2. I think it should be possible to completely remove the description field from the SearchQuery class. Just expose the rule (and query) via getters and then call the ̀ SearchDescriptor.getDescriptionFor(...)` every time you need the description in the gui. In this way, the description completely disappears from the logic package.

(Concerning 4: You can also embed JavaFX controls in swing, which is probably easier to do then creating a conversion to html)

jupf · 2017-11-21T13:38:36Z

Status update:

I removed the description from the SearchQuery class so it can remain in the logic package
I recreate an html string in the GroupDialog class for compatibility (was very easy this way)
modified the Tests for the SearchDescriber Implementations to properly check the TextFlow description
created Tests for the utility class I created

Now there is only one problem left: the broken localizations.
I looked into the localization files and it could be possible to rescue a lot of translations by just manually deleting the html tags from the translations (when they are at the beginning or end). But in some cases, after translating the text, the tags are positioned in the middle of words. There it would be necessary to create a new translation because I'm not able to just delete the tag from the beginning or end.

The only other alternative i see is using the old html strings for the localization and then break them up and convert them into javafx texts.. IMHO this would be really ugly and not a good readable solution

What do you think? Any other possible solutions?

Siedlerchr · 2017-11-21T19:41:25Z

When I see it right, you only need to replace the html tags for the Search Descriptions, right`?
So the rest of the translations can and should stay as it is.
Can't it be possible to strip/replace the tags on the fly? So if I encounter a in my text, I have to switch to bold face at that position?

jupf · 2017-11-26T12:50:28Z

Sadly its not so simple with TextFlows. They consist of multiple Text objects and you can set the font on each Text. So if there is a String like this:
XXX XXX XXXX XX
that translates into:
XX XXX XXX XX XXXX
Then there are three Text objects needed: Text("XX XXX"), Text("XXX"), Text("XX XXXX")

I will try to find an acceptable abstraction so the code will not be completely unreadable and the old translations can be used

* moved the search.rules.describer package from logic to gui package because the SearchDescriber now creates TextFlow Elements * moved the tests accordingly * modified creation of Tooltip in GlobalSearchBar to display the TextFlow created by the SearchDescribers * SearchQuery now stores a TextFlow as description * Added a stub for compatibility in GroupDialog because it uses the SearchQuery to get the description and needs an HTML string for its Swing Tooltip

It uses an own style class to not change any other styled elements. Styling was choosen to look like the other (swing) tooltips.

because the description of a SearchQuery is a gui element, it is not stored any more in this class. Everywhere the description is needed it is generated

now they work properly again and check the created Texts inside the description TextFlow

these old localized strings containing html tags are broken up and converted to Text objects to use in javafx TextFlow

jupf · 2017-11-26T16:36:27Z

So, now the old strings are used and the localization is working again. I tried to make it as readable as possible.

What do you think now? Any more things to do? Anything else I should have considered?

Siedlerchr · 2017-11-26T17:37:07Z

src/main/java/org/jabref/gui/util/TextUtil.java

+                break;
+            case MONOSPACED:
+                font = Font.font("Monospaced", size);
+                break;


Just add a default case.

Siedlerchr · 2017-11-26T17:45:11Z

src/main/java/org/jabref/gui/util/TextUtil.java

+    }
+
+    public static class TextReplacement {
+        String toReplace;


Please make the vars private final

tobiasdiez

Thank you very much for your work! I just have a few minor remarks before it can be merged (in my opinion).

tobiasdiez · 2017-11-26T17:31:16Z

src/main/java/org/jabref/gui/Main.css

@@ -66,6 +66,12 @@
    -fx-effect:none;
 }

+.search-tooltip {


I agree that the default JavaFX tooltips are hard to read! But I'm not a fan of the old swing tooltips either. I would suggest to use a grayish color scheme with less opacity (>= 90%): inspiration. Maybe increasing the text size (by default its 0.85em).

Please override the default .tooltip style so that tooltips look consistent.

The problem is that the TextFlow is not styled together with the tooltip its inside. So only the background can be changed with the tooltip class when using a TextFlow. It would be needed to add a style class to each used texts inside the textflow i think? then i was able to change the text font. But this would mean it would be necessary to make a text style class for each different style: bold, italic, monospaced etc. In my tests the css style had overwritten the programatically set font.

So im a little confused what to do here. It seems difficult to make a standardized look here. Depending on what is used inside the tooltip: normal text <-> TextFlow

Even if i create all the different text style classes, they had to be used explicitly in all other places. So no automatic default style possible here i think? Or is there any other possible solution i dont see?

You are talking about the font size? Using -fx-font-size does not work?
Moreover, you can influence the look of sub-items by using the appropriate css selector. For example, with .tooltip > .text you should be able to modify the look of the text elements in the tooltip. However, I have to admit that I don't really understand where you problem lies.

The selector .tooltip > .text { ... does not work here. The text element has no default style class.
I just found out I could use this: .tooltip Text { ... but then all texts have the same style (if i set the font size and color in css no text is bold or italic anymore). But we need to have some Texts with Bold, etc. properties and then the Text.setFont method for setting the font progammatically is not working anymore..

That is strange. Usually styles specified in the code take priority over css files. You could add PseudoClasses (e.g. bold) to the text element and style them via the selector .tooltip Tex :bold.

But maybe the best solution is to actually leave all text related styles at their default value and only play a bit with the background color?

That would be the easiest solution :)

But a good styling would be nice too.

I played around a bit and the following would also be possible:

.tooltip Text { /* this styles all texts inside a tooltip */ -fx-fill: rgba(255, 255, 255, 0.9); -fx-font-size: 1em; } .tooltip > TextFlow > .tooltip-text-bold { -fx-font-weight: bold; } .tooltip > TextFlow > .tooltip-text-italic { -fx-font-style: italic; } .tooltip > TextFlow > .tooltip-text-monospaced { -fx-font-family: monospace; }

So every text gets a basic styling and when you want to make one Text e.g. bold you can add the "tooltip-text-bold" style class, then additionally the text will be bold. It is even possible to combine styles like italic and bold

I think I could integrate this rather easily. I didn't look into PseudoClasses but if you say it is the best way to go I can look into them. So what should it be?

Your solution looks very clean! Go for it, if it works! (Pseudo classes are very similar, not sure what is preferable here).

tobiasdiez · 2017-11-26T17:34:16Z

src/main/java/org/jabref/gui/groups/GroupDialog.java

@@ -566,7 +571,9 @@ private void updateComponents() {
            s1 = searchGroupSearchExpression.getText().trim();
            okEnabled = okEnabled & !s1.isEmpty();
            if (okEnabled) {
-                setDescription(new SearchQuery(s1, isCaseSensitive(), isRegex()).getDescription());
+                SearchQuery searchQuery = new SearchQuery(s1, isCaseSensitive(), isRegex());
+                setDescription(fromTextFlowToHTMLString(SearchDescribers.getSearchDescriberFor(searchQuery.getRule(),


I think it makes sense to introduce an overload of getSearchDescriberFor that accepts a SearchQuery (or even change the signature directly).

tobiasdiez · 2017-11-26T17:50:50Z

src/test/java/org/jabref/gui/util/TextUtilTest.java

+        Assert.assertEquals("this<br>is a<br>test text", htmlString);
+    }
+
+    private boolean checkIfTextsEqualsExpectedTexts(List<Text> texts, List<Text> expectedTexts) {


Please extract this method to a new util class, since it is used in many tests.
Also remove the System.out statements.

Siedlerchr · 2017-11-26T17:54:11Z

src/test/java/org/jabref/gui/search/ContainsAndRegexBasedSearchRuleDescriberTest.java

+        if (expectedTexts.size() != description.getChildren().size())
+            return false;
+        Text expectedText;
+        for (int i = 0; i < expectedTexts.size(); i++) {


Better convert the two lists to sets and use streams
https://stackoverflow.com/a/30346012 (second approach)
Furthermore, as this method is also used in the other test it makes sense to extract it as one TestUtil method, to a new or an existing "testutil" class.

but the order is important for the test so I dont think a set is helpful here. Every item has to be in the right position with the correct properties.

Yeah, okay, then stick with this. But please extract it to a new class so that it can be reused across the tests

Siedlerchr

In general very good work, just some minor things.
We really appreciate your effort and your contribution

now a whole SearchQuery object is sufficient to get a corresponding SearchDescriber

also refactored the GlobalSearchBar tooltip to use this new styling

It was necessary to modify the tests of the SearchDescriber Implementations and the TooltipTextUtil class because of the new css styling for tooltips. Also refactored for better style

jupf · 2017-11-27T16:50:45Z

So, I think I made all your requested changes, it now looks like this with the new styling:

tobiasdiez · 2017-11-27T17:58:06Z

Thank you very much for the quick follow-ups and, of course, for your contribution in general. We are looking forward to see more PRs from you 😉

jupf · 2017-11-27T18:09:01Z

Your Welcome! 😁

jupf force-pushed the fix-for-issue-3381 branch from d5cdce3 to 431ede2 Compare November 26, 2017 15:58

jupf added 12 commits November 26, 2017 17:05

Styled the SearchBar Tooltip for readability

7c6d02e

It uses an own style class to not change any other styled elements. Styling was choosen to look like the other (swing) tooltips.

refactor for better readability

c29cda4

removed description member of SearchQuery class

450a1b5

because the description of a SearchQuery is a gui element, it is not stored any more in this class. Everywhere the description is needed it is generated

add method to create HTML string from javafx Text

a6badaf

create HTML string from description for compat.

a99f951

fixed formatting for style

a501172

created tests for TextUtil class

b8f1529

modified the SearchDescriber Implementations tests

6f8a66d

now they work properly again and check the created Texts inside the description TextFlow

split up testcases into single methods

714d951

using old localized strings again

6b437df

these old localized strings containing html tags are broken up and converted to Text objects to use in javafx TextFlow

added and modified tests

163bb3c

jupf force-pushed the fix-for-issue-3381 branch from 431ede2 to 163bb3c Compare November 26, 2017 16:06

added changelog entry

f0377cb

Siedlerchr reviewed Nov 26, 2017

View reviewed changes

tobiasdiez requested changes Nov 26, 2017

View reviewed changes

tobiasdiez changed the title ~~[WIP] Fix for issue 3381~~ Fix for issue #3381 Nov 26, 2017

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 26, 2017

Siedlerchr reviewed Nov 26, 2017

View reviewed changes

Siedlerchr approved these changes Nov 26, 2017

View reviewed changes

changed syntax for getting SearchDescriber

a57e902

now a whole SearchQuery object is sufficient to get a corresponding SearchDescriber

jupf added 3 commits November 27, 2017 16:45

refactored for better style

4754f80

javafx tooltips styled per css

81b4a2c

also refactored the GlobalSearchBar tooltip to use this new styling

modified and restructured tests

db0b83c

It was necessary to modify the tests of the SearchDescriber Implementations and the TooltipTextUtil class because of the new css styling for tooltips. Also refactored for better style

tobiasdiez merged commit e944c7a into JabRef:master Nov 27, 2017

halirutan mentioned this pull request Mar 10, 2018

[WIP] Main table JavaFX migration #3621

Merged

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #3381 #3443

Fix for issue #3381 #3443

jupf commented Nov 18, 2017 •

edited

Loading

Siedlerchr commented Nov 18, 2017

jupf commented Nov 18, 2017

tobiasdiez commented Nov 19, 2017

jupf commented Nov 21, 2017

Siedlerchr commented Nov 21, 2017

jupf commented Nov 26, 2017

jupf commented Nov 26, 2017

Siedlerchr Nov 26, 2017

Siedlerchr Nov 26, 2017

tobiasdiez left a comment

tobiasdiez Nov 26, 2017

jupf Nov 27, 2017

tobiasdiez Nov 27, 2017

jupf Nov 27, 2017 •

edited

Loading

tobiasdiez Nov 27, 2017

jupf Nov 27, 2017

tobiasdiez Nov 27, 2017

tobiasdiez Nov 26, 2017

tobiasdiez Nov 26, 2017

Siedlerchr Nov 26, 2017 •

edited

Loading

jupf Nov 26, 2017

Siedlerchr Nov 26, 2017

Siedlerchr left a comment

jupf commented Nov 27, 2017

tobiasdiez commented Nov 27, 2017

jupf commented Nov 27, 2017

Fix for issue #3381 #3443

Fix for issue #3381 #3443

Conversation

jupf commented Nov 18, 2017 • edited Loading

Siedlerchr commented Nov 18, 2017

jupf commented Nov 18, 2017

tobiasdiez commented Nov 19, 2017

jupf commented Nov 21, 2017

Siedlerchr commented Nov 21, 2017

jupf commented Nov 26, 2017

jupf commented Nov 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jupf Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

jupf commented Nov 27, 2017

tobiasdiez commented Nov 27, 2017

jupf commented Nov 27, 2017

jupf commented Nov 18, 2017 •

edited

Loading

jupf Nov 27, 2017 •

edited

Loading

Siedlerchr Nov 26, 2017 •

edited

Loading