[RFC] Threads Page Improvements

Thu Nov 7 19:40:16 UTC 2019

On Fri, Nov 1, 2019 at 12:23 PM Alex Macdonald <almacdon at redhat.com> wrote:
>
> Hi Jie,
>
> On Fri, Nov 1, 2019 at 10:17 AM Jie Kang <jkang at redhat.com> wrote:
>>
>> Hey Alex,
>>
>> Wow this is an extensive re-work of the page. It looks pretty neat.
>> I've put my comments below in a bullet-point sort of format. I
>> understand this is a WIP, hopefully not too many of the comments are
>> known and going to be addressed already :p
>
>
> Thanks for taking a look!
>
>>
>>
>> * The thread table can have text cutoff depending on the space given.
>
>
> The name portion to the left of the chart? It's in a sashform so it should be able to resize horizontally.

Maybe it has to do with the font sizes? On my setup, the horizontal
scroll doesn't have much movement to it so text is cutoff and a bit of
it can never be seen.

>
>>
>> It would be good to add tooltips for the text content being shown.
>> Tooltips should also be added to the buttons that inform users of the
>> purpose (in case the icons aren't obvious)
>
>
> Tooltips are a nice idea, I've recently addressed this on the GitHub repo, and can make another webrev with a more up-to-date version if required.
>
>>
>>
>> * The orange bar at the bottom overlaps with larger system font sizes.
>
>
> Nice catch.
>
>>
>>
>> * Opinion: The zoom in and out selector buttons should just function
>> as zoom in/out. The pointer selector seems like it could be scrapped
>> then while the pop-out map enable/disable button can remain. For
>> example, having to click zoom in and then click on the view, when
>> there isn't any other view to click zoom in on doesn't make sense.
>> When user testing there were many times even after I realized the
>> functionality where I tried to zoom in or out by pressing the button
>> repeatedly.
>
>
> Great feedback, and perhaps we could use an info blurb on how to use these tools better.
>
> I believe the intended reasoning behind the cursor changes is that it might make it easier to search for specific activities or change in activities. For example, if I'm looking for some specific, short-lived activity, with the zoom cursor active my workflow would involve clicking on the chart and using the zoom-pan functionality to search for whatever I'm looking for; my mouse cursor never has to leave the chart area. In this case if the button was a click-to-zoom, then I'd have to move the mouse back and forth between the zoom button every time I wanted to get a better look at something, which could be potentially tedious.
>
> There is also functionality to just hold the button and it'll continuously zoom, I think I added that to the summary notes but I know it was a wall of text and tough to digest in a short time frame. There's a tooltip coming for these zoom buttons which will specify what action happens on a short click vs. a long click.

Okay, that sounds fine to me.

>
>>
>>
>> * I believe the standard is not to add Red Hat copyright to existing
>> files, only to completely new ones
>
>
> Right, I'll take note and run through the license headers again.
>
>>
>>
>> * The filter button to set the view's duration need not exist, it
>> should just update the view either when the user enters valid input
>
>
> I think this could work well if the full time format is required as an input. From an error handling perspective I don't think this would play too nicely with your later idea of a smarter input, because of potential ambiguity of time (e.g., is 3:25 H:MM or M:SS?), and how error dialogs are currently displayed. At the moment, when the Filter button is clicked the time filters are evaluated and a specific error message is displayed in a dialog explaining why the new time is invalid. If the button disappears, either we need to rethink how we display the error message, or the dialog needs to appear only when it's certain that the format is invalid.
>
>>
>> or
>> after the user hits enter, after entering valid input
>
>
> Making the user hit enter to trigger the filter would be the same flow as having the button IMO, and in which case would probably be better to keep the button so there is no mistaking that an action has to be performed to edit the visible range.

It should be possible to evaluate and notify as necessary after user
input without having to wait for them to hit the button or the enter
key. In fact, neither are really necessary, once valid input is
received, it can immediately update the relevant UI. At the least, I'd
expect the enter key to function the same as the button.

>
>>
>>
>> * For duration entry, it could be smarter and allow somewhat invalid
>> entries that could be augmented to be valid: e.g. 3:25 -> 03:25:00:00
>
>
> That could work, and I'm guessing because we're so used to colon separation for hours & minutes that it would be okay that way.. but depending on context it could be end up being inferred as m:ss too?

I think if we pick one way and stay consistent, it's still better than
having neither.

On another related note, it seems this filter needs adjustment to
better work with events with time in PM, AM and different days. For
example. how about a recording that has 1 hour of events but between
23:59 day one and 00:59 day two? Or (maybe unlikely) a set of events
across 3 days of time.

>
>>
>>
>> * Opinion: Thread state selection should modify current Thread
>> Activity Lane profile rather than creating a new "Quick Filter".
>
>
> The reasoning behind this one is that the page by default has two filters with pre-set lanes: Java Latencies (enabled) and JVM Compiler (disabled). IMO it feels more intuitive to make a copy of the enabled lanes and add/remove from a new active filter than to add/remove from a list of predetermined ones. An example use case being if I wanted to go in and see where my threads are sleeping, I could use the dropdown to hide everything except the Thread Sleep lanes, and by making a new "quick filter" I preserve the original "Java Latencies" filter so I can toggle it back on to pick up where I was before.
>

I guess this is subjective but I don't agree that it is intuitive. If
I understand that I have profiles for thread activity lane selections
that I can create and modify, I'd expect this selection UI to
manipulate the profile that I have currently selected. There isn't any
sign to me that it will create a new 'temporary' profile that I can't
update the actual profile setup I have to match. I agree with the idea
of having a state from which I can revert back from, but I don't think
it makes sense unless it is touching the profile I have set-up where I
can choose to update to my new settings, or revert to my old settings.
E.g. writing text to a file, in vim I can choose to save and exit or
exit discarding changes.

>>
>> * The view does not horizontally scroll or fit to window size like the
>> other pages so if the user has a small window, they cannot see a large
>> portion of the threads page. Either fit to window size like the other
>> pages, or allow for appropriate scrolling.
>
>
> Good call, I'll add this to the list of tasks to re-visit.
>
>>
>>
>> * I feel like you should be able to zoom out from the "100%" view,
>> mainly to see more threads on the screen at once. Is this even useful?
>> No idea...
>
>
> That could be interesting, as in decreasing the lane heights to fit more threads on the screen? And in which case, I wonder if it would be better to have separate controls for this.

Yeah, but definitely something for another issue, if it's even deemed
appropriate. I'm actually not too sure on this idea.

>
>>
>>
>> * The zoom slider being visible but not usable is very strange. I'd
>> make it functional or drop it from the design
>
>
> There was a slider included in the original design to show zoom level, but the SWT slider being functional didn't pan out as expected. I can take a look back into it, IIRC the problem was the listener not always getting fired on every drag event when dragging up and down during a single mouse down click - so the slider could end up in a state where the thumb is at a location where the event hasn't fired, and cause a disconnect between it's supposed zoom amount and what the chart is actually showing.

Okay. Well if it can't be made usable, I think the percentage
indicator is good enough.

>
>>
>>
>> +               List<IRenderedRow> subdivision = row.getNestedRows();
>> // height 1450, has all 32 rows
>> * Can you elaborate on the comment added above.
>
>
> Whoops, that was a comment I added early on when I was trying to figure out the vertical sizing of the chart.
>
>>
>>
>> ```
>>                         resetButton.addListener(SWT.Selection, e ->
>> on.accept(false));
>> +                       resetButton.addListener(SWT.Selection, new Listener() {
>> +                               @Override
>> +                               public void handleEvent(Event event) {
>> +                                       on.accept(false);
>> +                               }
>> +                       });
>> ```
>>
>> * This added listener seems to be doing the same thing as the listener
>> added just before it. Am I misreading it this?
>
>
> Ah yes, thanks for catching that.
>
>>
>>
>> ```
>> +       public Set<String> getEnabledLanes() {
>> +               List<IItemFilter> laneFilters = laneDefs.stream()
>> +                               .filter((Predicate<? super
>> LaneDefinition>) LaneDefinition::isEnabledAndNotRestLane).map(ld ->
>> ld.getFilter())
>> +                               .collect(Collectors.toList());
>> +               return ((Types) ItemFilters.or(laneFilters.toArray(new
>> IItemFilter[laneFilters.size()]))).getTypes();
>> +       }
>> ```
>>
>> This cast makes me wonder if there's a better way to get the
>> information needed. If necessary, maybe the underlying classes can be
>> augmented to provide it?
>>
>>
>> Regards,
>> Jie Kang
>>
>>
>> On Tue, Oct 22, 2019 at 9:31 AM Alex Macdonald <almacdon at redhat.com> wrote:
>> >
>> > Hi,
>> >
>> > Demo video: https://www.youtube.com/watch?v=H_szS2gfXlo
>> >
>> > GIFs: https://imgur.com/a/KhmQThl#RWmGCbW
>> >
>> > Repo (on top of JMC GitHub mirror): https://github.com/aptmac/jmc
>> >
>> > Webrev (on top of current Mercurial repo):
>> > http://cr.openjdk.java.net/~aptmac/jfr-threads-page/webrev.00/
>> >
>> > The following work has been in-development to improve upon the usability of
>> > the JFR Threads Page.
>> >
>> > While taking a look into the JIRA ticket for improving the Threads Page
>> > [0], Mario and I discussed some of the pain-points and use-cases of the
>> > Threads page with one of our UX teams. The end result of these discussions
>> > were some mock-ups that would leverage the existing components and codebase
>> > to make the chart and table more interactable. At a high-level, the idea
>> > was to relocate the existing table to regain screen real-estate, add a
>> > secondary canvas (Text Canvas) for rendering the Thread names, and add
>> > toolbars (x2) to help filter through the information and affect how it is
>> > displayed on the screen. In an attempt to not create a wall of text here
>> > describing the specific changes I’ll include some bullet-point notes
>> > towards the end of this e-mail that will go more into the changes and
>> > additions. Additionally, I have included a demo video at the top of this
>> > e-mail that goes over the changes, and an imgur album that contains GIFs of
>> > the features shown in the video. This work has been a combined effort from
>> > myself and Jessye, so if you have any specific questions about the
>> > implementation just let us know.
>> >
>> > Currently this update runs correctly on Fedora (Linux), but there seem to
>> > be a couple issues (with regards to painting) in Windows that I’ll be
>> > investigating shortly. We also don’t have easy access to Mac machines at
>> > the moment, so I’d be curious to see what it looks like and if it needs
>> > further modifications there too.
>> >
>> > For the most part the development of the functional elements is wrapped-up
>> > and we’re working on cleanup and testing. It’d be nice to have some
>> > feedback prior to writing a bunch of uitests just in case things have to be
>> > modified.
>> >
>> > These updates to the Threads Page also happen to coincide with (and
>> > address) a number of outstanding issues on the JIRA, such as:
>> >
>> > [0] JMC-6364: Improvements to the Thread Graph (Tracking bug)
>> >
>> > [1] JMC-4475: Zoom out to full range in chart is not obvious
>> >
>> >    -
>> >
>> >    SWT Scale in the display bar indicates current zoom power
>> >
>> > [2] JMC-5001: Difficult to unselect in graph
>> >
>> >    -
>> >
>> >    The ESC key now un-highlights all lanes
>> >
>> > [3] JMC-5301: Shorter events hidden if drawn in same pixels as a long one
>> >
>> >    -
>> >
>> >    Not completely addressed, but can toggle activity visibility to find
>> >    hidden events
>> >
>> > [4] JMC-6165: Missing thread names in graph view (Thread view)
>> >
>> >    -
>> >
>> >    There’s now a fixed amount of vertical space for each lane; the names
>> >    are always visible
>> >
>> > [5] JMC-6366: [JFR] Event graph, make label area resizable
>> >
>> >    -
>> >
>> >    Text and Chart canvases connected by SashForm; no more distinct SWT
>> >    table & AWT chart
>> >
>> > [6] JMC-6368: Foldable threads table in the Threads page
>> >
>> >    -
>> >
>> >    Table converted to a popup table to save on screen real-estate
>> >
>> >
>> > Summary of changes:
>> >
>> > - New: Chart Filter Bar (Top)
>> >
>> > - - Controls: time filters, reset button, dropdown thread activity filter
>> >
>> > - New: Dropdown Thread Activity Filter
>> >
>> > - - Dropdown combobox that allows for toggling of visible lane activity
>> >
>> > - New: Time Filter
>> >
>> > - - Displays for the start and end time of the visible range
>> >
>> > - - Can input specific start & end times to update the visible range
>> >
>> >
>> >
>> > - New: Chart Display Bar (Right)
>> >
>> > - - Controls: selection, zoom-in, zoom level indicator, zoom-out, zoom-pan,
>> > and scale-to-fit
>> >
>> > - New: Zoom-In Mode
>> >
>> > - - Zoom in chart incrementally on chart click
>> >
>> > - - Zoom in chart continuously on long press of button
>> >
>> > - - Zoom to selection automatically by highlighting area on chart
>> >
>> > - New: Zoom Level Indicator
>> >
>> > - - Vertical SWT Scale + Label to show the current level of zoom
>> >
>> > - New: Zoom-Out Mode
>> >
>> > - - Zoom out chart incrementally on chart click
>> >
>> > - - Zoom out chart continuously on long press of button
>> >
>> > - New: Zoom-Pan Functionality
>> >
>> > - - Moving the display moves the location of the chart view
>> >
>> > - Changed: Timeline Display
>> >
>> > - - Rendered timeline display previously located below the chart has been
>> > relocated to its own component
>> >
>> > - - The orange range indicator can be moved horizontally to pan the chart
>> > view
>> >
>> > - Changed: Chart Canvas
>> >
>> > - - Chart moved into a scrolled composite that allows for vertical
>> > scrolling from the chart canvas, horizontal scrolling from the timeline
>> > canvas
>> >
>> > - Changed: Text Canvas
>> >
>> > - - ESC key can be pressed to remove lane highlighting
>> >
>> > - Changed: Zoom & Pan Mechanics
>> >
>> > Let us know what you think!
>> >
>> > Cheers,
>> >
>> > Alex
>> >
>> > [0] https://bugs.openjdk.java.net/browse/JMC-6364
>> >
>> > [1] https://bugs.openjdk.java.net/browse/JMC-4475
>> >
>> > [2] https://bugs.openjdk.java.net/browse/JMC-5001
>> >
>> > [3] https://bugs.openjdk.java.net/browse/JMC-5301
>> >
>> > [4] https://bugs.openjdk.java.net/browse/JMC-6165
>> >
>> > [5] https://bugs.openjdk.java.net/browse/JMC-6366
>> >
>> > [6] https://bugs.openjdk.java.net/browse/JMC-6368