Bug 463245 - Debug toolbar items not visible until restart with CDT installed
Summary: Debug toolbar items not visible until restart with CDT installed
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.5   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.5 M7   Edit
Assignee: Marc-André Laperle CLA
QA Contact:
URL:
Whiteboard:
Keywords: greatfix
Depends on:
Blocks: 497705
  Show dependency tree
 
Reported: 2015-03-26 23:36 EDT by Marc-André Laperle CLA
Modified: 2017-06-07 04:58 EDT (History)
6 users (show)

See Also:


Attachments
No debug buttons screenshot (58.35 KB, image/png)
2015-03-27 00:11 EDT, Marc-André Laperle CLA
no flags Details
Sample plugin (2.72 KB, application/zip)
2015-04-19 15:13 EDT, Marc-André Laperle CLA
no flags Details
Crazy debug *drop-down* on GTK2 (32.83 KB, image/png)
2015-04-27 16:57 EDT, Andrey Loskutov CLA
no flags Details
No debug toolbar on GTK3 with contributed command (23.45 KB, image/png)
2015-04-27 16:58 EDT, Andrey Loskutov CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marc-André Laperle CLA 2015-03-26 23:36:21 EDT
Using Eclipse Mars M6 (CDT 8.6.0.201503221005). Linux, Windows and Mac.
https://hudson.eclipse.org/packaging/job/mars.epp-tycho-build/188/artifact/org.eclipse.epp.packages/archive/

1. Start Eclipse with CDT installed (M6 EPP)
2. Open Debug perspective. Debug toolbar buttons are not visible (step over, resume, etc).
3. Restart Eclipse. Buttons are now visible.

This is not a problem with the same CDT version using Eclipse M5a.
Comment 1 Marc-André Laperle CLA 2015-03-27 00:11:36 EDT
Created attachment 251942 [details]
No debug buttons screenshot
Comment 2 Lars Vogel CLA 2015-03-27 00:23:15 EDT
Can you also observe this with a standard Eclipse SDK and the CDT plug-ins installed afterwards?
Comment 3 Andrey Loskutov CLA 2015-03-27 01:38:11 EDT
Looking on the screenshot I'm wondering if a repaint is missing (there is empty area in the middle)? What happens if you switch now to another perspective and back, or just open Java editor?
Comment 4 Andrey Loskutov CLA 2015-03-27 01:50:45 EDT
Is this only *first* startup issue? If you open Customize Perspective Dialog, ist the toolbar shown as enabled or not? I know there were changes on the "Skip Breakpoints" button in M6, so can this be somehow related? I will debug on weekend. BTW which gtk version are you using - gtk 3?
Comment 5 Andrey Loskutov CLA 2015-03-27 02:11:01 EDT
Can also be something in the toolbar manager. Possibly related CoolbarToTrimManager changes in M6:
https://git.eclipse.org/r/#/c/40449/
https://git.eclipse.org/r/#/c/40825/
Just wondering if CDT re-defines debug toolbar visibility?
Comment 6 Lars Vogel CLA 2015-03-27 05:18:03 EDT
(In reply to Andrey Loskutov from comment #5)
> Can also be something in the toolbar manager. Possibly related
> CoolbarToTrimManager changes in M6:
> https://git.eclipse.org/r/#/c/40449/
> https://git.eclipse.org/r/#/c/40825/
> Just wondering if CDT re-defines debug toolbar visibility?

Bug 400217 might be related.
Comment 7 Marc-André Laperle CLA 2015-03-27 10:14:15 EDT
(In reply to Lars Vogel from comment #2)
> Can you also observe this with a standard Eclipse SDK and the CDT plug-ins
> installed afterwards?

Yes. I installed it on top of the Eclipse Platform (so even less stuff than SDK).
http://download.eclipse.org/eclipse/downloads/drops4/S-4.5M6-201503200800/#PlatformRuntime
+
http://download.eclipse.org/tools/cdt/builds/mars/milestones/m6 (C/C++ Development Tools feature)

(In reply to Andrey Loskutov from comment #3)
> Looking on the screenshot I'm wondering if a repaint is missing (there is
> empty area in the middle)? What happens if you switch now to another
> perspective and back, or just open Java editor?

There is no Java editor in the C/C++ EPP or just with CDT installed on top of the Eclipse Platform. But I did test the Eclipse for Java EPP and it works correctly. I tried to switch back and forth the C/C++ perspective and Debug perspective but it does not bring back the toolbar. I also tried to reset the Debug perspective.

(In reply to Andrey Loskutov from comment #4)
> Is this only *first* startup issue? If you open Customize Perspective
> Dialog, ist the toolbar shown as enabled or not? I know there were changes
> on the "Skip Breakpoints" button in M6, so can this be somehow related? I
> will debug on weekend. BTW which gtk version are you using - gtk 3?

Yes it looks like it's just the first startup. In the Customize perspective dialog, the toolbar is shown as enabled. I reproduced the bug on Windows, Mac and Linux so I don't think the GTK version is at play.

(In reply to Andrey Loskutov from comment #5)
> Can also be something in the toolbar manager. Possibly related
> CoolbarToTrimManager changes in M6:
> https://git.eclipse.org/r/#/c/40449/
> https://git.eclipse.org/r/#/c/40825/
> Just wondering if CDT re-defines debug toolbar visibility?

I'll try to revert those changes to see if it fixes the problem. Thank you for your very quick responses!
Comment 8 Marc-André Laperle CLA 2015-03-27 15:51:52 EDT
I bisected the eclipse.platform.ui repo and found this to be the first "bad" commit:

commit d27e6b625421d8fd6f59c5d434191c74890dc54a
Author: Dirk Fauth <dirk.fauth@googlemail.com>
Date:   Tue Feb 24 15:26:42 2015 +0100

    Bug 431990 - [Renderer] Settting the visible property to false of the
    MToolbar is ignore by the renderer
    
    Change-Id: I044cc2764004125dc873e8ad53db37e7b8a6e0f9
    Signed-off-by: Dirk Fauth <dirk.fauth@googlemail.com>

I'm not sure why yet.
Comment 9 Dirk Fauth CLA 2015-03-27 16:11:23 EDT
What I added was to pack the toolbar parent so it is resized correctly if button visibility changes.

From the Javadoc of pack()

 * Causes the receiver to be resized to its preferred size.
 * For a composite, this involves computing the preferred size
 * from its layout, if there is one.

I can't explain why such a call should have the described behavior? Could it be that there is no layout set on initial startup somehow?
Comment 10 Andrey Loskutov CLA 2015-03-27 16:45:03 EDT
(In reply to Dirk Fauth from comment #9)
> I can't explain why such a call should have the described behavior? Could it
> be that there is no layout set on initial startup somehow?

What I can imagine is: your change just revealed existing bug. What is really interesting, why this happens only once on first startup, why only for CDT and why all the reset perspectives etc does not help, but only restart?
Comment 11 Marc-André Laperle CLA 2015-03-27 16:46:17 EDT
It works if I add a null check for the layout, I will push a patch to Gerrit.
Comment 12 Eclipse Genie CLA 2015-03-27 16:53:06 EDT
New Gerrit change created: https://git.eclipse.org/r/44785
Comment 13 Marc-André Laperle CLA 2015-03-30 15:40:51 EDT
(In reply to Eclipse Genie from comment #12)
> New Gerrit change created: https://git.eclipse.org/r/44785

I just want to follow up that the patch on Gerrit is not good (brings back Bug 431990) so we don't have a solution right now.
Comment 14 Marc Khouzam CLA 2015-03-31 21:53:12 EDT
I can reproduce this too like Marc-Andre describes.
I noticed that if I drag the blank debug toolbar to another location, it forces it to redraw and it becomes visible.

So what could CDT be doing to affect the drawing of the toolbar at the very beginning of a workspace?
Comment 15 Dirk Fauth CLA 2015-04-07 10:23:43 EDT
(In reply to Marc Khouzam from comment #14)
> So what could CDT be doing to affect the drawing of the toolbar at the very
> beginning of a workspace?

I'm not quite sure. How are those menu items contributed to the toolbar? Maybe that information helps in finding what is the difference to other menu items.

I recently came across Bug 432856 where a bug was reported for debug buttons. I'm not sure if the CDT team worked around that issue somehow, and now that workaround causes the described behavior.
Comment 16 Marc-André Laperle CLA 2015-04-19 15:13:40 EDT
Created attachment 252517 [details]
Sample plugin

I'm looking into this a bit further. I noticed that the vertical location (y value) is wrong for the debug toolbar. Interestingly, if I skip the intro screen (I just removed intro.universal plugin from my launch config) then it works correctly. That would explain why the bug occurs the first time. I made a sample plugin. It contains no CDT code. It only contains a simple menu contribution at the location toolbar:org.eclipse.debug.ui.main.toolbar?after=additions
Comment 17 Andrey Loskutov CLA 2015-04-19 15:23:51 EDT
(In reply to Marc-Andre Laperle from comment #16)
> I'm looking into this a bit further. I noticed that the vertical location (y
> value) is wrong for the debug toolbar. Interestingly, if I skip the intro
> screen (I just removed intro.universal plugin from my launch config) then it
> works correctly. 

Which reminds me about bug 394386 (wrong order of buttons), only observable if the intro screen is *closed* via [x] button on first startup, see bug 394386 comment 31.

Have you tried to minimize or restore the intro screen instead of closing it - does this allow debug toolbar to be visible again?
Comment 18 Marc-André Laperle CLA 2015-04-19 15:45:08 EDT
(In reply to Andrey Loskutov from comment #17)
> (In reply to Marc-Andre Laperle from comment #16)
> > I'm looking into this a bit further. I noticed that the vertical location (y
> > value) is wrong for the debug toolbar. Interestingly, if I skip the intro
> > screen (I just removed intro.universal plugin from my launch config) then it
> > works correctly. 
> 
> Which reminds me about bug 394386 (wrong order of buttons), only observable
> if the intro screen is *closed* via [x] button on first startup, see bug
> 394386 comment 31.
> 
> Have you tried to minimize or restore the intro screen instead of closing it
> - does this allow debug toolbar to be visible again?

I tried both restoring and minimizing and it doesn't make the toolbar visible again. But maybe figuring one of the bug will help fix the other.
Comment 19 Marc-André Laperle CLA 2015-04-20 00:53:23 EDT
I didn't get very far into understanding what was going wrong but I did notice that the toolbar is wrapped into a ImageBasedFrame. The ImageBasedFrame is getting re-parented (limbo shell) when going from visible to invisible and again when going to visible. I tried to remove the ImageBasedFrame wrapping as a quick test and it worked. I changed this:
CSSRenderingUtils.frameMeIfPossible:

if (toFrame != null) {
	return toFrame;
}

Or course, this is not a fix but perhaps an indication that something is wrong in the way ImageBasedFrame wraps things.
Comment 20 Marc-André Laperle CLA 2015-04-21 10:00:18 EDT
First, the ToolBar is created and pack() is called on it's parent. When pack is called, it gets added to the composite's TrimBarLayout, as a "TrimLine". Then, the toolbar is being wrapped by the ImageBasedFrame and its location is set to a small offset to account for the width of the "handle" (to drag the toolbar). Then, because it is not visible, the Trim (containing the toolbar in its layout) is re-parented to the limbo shell. The limbo shell lays out its children and modifies the toolbar's location (setBounds). The relative location of the toolbar is now wrong in relation to the ImageBasedFrame. When the Welcome screen is closed, the Trim is brought back to the real shell and location of the toolbar in relation to the ImageBasedFrame is still wrong.

I see multiple things that could be considered wrong with this. First, I'm not sure that ToolBarManagerRenderer.createWidget should be calling processContribution (which in then end calls pack) before frameMeIfPossible (which creates the ImageBasedFrame). If it was the other way around, the ToolBar's location would not be modified and ImageBasedFrame would be the control being layed out. 

Second, the fact that the toolbar is re-parented to the ImageBasedFrame and that the toolbar is still in the TrimBarLayout seems wrong since the toolbar is not a child of of the trim anymore. One solution that I see is to flush the TrimBarLayout's cache (TrimLines) when layout is called subsequently with flushCache==true. Doing that seems to break the dragging of toolbars...

A less intrusive solution would be to just move back the toolbar to its location in relation to ImageBasedFrame every time the toolbar is moved, with a ControlListener. I tried that and it seems to work.
Comment 21 Eclipse Genie CLA 2015-04-21 10:05:37 EDT
New Gerrit change created: https://git.eclipse.org/r/46184
Comment 22 Marc-André Laperle CLA 2015-04-22 12:40:45 EDT
(In reply to Eclipse Genie from comment #21)
> New Gerrit change created: https://git.eclipse.org/r/46184

Dirk, do you know if this change brings back Bug 431990?
Comment 23 Dirk Fauth CLA 2015-04-22 16:58:01 EDT
(In reply to Marc-Andre Laperle from comment #22)
> (In reply to Eclipse Genie from comment #21)
> > New Gerrit change created: https://git.eclipse.org/r/46184
> 
> Dirk, do you know if this change brings back Bug 431990?

I tried to test, but suddently my workspace seems to be broken. Even with older commits Bug 431990 is back again even worse. But I guess it is my workspace that is broken and I need to setup a new one from scratch.

In the meanwhile you can test yourself using my toolbar example on GitHub:
https://github.com/fipro78/e4toolbarexample

I'll try to setup and test as soon as possible.
Comment 24 Marc-André Laperle CLA 2015-04-22 22:52:50 EDT
(In reply to Dirk Fauth from comment #23)
> In the meanwhile you can test yourself using my toolbar example on GitHub:
> https://github.com/fipro78/e4toolbarexample
> 
> I'll try to setup and test as soon as possible.

Thanks for the example! From what I tested, the new patch doesn't bring back bug 431990. I reverted both patches (bug 431990 and this one) to make sure I see the problem first then applied both and all seemed good.

I'm not sure what could've happened to your workspace, hopefully it's not my patch causing it ;) Since you've been working on debugging the workbench model, perhaps your workbench.xmi got corrupted. Sometimes I have to delete it (or rename it to keep a backup). It's in workspace/.metadata/.plugins/org.eclipse.e4.workbench. Just in case that helps!
Comment 25 Dirk Fauth CLA 2015-04-23 16:29:15 EDT
(In reply to Marc-Andre Laperle from comment #24)
> (In reply to Dirk Fauth from comment #23)
> > In the meanwhile you can test yourself using my toolbar example on GitHub:
> > https://github.com/fipro78/e4toolbarexample
> > 
> > I'll try to setup and test as soon as possible.
> 
> Thanks for the example! From what I tested, the new patch doesn't bring back
> bug 431990. I reverted both patches (bug 431990 and this one) to make sure I
> see the problem first then applied both and all seemed good.
> 
> I'm not sure what could've happened to your workspace, hopefully it's not my
> patch causing it ;) Since you've been working on debugging the workbench
> model, perhaps your workbench.xmi got corrupted. Sometimes I have to delete
> it (or rename it to keep a backup). It's in
> workspace/.metadata/.plugins/org.eclipse.e4.workbench. Just in case that
> helps!

Of course it was my fault and not a broker workspace. I was working on another issue regarding changing the visibility of menu items at runtime. But that patch is not yet approved. See Bug 400217. To see the full feature of the example, the patch I contributed for that ticket needs to be applied.

I applied my patch and yours, and it looks like everything works as intended.
Comment 26 Marc-André Laperle CLA 2015-04-27 11:26:05 EDT
Lars, Andrey, could you take a look at the patch for M7? I was hoping it could make it into M7 for extra testing. I also think it's the least intrusive and dangerous approach to fixing this. Apologies if you were already planning to look at it :)
Comment 28 Andrey Loskutov CLA 2015-04-27 14:39:00 EDT
(In reply to Marc-Andre Laperle from comment #26)
> Lars, Andrey, could you take a look at the patch for M7? I was hoping it
> could make it into M7 for extra testing. I also think it's the least
> intrusive and dangerous approach to fixing this. Apologies if you were
> already planning to look at it :)

Mark-Andre, as I wrote in the review, the change looks OK. I've merged it now and hope it is not too late for M7.

How did you validate the fix? I was not able to reproduce the original issue, so change seem not to make any difference for me.

If you can observe/verify the fix, please change the bug status to closed/fixed.
Comment 29 Marc-André Laperle CLA 2015-04-27 14:59:02 EDT
(In reply to Andrey Loskutov from comment #28)
> (In reply to Marc-Andre Laperle from comment #26)
> > Lars, Andrey, could you take a look at the patch for M7? I was hoping it
> > could make it into M7 for extra testing. I also think it's the least
> > intrusive and dangerous approach to fixing this. Apologies if you were
> > already planning to look at it :)
> 
> Mark-Andre, as I wrote in the review, the change looks OK. I've merged it
> now and hope it is not too late for M7.

Thanks a lot!

> How did you validate the fix? I was not able to reproduce the original
> issue, so change seem not to make any difference for me.

Did you try the plugin mentioned in comment 16? I import that plugin in my workspace, launch as an Eclipse Application, then I see the missing debug toolbar. Make sure you are launching in a new workspace so that the Welcome screen shows up. I also tested launching with CDT plugins to verify that the original report is fixed.

> If you can observe/verify the fix, please change the bug status to
> closed/fixed.

Will do, I'll test tomorrow's build (assuming there is an integration build as usual).
Comment 30 Andrey Loskutov CLA 2015-04-27 16:57:35 EDT
Created attachment 252815 [details]
Crazy debug *drop-down* on GTK2

(In reply to Marc-Andre Laperle from comment #29)
> Did you try the plugin mentioned in comment 16? 

A-ha, I missed this. This way I see the original problem and can confirm that at least on GTK2/GTK3 in KDE on Fedora 21 the fix works now (which is cool).

What is not so cool - I think we have even more bugs in the toolbar rendering area. Your example reveals really funny effects - see attached pictures.

With your example contribution on GTK2 I see now funny debug *drop-down* (!?!) containing "skip breakpoints" and your example command on first startup in the default perspective. On GTK3 I see *empty area* at this place.

In both cases switching to other perspective and back fixes the rendering of the toolbar without restart. I think this deserves another, separated bug. WDYT?

I think the reason could be (?) wrong size calculations for the toolbar at the beginning, so that GTK2 renders a drop-down and  GTK3 decides to not to render anything.
Comment 31 Andrey Loskutov CLA 2015-04-27 16:58:22 EDT
Created attachment 252816 [details]
No debug toolbar on GTK3 with contributed command
Comment 32 Marc-André Laperle CLA 2015-04-27 17:14:24 EDT
(In reply to Andrey Loskutov from comment #30)
> What is not so cool - I think we have even more bugs in the toolbar
> rendering area. Your example reveals really funny effects - see attached
> pictures.
> 
> With your example contribution on GTK2 I see now funny debug *drop-down*
> (!?!) containing "skip breakpoints" and your example command on first
> startup in the default perspective. On GTK3 I see *empty area* at this place.

Strange, I was not seeing that on Windows. I'll try to reproduce this on GTK. It could also be because I was not launching with JDT in my target platform (I see JDT in your screenshot). BTW, have you tried in Gnome? I'm thinking it could also be a bug in the GTK theme that KDE is using.

> In both cases switching to other perspective and back fixes the rendering of
> the toolbar without restart. I think this deserves another, separated bug.
> WDYT?

In a pre-existing workspace (no welcome screen), does it happen both with and without the new fix (of this bug)? If so, I think it warrants a new bug yes. I can take a look since I'm a bit more familiar with this area now.

> I think the reason could be (?) wrong size calculations for the toolbar at
> the beginning, so that GTK2 renders a drop-down and  GTK3 decides to not to
> render anything.

Good idea. I think checking the values in TrimBarLayout could help confirm that.
Comment 33 Marc-André Laperle CLA 2015-04-28 13:46:37 EDT
I tested the fix with I20150428-0800 and it looks good. I was not able to reproduce the issue in comment 30, so I think it can be a separate bug (we can discuss how to reproduce there). I tested the fix on Ubuntu 14.04/Unity, Fedora 21/Gnome with both GTK2 and GTK3.
Comment 34 Marc Khouzam CLA 2015-04-28 14:30:08 EDT
(In reply to Marc-Andre Laperle from comment #33)
> I tested the fix with I20150428-0800 and it looks good. I was not able to
> reproduce the issue in comment 30, so I think it can be a separate bug (we
> can discuss how to reproduce there). I tested the fix on Ubuntu 14.04/Unity,
> Fedora 21/Gnome with both GTK2 and GTK3.

Thanks to everyone involved in getting this resolved!
Comment 35 Marc-André Laperle CLA 2015-05-03 13:11:43 EDT
(In reply to Marc-Andre Laperle from comment #33)
> I tested the fix with I20150428-0800 and it looks good. I was not able to
> reproduce the issue in comment 30, so I think it can be a separate bug (we
> can discuss how to reproduce there).

I can reproduce it now. The key was the initial perspective. I had the wrong product in my launch configuration (eclipse.platform) so I was not getting the Java perspective initially. I opened bug 466233.
Comment 36 Leo Ufimtsev CLA 2017-06-06 16:31:45 EDT
This bugfix is generating a regression on Ubuntu:
Bug 497705 – Unhandled event loop > StackOverflow in Perspective 
https://bugs.eclipse.org/bugs/show_bug.cgi?id=497705

The ControlMoveListener sets the location, which triggers a move event, which triggers the controlMoveListener. > infinite loop.

It's probable that on SWT's side, the setLocation() doesn't realize that the old location is the same as the new location because CSS/padding is messing with the location.
So what might be happening is this ControlMoved keeps trying to move the Widget to a certain location and SWT doesn't realize that it's the same location as before and sends a new SWT.move event.
Comment 37 Leo Ufimtsev CLA 2017-06-06 18:24:55 EDT
It may be something I can fix in SWT. Investigating....
Comment 38 Dani Megert CLA 2017-06-07 04:56:59 EDT
We usually don't reopen the original bug when there's a regression. Especially for such old bugs. Reopen is OK if the regression is close to the fix, e.g. same milestone.