Non-critical mode for CR

[GabrielXia]

Contains

Fix #35, also for issue MovingBlocks/Terasology#2765 and PR MovingBlocks/Terasology#2777

What I have done

My idea is to add an new parameter boolean ifCritical to verify whether the CR in critical mode or not, so there should be some changes in Terasology code when we use CR Class. Please see PR MovingBlocks/Terasology#2777

Need Suggestions !

• There might be another way to distinguish critical mode or non-critical mode. e.g. when we register non-critical CR in the button, we use new Throwable("Report an error.") as a throwable parameter. In real crash, the throwable parameter must have differences with new Throwable("Report an error."), e.g. their messages are different. Using this way we could do less modification in code because we don't need boolean ifCriticalparameter anymore, but it may make the code less readable. What's your opinion ? or you have a better idea ? 👍
• For the part with red underline "Throwable : Report a error", I'm wondering if we need this since we don't have any exceptions in non-critical mode. If we should just delete it or replace it by another phrase ?
• Just as mentioned in MovingBlocks/Terasology#2765, we think that the CR window might better be a non modal window in order not to lock the game when we open the issue reporter. I've tried just setting the jDialog to type modeless. It do not lock the game anymore, but it don't update the log information and it can be reopened several times (we can have lots of windows in one time). I will try to get the log update and to make only one CR window appear in one time. Any suggestions are welcomed !!!

[Cervator]

Provided some feedback on the engine PR MovingBlocks/Terasology#2777 :-)

Both PRs work for me. Further improvements can be made but nothing IMHO prevents this from being merged now and further additions made later.

Pinging @skaldarnar and @msteiger for a second opinion or two.

[msteiger]

I suggest renaming it to Issue Reporter in general.

The critical and non-critical modes don't differ in terms of functionality, so I'd rather not do it.

[msteiger]

Alternatively, you could have a method that just takes a String (or even nothing) instead of a Throwable instance to differentiate between the two modes.

Maybe this could be turned in a more powerful "Severity" setting? Exceptions are the strongest one, issues are at medium severity and "feedback" could be low severity. This leaves more flexibility compared to a boolean flag.

[GabrielXia]

I love that idea :-) I've changed to Severity setting, thanks for reviewing again

[Cervator]

@msteiger & @GabrielXia - are we happy with this one and the associated engine PR?

[GabrielXia]

Hey @Cervator , sorry for the delay. I just modified the issue report to non-modal mode, and now it can update log information.

As mentioned in MovingBlocks/Terasology#2765, the short term easy fix might be done soon (if there is no big problem in the code). Could you please tell me what I can prepare for GSOC ? Should I start to work on the GSOC stuff or ?

p.s. I'll be glad if my code can be used :-)

[Cervator]

I'd be glad to see your code get used too! We're probably close right now, @msteiger do you want to provide any more feedback?

For GSOC there is like half a day left till we know if we got in or not. Then we can plan better :-)

In the meantime you can look around more in the GSOC labelled issues to see which you might find interesting to work on: https://github.com/MovingBlocks/Terasology/labels/GSOC

Some of the issues that deal with this kind of cross-project integration and niceties like better UI options maybe look especially at MovingBlocks/Terasology#1487 or MovingBlocks/Terasology#1402 or indeed MovingBlocks/Terasology#2765 like you commented on earlier. For that one maybe examine how other projects have done something similar?

[skaldarnar]

The changes look good and I like the modes. We can make another iteration to report something else than a Throwable (e.g., String).

[skaldarnar]

@msteiger I merged this for now, we can make another iteration to further improve on different modes.

@GabrielXia thanks for the contribution!

[GabrielXia]

@skaldarnar Thanks for the review. Maybe it should be merged together with MovingBlocks/Terasology#2777, since there is a modification in method parameters (don't know whether there will be compile error if it's merged alone).

[Cervator]

We'd need to release the CrashReporter then increase the version Terasology depends on. Nothing changes until we do that, the new version here is available but not in use yet :-)

I added a todo item on your PR as a reminder for us to include releasing the CR and adjusting the dependency.