Implemented error console in JavaFX #1383

motokito · 2016-05-11T11:42:51Z

This is the error console in JabRef being implemented in JavaFX. The Console is highly flexible in positioning and styling its components through a fxml and corresponding css stylesheet.

Also to show better the error console content (Log, Output, Exception), i have allowed to resize error console windows manually.

PS:

update the error console screenshot
new feature:
- copy log: user can copy the log
- report issue: user can report directly issues from console.
  The error console should be resizable #872: have done
Change in CHANGELOG.md described
Screenshots added (for bigger UI changes)

tobiasdiez · 2016-05-11T12:56:32Z

Please also add a screenshot for these new javafx dialogs.
Moreover, it would be nice if you could address the issues #882, #872 (I think, this is already covered with this PR...in this case please add a changelog entry) and #819, which are all connected to the error console.

Siedlerchr · 2016-05-11T13:01:08Z

Please adress the Language-key related errors, The first build failed because there were still some obsolete keys...

matthiasgeiger · 2016-05-11T13:03:53Z

@tobiasdiez @Siedlerchr slowly, slowly... let the students check this PR first and wait until it is labeled with "ready-for-review" 😉

tobiasdiez · 2016-05-27T07:06:59Z

src/main/java/net/sf/jabref/gui/errorconsole/ErrorConsoleViewModel.java

+            while (true) {
+                Thread.sleep(REFRESH_RATE);
+
+                // IMO this is a really, really bad way to do this, but a saw no other way since the stage is


Suggestion: don't use a refresh thread but data binding.
So you add a property to ErrorConsoleViewModel to return a FX-observable list of output/errormessages which are always up-to-date and then bind the text property of the textarea to it.

how can i bind the observable list to this outputStream. The problem is that all output gets written to a stream which in my opinion is incompatibe with a observable List.

You can register a custom PrintStream as a TeeStream, similar to what is done in

jabref/src/main/java/net/sf/jabref/logic/error/StreamEavesdropper.java

Line 49 in 144c03c

return new TeeStream(consoleOut, systemOut);

.
This custom PrintStream writes to a (observable) list.

new PrintStream(System.out) { List<String> messages = new blabla(); @Override public void println(String s) { messages.add(s); super.println(s); //needed? }

codecov-io · 2016-06-20T14:30:56Z

Current coverage is 28.39% (diff: 10.52%)

Merging #1383 into javafx will increase coverage by 17.61%

@@             javafx      #1383   diff @@
==========================================
  Files           705        705           
  Lines         46412      46332     -80   
  Methods           0          0           
  Messages          0          0           
  Branches       7637       7639      +2   
==========================================
+ Hits           5002      13154   +8152   
+ Misses        40963      32053   -8910   
- Partials        447       1125    +678

Powered by Codecov. Last update bc6f0c9...9e9bbc2

motokito · 2016-06-20T14:41:32Z

I am having trouble to hide the developer information. The css file probably needs some changes, I tried to set the visibilty but that didn't work out. @tobiasdiez Do you have some tips to handle this?

tobiasdiez · 2016-06-29T08:58:47Z

Sorry for my later answer. Normally you do this by using a PseudoClass inactive and change it with pseudoClassStateChanged, see for example https://github.com/JabRef/jabref/pull/1438/files#diff-9a2c5dd139caeeb5e3e3eebb9adb24b3R92. Then you can style them via .list-cell:active and .list-cell:inactive.

I would however only use the PseudoClasses for real styling purposes, for example .list-cell:error shows an error icon and sets the font color to red. Filtering should really happen on the basis of the viewmodel to which the listview binds (as you have done it right now).

For debugging UI issues, the tool Scenic View is very helpful (http://fxexperience.com/scenic-view/). I had some problems with running it separately and bind it to JabRef. However, adding it to the dependencies (Project Structure -> Modules -> Dependencies -> Add) and then invoke ScenicView.show(scene); somewhere works.

tobiasdiez · 2016-06-29T09:17:41Z

src/main/java/net/sf/jabref/gui/errorconsole/ErrorConsoleViewModel.java

+
+        developerInformation.bind(developerButton.selectedProperty());
+        developerInformation.addListener((observable, oldValue, newValue) -> {
+            ObservableList<ObservableMessageWithPriority> emptyList = FXCollections.observableArrayList(Collections.EMPTY_LIST);


I think there is no need to replace the items in allMessage, just use filteredData.setPredicate. Have a look at http://stackoverflow.com/questions/30915007/javafx-filteredlist-filtering-based-on-property-of-items-in-the-list

motokito · 2016-07-04T12:41:35Z

@tobiasdiez
I have created FilterList to filter messages. But I have one problem to view correctly entries in Listview (have a look at my Screenshot). It only runs correctly if entries run out of the Listview. I have created a Test Button to test this behavior. This Test Button generates 1 entry in System.out, throws 1 exception and generates 1 entry in the Cache. This will be removed later.
Can you help me and tell me, why Listview doesn't displays the entry correctly?

tobiasdiez · 2016-07-04T13:38:40Z

Sorry but I don't understand where your problem is. It looks like it prints the Test Cache message as well as the (first line of) the error stack trace. The system.out is ignored as I would have expected since you hide low priority messages.

motokito · 2016-07-04T17:47:51Z

Hi @tobiasdiez ,
So i will explain you, what is my problem.

The ListView shoulds show you all log entries and other entries like system.out and exception, if the button "Development Information" disable.
If the button on click, it shoulds hide all system.out and exception entries.
I have handle this will a filterlist. But the behavior by showing of entries is not correct.
Reproduct:
- Start error console
- Click on "Test" Button (1 time or more times)
- Click on "Development Information" Button
- Click on this again
  If you do so like i have written, then you will understand my problem.
  Sorry about my bad english. I hope that you understand what I mean. I need you help or some tipp how i can clear this problem :-)

Thx you

tobiasdiez · 2016-07-04T20:32:37Z

src/main/java/net/sf/jabref/logic/error/TeeStream.java

+        this.outStream = out2;
+        this.priority = priority;
+
+    }

    public TeeStream(PrintStream out1, PrintStream out2) {
        super(out1);


Call the other constructor using this(...) here.

tobiasdiez · 2016-07-04T20:57:22Z

Thanks for the additional description. I added a comment to ErrorConsoleViewModel which should help you to solve the problem.

For the next time, please try to add some tests first. There were quite a few things which could have been wrong:

Are the messages correctly passed to the T-Stream?
Do they have the correct priority?
Is filtering of the list working properly?
And finally: is is just a problem with the way how messages are displayed in the list view.

The first 3 options could have been ruled out by automatic tests. It would be good if you could add corresponding tests (which also require some positve refactoring, like making filteredList a (public) class variable and moving its initialization to the constructor).

motokito · 2016-08-01T14:54:07Z

There are three solution for this problem with create issue function:

Solution 1: Using of a GitHub API to create issue directly from JabRef with log in data, which user not like to do it. But I think, that way will be nice solution and GitHub will get more member (take a look in create issue dialog screenshot).
From customer: it will be nice this solution with a default account, but it will be not work.

Solution 2: Using OAuth2 to create issue directly from JabRef without log in data, but the user must give the application access in their account to use it, That will be difficult for some user.
Solution 3 (This is my actual implement solution): Create issue over default browser with help of saved log and exception information in clipboard. (take a look in create issue pop up screenshot).

After close or click on OK Button. The default browser will open and show create issue site with default title (automatic bug report-"current time with this format yyyyMMddHHmmss") and a default issue description (JabRef version, operating system version, Java version). During the show of this create issue site, the log and exception information will be saved in clipboard. So the user can be paste it for detail information in issue description. (take a look in create issue in default browser with paste detail information screenshot)

Braunch · 2016-08-01T14:56:50Z

Which one did you implement?

tobiasdiez · 2016-08-01T16:47:48Z

I really like the copied-to-clipboard-solution. Its simple yet effective.

Siedlerchr · 2016-08-15T13:55:26Z

Can you please post a new screenshot of the actual look?
Edit// and could you please test the look on HIDPI screens,too?

tobiasdiez · 2016-09-07T18:33:21Z

src/main/java/net/sf/jabref/logic/error/LogMessageWithPriority.java

+        this.message = message;
+    }
+
+    public void setIsFiltered(boolean isFiltered) {


Is this still used?

tobiasdiez · 2016-09-07T18:40:21Z

Thanks for the update. The code looks better now. I have just a few more remarks, then this can be merged. @koppor pls do the final review

koppor · 2016-09-07T20:39:05Z

src/main/java/net/sf/jabref/logic/logging/LogMessage.java

+/**
+ * This LogMessage will be save all message entries in a ListProperty.
+ * <p></p>
+ * This list will be use late in ErrorConsoleViewModel to filter message after their priority


Please fix English. Proposal:

This call saves all messages in a list. The list is used in ErrorConsoleViewModel to filter messages according to their priority.

motokito · 2016-09-08T12:35:01Z

@tobiasdiez @koppor: feedbacks are done. Only this teestream thing, i am not sure how to fix it 😄

koppor · 2016-09-08T20:36:06Z

Maybe someone else from @JabRef/stupro can help. @bartsch-dev maybe? It's a kind of architectural decision.

tobiasdiez · 2016-09-13T12:10:49Z

Try to use StreamEavesdropper instead of TeeStream

boceckts · 2016-09-14T15:01:40Z

@tobiasdiez Is this what you meant with moving the code to the streamEavesdropper?

tobiasdiez · 2016-09-14T15:50:30Z

src/main/java/net/sf/jabref/logic/error/StreamEavesdropper.java

+                super.write(buf, off, len);
+                String s = new String(buf, off, len);
+                addToLog(s, MessageType.EXCEPTION
+                );


Formatting.

tobiasdiez · 2016-09-14T16:14:54Z

src/main/java/net/sf/jabref/logic/error/StreamEavesdropper.java

+                );
+            }
+        };
+        return new TeeStream(consoleErr, systemErr);


If I understand it correctly, then everything from systemErr is also written to consoleErr. With your implementation all messages to consoleErr are posted to the cache (addToLog).
So far so good. But they also get posted as well as to the underlying errByteStream, which is no longer required (is it?). Thus could you remove the byte stream part?

tobiasdiez · 2016-09-14T16:48:21Z

Yes this looks better.
So now I had a look at the code. If I understand it correctly, then there already exist two ways of listen to logger messages:

GuiAppender: listens to everything which goes through the Log4j API (i.e. LOGGER.log(...)) and writes it to Cache
StreamEavesdropper: listens to everything which goes through the System streams (System.out/err) and writes it to some streams

And you added:

LogMessages: listens to everything and writes it some some internal list

Could you refactor the code so that everything is only written ̀to the internal list of LogMessages, i.e. get rid of the Cache class and the printstreams in StreamEavesdropper. (And please add code comments to the classes, it was hard to understand what they actually do)

Appendum for myself:
Since there is essentially no code which still uses System.out/err.printLn, I would actually try to get rid of the StreamEavesdropper completely. Only problem: uncaught exceptions are not written via the Log4j interface. But this could be solved by setting the default handler.

tobiasdiez · 2016-09-14T17:00:46Z

src/main/java/net/sf/jabref/logic/error/LogMessage.java

+ * <li>MessageType.EXCEPTION is define for exception entries
+ * </ul>
+ */
+public class LogMessage {


Please replace this class by org.apache.logging.log4j.core.LogEvent and Log4jLogEvent.newBuilder().setMessage(message).build() if you need to convert a string to the LogEvent.

… 15.09.2016

motokito · 2016-09-15T16:57:37Z

@tobiasdiez I have refactored and have done feedback. Hope it will be merged this time 😄

tobiasdiez

Not quite mergable but only a few final remarks. Good work.

tobiasdiez · 2016-09-15T18:18:51Z

src/main/java/net/sf/jabref/gui/errorconsole/ErrorConsoleView.java

@@ -59,12 +66,14 @@ public void show() {
        errorConsole.setDialogPane(pane);
        errorConsole.setResizable(true);
        errorConsole.show();
+        Log log = LogFactory.getLog(this.getClass());


I think this was for test purposes, right? Please remove it.

tobiasdiez · 2016-09-15T18:21:12Z

src/main/java/net/sf/jabref/gui/errorconsole/ErrorConsoleView.java

+                                    setText(logMessage.getMessage().toString());
+
+                                    Level prio = logMessage.getLevel();
+                                    if (prio.equals(ERROR)) {


pls use a switch statement here and show an info by default (instead of nothing with null/null)

tobiasdiez · 2016-09-15T18:23:08Z

src/main/java/net/sf/jabref/logic/error/StreamEavesdropper.java

+     * @param message message of log event
+     * @param priority level of log event
+     */
+    private void addToLog(String message, Level priority) {


Please go through the code that all priority and prio are properly renamed to level.

tobiasdiez · 2016-09-15T19:00:05Z

src/main/java/net/sf/jabref/logic/logging/GuiAppender.java

+        // stack traces logged by 'Log.error("message"), e' will be split by new lines so we can create a new log event for each line as 'e.printStackTrace()' would do.
+        if (event.getLevel() == Level.ERROR) {
+            Arrays.asList(message.split("\n")).stream().filter(s -> !s.isEmpty()).forEach(log -> {
+                LogEvent messageWithPriority = Log4jLogEvent.newBuilder().setMessage(new SimpleMessage(log.replace("\n", ""))).setLevel(event.getLevel()).build();


Is this kind of message-rework really necessary?
I had the hope that here a simple LogMessages.getInstance().add(event) would satisfy and then maybe use event.getMessage().getFormattedMessage() in the view (d6c8eb1#diff-d08231c1b1e93f3d52d35f72f3b55821R112).

If this is not possible, please move the code to the view, i.e. determine there how a LogEvent should be displayed.

@tobiasdiez if i don't split this it will be show me not so nice in list view. It looks like this, because in append-method of GuiAppender I get only one LogEvent entry with all information by insert a log entry like Log.error (e). It is not like System.err:

And if i don't replace System.lineseparator with "" then i will get 2 line each list cell.

That is the reason, why i have reformat this log event entry. That look better. 😄

I see. What happens if you use setText(logMessage.getMessage().getFormattedMessage()) to show the message instead of setText(logMessage.getMessage().toString()) ?

I have tried it, but i get the same effect like that screenshot 😄

Ok, then keep your code but move it to the view if this is possible.

Sorry I have tried it, but it doesn't let me extract to view. 😢. Other feedback I have done it 😄

Ok, then we leave it like that for the moment and maybe fix it later.

boceckts added ui stupro labels May 11, 2016

tobiasdiez removed the after-migration-to-javafx label May 11, 2016

tobiasdiez reviewed May 27, 2016
View reviewed changes

tschechlovdev removed the stupro-ready-for-internal-review label Jun 27, 2016

tobiasdiez reviewed Jun 29, 2016
View reviewed changes

tobiasdiez reviewed Jul 4, 2016
View reviewed changes

Braunch changed the title ~~[WIP] Implemented error console in JavaFX~~ Implemented error console in JavaFX Aug 8, 2016

Braunch added the stupro-ready-for-internal-review label Aug 8, 2016

Braunch changed the title ~~Implemented error console in JavaFX~~ [WIP] Implemented error console in JavaFX Aug 8, 2016

Siedlerchr mentioned this pull request Aug 15, 2016

Program output symbol #1742

Closed

tobiasdiez reviewed Sep 7, 2016
View reviewed changes

koppor reviewed Sep 7, 2016
View reviewed changes

motokito added 2 commits September 8, 2016 12:28

Merge remote-tracking branch 'upstream/javafx' into ErrorConsoleDialogFX

cb146de

Second time work on feedback of tobias diez and oliver kopper

9b821d7

Merge remote-tracking branch 'upstream/javafx' into ErrorConsoleDialogFX

910d9a9

Improvements

1b7eccd

tobiasdiez reviewed Sep 14, 2016
View reviewed changes

feedback of tobias diez (refactor)

0395b2c

tobiasdiez reviewed Sep 14, 2016
View reviewed changes

Refactoring - Delete TeeStream, Cache and use LogEvent incl. feedback…

d6c8eb1

… 15.09.2016

tobiasdiez requested changes Sep 15, 2016

View reviewed changes

Refactoring_19.09.2016 - Tobi Diez feedback

76d72dc

tobiasdiez merged commit 424cde8 into JabRef:javafx Sep 19, 2016

motokito deleted the ErrorConsoleDialogFX branch September 21, 2016 12:01

koppor mentioned this pull request Oct 29, 2016

Improve error console #2210

Closed

3 tasks

stefan-kolb mentioned this pull request Mar 13, 2017

Improve issue reporting a bit #2641

Merged

Implemented error console in JavaFX #1383

Implemented error console in JavaFX #1383

Conversation

motokito commented May 11, 2016 • edited Loading

tobiasdiez commented May 11, 2016

Siedlerchr commented May 11, 2016

matthiasgeiger commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 20, 2016 • edited Loading

Current coverage is 28.39% (diff: 10.52%)

motokito commented Jun 20, 2016

tobiasdiez commented Jun 29, 2016 • edited Loading

Choose a reason for hiding this comment

motokito commented Jul 4, 2016

tobiasdiez commented Jul 4, 2016 • edited Loading

motokito commented Jul 4, 2016

Choose a reason for hiding this comment

tobiasdiez commented Jul 4, 2016

motokito commented Aug 1, 2016 • edited Loading

Braunch commented Aug 1, 2016

tobiasdiez commented Aug 1, 2016

Siedlerchr commented Aug 15, 2016 • edited Loading

Choose a reason for hiding this comment

tobiasdiez commented Sep 7, 2016

Choose a reason for hiding this comment

motokito commented Sep 8, 2016

koppor commented Sep 8, 2016

tobiasdiez commented Sep 13, 2016

boceckts commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Sep 14, 2016

Choose a reason for hiding this comment

motokito commented Sep 15, 2016

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

tobiasdiez Sep 15, 2016 • 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

Choose a reason for hiding this comment

motokito commented May 11, 2016 •

edited

Loading

codecov-io commented Jun 20, 2016 •

edited

Loading

tobiasdiez commented Jun 29, 2016 •

edited

Loading

tobiasdiez commented Jul 4, 2016 •

edited

Loading

motokito commented Aug 1, 2016 •

edited

Loading

Siedlerchr commented Aug 15, 2016 •

edited

Loading

tobiasdiez Sep 15, 2016 •

edited

Loading