JMC-5372: Exception printed on page when opening invalid recording

Mon Nov 26 22:11:34 UTC 2018

Hi Alex,

Excellent! Thank you for your contribution!

Kind regards,

Marcus

From: Alex Macdonald <almacdon at redhat.com>
Date: Monday, 26 November 2018 at 20:46
To: Marcus Hirt <marcus.hirt at oracle.com>
Cc: Joshua Matsuoka <jmatsuok at redhat.com>, <jmc-dev at openjdk.java.net>
Subject: Re: JMC-5372: Exception printed on page when opening invalid recording

Great! Please find attached my exported patch, ready for pushing by someone with permissions.

Cheers,

Alex

On Mon, Nov 26, 2018 at 1:37 PM Marcus Hirt <marcus.hirt at oracle.com> wrote:

Looks good to me!

Kind regards,

Marcus

From: Alex Macdonald <almacdon at redhat.com>
Date: Monday, 26 November 2018 at 19:29
To: Marcus Hirt <marcus.hirt at oracle.com>
Cc: Joshua Matsuoka <jmatsuok at redhat.com>, <jmc-dev at openjdk.java.net>
Subject: Re: JMC-5372: Exception printed on page when opening invalid recording

Hi Marcus & Josh,

Thanks for taking another look at this. I've added the string into the messages.properties in this amended patch.

Cheers,

Alex

On Mon, Nov 26, 2018 at 5:47 AM Marcus Hirt <marcus.hirt at oracle.com> wrote:

Hi Josh,

Yes, it would make sense to externalize the strings.

Kind regards,

Marcus

From: Joshua Matsuoka <jmatsuok at redhat.com>
Date: Friday, 23 November 2018 at 17:07
To: Alex Macdonald <almacdon at redhat.com>
Cc: Marcus Hirt <marcus.hirt at oracle.com>, <jmc-dev at openjdk.java.net>
Subject: Re: JMC-5372: Exception printed on page when opening invalid recording

Hi Alex,

-    public static final String INVALID_RECORDING_DIALOG_TITLE = "Page could not display data"; //$NON-NLS-1$
-

-    public static final String INVALID_RECORDING_DIALOG_TEXT = "The page cannot be shown correctly for this recording. The recording may be from an old JVM, or invalid in some other way."; //$NON-NLS-1$
+    public static final String INVALID_RECORDING_TEXT = "The page cannot be shown correctly for this recording. The recording may be from an old JVM, or invalid in some other way."; //$NON-NLS-1$

minor nit/question for Marcus: Should this message be localized in the same way as all of the other UI messages to keep things consistent? I.E. adding them to messages.properties and handling them in the usual way.

Cheers,

- Josh

On Tue, Nov 20, 2018 at 4:36 PM Alex Macdonald <almacdon at redhat.com> wrote:

Ping - I sent this out about a week ago, let me know what you think.

On Wed, Nov 14, 2018 at 4:12 PM Alex Macdonald <almacdon at redhat.com> wrote:

> Hi Marcus,
>
> On Tue, Nov 13, 2018 at 7:59 PM, Marcus Hirt <marcus.hirt at oracle.com>
> wrote:
>
>> Hi Alex,
>>
>> Personally I am a bit allergic to modal dialogs. For example, if we have
>> multiple recordings ending with problems, I'd rather have the editors
>> opening
>> with details than one or more modal dialogs. That is, of course, a
>> personal
>> preference that I'd be happy to discuss here. That said, I think what is
>> shown,
>> and how it is shown, in the editor could be improved. For example we
>> should
>> show the error title and message (that would have been showed in the
>> modal
>> dialog) first, followed by the stack trace. Possibly not throwing the
>> stack trace in the user's face until it is asked for.
>>
>
> That sounds fair enough to me. I've taken what you said into account and
> made a couple of modifications to the error page.
>
> I've transferred the text explaining the error from the dialog to the
> error page, and removed the dialog modal. The dialog title text was very
> similar to the current error message printed on the screen, so I've only
> kept the latter to avoid redundancy. In response to your comment about
> showing the stack trace when it's asked for, the stack trace is now hidden
> under a expandable component so it can be collapsed & expanded as desired.
>
> I've included some images to show these changes.
>
> Error page (gif): https://imgur.com/g8hpXlg
> Error page (collapsed): https://imgur.com/irTarA6
> Error page (expanded): https://imgur.com/M51z8ct
>
> Cheers,
>
> Alex
>
>
>> Please let me know what you think!
>>
>> Kind regards,
>> Marcus
>>
>> On 2018-11-13, 21:45, "jmc-dev on behalf of Alex Macdonald" <
>> jmc-dev-bounces at openjdk.java.net on behalf of almacdon at redhat.com> wrote:
>>
>>     Hi,
>>
>>     This short patch addresses JMC-5372 [0], in which the error dialog is
>> not
>>     displayed when an exception is thrown in the JFR Editor.
>>
>>     As far as I can tell, the error here is within the catch block of the
>>     "displayPage" function in the JFR Editor [1]. When an exception is
>> caught
>>     there is an evaluation of the boolean property for
>> "showModalErrorDialog"
>>     [2], however I cannot find the instance where this property would be
>>     toggled from false to true. As a result, the if-statement takes the
>> branch
>>     that displays the error page, but not the error dialog. The proposed
>> fix
>>     here removes the check for the boolean (because we have already
>> caught the
>>     exception), and instead opt to display both the error page and the
>> modal.
>>
>>     I've included a couple of images to show the result of this patch
>> [3], as
>>     well as gifs showing the before [4] & after [5] experience.
>>     Before (gif): https://imgur.com/hHRmkx3 [3]
>>     After (gif): https://imgur.com/8rtaysS [4]
>>     After (img): https://imgur.com/0oBi6nH [5]
>>
>>     Thoughts?
>>
>>     Cheers,
>>
>>     Alex
>>
>>     [0] https://bugs.openjdk.java.net/browse/JMC-5372
>>     [1]
>>
>> http://hg.openjdk.java.net/jmc/jmc/file/a76a464b3764/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/JfrEditor.java#l240
>>     [2]
>>
>> http://hg.openjdk.java.net/jmc/jmc/file/a76a464b3764/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/JfrEditor.java#l243
>>     [3] https://imgur.com/hHRmkx3
>>     [4] https://imgur.com/8rtaysS
>>     [5] https://imgur.com/0oBi6nH
>>
>>
>>
>>
>>
>