Bug 531854 - View toolbar not drawn correctly when new commands are added/removed until view is resized
Summary: View toolbar not drawn correctly when new commands are added/removed until vi...
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.2   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: 4.9 M2   Edit
Assignee: Simeon Andreev CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 536409
  Show dependency tree
 
Reported: 2018-03-01 08:53 EST by Mike Parker CLA
Modified: 2021-06-07 11:38 EDT (History)
3 users (show)

See Also:


Attachments
Small plug-in project that can be imported to reproduce the problem (9.19 KB, application/zip)
2018-03-01 08:53 EST, Mike Parker CLA
no flags Details
Word document describing how to use provided project, with screenshots (132.00 KB, application/msword)
2018-03-01 08:55 EST, Mike Parker CLA
no flags Details
An example plug-in to reproduce the problem with. (5.39 KB, application/zip)
2018-06-06 08:59 EDT, Simeon Andreev CLA
no flags Details
An SWT snippet which reduces Eclipse UI code to SWT calls, showcasing the behaviour. (2.92 KB, text/x-java)
2018-06-06 09:16 EDT, Simeon Andreev CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Parker CLA 2018-03-01 08:53:53 EST
Created attachment 272955 [details]
Small plug-in project that can be imported to reproduce the problem

If certain commands are to be shown in a view tool bar based on some condition (e.g., a particular context becoming active, or an object of a particular type is selected), as defined via a "visibleWhen" command core expression in the plugin's plugin.xml file, then these commands are not shown as expected when the expression becomes true. Instead, rather than anything appearing in this case, existing commands behind the insertion point *disappear* from view's toolbar! The toolbar is correctly redrawn when the view is resized, but it is very annoying to have to keep on doing this every time the view's toolbar is not automatically updated to reflect the current state of the corresponding core expressions.

Furthermore, if commands are removed from a view toolbar by Eclipse due to a core expression state change and the number of toolbar rows is reduced as a result, then area previously occupied by the now unused toolbar rows is not drawn correctly and obscures any view content now overlapping this region.

I have set the priority of this issue to "major" because it seems to be a generic issue and because being able to seamlessly extend the UI is one of the cornerstones of the original Eclipse design and therefore should work reliably.

I am providing a zip file containing a sample project, as well as a Word document containing the steps necessary for reproducing the issues based on this project as well as screenshots showing the observed behaviour.
Comment 1 Mike Parker CLA 2018-03-01 08:55:49 EST
Created attachment 272956 [details]
Word document describing how to use provided project, with screenshots
Comment 2 Andrey Loskutov CLA 2018-03-13 17:47:21 EDT
Simeon, does it look similar to the problem what we see with our Maintenance toolbar?
Comment 3 Simeon Andreev CLA 2018-03-14 04:21:13 EDT
It definitely looks similar, though whether its the same cause is a different topic.

In our case, we show the console view on start if its not present. This causes the view toolbar to update itself, and paint on top of the part stack (still no idea why). Without this console view activation, the view toolbar is not updated (as reported in this ticket).

Unfortunately I've not checked if there are *other* cases when the view toolbar is not shown / updated, as reported in this ticket.
Comment 4 Simeon Andreev CLA 2018-03-18 13:15:49 EDT
There are 2 problems that I see here:

1. visibleWhen for a menuContribution with a view toolbar location will disregard whether the view is active.

As soon as the related expression evaluates to true, the toolar button is visible. Its painted at the beginning of the part stack tabs. Switching to the view tab and then to another tab "fixes" the problem.

Adding an <and> with a check whether the active part is the view is a workaround for the problem, so this is not too bad:

<with variable="activePartId"> <equals value="the.id.of.the.view"/> </with>

2. The first rendering when a tab becomes active doesn't correctly show all the toolbar buttons, if at least one depends on visibleWhen.

I don't observe this on Windows 10. I observe this on RHEL 7.2 (GTK 3.14) and RHEL 7.4 (GTK 3.22).

Behaviour here is extremely wacky:

a) If the last contribution for the view toolbar has visbileWhen condition, none of the visibleWhen contributions will be visible after switching to the views tab. Only after resize.

b) If the last contribution does not have a visibleWhen condition, then the last contribution is not visible. For some similar constellations, also the first contribution is not visible.

So far I've not determined what causes the buttons to be visible after resizing. Once I manage this, figuring out what is missing when the view is activated should hopefully be simple.
Comment 5 Simeon Andreev CLA 2018-03-20 08:42:25 EDT
On RHEL with GTK (3.14 and 3.22), the view toolbar is behind the CTabFolder minimize/maximize button toolbar, if the toolbar had no visible buttons at start.

Once the visibility is updated, the command button does become visible, but the toolbars placement is not updated. So the command button is "behind" the CTabFolder minimize/maximize toolbar.

This explains why I was not able to intercept any visibility calls, and also the very odd behaviour once the view toolbar has a few more commands.
Comment 6 Simeon Andreev CLA 2018-03-20 14:08:25 EDT
For GTK, an OK-ayish workaround is to:

1. Use activePartId variable to check that the view is active for the visibility.
2. Ensure that the toolbar button is visible when the view is opened for the first time. This should usually mean inverting the variable condition and the variable values.

This does leave the button background at the button location in the view toolbar, once the button is hidden with the variable. Depending on theme, this could look awkward. Resizing the view helps as before.

I've not been able to observe this on Windows 7 and Windows 10, with the code in our product. If we have a fix for GTK we'll need to validate with the attached example; maybe there are 2 different issues here.
Comment 7 Mike Parker CLA 2018-03-29 10:03:50 EDT
The workaround I am currently using is to manually update all action bars after activating/deactivating the context associated with the "visibleWhen" clause:

for (IViewReference viewRef : window.getActivePage().getViewReferences()) {
    IViewPart viewPart = viewRef.getView(false);
    if (viewPart != null)                     
        viewPart.getViewSite().getActionBars().updateActionBars();
}

However, although this causes the view toolbar buttons to be shown/hidden correctly, the toolbar background color is often wrong (e.g., light blue if view is active). That is, even if the toolbar is within the view's tab, it is usually re-drawn with the same background as if the toolbar were positioned in the view caption area, to the left of the minimize icon.
Comment 8 Simeon Andreev CLA 2018-04-01 02:57:34 EDT
From Andrey:

Possibly related Bug 201589 with https://git.eclipse.org/r/#/c/120384/.


I'll check if this helps, hopefully this week. I didn't look at the review so far.
Comment 9 Simeon Andreev CLA 2018-04-02 00:37:07 EDT
The patch doesn't seem to help for views. Looking at the review, I believe it aims for fixes in the global toolbar.
Comment 10 Simeon Andreev CLA 2018-05-02 01:04:14 EDT
I don't think I can reproduce this on Eclipse 4.6.3 but I can on Eclipse 4.7.0.

This is on RHEL 7.4, with Eclipses:

Eclipse SDK
Version: Neon.3 (4.6.3)
Build id: M20170301-0400


Eclipse SDK
Version: Oxygen (4.7)
Build id: I20170612-0950


I'll try to bisect. I don't think this is SWT related, since I took 4.7.2 SWT in the workspace and couldn't reproduce with Eclipse SDK 4.6.3.
Comment 11 Simeon Andreev CLA 2018-05-02 05:14:55 EDT
Bisection led to: https://git.eclipse.org/c/platform/eclipse.platform.ui.git/diff/bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/IDEWorkbenchPlugin.java?id=fcf5b6385147db2f0266993950402082e8519d8b

So unfortunately the problem is also present in 4.6, but having the Problems View in the same part stack hides the problem.

I removed everything but my test view, I don't see the problem in 4.2 but I see it  in 4.3.

Eclipse SDK
Version: 4.2.0
Build id: I20120608-1400

Eclipse SDK
Version: 4.3.0
Build id: I20130605-2000

I'll try to bisect again.
Comment 12 Andrey Loskutov CLA 2018-05-02 05:34:16 EDT
(In reply to Simeon Andreev from comment #11)
> Bisection led to:
> https://git.eclipse.org/c/platform/eclipse.platform.ui.git/diff/bundles/org.
> eclipse.ui.ide/src/org/eclipse/ui/internal/ide/IDEWorkbenchPlugin.
> java?id=fcf5b6385147db2f0266993950402082e8519d8b

???

This is completely unrelated to the toolbars and was added in 4.7, but you mean this can be reproduced in 4.6?

> So unfortunately the problem is also present in 4.6, but having the Problems
> View in the same part stack hides the problem.
> 
> I removed everything but my test view, I don't see the problem in 4.2 but I
> see it  in 4.3.
> 
> Eclipse SDK
> Version: 4.2.0
> Build id: I20120608-1400
> 
> Eclipse SDK
> Version: 4.3.0
> Build id: I20130605-2000
> 
> I'll try to bisect again.

Sorry I'm not sure I understand you. You first "bisect" to some unrelated commit in 4.7 and few lines later says it is in 4.6 and even 4.3?

Can you please first finish your investigation, and *after* that report your findings?.
Comment 13 Simeon Andreev CLA 2018-06-06 08:59:45 EDT
Created attachment 274349 [details]
An example plug-in to reproduce the problem with.

To reproduce with the attached plug-in:

1. Import plug-in from attached archive "ExamplePlugin.zip".
2. Start Eclipse SDK.
3. Close all views.
4. Open view "Sample View".
5. Restart Eclipse SDK.
6. Wait 2-3 seconds.
7. Observe that the "Sample View" has no visible toolbar items.
8. Resize the part stack of "Sample View", observe that a toolbar item becomes visible.

I have bisected this issue to the following commit (introduced between 4.2.0 and 4.2.1):

https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=b5a24411e872c0006c0c1d8970bd74e551767805

The commit removes a composite parent of the view command toolbar, see the removed method ToolBarManagerRenderer.createIntermediate:

https://git.eclipse.org/c/platform/eclipse.platform.ui.git/diff/bundles/org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt/ToolBarManagerRenderer.java?id=b5a24411e872c0006c0c1d8970bd74e551767805

I've debugged some more and I noticed that this may be an SWT issue. A rudimentary SWT snippet that shows the problem will follow shortly. The snippet is not extensive enough to show why before the commit above the command toolbar behaved well.

In particular, there are actually two toolbars for the view. A toolbar which displays the view menu drop-down button, and a toolbar which displays commands. 

The view menu toolbar is hidden if the view has no menu items, see implementation in org.eclipse.e4.ui.workbench.renderers.swt.StackRenderer.addTopRight(CTabFolder). Hiding is done with RowData.exclude=true.

The view command toolbar has no items at creation, due to the visibleWhen expression; visibility is handled by adding and removing elements from the toolbar.

I.e. we have a combination of a toolbar with one item, which is hidden with RowData.exclude=true and an empty toolbar; both reside in one composite. The composite is packed (pack() method) only upon creation of the view part stack. As a result, it has a wrong size computed and is very narrow.

All items added to the view command toolbar are therefore not visible in the narrow area. The composite is re-packed upon re-size, resulting in correct behaviour.

The workaround suggested by Mike Parker (call ActionBars.updateActionBars() works, since ActionBars.updateActionBars() calls org.eclipse.e4.ui.workbench.renderers.swt.StackRenderer.adjustTopRight(CTabFolder). This calls pack() on the parent composite of the two toolbars. It will also re-create both toolbars.
Comment 14 Eclipse Genie CLA 2018-06-06 09:02:40 EDT
New Gerrit change created: https://git.eclipse.org/r/124112
Comment 15 Simeon Andreev CLA 2018-06-06 09:16:48 EDT
Created attachment 274350 [details]
An SWT snippet which reduces Eclipse UI code to SWT calls, showcasing the behaviour.

To observe the same odd toolbar behaviour, run the snippet "Bug531854_CTabFolder_topRight_has_wrong_size.java".

Observe that the top right of the tab folder shows a part of a button, since there is no more drawing area to show more.

Observe that calling a pack() before layout() (as indicated in a commented-out line) will repair the "bad" behaviour.

The RowData.exclude=true doesn't seem to be relevant for the snippet; the entire RowLayout can be commented out. I've kept it in the snippet to showcase what Eclipse does; the 2nd toolbar will not have visible buttons either way, due to too little space in the parent composite.

I don't think this is an actual SWT bug though. Composite.layout() only mentions sizes of children; it doesn't mention anything about the size of the composite itself.
Comment 16 Simeon Andreev CLA 2018-06-06 10:23:57 EDT
OK, for validation, Eclipse behaviour is somewhat odd.

The problem described by Mike Parker in https://bugs.eclipse.org/bugs/attachment.cgi?id=272956, when using https://bugs.eclipse.org/bugs/attachment.cgi?id=272955 (i.e. comment 1), can be reproduced on Windows 10 but not on RHEL 7.4 (GTK 3.22).

The problem from my example https://bugs.eclipse.org/bugs/attachment.cgi?id=274349 (i.e. comment 13) cannot reproduced on Windows 10, but can be reproduced on RHEL 7.4.

In all cases, I tried with:

Eclipse SDK
Version: Photon (4.8)
Build id: I20180531-0700 

With https://git.eclipse.org/r/124112, I see no problems with either example, both on Windows 10 and on RHEL 7.4. So I guess on that account we should be fine.


Andrey, in comment 4 I described the other issue we have in our product:

> 1. visibleWhen for a menuContribution with a view toolbar location will disregard whether the view is active.

Do we deal with this in this ticket, or do we open a new one? From my POV we shouldn't clutter one bug report with 15 fixes.
Comment 17 Andrey Loskutov CLA 2018-06-06 10:29:57 EDT
(In reply to Simeon Andreev from comment #16)
> Andrey, in comment 4 I described the other issue we have in our product:
> 
> > 1. visibleWhen for a menuContribution with a view toolbar location will disregard whether the view is active.
> 
> Do we deal with this in this ticket, or do we open a new one? From my POV we
> shouldn't clutter one bug report with 15 fixes.

New one. One bug for one ticket.
Comment 18 Simeon Andreev CLA 2018-06-06 11:16:12 EDT
Reported bug 535603 for issue 1. from comment 4:

> 1. visibleWhen for a menuContribution with a view toolbar location will disregard whether the view is active.
Comment 20 Simeon Andreev CLA 2018-06-07 10:05:35 EDT
Do we close this? I've checked the snippet from Mike Parker, it works on Windows 10.

Our problem is fixed on RHEL 7.4 as well.
Comment 21 Andrey Loskutov CLA 2018-06-07 10:08:58 EDT
(In reply to Simeon Andreev from comment #20)
> Do we close this? I've checked the snippet from Mike Parker, it works on
> Windows 10.
> 
> Our problem is fixed on RHEL 7.4 as well.

Sorry, forgot to close. Thanks for the fix.
Comment 22 Eclipse Genie CLA 2018-06-28 06:16:30 EDT
New Gerrit change created: https://git.eclipse.org/r/125157
Comment 24 Dani Megert CLA 2018-06-28 06:18:46 EDT
(In reply to Eclipse Genie from comment #23)
> Gerrit change https://git.eclipse.org/r/125157 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=43b2a6035280803553e0c8ac86dd46b547ea7312
> 

This reverts the change as it badly broke the Compare editor. See bug 536409 for details.
Comment 25 Andrey Loskutov CLA 2018-06-28 07:02:51 EDT
(In reply to Dani Megert from comment #24)
> (In reply to Eclipse Genie from comment #23)
> > Gerrit change https://git.eclipse.org/r/125157 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=43b2a6035280803553e0c8ac86dd46b547ea7312
> > 
> 
> This reverts the change as it badly broke the Compare editor. See bug 536409
> for details.

Simeon, we should check why compare editor is affected. I guess the layout change is not fully propagated through the editor widgets hierarchy. Probably we should request layout on the parent if we are touching toolbar. I mean not layout() but requestLayout(), see bug 468854 for details.
Comment 26 Eclipse Genie CLA 2018-06-28 07:56:43 EDT
New Gerrit change created: https://git.eclipse.org/r/125161
Comment 27 Andrey Loskutov CLA 2018-06-28 08:03:01 EDT
(In reply to Andrey Loskutov from comment #25)
> Probably we should request layout on the parent if we are touching toolbar.
> I mean not layout() but requestLayout(), see bug 468854 for details.

Yep. Stefan's magic API rocks!

(In reply to Eclipse Genie from comment #26)
> New Gerrit change created: https://git.eclipse.org/r/125161

@Mike, @Simeon: can you please check if this works for affected use cases (it should)?
Comment 28 Simeon Andreev CLA 2018-06-28 09:08:26 EDT
I've checked on RHEL 7.4 and Windows 10 with our problem, looks good.

Also checked on Windows 10 with the example from Mike Parker. Looks good as well.
Comment 29 Andrey Loskutov CLA 2018-06-28 09:10:05 EDT
(In reply to Simeon Andreev from comment #28)
> I've checked on RHEL 7.4 and Windows 10 with our problem, looks good.
> 
> Also checked on Windows 10 with the example from Mike Parker. Looks good as
> well.

Thank you Simeon!
Comment 31 Dani Megert CLA 2018-07-31 12:25:50 EDT
(In reply to Eclipse Genie from comment #30)
> Gerrit change https://git.eclipse.org/r/125161 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3103ff9603292254334606b3db510f52f1f9f504
> 

So, is this fixed and no regression?
Comment 32 Andrey Loskutov CLA 2018-07-31 12:45:46 EDT
Yep. Works.
Comment 33 Dani Megert CLA 2018-08-02 11:22:54 EDT
(In reply to Andrey Loskutov from comment #32)
> Yep. Works.

Verified in I20180801-2000.