RFR: JMC-4466 - Hide thread directly from Thread graph context menu
Alex Macdonald
almacdon at redhat.com
Tue Jun 4 12:22:54 UTC 2019
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