RFR: JMC-4466 - Hide thread directly from Thread graph context menu

Alex Macdonald almacdon at redhat.com
Fri Jun 7 15:57:29 UTC 2019 

On Tue, Jun 4, 2019 at 8:35 AM Marcus Hirt <marcus.hirt at datadoghq.com>
wrote:

> Hi Alex,
>
> Thank you for looking into this. Looks good to me!
>

Great, thanks for the review! I'll push this momentarily.


>
> Kind regards,
> Marcus
>
> On Tue, Jun 4, 2019 at 2:23 PM Alex Macdonald <almacdon at redhat.com> wrote:
>
>> Hi everyone,
>>
>> On Thu, May 9, 2019 at 11:32 AM Mario Torre <neugens at redhat.com> wrote:
>>
>> > Likewise!
>> >
>> > Cheers,
>> > Mario
>> >
>>
>> Last week Marcus reported that this functionality was not working on
>> Windows [0], which also causes uitests to fail.
>>
>> I setup Eclipse on my home Windows machine and took a look into what was
>> happening. In the original JMC-4466 patch, the intention was that hovering
>> an item in the ChartCanvas would set the hovered item in a variable that
>> could be found by the threads page remove thread action. This would allow
>> for hideThread() to see where the hovered item should be removed from the
>> list of threads. When the cursor exits the chart canvas, this hovered item
>> variable would be reset [1], which is where the problem comes in. My
>> oversight here was that mouseExit() gets called when using clicking a
>> context menu item (in addition to simply moving the cursor off the chart).
>> On my Linux machines, mouseExit() gets called *after* the hide thread
>> action [2] occurs, so there isn't a problem. However, on Windows the
>> mouseExit() gets called immediately after clicking a context menu action,
>> and *before* hideThread() gets a chance to run, so the action tries to
>> remove a null thread and the chart remains unchanged.
>>
>> webrev: http://cr.openjdk.java.net/~aptmac/JMC-4466/fix-4466-0/
>>
>> The proposed fix here adds a conditional to the mouseExit() to verify that
>> the hovered item value only gets reset if the cursor it outside the bounds
>> of the chart canvas.
>>
>> Additionally, there appears to an issue with the focusing of the chart and
>> table when running uitests. As a result, the test fails to select the 7
>> threads from the table and hide them from the chart. It's interesting
>> because doesn't happen on my F29 machine, but I have seen it in Fedora 30
>> and on my Windows machine. The proposed fix for this is to ensure that JMC
>> has focus when the mouse & keyboard actions are taking place. I'm also
>> curious if this focusing issue could be related to JMC-6488, where the
>> uitests fail to open the preferences dialog. I see this on Windows (but
>> not
>> all the time), and when it occurs I can usually correct it by manually
>> hovering my mouse over top of JMC.. but this will require further
>> investigation.
>>
>> Thoughts?
>>
>> Cheers,
>>
>> Alex
>>
>> [0] https://bugs.openjdk.java.net/browse/JMC-4466
>> [1]
>>
>> http://hg.openjdk.java.net/jmc/jmc/file/e3ea3697d0b3/application/org.openjdk.jmc.ui/src/main/java/org/openjdk/jmc/ui/misc/ChartCanvas.java#l156
>> [2]
>>
>> http://hg.openjdk.java.net/jmc/jmc/file/e3ea3697d0b3/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/ThreadsPage.java#l235
>> [3] https://bugs.openjdk.java.net/browse/JMC-6488
>>
>>
>> > On Mon, May 6, 2019 at 7:34 AM Guru <guru.hb at oracle.com> wrote:
>> > >
>> > > +1, Looks good to me.
>> > >
>> > > Thanks
>> > > Guru
>> > > > On 04-May-2019, at 1:54 AM, Alex Macdonald <almacdon at redhat.com>
>> > wrote:
>> > > >
>> > > > Hi,
>> > > >
>> > > > On Wed, Apr 17, 2019 at 12:33 PM Alex Macdonald <
>> almacdon at redhat.com
>> > <mailto:almacdon at redhat.com>> wrote:
>> > > > Hi Guru,
>> > > >
>> > > > On Fri, Apr 12, 2019 at 2:04 PM Guru <guru.hb at oracle.com <mailto:
>> > guru.hb at oracle.com>> wrote:
>> > > > Hi Alex,
>> > > >
>> > > > I have tested your patch (Works as expected) and Looks good (except
>> > for Mario’s Comment for which you are addressing the same).
>> > > >
>> > > > +nit :
>> > > > Most of the places the some of the declaration is in alphabetical
>> > order and for some reason over the time its not. Wish we could spend
>> little
>> > more time to keep the uniformity going ahead.
>> > > > Please re-arange the lines accordingly. Find the same example for
>> one
>> > of the file and few more which needs to be taken care of.
>> > > > + Messages.java :
>> > > >  492         public static String ThreadsPage_HIDE_THREAD_ACTION;
>> > > >  493         public static String
>> > ThreadsPage_RESET_CHART_TO_SELECTION_ACTION;
>> > > >  494         public static String ThreadsPage_EDIT_LANES;
>> > > >
>> > > > Could have been like below
>> > > >  491         public static String ThreadDumpsPage_PAGE_NAME;
>> > > >  492         public static String ThreadsPage_EDIT_LANES;
>> > > >  493         public static String ThreadsPage_HIDE_THREAD_ACTION;
>> > > >  494         public static String ThreadsPage_LANE_TOOLTIP_TITLE;
>> > > >  495         public static String ThreadsPage_NAME;
>> > > >  496         public static String
>> > ThreadsPage_RESET_CHART_TO_SELECTION_ACTION;
>> > > >  497         public static String TlabPage_PAGE_NAME;
>> > > >
>> > > > Few more below which can be also re-aligned
>> > > >
>> > > > + flightrecorder/ui/messages/internal/messages.properties
>> > > > + ui/charts/messages.properties
>> > > > + MCJemmyBase.java
>> > > >       77 import org.jemmy.interfaces.Mouse.MouseButtons;
>> > > >
>> > > > + ThreadsPage.java:
>> > > >  comment "Adds the hide thread and reset chart actions to the
>> context
>> > menu"
>> > > >  Please update some thing like or a better one.
>> > > >  "Context menu will be udpated with thread which needs to be hiden
>> as
>> > well as resets the chart action."
>> > > >
>> > > > "the point at which to right-click and open the context menu" -->
>> > origin point of context (right-click)
>> > > >
>> > > > ChartCanvas.java :
>> > > > Please change the order of getter/setters followed by reset i.e
>> > > > (1) getActiveScopeName, (2) setActiveScopeName and (3)
>> > resetActiveScopeName instead of (1), (3) and (2).
>> > > >
>> > > > Great, thank you for the review! I've addressed the formatting
>> issues
>> > and have an updated webrev for this patch.
>> > > >
>> > > > Webrev: http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.01/ <
>> > http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.01/> [6]
>> > > >
>> > > > Previously I used string equivalence to determine which thread lane
>> to
>> > hide, however as Mario pointed out this would not work in the cases
>> where
>> > multiple threads share the same name. The rendered row on the chart does
>> > not contain information regarding the source material (other than it's
>> > name), so I've added an attribute that will hold the IMCThread
>> information
>> > so when the lane is hovered the IMCThread objects can be checked for
>> > equivalence instead. Let me know what you think about this approach.
>> > > >
>> > > > I've updated the patch to add a couple more assertions to one of the
>> > unit tests. Please let me know what you think.
>> > > >
>> > > > Webrev: http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.02/ <
>> > http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.02/> [7]
>> > > > Travis CI Build:
>> https://travis-ci.org/aptmac/jmc-qa/builds/527928758
>> > <https://travis-ci.org/aptmac/jmc-qa/builds/527928758> [8]
>> > > >
>> > > >
>> > > >
>> > > > Thanks,
>> > > > Guru
>> > > >
>> > > >> On 12-Apr-2019, at 7:58 PM, Alex Macdonald <almacdon at redhat.com
>> > <mailto:almacdon at redhat.com>> wrote:
>> > > >>
>> > > >> On Fri, Apr 12, 2019 at 10:19 AM Mario Torre <neugens at redhat.com
>> > <mailto:neugens at redhat.com>> wrote:
>> > > >>
>> > > >>> On Fri, 2019-04-05 at 13:28 -0400, Alex Macdonald wrote:
>> > > >>>> Hi,
>> > > >>>>
>> > > >>>> The following webrev [0] addresses issue JMC-4466 [1], in which
>> the
>> > > >>>> user
>> > > >>>> should be able to hide threads from the thread chart using
>> context
>> > > >>>> menu
>> > > >>>> actions.
>> > > >>>>
>> > > >>>> This patch adds functionality to the context menu to hide threads
>> > > >>>> from the
>> > > >>>> chart. Additionally, I thought there should be an option to
>> restore
>> > > >>>> the
>> > > >>>> chart to the current selection to "unhide" the threads, so I've
>> also
>> > > >>>> included a menu item that does such that. I've included a GIF
>> [2] to
>> > > >>>> demonstrate how the actions work (note the gif was made before I
>> > > >>>> touched up
>> > > >>>> some of the strings, so some of the wording may be off).
>> > > >>>>
>> > > >>>> The "hide thread" action is only enabled while there are threads
>> > that
>> > > >>>> can
>> > > >>>> be hidden, and the "reset chart" option is only enabled while the
>> > > >>>> chart has
>> > > >>>> been modified (i.e., has thread(s) hidden) [3][4]. I've also
>> > included
>> > > >>>> uitests that perform the new actions on the chart, and verify the
>> > > >>>> results
>> > > >>>> based on the enablement values of the menu items.
>> > > >>>>
>> > > >>>> Lastly, I've tested these tests and changes locally on my machine
>> > and
>> > > >>>> using
>> > > >>>> Travis [5], but these are both Linux environments and it would be
>> > > >>>> nice to
>> > > >>>> make sure there are no issues on Windows / Mac.
>> > > >>>>
>> > > >>>> Thoughts?
>> > > >>>>
>> > > >>>> Cheers,
>> > > >>>>
>> > > >>>> Alex
>> > > >>>>
>> > > >>>> [0] http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.00/ <
>> > http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.00/>
>> > > >>>> [1] https://bugs.openjdk.java.net/browse/JMC-4466 <
>> > https://bugs.openjdk.java.net/browse/JMC-4466>
>> > > >>>> [2] https://imgur.com/BkeXkVX <https://imgur.com/BkeXkVX>
>> > > >>>> [3] https://imgur.com/dhuTmkE <https://imgur.com/dhuTmkE>
>> > > >>>> [4] https://imgur.com/W7NzOj7 <https://imgur.com/W7NzOj7>
>> > > >>>> [5] https://travis-ci.org/aptmac/jmc-qa/builds/516268811 <
>> > https://travis-ci.org/aptmac/jmc-qa/builds/516268811>
>> > > >>>
>> > > >>> Hi Alex,
>> > > >>>
>> > > >>> The patch looks generally good, the only question I have is
>> regarding
>> > > >>> the thread selection by name:
>> > > >>>
>> > > >>> + private int indexOfThreadName(String name) {
>> > > >>> +   for (int i = 0; i < this.threadRows.size() && name != null;
>> i++)
>> > {
>> > > >>> +     if (this.threadRows.get(i) instanceof QuantitySpanRenderer)
>> {
>> > > >>> +       if (name.equals(((QuantitySpanRenderer)
>> > > >>> +           this.threadRows.get(i)).getName())) {
>> > > >>> +             return i;
>> > > >>> +       }
>> > > >>> +     }
>> > > >>> +   }
>> > > >>> +  return -1;
>> > > >>> + }
>> > > >>>
>> > > >>> What happens when two threads have the same name? This will make
>> > > >>> disappear the very fist one with this name encountered, not
>> > necessarily
>> > > >>> the one the user clicked on, isn't it?
>> > > >>>
>> > > >>
>> > > >> Ah good call, I hadn't thought about that case. I'll come back
>> with a
>> > > >> revised webrev.
>> > > >>
>> > > >>
>> > > >>>
>> > > >>> Cheers,
>> > > >>> Mario
>> > > >>> --
>> > > >>> Mario Torre
>> > > >>> Associate Manager, Software Engineering
>> > > >>> Red Hat GmbH <https://www.redhat.com <https://www.redhat.com/>>
>> > > >>> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
>> > > >>>
>> > > >>
>> > > >> Thanks,
>> > > >>
>> > > >> Alex
>> > > >
>> > > >
>> > > > Cheers,
>> > > >
>> > > > Alex
>> > > >
>> > > > [6] http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.01/ <
>> > http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.01/>
>> > > >
>> > > > Cheers,
>> > > >
>> > > > Alex
>> > > >
>> > > > [7] http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.02/ <
>> > http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.02/>
>> > > > [8] https://travis-ci.org/aptmac/jmc-qa/builds/527928758 <
>> > https://travis-ci.org/aptmac/jmc-qa/builds/527928758>
>> >
>> >
>> >
>> > --
>> > Mario Torre
>> > Associate Manager, Software Engineering
>> > Red Hat GmbH <https://www.redhat.com>
>> > 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
>> >
>>
>

More information about the jmc-dev mailing list