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

Mon Nov 26 19:45:49 UTC 2018

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
> >>
> >>
> >>
> >>
> >>
> >
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 5372-export.patch
Type: text/x-patch
Size: 6812 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/jmc-dev/attachments/20181126/aece57cc/5372-export.patch>