[RFC] Threads Page Improvements
Alex Macdonald
almacdon at redhat.com
Fri Nov 1 16:22:49 UTC 2019
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.
> 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.
>
> * 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.
>
> * 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?
>
> * 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.
> * 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.
>
> * 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.
>
> + 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
>
More information about the jmc-dev
mailing list