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

Fri May 3 20:24:40 UTC 2019

Hi,

On Wed, Apr 17, 2019 at 12:33 PM Alex Macdonald <almacdon at redhat.com> wrote:

> 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.
>

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/ [7]
Travis CI Build: 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> 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/
>

Cheers,

Alex

[7] http://cr.openjdk.java.net/~aptmac/JMC-4466/webrev.02/
[8] https://travis-ci.org/aptmac/jmc-qa/builds/527928758