RFR: JMC-6555 Convert JOverflow plugin to SWT

Fri Oct 25 10:08:33 UTC 2019

Good job!

Kind regards,
Marcus

On Fri, Oct 25, 2019 at 3:45 AM Alex Macdonald <almacdon at redhat.com> wrote:

> On Thu, Oct 24, 2019 at 4:06 PM Arvin Kangcheng Xu <kxu at redhat.com> wrote:
>
>> Hello,
>>
>> After discussing with Marcus, the following WebRev revert adding Red
>> Hat Inc to existing license headers.
>>
>> WevRev:
>> http://cr.openjdk.java.net/~jkang/jmc-6555/webrev.11/
>
>
> Great, I'll sponsor this changeset and will have it pushed shortly.
>
>
>>
>>
>> Regards,
>>
>> On Thu, 24 Oct 2019 at 14:35, Marcus Hirt <marcus.hirt at datadoghq.com>
>> wrote:
>> >
>> > No problem! Looks good to me!
>> >
>> > Kind regards,
>> > Marcus
>> >
>> > On Thu, Oct 24, 2019 at 8:10 PM Arvin Kangcheng Xu <kxu at redhat.com>
>> wrote:
>> >>
>> >> Thank you Marcus.
>> >>
>> >> I've just realized license headers are missing in a few files I added.
>> >> The following changes add headers to new files and updated the year in
>> >> the old headers.
>> >>
>> >> WebRev:
>> >> http://cr.openjdk.java.net/~jkang/jmc-6555/webrev.10/
>> >>
>> >> Sorry for not spotting this earlier.
>> >>
>> >> On Thu, 24 Oct 2019 at 07:02, Marcus Hirt <marcus.hirt at datadoghq.com>
>> wrote:
>> >> >
>> >> > Looks good to me now! :)
>> >> >
>> >> > Kind regards,
>> >> > Marcus
>> >> >
>> >> > On Mon, Oct 21, 2019 at 6:50 PM Arvin Kangcheng Xu <kxu at redhat.com>
>> wrote:
>> >> > >
>> >> > > Hello Christoph,
>> >> > >
>> >> > > Thanks for pointing that about the new 2019-09 target definition.
>> >> > >
>> >> > > This updated patch is base on current HEAD, 15a371160527 (JMC-6596:
>> >> > > Remove extra text in platform-definition-photon), with extra commit
>> >> > > messages stripped for a cleaner WebRev.
>> >> > >
>> >> > > WebRev:
>> >> > > http://cr.openjdk.java.net/~jmatsuoka/JMC-6555/webrev/
>> >> > >
>> >> > > Thanks to Joshua (jmatsuoka) for creating this WebRev.
>> >> > >
>> >> > >
>> >> > >
>> >> > > On Fri, 18 Oct 2019 at 17:03, Langer, Christoph
>> >> > > <christoph.langer at sap.com> wrote:
>> >> > > >
>> >> > > > 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,
>> >> > > > > >> > > > > >> > > >
>> >> > > > > >> > >
>> >> > > > > >>
>> >> > > >
>> >> > >
>> >>
>>
>>