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

[Guru]

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

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 <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/
>>> [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 <https://www.redhat.com/>>
>> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
>> 
> 
> Thanks,
> 
> Alex