Implement activity support filtering for e4 dynamic menu contribution.

[raghucssit]

Similar to PluginActionContributionItem we can support activity filtering of menu item to be shown or not.

see #2217

[github-actions]

Test Results

 1 821 files  ±0   1 821 suites  ±0   1h 30m 28s ⏱️ - 5m 8s
 7 719 tests ±0   7 491 ✅ +2  228 💤 ±0  0 ❌  - 2 
24 318 runs  ±0  23 569 ✅ +2  749 💤 ±0  0 ❌  - 2 

Results for commit f6af0c3. ± Comparison against base commit 55481d3.

♻️ This comment has been updated with latest results.

[raghucssit]

@iloveeclipse FYI

[iloveeclipse]

I've just rebased on head to see current test result.

[iloveeclipse]

Please rebase on latest master before next update.

[iloveeclipse]

Only few minor issues left

[iloveeclipse]

@raghucssit : could you please update the fix & rebase?

[raghucssit]

#2217 (comment)
This issue was due to the scheme that e4 view contribution follows.
All the old view contributions have the same URI bundleclass://org.eclipse.ui.workbench/org.eclipse.ui.internal.e4.compatibility.CompatibilityView.
But e4 view contributions have distinct uri for each contribution like bundleclass://org.eclipse.pde.spy.bundle/org.eclipse.pde.spy.bundle.BundleSpyPart.
And all follows the same prefix scheme bundleclass://. In case of e4 view, we just have to use it's actual URI to ask activity support if it is enabled or not. Because activity support checks pattern that are defined and patterns always use bundle/element scheme.

[raghucssit]

@iloveeclipse I have fixed all the review comments and Quick Access leak also. Please check.

[iloveeclipse]

I've first rebased but then I saw that version (and some smaller code) changes needed so I've amended last commit.

Here is the diff: https://github.com/eclipse-platform/eclipse.platform.ui/compare/ee5c4b2479915e3b26c3f4b5c552d9dc1d041c88..73abc2ff90c58692f34047a21712f5d299b9ca57

[laeubi]

The service handling looks fine now I just have some remarks as this is adding new API at a very late stage so maybe others should review this as well:

1. The service should be mentioned here: https://github.com/eclipse-platform/eclipse.platform.ui/blob/master/docs/Eclipse4_RCP_EAS_List_of_All_Provided_Services.md this might also be a better place for explain what it does
2. E4 Services sometimes start wit E so maybe it should be EActivityManager
3. Usually an E4 service should replace the existing functionality and at best the old E3 simply implements both interfaces, can we probably make use of the method org.eclipse.ui.activities.IActivityManager.getEnabledActivityIds() instead of go through the identifier? Then the new EActivityManager can have method EActivityManager#getEnabledActivityIds() and org.eclipse.ui.internal.activities.AbstractActivityManager can just implement the new interface.

[iloveeclipse]

The service handling looks fine now I just have some remarks as this is adding new API at a very late stage

Come on, we are in M2 now. "Very late" is RC2 and we had a "good tradition" to have RC2a in the past.

1. The service should be mentioned here: https://github.com/eclipse-platform/eclipse.platform.ui/blob/master/docs/Eclipse4_RCP_EAS_List_of_All_Provided_Services.md this might also be a better place for explain what it does

I see not a single word of explanation on this document, so I don't think we should be inconsistent. I can add this service there even if it is of not big interest for E4 as it only used a s a bridge between E4 and E3 IDE parts.

2. E4 Services sometimes start wit E so maybe it should be EActivityManager

Why not.

can we probably make use of the method org.eclipse.ui.activities.IActivityManager.getEnabledActivityIds() instead of go through the identifier?

The code uses following pattern to filter out contributions:

		IActivityManager activityMgr = PlatformUI.getWorkbench().getActivitySupport().getActivityManager();
		IIdentifier id = activityMgr.getIdentifier(someParticularContributionId); 
		return (!id.isEnabled());

The reason is that the getEnabledActivityIds() returns top level "categories" and getIdentifier(String).isEnabled() checks for specific patterns bound to that activity.

3. Usually an E4 service should replace the existing functionality and at best the old E3 simply implements both interfaces,

Here we have an advanced functionality that was provided at E3 layer, we do not plan to refactor that and move entire E3 code to E4 layer only to fix one inconsistency with missing filtering of particular menus at E4 level. What we want is not re-implement or move activity support from E3 to E4, just make sure that as long as E3 and E4 layers co-exist (forever?) the missing functionality we found is implemented in the IDE build on top of the mixed architecture.

[laeubi]

Here we have an advanced functionality that was provided at E3 layer, we do not plan to refactor that and move entire E3 code to E4 layer only to fix one inconsistency with missing filtering of particular menus at E4 level.

there is no need to re-implement the functionality here, but even though Eclipse IDE is using the legacy layer (maybe forever) this does not is the case for all use-case, so one needs to take into account that other (pure E4 RCP Applications) will want to maybe enhance or re-implement this functionality.

So when adding a new E4 service it should at laest be designed with such use-case in mind, I would like to get some feedback from @vogella @mickaelistria on that topic maybe, in general the PR looks better now.

Is there a good way to actually test this?

[iloveeclipse]

Is there a good way to actually test this?

Start IDE in debugger on this PR, go to Preferences / Capabilities, switch off PDE

and check that Window -> Spies Menu

disappear after changing preference.

[iloveeclipse]

So when adding a new E4 service it should at laest be designed with such use-case in mind

Let name it E3ActivityManagerBridge or E3ActivityManagerProxy or InternalE3ActivityManagerServiceHelper to avoid confusion if someone expects "pure" E4 ActivityManager service with full functionality.

Technically we simply need an interface accessible from both worlds that can be implemented in E3 layer and consumed in E4 layer.

[laeubi]

Let name it E3ActivityManagerBridge or E3ActivityManagerProxy or InternalE3ActivityManagerServiceHelper to avoid confusion if someone expects "pure" E4 ActivityManager service with full functionality.

This is not about "confusion" it is about features. here is an e3 compatibility layer and we really don't want that to be inside E4 itself as a core interface. "activity" is currently completely bound to the e3 extension point (what E4 tries to overcome by design) so we either need a real E4 counterpart (that e3 can implement like IWorkbench and similar) or e3 layer can use the e4 model service + events (aka Addon) that redirects things from e3 > e4.

I'll try to take a look if we can probably find a better extension point here, or one should probably rethink the actual Naming and use-case.

Start IDE in debugger on this PR, go to Preferences / Capabilities, switch off PDE

Thanks I'll try that out, so mentioned "category" (==getEnabledActivityIds() would be "general") and "Identifier" (== PDE) ?

[iloveeclipse]

Thanks I'll try that out, so mentioned "category" (==getEnabledActivityIds() would be "general") and "Identifier" (== PDE) ?

No. Just check in debugger what you will see in ActivityManagerProxy.isIdentifierEnabled().

[laeubi]

@raghucssit @iloveeclipse I have now created

• Consider Activity Filtering when running in E3 compatibility mode #2768

this does not require any new service and only provisional API changes and should be much more flexible here, it could also be used in other context. I also took the liberty to enhance the scope for handled and direct contributions as well.

[iloveeclipse]

@raghucssit @iloveeclipse I have now created

• Consider Activity Filtering when running in E3 compatibility mode #2768

this does not require any new service and only provisional API changes and should be much more flexible here

Great, thanks. What is missing in #2768 is the view filtering from quick assist from this PR.

I will then keep only this one change here, which is unrelated to e4 menu stuff.

[iloveeclipse]

Thanks Raghu & Christoph.