Bug 404894 - [visualizer] Make CPUs and cores selectable in multicore visualizer
Summary: [visualizer] Make CPUs and cores selectable in multicore visualizer
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf-gdb (show other bugs)
Version: 8.2   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 8.2   Edit
Assignee: Marc Dumais CLA
QA Contact: Marc Khouzam CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 405390
  Show dependency tree
 
Reported: 2013-04-04 08:31 EDT by Marc Dumais CLA
Modified: 2013-05-06 09:12 EDT (History)
4 users (show)

See Also:


Attachments
Example of enhanced selection (10.35 KB, image/png)
2013-04-10 11:52 EDT, Marc Dumais CLA
no flags Details
Enhanced selection 2 (10.25 KB, image/png)
2013-04-10 12:02 EDT, Marc Dumais CLA
no flags Details
Another example, with more (simulated) CPUs (10.27 KB, image/png)
2013-04-16 08:32 EDT, Marc Dumais CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Dumais CLA 2013-04-04 08:31:58 EDT
At the moment, the only graphical objects that are selectable in the multicore visualizer are the threads.  It would be nice to be able to select other element: cores and CPUs.   

Doing this will enable further development in the visualizer, for features that might apply to cores/CPU, like permitting the user to filter the cores/CPUs being displayed.
Comment 1 Marc Dumais CLA 2013-04-10 11:52:50 EDT
Created attachment 229572 [details]
Example of enhanced selection

Here is an example of what the selection could look-like.   Here we have 4 CPUs each having 4 cores.  CPUs 1 and 3 are selected.  In CPU0, cores 1 and 2 are selected.  In CPU3, cores 14 and 16 are selected. Some of the threads are selected also.
Comment 2 Marc Dumais CLA 2013-04-10 12:02:50 EDT
Created attachment 229573 [details]
Enhanced selection 2

Another example of the same selection, but re-using the same blue color for the CPUs and cores as is used for the thread selection, instead of the red of the previous example.   I think it's a bit more difficult to distinguish what's selected with this softer color, but it's more consistent.
Comment 3 Marc Dumais CLA 2013-04-11 13:27:35 EDT
I have uploaded a patch to gerrit for this bug, that's ready for review:

https://git.eclipse.org/r/#/c/11824/2

Here are some details about the changes : 

I have tried to seamlessly integrate the support for the new graphical objects while respecting the way things already worked :
- selecting a thread works as before
- selecting within a core but not on a thread selects the core and all contained threads.
- selecting withing a CPU but outside a core selects the CPU, all contained cores and all contained threads
- selecting outside a CPU selects all CPUs, cores and threads.
- The "add" and "toggle" selection modifiers work as before
- Selecting a region works as before, but now also selects cores and CPU objects, if within the selected region.

Synchronization with the debug view:
- since cores and CPUs are not represented in the debug view, selecting them does not have a direct effect on the debug view.  However any contained thread will be synched like before.  This makes it easy to select all threads / all threads withing a CPU or core, with one click.
- selected cores and CPUs will stay selected as long as the focus stays on the visualizer.  When the focus moves away, for example selecting the debug view, they become unselected (threads remain selected).  At first I thought this was a problem, but now I think it may be ok; those graphical elements are not represented in the debug view and will be used either as a shortcut to select threads quickly or for an immediate operation on those elements (for example: filter-to selected cores/CPUs).  

Other changes:

MulticoreVisualizerCanvas :
- decreased font size so that CPU number will fit in the margin
- increased core margin so that we have more space to select a whole CPU in one click.  
- increased core separation so that selected core stand-out more clearly
- added support for selecting CPUs and cores in a bunch of selection-related methods
- selectPoint():  I have removed support for the "addSelection - include any threads in the region bracketed by last selection click and current click".  IMHO is was a bit of an obscure way to select (I only realized it was possible looking at the code) and it was not easily extended to support cores and CPUs, in a useful way.  Furthermore, the new selection options make it easier to select multiple threads in other ways.  

MulticoreVisualizerCore:
- Made the core border a different color when selected

MulticoreVisualizerCPU:
- Made the CPU border a different color when selected
- Played with the x and y offsets so that the CPU number would be displayed in a free area, given the new margins and font size.
Comment 4 Marc Dumais CLA 2013-04-11 13:34:18 EDT
w/r to the patch submitted in this bug, I declare the following:

1) that I am authorized to submit this code change
2) that the code is 100% my own work

Marc Dumais
Comment 5 William Swanson CLA 2013-04-15 12:55:29 EDT
First off, I agree with extending selection to cores/CPUs,
that was always the intent. Thanks for doing this!

Some feedback on the selection model:

- an unmodified click outside any CPU should de-select everything,
  rather than selecting everything (selecting everything also makes
  ctrl-click behave in an unexpected way: you have things that
  you're _not_ clicking on toggling their selection state)

- allowing a click on a core/CPU to include its contents makes sense,
  but only if the cores/CPUs are considered merely as containers,
  not separate entities with their own behavior -- it could become
  problematical if we wanted to allow selections of cores/CPUs alone.
  (You'd have to select the cores/CPUs, then laboriously un-select threads.)

  I suggest that the "normal" behavior should be that clicking selects just
  the container, not its contents, and auto-selection of contents can be
  considered as a toggleable user preference. (One can, of course,
  drag-select a region to select everything in that region, as per usual.)

- Ctrl-clicking should let one toggle the state of the thing clicked;
  Currently, if you have a core/CPU and its contained thread(s) selected,
  and you ctrl-click the core/CPU, the threads are de-selected, too,
  not just the core. Same if one ctrl-clicks the thread instead, except
  here the thread's state changes but the core/cpu remains clicked.

Otherwise selection behaviour feels basically "right" to me, though it's
always possible the items noted above are obscuring a corner-case.

Regarding interaction with the Debug View, your choices make sense:
the "consuming" view (the Debug view) should select whatever makes sense
based on the selection, and going the other way, if the Debug view only allows
one to select processes/threads, then the Visualizer should update itself
to "reflect reality" and only select those processes/threads.

One could argue that the Visualizer might "helpfully" figure out what
cores/cpus the selection is on, and select those too, but (a) this would
then be inconsistent with the selection in the Debug view, and (b) this
would only be of use within the Visualizer, since other views won't see
that the cores/cpus are selected too.

Another possible option would be for the Visualizer to "notice" when
all the threads/processes on a given core or CPU are selected,
and only then auto-select, but again this means the Visualizer is
presenting a different version of reality from the selection producer.
(Maybe this is a good candidate for another user preference, though.)

Regarding selectPoint(), I'd rather you didn't remove the shift-click behavior,
since even though it _is_ obscure, it's technically part of the "standard"
behavior for shift-selection: shift-clicking an item should "extend" the current
selection to include everything in a region bounded by the current click
and the previous selection. (I simply used the last click point as a convenient
stake in the ground for determining the region to check for selectable items.)

Technically, we also should probably also be extending the selection when the
shift-click is on an item rather than the background. Not sure why I left it
like that; might be that I mentally pegged it as "good enough for now".

It should not be hard to include cores/cpus, since you're just checking
whether any cores/cpus are entirely contained in the region currently being
checked for threads to add.

Otherwise the new spacing, font sizes, use of the selection color, etc.
all looks good. Thanks again!
Comment 6 Marc Dumais CLA 2013-04-16 08:32:11 EDT
Created attachment 229759 [details]
Another example, with more (simulated) CPUs
Comment 7 Marc Dumais CLA 2013-04-16 09:44:34 EDT
Thanks Bill, for the detailed, constructive comments.  I will soon push a new version of the patch.

I think I have addressed all your comments.  Please have a look and give it a try!

Regards,

Marc
Comment 8 Marc-André Laperle CLA 2013-05-06 09:12:13 EDT
The patch set has been merged.