RFR: JMC-6555 Convert JOverflow plugin to SWT

Fri Oct 18 20:17:52 UTC 2019

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/org/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/jmc/joverflow/ui/HeapDumpAction.java
>> > > > > >> > > 2019-09-10 15:31:43.946263444 -0400
>> > > > > >> > > +++ new/application/org.openjdk.jmc.joverflow.ui/src/main/java/org/openjdk/jmc/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/jmc/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,
>> > > > > >> > > >
>> > >
>>