Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 333778

Summary: [Compatibility] 'Breakpoints' view auto shows selection
Product: [Eclipse Project] Platform Reporter: Remy Suen <remy.suen>
Component: DebugAssignee: Pawel Piech <pawel.1.piech>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: daniel_megert, pawel.1.piech
Version: 3.7Flags: daniel_megert: review+
Target Milestone: 3.7 M7   
Hardware: PC   
OS: Windows XP   
See Also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=301118
Whiteboard:
Attachments:
Description Flags
Fix attempt. daniel_megert: review+

Description Remy Suen CLA 2011-01-07 13:05:41 EST
Leapin' lizards, this is driving me mad!

1. Create several breakpoints so that a scrollbar appears.
2. Select the last breakpoint.
3. Scroll back up to the top.
4. Check/uncheck the top one.
5. The viewer will autoscroll show the selection at the bottom!
Comment 1 Remy Suen CLA 2011-01-07 13:11:00 EST
May be related to bug 302259.

4.x trace as follows:

Thread [main] (Suspended (breakpoint at line 360 in TreeViewer))	
	TreeModelViewer(TreeViewer).showItem(Item) line: 360	
	TreeModelViewer(AbstractTreeViewer).reveal(Object) line: 2241	
	BreakpointsView(VariablesView).revealTreeSelection() line: 871	
	BreakpointsView(VariablesView).showDetailPane() line: 857	
	BreakpointsView(VariablesView).buildDetailPane(int) line: 832	
	BreakpointsView(VariablesView).refreshDetailPaneContents() line: 1125	
	VariablesView$9.selectionChanged(SelectionChangedEvent) line: 1053	
	StructuredViewer$3.run() line: 867	
	SafeRunner.run(ISafeRunnable) line: 42	
	JFaceUtil$1.run(ISafeRunnable) line: 49	
	SafeRunnable.run(ISafeRunnable) line: 175	
	TreeModelViewer(StructuredViewer).firePostSelectionChanged(SelectionChangedEvent) line: 865	
	TreeModelViewer(StructuredViewer).handlePostSelect(SelectionEvent) line: 1205	
	StructuredViewer$5.widgetSelected(SelectionEvent) line: 1230	
	OpenStrategy.firePostSelectionEvent(SelectionEvent) line: 252	
	OpenStrategy.access$5(OpenStrategy, SelectionEvent) line: 246	
	OpenStrategy$3.run() line: 423	
	RunnableLock.run() line: 35	
	UISynchronizer(Synchronizer).runAsyncMessages(boolean) line: 134	
	Display.runAsyncMessages(boolean) line: 4059
Comment 2 Remy Suen CLA 2011-01-07 13:40:43 EST
Hahaha, oh wow. This seems to be related to the background colour of the view which decides whether it should recreate controls or not. This recreation process seems to cause the selection to be revealed.
Comment 3 Remy Suen CLA 2011-01-07 15:24:06 EST
This code seems to be potentially volatile based on what colours the user has for their background. Is there a better way of deciding when the view should rebuild its details pane?
Comment 4 Remy Suen CLA 2011-03-11 08:08:47 EST
This bug drives me mad and makes 4.x difficult to use as it is not at all obvious to the user why the viewer would want to autoscroll when you un/check items.

How can I help make progress on this?
Comment 5 Pawel Piech CLA 2011-03-15 00:53:19 EDT
Created attachment 191187 [details]
Fix attempt.

The detail pane layout stuff was pretty much owned by Darin, so I'm rather reluctant to touch it.  

That said, in this case it probably only makes sense to force the selection reveal when an explicit user action causes the change in detail pane layout (and thus possibly hiding the previously visible selection).  

Does this patch do the trick for you?
Comment 6 Remy Suen CLA 2011-03-17 16:39:19 EDT
(In reply to comment #5)
> Does this patch do the trick for you?

Yes it does, but I was hoping you would change the strange colour getting/setting code. Get a 4.1 SDK and try to un/check and/or un/select breakpoints, you will see the details pane's background colour switching constantly.
http://download.eclipse.org/e4/sdk/
Comment 7 Pawel Piech CLA 2011-03-18 00:50:02 EDT
(In reply to comment #6)
The troublesome background color handling was added to make the breakpoint details look nicer (bug 301118).  Darin did the implementation, but I think Dani was driving the desired look and feel.  

Dani do you know if this background color switching is necessary?
Comment 8 Dani Megert CLA 2011-03-18 04:25:58 EDT
(In reply to comment #7)
> (In reply to comment #6)
> The troublesome background color handling was added to make the breakpoint
> details look nicer (bug 301118).  Darin did the implementation, but I think
> Dani was driving the desired look and feel.  
> 
> Dani do you know if this background color switching is necessary?
It was done to achieve showing the separator correctly, which is a bit tricky because the top is white and detail pane is gray. This must not be changed in 3.x.

I agree that it looks awful in 4.1 but that is a separate issue which I raised long time ago and which is a general problem introduced by 4.x: it is a wrong assumption that one can easily change the 3.x gray background to white (that's what 4.x does) and then expect that everything that was built for 3.x against a gray background continues to look OK. For other such issues see e.g. bug 320901 or bug 320238.
Comment 9 Pawel Piech CLA 2011-03-18 19:52:37 EDT
I committed the change to at least avoid changing the selection upon detail pane rebuild.  Dani, please review the change.
Comment 10 Dani Megert CLA 2011-03-23 09:13:19 EDT
The fix is good.
Comment 11 Pawel Piech CLA 2011-05-06 18:47:04 EDT
Marking as verified per our bug process.