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

Wed Apr 17 16:33:08 UTC 2019

Hi Guru,

On Fri, Apr 12, 2019 at 2:04 PM Guru <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/ [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.

>
> Thanks,
> Guru
>
> On 12-Apr-2019, at 7:58 PM, Alex Macdonald <almacdon at redhat.com> wrote:
>
> On Fri, Apr 12, 2019 at 10:19 AM Mario Torre <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/
> [1] https://bugs.openjdk.java.net/browse/JMC-4466
> [2] https://imgur.com/BkeXkVX
> [3] https://imgur.com/dhuTmkE
> [4] https://imgur.com/W7NzOj7
> [5] 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>
> 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/