Comment 1 Marco Descher 2020-04-30 05:39:48 EDT

For the sake of completeness - I see the following exception multiple times before this, maybe the problems are connected

java.lang.IllegalArgumentException: The 'no duplicates' constraint is violated
    at org.eclipse.emf.common.util.AbstractEList.add(AbstractEList.java:337)
    at org.eclipse.emf.common.util.ECollections.setEList(ECollections.java:228)
    at org.eclipse.e4.ui.workbench.renderers.swt.ToolBarManagerRenderer.reconcileManagerToModel(ToolBarManagerRenderer.java:984)
    at org.eclipse.ui.internal.e4.compatibility.ActionBars.updateActionBars(ActionBars.java:97)
    at org.eclipse.ui.internal.e4.compatibility.CompatibilityView$1.compute(CompatibilityView.java:204)
    at org.eclipse.e4.ui.workbench.renderers.swt.ToolBarManagerRenderer.postProcess(ToolBarManagerRenderer.java:996)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:680)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$1.run(PartRenderingEngine.java:547)
    at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
....

Comment 2 Rolf Theunissen 2020-04-30 16:01:40 EDT

(In reply to Marco Descher from comment #1)
> For the sake of completeness - I see the following exception multiple times
> before this, maybe the problems are connected
> 
> java.lang.IllegalArgumentException: The 'no duplicates' constraint is
> violated
>     at org.eclipse.emf.common.util.AbstractEList.add(AbstractEList.java:337)
>     at
> org.eclipse.emf.common.util.ECollections.setEList(ECollections.java:228)
>     at
> org.eclipse.e4.ui.workbench.renderers.swt.ToolBarManagerRenderer.
> reconcileManagerToModel(ToolBarManagerRenderer.java:984)
>     at
> org.eclipse.ui.internal.e4.compatibility.ActionBars.
> updateActionBars(ActionBars.java:97)
>     at
> org.eclipse.ui.internal.e4.compatibility.CompatibilityView$1.
> compute(CompatibilityView.java:204)
>     at
> org.eclipse.e4.ui.workbench.renderers.swt.ToolBarManagerRenderer.
> postProcess(ToolBarManagerRenderer.java:996)
>     at
> org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.
> safeCreateGui(PartRenderingEngine.java:680)
>     at
> org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$1.
> run(PartRenderingEngine.java:547)
>     at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
> ....

This error can only occur if the same IContributionItem is added multiple times to the same IToolBarManager. In the ToolbarManagerRenderer it is assumed that there are no duplicates. As of Bug 378495, this results in these errors, before that it could cause unexpected reordering of the toolbar items.
However, the IContributionManager does not mention duplicates, so they should be allowed.

Do you know in which view this error occurs? Do you see any visual errors?

W.r.t. the NPE, that should be related to the view menu of the part stack you are switching. Is the view menu visible? (Should it be visible?) Is it the last item in the toolbar?

Comment 3 Marco Descher 2020-05-04 05:00:28 EDT

Created attachment 282681 [details]
NPE happens on every tab switch

Comment 4 Marco Descher 2020-05-04 05:01:35 EDT

Do you consider these exceptions as being connected? If not, I would create a separate bug for the "constraint violated" thing.

The NPE happens regularily when switching tabs as seen on the enclosed image.

Comment 5 Marco Descher 2020-05-04 05:19:52 EDT

Created attachment 282682 [details]
Oxygen vs 2020-03

>W.r.t. the NPE, that should be related to the view menu of the part stack you are >switching. Is the view menu visible? (Should it be visible?) Is it the last item >in the toolbar?

The view menu is visible. It was meant to be the last entry in the toolbar menu. In the image you can see the difference in using an Oxygen and 2020-03 target. When the NPE is first hit, the toolbar menu does not update anymore on switching the tab.

Comment 6 Rolf Theunissen 2020-05-04 06:01:38 EDT

(In reply to Marco Descher from comment #4)
> Do you consider these exceptions as being connected? If not, I would create
> a separate bug for the "constraint violated" thing.
> 
> The NPE happens regularily when switching tabs as seen on the enclosed image.

The NPE is a result from the view menu not being the last item in the top right toolbar. The 'no duplicates' occurs in code that is updating the top right toolbar items, as a result, the update runs only partially. So I think these issues are related. 

To better understand the problem and to check if there is no deeper bug triggering this case. In your screenshot I see an green '+' icon repeated. Is this icon supposed to be repeated? How is this tool item added? Is it indeed twice the same contribution item? In other words, do you have a clue why this item duplicates?

Comment 7 Marco Descher 2020-05-04 06:52:51 EDT

Created attachment 282683 [details]
Sequence to generate the problems

I found out the sequence that leads to this problems. 

It seems that adding one and the same IAction object to multiple toolbars is not possible anymore - it was not a problem until now.

Is this wanted behaviour, and the bug is on my site, or is this an Eclipse bug?!

Comment 8 Marco Descher 2020-05-04 07:00:03 EDT

(In reply to Rolf Theunissen from comment #6)
> (In reply to Marco Descher from comment #4)
> > Do you consider these exceptions as being connected? If not, I would create
> > a separate bug for the "constraint violated" thing.
> > 
> > The NPE happens regularily when switching tabs as seen on the enclosed image.
> 
> The NPE is a result from the view menu not being the last item in the top
> right toolbar. The 'no duplicates' occurs in code that is updating the top
> right toolbar items, as a result, the update runs only partially. So I think
> these issues are related. 
> 
> To better understand the problem and to check if there is no deeper bug
> triggering this case. In your screenshot I see an green '+' icon repeated.
> Is this icon supposed to be repeated? How is this tool item added? Is it
> indeed twice the same contribution item? In other words, do you have a clue
> why this item duplicates?

As described in the previous post. Yes, it is the same item. It is programmatically added  (see https://github.com/elexis/elexis-3-core/blob/f82ecb3638044b0d77e2047150a065dfdc462b77/bundles/ch.elexis.core.ui/src/ch/elexis/core/ui/util/ViewMenus.java#L88 for the code) and it is supposed to be repeated.

Comment 9 Rolf Theunissen 2020-05-30 17:58:39 EDT

This is a regression of Bug 378495.

What happens:
1. ActionContributionItem is added to the toolbar of the first view
2. The change is correctly synced the the model:
  - The contribution item is not in the administration: A new Opaque toolbar item is created and added to administration 
  - The Opaque toolbar item is correctly added to the MToolbar
3. ActionContributionItem is added to the toolbar of the second view
4. The change is incorrectly synced to the model:
  - The contribution item is already in the administration: No new Opaque toolbar item is created (This is incorrect, the item occurs twice so two model items are needed)
  - The Opaque toolbar item is added to the second MToolbar. This triggers that the item is *removed* from the first MToolbar
  - The remove event triggers, this results that the contribution item is removed from the administration.
  - The insert event triggers, now the contribution item is not in the administration. As a result, the item is inserted another time in the ToolbarManager (i.e. this creates the duplicate item)
5. On a successive call of syncing the ToolbarManager to the model, the "java.lang.IllegalArgumentException: The 'no duplicates' constrains is violated", this aborts the adjustTopRight. This results in an inconsistent state of the TopRight
6. On successive calls to adjustTopRight, the NullPointerException is thrown every time.


The code before the fix of Bug 378495 was already problematic w.r.t. duplication of contribution items in the UI, resulting in inconsistencies in the model. After the changes for Bug 378495 these inconsistencies result in the problem described above. 

In E3 it was allowed to have the same contribution item in multiple places, this should be supported in E4 as well.

Comment 10 Rolf Theunissen 2020-06-02 02:34:49 EDT

On a closer look, the behavior of comment 9 is the result of an deeper problem.

When adding an action two times to a ContributionManager, two distinct ActionContributionItems are created. The ActionContributionItem has an overridden equals method, making them equal for the same action.
The ToolbarManagerRender internally uses HashSets to keep the relation between model elements and contributions items. As a result the two ActionContributionItems are considered equal in the ToolbarManagerRenderer. This is incorrect. The behavior of comment 9 will not occur for action contributions if these items are not considered equal.

Comment 11 Eclipse Genie 2020-06-02 02:42:26 EDT

New Gerrit change created: https://git.eclipse.org/r/163963

Comment 12 Rolf Theunissen 2020-06-15 15:14:43 EDT

*** Bug 564205 has been marked as a duplicate of this bug. ***

Comment 13 Hristo Bojilov 2020-06-15 17:19:38 EDT

Please also fix this code inside org.eclipse.ui.internal.e4.compatibility.CompatibilityView:

final AtomicBoolean toolbarContributed = new AtomicBoolean();
final IContextFunction func = new ContextFunction() {
	@Override
	public Object compute(IEclipseContext context, String contextKey) {
		if (toolbarContributed.get()) { // replace by getAndSet()
		//fix for bug 448873: don't contribute to the toolbar twice
		return null;
		}
		toolbarContributed.set(true); // Remove that
...
Obviously this is not proper way for using 
java.util.concurrent.atomic.AtomicBoolean and this also can cause duplicated buttons on toolbars/menus.

Comment 14 Rolf Theunissen 2020-06-16 15:18:06 EDT

(In reply to Hristo Bojilov from comment #13)
> Please also fix this code inside
> org.eclipse.ui.internal.e4.compatibility.CompatibilityView:
> 
> final AtomicBoolean toolbarContributed = new AtomicBoolean();
> final IContextFunction func = new ContextFunction() {
> 	@Override
> 	public Object compute(IEclipseContext context, String contextKey) {
> 		if (toolbarContributed.get()) { // replace by getAndSet()
> 		//fix for bug 448873: don't contribute to the toolbar twice
> 		return null;
> 		}
> 		toolbarContributed.set(true); // Remove that
> ...
> Obviously this is not proper way for using 
> java.util.concurrent.atomic.AtomicBoolean and this also can cause duplicated
> buttons on toolbars/menus.

Create Bug 564354 for this.

Comment 15 Eclipse Genie 2020-06-18 15:03:24 EDT

Gerrit change https://git.eclipse.org/r/163963 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7e703fc4d95d12a47428500a830594fcc0702512

Comment 16 Rolf Theunissen 2020-07-05 07:52:54 EDT

The problem with respect to multiple usage of Actions is resolved.

The problem with respect to multiple usage Contribution items is not resolved. I have opened Bug 564935 to discuss what is allowed behavior and what not.

Closing this one as fixed.

Comment 17 Dani Megert 2020-07-13 04:41:32 EDT

(In reply to Rolf Theunissen from comment #16)
> Closing this one as fixed.
Closed now :-)