RFR: JMC-6555 Convert JOverflow plugin to SWT

Fri Oct 18 21:02:54 UTC 2019

Hi Arvin,

after JMC-6595, you should also adapt the new 2019-09 target definition.

Furthermore, your webrev is really hard to read. Can you maybe change the patch text to something more concise which would allow to fit several modified files on one page?

Thanks
Christoph

> -----Original Message-----
> From: jmc-dev <jmc-dev-bounces at openjdk.java.net> On Behalf Of Arvin
> Kangcheng Xu
> Sent: Freitag, 18. Oktober 2019 22:18
> To: Marcus Hirt <marcus.hirt at datadoghq.com>
> Cc: jmc-dev at openjdk.java.net
> Subject: Re: RFR: JMC-6555 Convert JOverflow plugin to SWT
> 
> Hello Marcus,
> 
> Thanks for the feedback. The changes you requested are now implemented.
> 
> Changelog:
>   - reintroduce referrer direction arrows
>   - extend pie chart slices upon mouse hovering over tables
>   - remove extra padding
>   - enable anti-aliasing and double buffering
>   - suppress warnings for raw types
> 
> This patch applies on a31b291 (JMC-6595: Add Eclipse 2019-09 target
> definition).
> 
> WevRev:
> http://cr.openjdk.java.net/~jkang/jmc-6555/webrev.08/
> 
> Regards,
> 
> 
> On Tue, 15 Oct 2019 at 18:45, Marcus Hirt <marcus.hirt at datadoghq.com>
> wrote:
> >
> > Hi Arvin,
> >
> > It mostly works for me now! Good job!
> >
> > Some comments:
> > * The referrer direction arrow got lost. I think it should be reintroduced.
> > * Mouse over on rows in the lower tables used to extend the
> corresponding slice in the pie - that's pretty nice, but can be added as a later
> enhancement if there isn't enough time.
> > * I think we should skip the extra padding around the tables and around
> the lower pie + table combos
> > * The pie charts look very blocky on Windows, is anti-aliasing on?
> >
> > Some nits:
> > For PieChartViewer.getSelectionFromWidget, either add annotation
> @SuppressWarnings("rawtypes"), or return List<?>.
> > For PieChartViewer.setSelectionToWidget, add annotation
> @SuppressWarnings("rawtypes").
> >
> > Kind regards,
> > Marcus
> >
> > On Fri, Oct 11, 2019 at 11:34 PM Arvin Kangcheng Xu <kxu at redhat.com>
> wrote:
> >>
> >> Hi all,
> >>
> >> As documented on JMC-6499 [0], only platform-definition-photon can be
> >> imported into Eclipse for development on OSX due to a long-standing
> >> eclipse bug. For some reason, Eclipse Photon has different behaviours
> >> and causes JOverflow UI not being laid out immediately after its
> >> creation. This bug is Photon-specific, and exists also on Linux.
> >>
> >> This is not an issue if built with the latest
> >> platform-definition-2019-06, but I've included a workaround
> >> specifically for Photon. See JOverflowEditor.java:218 [1] in the
> >> updated WebRev for detail.
> >>
> >> WebRev:
> >> http://cr.openjdk.java.net/~jkang/jmc-6555/webrev.07
> >>
> >> Thanks to Jie for updating this WebRev.
> >>
> >> Regards,
> >>
> >> [0] https://bugs.openjdk.java.net/projects/JMC/issues/JMC-6499
> >> [1] http://cr.openjdk.java.net/~jkang/jmc-
> 6555/webrev.07/application/org.openjdk.jmc.joverflow.ui/src/main/java/or
> g/openjdk/jmc/joverflow/ui/JOverflowEditor.java.html
> >>
> >>
> >>
> >> On Thu, 10 Oct 2019 at 04:58, Marcus Hirt <marcus.hirt at datadoghq.com>
> wrote:
> >> >
> >> > Sent it to you directly Arvin.
> >> >
> >> > Kind regards,
> >> > Marcus
> >> >
> >> > On Tue, Oct 8, 2019 at 5:27 PM Arvin Kangcheng Xu <kxu at redhat.com>
> wrote:
> >> > >
> >> > > I now have access to a MacBook running macOS Mojave, but I wasn't
> able
> >> > > to reproduce the problem you encountered. May I have more
> information
> >> > > on your environment?  macOS version, JDK version/distribution and
> >> > > such.
> >> > >
> >> > > Regards,
> >> > >
> >> > > On Thu, 3 Oct 2019 at 09:34, Arvin Kangcheng Xu <kxu at redhat.com>
> wrote:
> >> > > >
> >> > > > No. I don't have access to a Mac or Windows machine at the
> moment.
> >> > > >
> >> > > > On Wed, 2 Oct 2019 at 16:42, Marcus Hirt
> <marcus.hirt at datadoghq.com> wrote:
> >> > > > >
> >> > > > > Have you tried running this on Mac?
> >> > > > >
> >> > > > > /M
> >> > > > >
> >> > > > >
> >> > > > > On Wed, Oct 2, 2019 at 9:42 PM Marcus Hirt
> <marcus.hirt at datadoghq.com> wrote:
> >> > > > > >
> >> > > > > > Excellent! Will take a look!
> >> > > > > >
> >> > > > > > /M
> >> > > > > >
> >> > > > > > On Wed, Oct 2, 2019 at 4:07 PM Arvin Kangcheng Xu
> <kxu at redhat.com> wrote:
> >> > > > > >>
> >> > > > > >> Hello all,
> >> > > > > >>
> >> > > > > >> This is the latest updated patch. This is the latest patch that
> includes a fix
> >> > > > > >> for the lazy table viewer. I rebased and made sure it applies to
> the current
> >> > > > > >> HEAD, 198:ceedb367dc18.
> >> > > > > >>
> >> > > > > >> Webrev:
> >> > > > > >> https://cr.openjdk.java.net/~jkang/jmc-6555/webrev.05/
> >> > > > > >>
> >> > > > > >> Thanks to Jie for creating this webrev.
> >> > > > > >>
> >> > > > > >> Regards,
> >> > > > > >>
> >> > > > > >>
> >> > > > > >> On Wed, 18 Sep 2019 at 09:53, Arvin Kangcheng Xu
> <kxu at redhat.com> wrote:
> >> > > > > >> >
> >> > > > > >> > On Tue, 17 Sep 2019 at 13:02, Jie Kang <jkang at redhat.com>
> wrote:
> >> > > > > >> > >
> >> > > > > >> > > On Tue, Sep 10, 2019 at 4:02 PM Arvin Kangcheng Xu
> <kxu at redhat.com> wrote:
> >> > > > > >> > > >
> >> > > > > >> > > > This is the latest updated patch for fixes of the following,
> as
> >> > > > > >> > > > mentioned earlier in this thread:
> >> > > > > >> > > >
> >> > > > > >> > > >  - wildcard imports were replaced with single class imports
> >> > > > > >> > > >  - unnecessary white spaces were removed
> >> > > > > >> > > >  - indentations were changed to using tabs instead of
> spaces
> >> > > > > >> > > >  - removed mIsUpdatingModel guard
> >> > > > > >> > > >  - removed getHeapSize and mHeapSize in BaseViewer
> >> > > > > >> > > >  - declared setHeapSize in BaseViewer abstract
> >> > > > > >> > > >  - initialized mHeapSize to 1 to avoid division by zero
> >> > > > > >> > > >  - numbers are now rounded instead of truncated
> >> > > > > >> > > >  - number displays are now comma-separated
> >> > > > > >> > > >  - removed global jfx dependencies (javafx.osgi, p2 repo,
> target platforms)
> >> > > > > >> > > >  - refactored sub-component calls
> >> > > > > >> > > >  - used a more contrasting color palette for pie charts
> >> > > > > >> > > >  - fixed rotating table color
> >> > > > > >> > > >  - memory column displays 2 decimal places. add tooltips
> >> > > > > >> > > >  - fixed JavaThingPage NPE
> >> > > > > >> > > >
> >> > > > > >> > > > Spotbug config typos were resolved in another issue and
> are now fixed and push.
> >> > > > > >> > > >
> >> > > > > >> > > > Webrev:
> >> > > > > >> > > > http://cr.openjdk.java.net/~aptmac/JMC-
> 6555/webrev.03/
> >> > > > > >> > >
> >> > > > > >> > > Hey Arvin,
> >> > > > > >> > >
> >> > > > > >> > > Thanks for your continued efforts here! A few more small
> things below from me:
> >> > > > > >> > >
> >> > > > > >> > > ---
> old/application/org.openjdk.jmc.joverflow.ui/src/main/java/org/openjdk/j
> mc/joverflow/ui/HeapDumpAction.java
> >> > > > > >> > > 2019-09-10 15:31:43.946263444 -0400
> >> > > > > >> > > +++
> new/application/org.openjdk.jmc.joverflow.ui/src/main/java/org/openjdk/j
> mc/joverflow/ui/HeapDumpAction.java
> >> > > > > >> > > 2019-09-10 15:31:43.864262593 -0400
> >> > > > > >> > > -import java.io.File;
> >> > > > > >> > > -
> >> > > > > >> > > -import javax.management.MBeanServerConnection;
> >> > > > > >> > > -import javax.management.ObjectName;
> >> > > > > >> > > -
> >> > > > > >> > > import org.eclipse.jface.dialogs.InputDialog;
> >> > > > > >> > > import org.eclipse.jface.window.Window;
> >> > > > > >> > > import org.eclipse.swt.SWT;
> >> > > > > >> > > import org.eclipse.swt.widgets.Display;
> >> > > > > >> > > import org.eclipse.swt.widgets.FileDialog;
> >> > > > > >> > > -
> >> > > > > >> > > import org.openjdk.jmc.common.io.IOToolkit;
> >> > > > > >> > > import org.openjdk.jmc.rjmx.IConnectionHandle;
> >> > > > > >> > > import org.openjdk.jmc.rjmx.IServerHandle;
> >> > > > > >> > > @@ -56,10 +50,14 @@
> >> > > > > >> > > import org.openjdk.jmc.ui.misc.DialogToolkit;
> >> > > > > >> > > import org.openjdk.jmc.ui.misc.DisplayToolkit;
> >> > > > > >> > > +import javax.management.MBeanServerConnection;
> >> > > > > >> > > +import javax.management.ObjectName;
> >> > > > > >> > > +import java.io.File;
> >> > > > > >> > >
> >> > > > > >> > > The import order change here should not be made. This
> applies
> >> > > > > >> > > elsewhere in the patch. I'm guessing an auto-formatter was
> used but
> >> > > > > >> > > it's configuration for imports wasn't set correctly. If it's the
> >> > > > > >> > > configuration included in the jmc repository that's a minor
> issue that
> >> > > > > >> > > could be addressed separately.
> >> > > > > >> >
> >> > > > > >> > I mainly use another IDE for development and imported
> included Eclipse
> >> > > > > >> > formatting configuration there. It seems the imported config
> from Eclipse is
> >> > > > > >> > not well-supported. I've reorganized class imports for all files
> with Eclipse.
> >> > > > > >> >
> >> > > > > >> > > --- /dev/null 2019-09-10 09:22:37.353999759 -0400
> >> > > > > >> > > +++
> new/application/org.openjdk.jmc.joverflow.ui/src/main/java/org/openjdk/j
> mc/joverflow/ui/JavaThingPage.java
> >> > > > > >> > >  2019-09-10 15:31:50.541331853 -0400
> >> > > > > >> > > @@ -0,0 +1,134 @@
> >> > > > > >> > >
> >> > > > > >> > > ++public class JavaThingPage extends Page implements
> ModelListener {
> >> > > > > >> > > + private final JOverflowEditor mEditor;
> >> > > > > >> > > + private JavaThingTreeViewer mTreeViewer;
> >> > > > > >> > >
> >> > > > > >> > > JavaThingTreeViewer is defined as "public class
> JavaThingTreeViewer<T
> >> > > > > >> > > extends JavaThingItem> extends TreeViewer {". Can
> mTreeViewer instance
> >> > > > > >> > > by typed to "JavaThingTreeViewer<JavaThingItem>"?
> >> > > > > >> >
> >> > > > > >> > Yes. mTreeViewer instance is now typed to
> JavaThingTreeViewer<JavaThingItem>.
> >> > > > > >> >
> >> > > > > >> > Since these changes are a bit trivial. I'd refrain from spamming
> the
> >> > > > > >> > mail list with
> >> > > > > >> > another patch. These changes will be included in my next
> update. Thank you
> >> > > > > >> > very much!
> >> > > > > >> >
> >> > > > > >> > > Apart from these, the patch looks pretty good!
> >> > > > > >> > >
> >> > > > > >> > >
> >> > > > > >> > > Regards,
> >> > > > > >> > >
> >> > > > > >> > >
> >> > > > > >> > > >
> >> > > > > >> > > > Thanks to Alex for creating this webrev.
> >> > > > > >> > > >
> >> > > > > >> > > > Regards,
> >> > > > > >> > > >
> >> > >
> >>