Bugzilla – Full Text Bug Listing
| Summary: | [visualizer] disable or remove the debug buttons from MV toolbar, when pinned? | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc Dumais <marc.dumais> |
| Component: | cdt-debug-dsf-gdb | Assignee: | Marc Dumais <marc.dumais> |
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> |
| Severity: | normal | ||
| Priority: | P3 | CC: | cdtdoug, pawel.1.piech, WilliamRSwanson, xraynaud |
| Version: | Next | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Bug Depends on: | 441713 | ||
| Bug Blocks: | 442584 | ||
| Attachments: | |||
|
Description
Marc Dumais
It was suggested to either disable or remove the debug-related buttons from the MV toolbar, but only when the MV instance is pinned (to a debug session). This way the debug buttons are present and work as expected in the non-pinned case, where they are enabled and act upon the currently selected debug session / thread(s), in the debug view. The problematic pinned case, where a MV is pinned to a session different from the one selected in the debug view, would then be avoided. Created attachment 246246 [details]
example of cloned/pinned MVs with the debug buttons present in the MV toolbars
In the attached screenshot we see 4 concurrent debug sessions and 5 instances of the Multicore Visualizer: each debug session has a MV pinned to it, and there is a fifth visualizer that is not pinned, and so will "follow" the session selected in the debug view. Each instance of the MV has the debug buttons in its toolbar. As it is, all those buttons follow the current debug view selection, no matter what session they are pinned-on. That can be confusing to users, that could expect that the buttons on one MV instance would act on the session/thread(s) being displayed or selected in that MV. Note that the debug buttons in the main toolbar also follow the debug view selection. So in this screenshot, we have 7 sets of debug buttons, including the optional set in the debug view (off by default, on here). Created attachment 246261 [details]
Debug buttons removed from the MV toolbar when pinned
Here is a patch where I remove the debug buttons from the toolbar when the MV is pinned. See attached screenshot #2 for an example of this in action. Gerrit review: https://git.eclipse.org/r/32177 I also have an alternative patch where I disable (gray-out) the debug buttons when pinned, instead of removing them. While doing it, I noticed the following warning in MulticoreVisualizer#updateActions(): // We should not change the enablement of the debug view // actions, as they are automatically enabled/disabled // the platform. Which is exactly what I had to do to disable the buttons. It still seemed worked - I suspect that I was just patching-over the state of the buttons set by the platform. Hi, Marc -- do you have a screenshot of the visualizer with the debug actions grayed out rather than removed that you could add as an attachment? (In reply to William Swanson from comment #7) > Hi, Marc -- do you have a screenshot of the visualizer with the debug > actions grayed out rather than removed that you could add as an attachment? Hi Bill, Sure thing. I'll upload it in a minute. > While doing it, I noticed the following warning in
> MulticoreVisualizer#updateActions():
>
> // We should not change the enablement of the debug view
> // actions, as they are automatically enabled/disabled
> // the platform.
>
> Which is exactly what I had to do to disable the buttons. It still seemed
> worked - I suspect that I was just patching-over the state of the buttons
> set by the platform.
Just a thought, is the issue that later platform state changes might un-disable them?
Created attachment 246263 [details]
MV with toolbar debug buttons disabled when pinned
(In reply to William Swanson from comment #9) > Just a thought, is the issue that later platform state changes might > un-disable them? Yes, this is a concern. In my first prototype, I had disabled the buttons in MulticoreVisualizer#populateToolbar(), and in some cases some of the buttons would become enabled again afterwards. (In reply to Marc Dumais from comment #10) > Created attachment 246263 [details] > MV with toolbar debug buttons disabled when pinned Thanks, Marc -- yeah, looking at the two I'd say go with hiding the debug actions, since it does keep the UI more clean. If down the debug options on pinned visualizers were re-enabled, the current design allows simply un-hiding them in such cases. (I'd suggest adding a comment to this effect in populateToolbar(), so that later changes preserve this as an option.) (In reply to William Swanson from comment #12) > Thanks, Marc -- yeah, looking at the two I'd say go with hiding the debug > actions, since it does keep the UI more clean. If down the debug options on > pinned visualizers were re-enabled, the current design allows simply > un-hiding them in such cases. (I'd suggest adding a comment to this effect > in populateToolbar(), so that later changes preserve this as an option.) Bill, just to be sure I understand correctly what you mean - by "hiding" you mean like in attachment #2 [details]? What I had called "removing"? Or am I misunderstanding? (In reply to Marc Dumais from comment #13) > Bill, just to be sure I understand correctly what you mean - by "hiding" you > mean like in attachment #2 [details]? What I had called "removing"? Or am I > misunderstanding? Yes, you have it right. Apologies, I called it "hiding" rather than "removing" because the buttons _could_ be there, we're just not adding them for pinned views. (In reply to William Swanson from comment #14) > Yes, you have it right. Apologies, I called it "hiding" rather than > "removing" > because the buttons _could_ be there, we're just not adding them for pinned > views. Understood, thanks Bill. I will push a new version of the patch soon. I have struggled a bit, trying to find a better way to get the toolbar to refresh after a pin action - I could not find a better way than my original hack of setting the current visualizer selection to what it already is, having as a side-effect to trigger a call to VisualizerViewer#updateUI(), which re-creates the toolbar. I have however encapsulated the hack a bit better - please have a look and comment. Do you see a better, cleaner way - ideally that would not require an API break? Thanks, Marc (In reply to William Swanson from comment #12) > [...] If down the debug options on > pinned visualizers were re-enabled, the current design allows simply > un-hiding them in such cases. (I'd suggest adding a comment to this effect > in populateToolbar(), so that later changes preserve this as an option.) Hi Bill, I am afraid I did not get this part either :) Could you please rephrase? Thanks, Marc (In reply to Marc Dumais from comment #16) > (In reply to William Swanson from comment #12) > > [...] If down the debug options on > > pinned visualizers were re-enabled, the current design allows simply > > un-hiding them in such cases. (I'd suggest adding a comment to this effect > > in populateToolbar(), so that later changes preserve this as an option.) > > I am afraid I did not get this part either :) Could you please rephrase? Sorry -- typo, that should have been :"If down _the road_ the debug options... are re-enabled...". Basically, the way you have things now supports re-adding the debug options later, so I'd suggest adding a comment to that effect, next to the if-then in populateToolbar(). (In reply to William Swanson from comment #17) > Basically, the way you have things now supports re-adding the debug options > later, > so I'd suggest adding a comment to that effect, next to the if-then in > populateToolbar(). Thanks Bill - will add the comment and push new patch. (In reply to Marc Dumais from comment #15) > I have struggled a bit, trying to find a better way to get the toolbar to > refresh after a pin action - I could not find a better way than my original > hack of setting the current visualizer selection to what it already is, > having as a side-effect to trigger a call to VisualizerViewer#updateUI(), > which re-creates the toolbar. > > I have however encapsulated the hack a bit better - please have a look and > comment. Do you see a better, cleaner way - ideally that would not require > an API break? Pinning is a property of a specific visualizer, correct? That is, in the case of the multicore visualizer, it provides a "pin" action that causes it to be pinned to a specific debug session, which implicitly causes it to change the set of actions it wants to display on its corresponding toolbar. This implies that a visualizer instance can modify its own state in such a way that the visualizer view/viewer need to update themselves accordingly. That is, VisualizerView.UpdateUI() needs to be called. At present, this only happens when: (1) the VisualizerViewer is initially created (2) the VisualizerViewer raises a VISUALIZER_CHANGED event (e.g. as a result of a different visualizer being selected due to a change in the workbench selection) (3) the current visualizer's selection changes (e.g. in response to a user action that modified the selection) The current hack is to perform a non-change of the visualizer's selection, which implicitly causes UpdateUI() to be called by path (3) above. However, it would seem a more correct approach to use path (2), and add to IVisualizer the capacity to raise a "VisualizerChanged" event of its own, which would be listened for by the VisualizerViewer, and which would cause it to raise its own VISUALIZER_CHANGED event for the VisualizerView, thus causing the view to invoke its UpdateUI() method, and updating the toolbar. I realize this means changing the IVisualizer interface, so not something we could do in a point release. How about this in the interim: - rename MulticoreVisualizer.updateUI() to the method we'd want to add, e.g. raiseVisualizerChangedEvent(), which for now just does the non-change to the selection that has the appropriate effect. - add a FIXME: comment to this method that points out it wants to raise a real event, which means we need to extend the IVisualizer interface with appropriate add/removeListener calls, and have the VisualizerViewer add/remove itself as a listener accordingly. - open a separate bug to track the requirement for a VisualizerChanged event on the IVisualizer interface, so we don't lose track of it. Does this sound reasonable? Any cases I'm missing here? (In reply to William Swanson from comment #19) > - rename MulticoreVisualizer.updateUI() to the method we'd want to add, > e.g. raiseVisualizerChangedEvent(), which for now just does the > non-change to the selection that has the appropriate effect. > > - add a FIXME: comment to this method that points out it wants to > raise a real event, which means we need to extend the IVisualizer > interface with appropriate add/removeListener calls, and have the > VisualizerViewer add/remove itself as a listener accordingly. > > - open a separate bug to track the requirement for a VisualizerChanged > event on the IVisualizer interface, so we don't lose track of it. > > Does this sound reasonable? Any cases I'm missing here? Hi Bill, Thanks for the very good, detailed description of the issue. I have opened a new bug to track the new requirement: bug 442584 . I agree with the steps above. I will submit a new patch in consequence. Committed to master |