CrashReporter improvement : IssueReporter ? #2777

GabrielXia · 2017-02-07T21:38:37Z

Contains

Fixes #2765

How to test

Rename the button to "Issue Reporter".

However I still don't know why my other text on button disappear. Appreciate badly someone can tell me the reason 👍
I just changed the text value in .lang file but haven't changed others, like "crash-reporter": "Issue Reporter", I'm wondering if it's better change all "crash reporter" to "issue reporter", like "Issue-reporter": "Issue Reporter"

I've moved this button under Developer Tools menu

I'm wondering if it's necessary rename Developer Tools button to Extras

Outstanding before merging

Release CrashRreporter v4.1.0 and bump the Terasology dependency to match it
I've tried to set crash reporter window to non modal windows simply by setting set jDialog to modeless type, sadly log doesn't update @Cervator What's more, in the non modal windows, you can reopen this windows, and there can be more than one CR windows :( Other than making this window to update log, I think we should think about how to make sure there is only one CR window, like if CR window is open, press the CR button to make this windows in front of all other windows.
I think that there are some texts to be changed from "Crash Reporter" to "Issue Reporter" :
Ah it's mentioned in MovingBlocks/CrashReporter#35

GooeyHub · 2017-02-07T21:38:38Z

Can one of the admins please verify this patch?

…guish the real fatal crash or not

Cervator

This actually works perfectly fine for me - no problems with the labels, even after switching to another language and back. Maybe there is something weird going on in your workspace?

If a thorough attempt to tidy up doesn't resolve it maybe we should try to find out if there are bugs that can mess up the display like that. I didn't do anything other than "Build Project" just in case, didn't delete my config.cfg or anything

I used the embedded CR and it worked beautifully :-)

Only one tiny comment here about grammar (I think?) and that's a non-issue. We can improve it further especially the modal thing and more label fixes but I'm good to see both PRs get merged.

Edit: Oh, and yes probably "Extras" on the button, but am curious if others agree. Really I think we need to keep that button on just about any screen so it'll be consistent.

Cervator · 2017-02-10T03:09:41Z

engine/src/main/java/org/terasology/rendering/nui/layers/mainMenu/MainMenuScreen.java

@@ -56,7 +56,7 @@ public void initialise() {
        WidgetUtil.trySubscribe(this, "settings", button -> triggerForwardAnimation(SettingsMenuScreen.ASSET_URI));
        WidgetUtil.trySubscribe(this, "credits", button -> triggerForwardAnimation(CreditsScreen.ASSET_URI));
        WidgetUtil.trySubscribe(this, "exit", button -> engine.shutdown());
-        WidgetUtil.trySubscribe(this, "crashReporter", widget -> CrashReporter.report(new Throwable("Report an error."), LoggingContext.getLoggingPath()));
+        WidgetUtil.trySubscribe(this, "crashReporter", widget -> CrashReporter.report(new Throwable("Report a error."), LoggingContext.getLoggingPath(), false));


I believe "Report an error" is more correct in this case than "Report a error" - English is weird that way. You can generally go with an if the next word starts with a vovel and otherwise a but there are exceptions ...

Hey, I'm just thinking about deleting all the text "Report a error" or changing it, because there is actually no error in issue reporter.

GabrielXia · 2017-02-10T23:30:48Z

Problem found ! It's from system language setting, seems that Chinese support doesn't work well ! :(

Cervator · 2017-02-12T07:52:17Z

@GabrielXia just responded with a big comment over on #2677 - just to clarify, you picked Chinese yourself under the Player Settings, right? It didn't get picked by the game automatically from simply running on your system? I'd think it would start out English in any case :-)

msteiger · 2017-02-12T16:26:36Z

Why are you removing the button from the in-game menu?

GooeyHub · 2017-02-12T21:14:04Z

Uh oh, something went wrong with the build. Need to check on that

Cervator · 2017-02-12T21:34:23Z

Why are you removing the button from the in-game menu?

@msteiger that one is on me, I raised it as an idea over in #2765 but it was honestly more of a stray thought / suggestion I had hoped to get some more opinions on first :-)

To be clear the button just moved one level deeper, it didn't go away entirely. But I am curious what the most ideal setup there would be. Should reporting an issue be right there as a top level button users can easily spot, or grouped with other stuff under "Extras" ?

I would love to instead see the thing I mentioned over in #2765 about what @iojw did in the CR, so we can have a little warning icon in a bottom corner instead of a full button. Make it highlight if anything has occurred we think might be worth reporting. Otherwise it is just a small icon hiding in a corner, not adding much to clutter.

GabrielXia · 2017-02-12T21:36:21Z

Hello :-) I've moved this button to the Developer Tools. I think the issue reporter might rather be a tool for developer, but you are right, put it in in-game menu is more convenient for user to check. Maybe we can ask @Cervator here to discuss about where we put this button :-)

p.s. Why there is a red cross ? Scared ...

Cervator · 2017-02-13T01:47:43Z

The red x is because the build in Jenkins failed - in this case that's just the unfortunate reality of making a change that requires a new version of an external project like the CrashReporter. Since the engine build test uses the latest CR without your change there is a simple syntax error where the new thing is missing :-)

Poking @rzats for an additional opinion on the button since he does UI stuff!

rzats · 2017-02-13T15:23:55Z

engine/src/main/java/org/terasology/rendering/nui/layers/mainMenu/MainMenuScreen.java

@@ -27,6 +27,8 @@
 import org.terasology.rendering.nui.widgets.UILabel;
 import org.terasology.version.TerasologyVersion;

+import javax.print.attribute.standard.Severity;


The enum you're using is intended for a printer API and currently happens to contain the three values we need, but can be arbitrarily changed in the future. Can you replace it with a custom enum located within the CrashReporter so that we "own" the enum's code?

rzats · 2017-02-13T15:24:04Z

engine/src/main/java/org/terasology/rendering/nui/layers/ingame/PauseMenu.java

@@ -33,7 +31,6 @@ public void initialise() {
        WidgetUtil.trySubscribe(this, "settings", widget -> getManager().pushScreen("settingsMenuScreen"));
        WidgetUtil.trySubscribe(this, "mainMenu", widget -> CoreRegistry.get(GameEngine.class).changeState(new StateMainMenu()));
        WidgetUtil.trySubscribe(this, "exit", widget -> CoreRegistry.get(GameEngine.class).shutdown());
-        WidgetUtil.trySubscribe(this, "crashReporter", widget -> CrashReporter.report(new Throwable("Report an error."), LoggingContext.getLoggingPath()));


@Cervator I'd keep the button in the in-game menu. It's fairly well-hidden anyways.

Yeah I'm in no rush to move stuff around. More curious what we should be aiming for :-)

rzats · 2017-02-13T15:26:14Z

facades/PC/src/main/java/org/terasology/engine/Terasology.java

@@ -77,7 +77,7 @@
 * <tr><td>-headless</td><td>Start headless.</td></tr>
 * <tr><td>-loadlastgame</td><td>Load the latest game on startup.</td></tr>
 * <tr><td>-noSaveGames</td><td>Disable writing of save games.</td></tr>
- * <tr><td>-noCrashReport</td><td>Disable crash reporting.</td></tr>
+ * <tr><td>-noCrashReport</td><td>Disable crash reporting when having a crash.</td></tr>


No need to change the documentation - it still makes sense (the option disables crash reporting, but not issue/feedback reporting)

My thought was emphasizing the flag doesn't disable the usage of the CR for issue reporting (just on triggering actual crashes) ... but yeah this line as written kinda ends up working anyway, since the new option is indeed more about issue/feedback reporting, not crashes :-)

GooeyHub · 2017-02-13T17:20:06Z

Uh oh, something went wrong with the build. Need to check on that

GooeyHub · 2017-02-13T17:20:41Z

Uh oh, something went wrong with the build. Need to check on that

GooeyHub · 2017-03-13T23:36:15Z

Uh oh, something went wrong with the build. Need to check on that

oniatus · 2017-05-11T16:11:31Z

MovingBlocks/CrashReporter#36 has been merged some time ago. Is there something outstanding here except the new release and version bump?
The conflict in the language file seems to be pretty small.

GabrielXia · 2017-05-11T17:17:09Z

Thanks for keeping on this, I think it's ok for the moment. :-)
The enhancement of CR could be:

Show warning icon somewhere in the game when there is an error
Based on the telemetry system, maybe a new page showing all the information sent to the server

skaldarnar · 2017-07-09T19:53:32Z

I've just released a new version of CR (4.1.0). We can probably merge this after the next Alpha release.

@GabrielXia could you please update the dependency in this PR?

GooeyHub · 2017-07-09T19:54:34Z

Uh oh, something went wrong with the build. Need to check on that

GabrielXia · 2017-07-10T02:08:08Z

I'm so glad to see this to be merged :-)

@skaldarnar which dependency do you refer to? Is that the exclusion of jackson in CR: MovingBlocks/CrashReporter#39 or the one to import snowplow: #2968 ?

Minor conflicts resolved - just conflicting additions on the same line.

Cervator · 2017-07-10T03:55:17Z

I went ahead and did the version bump (just the CR itself in the engine's build.gradle from 4.0.1 to 4.1.0) while merging. Conflicts were benign as expected and it tested out fine :-)

0shine0 · 2017-07-13T20:35:53Z

@Cervator There seems to be a problem with my build. Although you bumped the version of CR, do i need to do anything else manually like download the new version jar for CR?

skaldarnar · 2017-07-13T20:49:47Z

@0shine0 running gradlew idea or gradlew jar should put you on the safe side and download the updated dependency. Be sure that you don't have the Crash Reporter checked out as source in case you run into problems.

GabrielXia added 2 commits February 2, 2017 11:28

Rename CR button and put it under dev menu

7882522

Delete two import

3fe7b95

GabrielXia mentioned this pull request Feb 8, 2017

Non-critical mode for CR MovingBlocks/CrashReporter#36

Merged

3 tasks

Add "boolean ifCritical" to CrashReporter in order that it can distin…

47571f9

…guish the real fatal crash or not

GabrielXia mentioned this pull request Feb 9, 2017

CrashReporter improvements: Full reporting system? #2765

Closed

6 tasks

Cervator approved these changes Feb 10, 2017

View reviewed changes

Cervator added the Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience label Feb 10, 2017

Change text and clarify "noCrashReport"

27949a6

GabrielXia mentioned this pull request Feb 11, 2017

Add an exclusion list for unready languages to the player settings #2784

Merged

2 tasks

Change boolean to Severity

54ab3ff

rzats reviewed Feb 13, 2017

View reviewed changes

GabrielXia added 2 commits February 13, 2017 18:17

Move issue button to In game menu

1479620

delete space

1f97db4

Change a little order of the code

385544e

Cervator merged commit 385544e into MovingBlocks:develop Jul 10, 2017

Cervator added a commit that referenced this pull request Jul 10, 2017

Merge PR #2777 by @GabrielXia - CrashReporter improvements.

878d2ad

Minor conflicts resolved - just conflicting additions on the same line.

Cervator added a commit that referenced this pull request Jul 10, 2017

Update dependency on CrashReporter to v4.1.0 (as part of PR #2777)

5d0725c

Cervator added this to the Alpha 8 milestone Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CrashReporter improvement : IssueReporter ? #2777

CrashReporter improvement : IssueReporter ? #2777

GabrielXia commented Feb 7, 2017 •

edited by Cervator

Loading

GooeyHub commented Feb 7, 2017

Cervator left a comment •

edited

Loading

Cervator Feb 10, 2017

GabrielXia Feb 10, 2017

GabrielXia commented Feb 10, 2017

Cervator commented Feb 12, 2017

msteiger commented Feb 12, 2017

GooeyHub commented Feb 12, 2017

Cervator commented Feb 12, 2017

GabrielXia commented Feb 12, 2017

Cervator commented Feb 13, 2017

rzats Feb 13, 2017 •

edited

Loading

rzats Feb 13, 2017 •

edited

Loading

Cervator Feb 13, 2017

rzats Feb 13, 2017

Cervator Feb 13, 2017

GooeyHub commented Feb 13, 2017

GooeyHub commented Feb 13, 2017

GooeyHub commented Mar 13, 2017

oniatus commented May 11, 2017

GabrielXia commented May 11, 2017

skaldarnar commented Jul 9, 2017

GooeyHub commented Jul 9, 2017

GabrielXia commented Jul 10, 2017

Cervator commented Jul 10, 2017

0shine0 commented Jul 13, 2017

skaldarnar commented Jul 13, 2017

CrashReporter improvement : IssueReporter ? #2777

CrashReporter improvement : IssueReporter ? #2777

Conversation

GabrielXia commented Feb 7, 2017 • edited by Cervator Loading

Contains

How to test

Outstanding before merging

GooeyHub commented Feb 7, 2017

Cervator left a comment • edited Loading

Choose a reason for hiding this comment

Cervator Feb 10, 2017

Choose a reason for hiding this comment

GabrielXia Feb 10, 2017

Choose a reason for hiding this comment

GabrielXia commented Feb 10, 2017

Cervator commented Feb 12, 2017

msteiger commented Feb 12, 2017

GooeyHub commented Feb 12, 2017

Cervator commented Feb 12, 2017

GabrielXia commented Feb 12, 2017

Cervator commented Feb 13, 2017

rzats Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

rzats Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Cervator Feb 13, 2017

Choose a reason for hiding this comment

rzats Feb 13, 2017

Choose a reason for hiding this comment

Cervator Feb 13, 2017

Choose a reason for hiding this comment

GooeyHub commented Feb 13, 2017

GooeyHub commented Feb 13, 2017

GooeyHub commented Mar 13, 2017

oniatus commented May 11, 2017

GabrielXia commented May 11, 2017

skaldarnar commented Jul 9, 2017

GooeyHub commented Jul 9, 2017

GabrielXia commented Jul 10, 2017

Cervator commented Jul 10, 2017

0shine0 commented Jul 13, 2017

skaldarnar commented Jul 13, 2017

GabrielXia commented Feb 7, 2017 •

edited by Cervator

Loading

Cervator left a comment •

edited

Loading

rzats Feb 13, 2017 •

edited

Loading

rzats Feb 13, 2017 •

edited

Loading