Bugzilla – Full Text Bug Listing
| Summary: | [debug view] If user expands stack trace, it should re-expand after a suspended event. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] TCF | Reporter: | Pawel Piech <pawel.1.piech> | ||||||||
| Component: | Debug | Assignee: | Pawel Piech <pawel.1.piech> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Eugene Tarassov <eugene> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | Flags: | eugene:
review+
|
||||||||
| Version: | 1.0 | ||||||||||
| Target Milestone: | 1.0.1 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | 208939 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Pawel Piech
Created attachment 220917 [details]
Patch with fix.
Hi Eugene, I think the fix is fairly straightforward patch, but the logic governing the expansion itself is pretty convoluted, so if you can give the patch a quick look. (In reply to comment #2) > Hi Eugene, I think the fix is fairly straightforward patch, but the logic > governing the expansion itself is pretty convoluted, so if you can give the > patch a quick look. Hi Pawel, The expansion logic works fine on Windows, so the problem must be specific to Linux. I think I know what causes it: on Linux, SWT would clear expansion state of a tree widget element that has no children, but on Windows the state is preserved. I would call it SWT bug, but I don't think it will ever be fixed. Anyway, I don't want any changes in the expansion behavior on Windows, but the patch seems to be more invasive then that. Ideally, this kind of difference between Windows and Linux should be handled by Eclipse platform itself. If it is not an option, could you explain what exactly the patch does? In particular, expansion state must be per element, but the patch seems to use just one boolean per presentation context to save the state. How this supposed to work? I think I left out an important detail from my description. The request is to have the stack expand following a resume, or a long step. When stepping through code, the expansion state is preserved fine. However, after user resumes and the stack trace disappears, the next time the thread is suspended the stack is collapsed. This is actually desirable in some cases, since the expanded trace clutters the view, but for other cases users would like it automatically expanded. The patch adds a listener to user manually expanding and collapsing elements. It then checks whether it was a stack trace that was collapsed and saves it to a boolean. It doesn't distinguish between threads, so it either always expands or always leaves collapsed (making it per thread would be too subtle for the user to understand IMO). The setting is saved to a property in the presentation context, which in turn will serialize it on view close. The functionality overlaps somewhat with auto_expand_set, but I have to admit I don't quite understand what auto_expand_set is supposed to do. (In reply to comment #4) > I think I left out an important detail from my description. The request is > to have the stack expand following a resume, or a long step. When stepping > through code, the expansion state is preserved fine. However, after user > resumes and the stack trace disappears, the next time the thread is > suspended the stack is collapsed. Yes, I meant the same. However, the debugger does not collapse the thread node on resume, just removes the children. The node remains expanded if it was expanded before, but you cannot see it until the children are back. In other word, the debugger itself does not change expansion state, it remains whatever user choose it to be. However, on Linux, SWT appears to auto-collapse the node when all children are removed. This is SWT/Linux bug, and it requires an ugly workaround. Unfortunately, no perfect workaround exists. Whatever you do, it creates a very annoying racing condition between user, debugger and SWT, all trying to expand/collapse same node. > This is actually desirable in some cases, > since the expanded trace clutters the view, but for other cases users would > like it automatically expanded. > The patch adds a listener to user manually > expanding and collapsing elements. It then checks whether it was a stack > trace that was collapsed and saves it to a boolean. It doesn't distinguish > between threads, so it either always expands or always leaves collapsed Expanding all threads on suspend is absolutely not acceptable. If a thread is collapsed by the user, it must remain collapsed after resume/suspend. On a simulator or JTAG targets, all thread are resumed/suspended at same time, expanding all of them every time the target is suspended is not an option. > (making it per thread would be too subtle for the user to understand IMO). > The setting is saved to a property in the presentation context, which in > turn will serialize it on view close. > The functionality overlaps somewhat > with auto_expand_set, but I have to admit I don't quite understand what > auto_expand_set is supposed to do. auto_expand_set contains nodes that were expanded where they were removed. If such node is re-created, it must be created expanded. This is necessary to preserve expansion state over target reset, reverse execution, etc. (In reply to comment #5) Thanks for the clarifications. I will re-work the patch so that it preserves the expansion state per-thread. I won't try to persist it since the ID's are not persistable. I.e. it'll just be a workaround for the fact that SWT/Linux marks an element with no children as collapsed. BTW bug 208939 is opened for this issue, but it's not getting any love from the overstretched SWT team. Created attachment 220953 [details]
Updated patch.
This is an updated patch that tries to preserve the expanded state better. It replaces TCFModel.exapnded_nodes with auto_expand_user_nodes, which gets updated by user expand events.
I still don't understand completely the interaction with auto_expand_set, so it's possible that this patch may not work as expected with it. Could you give me a couple of examples of how the auto_expand_set should work? Could we duplicate any of those workflows with the diagnostic tests? If so I'll try to write some tests to validate them.
If at all possible, I'd like to squeeze this fix into 1.0 SR1 (In reply to comment #7) > Created attachment 220953 [details] > Updated patch. > > This is an updated patch that tries to preserve the expanded state better. > It replaces TCFModel.exapnded_nodes with auto_expand_user_nodes, which gets > updated by user expand events. There is regression in this change: the patch would auto-expand all new contexts regardless of reason they stopped - you use Boolean.TRUE as initial value of auto_expand_user_nodes for a context. Original code auto-expands new context only it is was stopped by something like a breakpoint. This is important usability feature that was explicitly requested by users. > > I still don't understand completely the interaction with auto_expand_set, so > it's possible that this patch may not work as expected with it. Could you > give me a couple of examples of how the auto_expand_set should work? Could > we duplicate any of those workflows with the diagnostic tests? If so I'll > try to write some tests to validate them. auto_expand_set contains nodes that were disposed while expanded. If for some reason such node is resurrected, it needs to be auto-expanded. If the node was disposed collapsed, it should stay collapsed when resurrected, unless it hits a breakpoint. It includes all nodes in the Debug view, not just threads. This is also usability feature that was explicitly requested by users. A node can be resurrected by, for example, reverse execution. You cannot reproduce this behavior with user mode agent. Essentially, it means you have to use both auto_expand_set and TCFModel.expanded_nodes to decide initial value of auto_expand_user_nodes for a context. Another problem with the patch - it is prone to racing errors. It is easy to reproduce: try expand/collapse a thread node with mouse click while holding step button (F5). With original code (at least, on Windows) it works as expected. With the patch, it fails about every second mouse click. The patch must have some logic that enables it only on Linux/GTK. On other systems, it is redundant, and should be disabled to avoid regression in usability. Created attachment 221006 [details] Updated patch targeted for linux. Here's an updated patch targeted for linux only. I still need to test it a bit, but I think it addresses the issues raised. (In reply to comment #9) > There is regression in this change: the patch would auto-expand all new > contexts regardless of reason they stopped <snip> Now I understand the special treatment for user request event type :-) It make sense. I adjusted the patch accordingly. > Essentially, it means you have to use both auto_expand_set and > TCFModel.expanded_nodes to decide initial value of auto_expand_user_nodes > for a context. I see. From what I can tell, auto_expand_set is used later (in postDelta) so it should override expanded_nodes if need be. > Another problem with the patch - it is prone to racing errors. Yep, I could reproduce with your procedure. I reduced the skit a little bit, but there is still a time lag between where model delta is created (and the expanded_nodes is read) and when the delta is consumed by the viewer. Hence the race condition. I don't think it should be a problem in practice though I agree that having the OS control remember the expanded state is much better. I have commited the patch after making changes to address couple of issues: 1. "if (is_linux)" was massplaced guarding wrong add listener. 2. "user request" flag should also be set for steps and "container suspended" events. (In reply to comment #11) Thanks! > 1. "if (is_linux)" was massplaced guarding wrong add listener. Oops, that's kind of funny :-) Do you have any objections to targeting this for 1.0 branch as well? (In reply to comment #12) > Do you have any objections to targeting this for 1.0 branch as well? No, I don't have any objections. (In reply to comment #12) > Do you have any objections to targeting this for 1.0 branch as well? Done. Thanks for all the help. http://git.eclipse.org/c/tcf/org.eclipse.tcf.git/commit/?h=1.0&id=63521f907e337fdef6ce99641b3a3efd8663df3f Comment on attachment 221006 [details]
Updated patch targeted for linux.
Marking iplog- since Pawel was a committer at the time this was attached.
|