Load marker content generator details from marker content generator extensions (fix for #2193)

[travkin79]

This patch fixes issue #2193. The implementation and tests ensure loading of marker content generator extensions, including new marker fields, marker types, and groups provided via marker content generator extensions.

This way, it is now possible to extends, e.g. the Problems View and the Markers View with new columns. For example, you could add a column with a URL to a vulnerability issue's detailed description (e.g. CWE-200) or in my case to an issue's description found by clang-tidy in a C++ program, like e.g. https://clang.llvm.org/extra/clang-tidy/checks/readability/magic-numbers.html.

Fixes: #2193

[github-actions[bot]]

Test Results

 1 818 files  ±0   1 818 suites  ±0   1h 35m 18s :stopwatch: + 2m 12s  7 709 tests +1   7 481 :white_check_mark: +2  228 :zzz: ±0  0 :x:  - 1  24 288 runs  +3  23 541 :white_check_mark: +4  747 :zzz: ±0  0 :x:  - 1 

Results for commit b3741a32. ± Comparison against base commit 4e0740d7.

:recycle: This comment has been updated with latest results.

[merks]

Note that we are in the RC1 phase of development where it's more difficult to push though changes that might cause regressions. I think this will unfortunately need to wait until the 4.34 stream opens for development.

Also, have a read through this:

https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#creating-a-pull-request

E.g.,

You will usually be asked to squash your commits to a single one before a PR will be merged.

[travkin79]

Note that we are in the RC1 phase of development where it's more difficult to push though changes that might cause regressions. I think this will unfortunately need to wait until the 4.34 stream opens for development.

Also, have a read through this:

https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#creating-a-pull-request

E.g.,

You will usually be asked to squash your commits to a single one before a PR will be merged.

Thank you for the hints, Ed. I guess, I have to wait at least until the latest commit in the master branch has a successful CI run. When will the 4.34 stream be open for development? Is there a timeline?

[merks]

Here's the release train schedule:

https://github.com/eclipse-simrel/.github/blob/main/wiki/SimRel/2024-09.md

the calendar details:

https://calendar.google.com/calendar/u/0/[email protected]&dates=20240901/20240930&hl=en&mode=AGENDA

The platform is due to contribute its final build on August 30:

So I expect the following week that new I-builds for 4.34 will be configured and at some point during that week master will be declared open.

[rubenporras]

@merks , without putting much pressure, I think this is a very nice addition to the product, and if the new extension point is not used, an small change.

Would it be possible to have it merged before RC1?

[akurtakov]

Not used but going in a release means this will become API without being even used with changes following 2 years deprecation period. IMO this is really bad idea. Practically tonight should be the last build with functional changes for 4.33 and getting new functionality in it means there is no room for maneuver. This one should wait couple of weeks and be submitter for 4.34 release. Releasing is already too cumbersome job to risk adding extra work at this point. Please see https://www.eclipse.org/lists/eclipse-dev/msg12304.html which describes what should happen during current week. With every release procedures are streamlined and freeze periods are reduced but this doesn't happen magically - it requires far bigger investments in stabilizing tests and etc. I haven't seen a clean build in long period (e.g. https://download.eclipse.org/eclipse/downloads/drops4/I20240819-1800/ ) which to me means we have reached the most open state of development for now. I understand this may not look strictly related (or even totally unrelated) to your request but for those of us that have to throw the whole release out all these things are one and the same as they make us spend time daily trying to get to a manageable state.

[travkin79]

I'd appreciate it if someone would review my PR, maybe @trancexpress?

[travkin79]

It seems, I fell into the pitfall of not knowing, I have to explicitly publish my answers to the reviewers' comments.

[iloveeclipse]

This way, it is now possible to extends, e.g. the Problems View and the Markers View with new columns.

Could you please add a screenshot with such example?

[trancexpress]

I did grep what platform/Eclipse/Xtext repositories I have, the extension in question is barely used. I only find mentions in the platform UI repository.

In directory: eclipse.platform.ui/
bundles/org.eclipse.ui.ide/schema/markerSupport.exsd:            <element ref="markerContentGeneratorExtension" minOccurs="0" maxOccurs="unbounded"/>
bundles/org.eclipse.ui.ide/schema/markerSupport.exsd:   <element name="markerContentGeneratorExtension">
bundles/org.eclipse.ui.ide/schema/markerSupport.exsd:            A markerContentGeneratorExtension is an extension to an existing markerContentGenerator.
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/views/markers/internal/MarkerSupportRegistry.java:        private static final String MARKER_CONTENT_GENERATOR_EXTENSION = "markerContentGeneratorExtension"; //$NON-NLS-1$
tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/markers/MarkerSupportViewTest.java:    public void markerContentGeneratorExtensionLoaded() throws Exception {
tests/org.eclipse.ui.tests/plugin.xml:       <markerContentGeneratorExtension
tests/org.eclipse.ui.tests/plugin.xml:       </markerContentGeneratorExtension>
tests/org.eclipse.ui.tests/plugin.xml:        <markerContentGeneratorExtension
tests/org.eclipse.ui.tests/plugin.xml:        </markerContentGeneratorExtension>
tests/org.eclipse.ui.tests/plugin.xml:        <markerContentGeneratorExtension
tests/org.eclipse.ui.tests/plugin.xml:        </markerContentGeneratorExtension>
tests/org.eclipse.ui.tests/plugin.xml:        <markerContentGeneratorExtension
tests/org.eclipse.ui.tests/plugin.xml:        </markerContentGeneratorExtension>

[trancexpress]

@travkin79 feel free to do code clean-ups after we've merged the PR, I've adjusted it to make the review as easy as possible.

[trancexpress]

OK, last remaining item:

@travkin79 , you mention groups in the original commit message. I didn't notice a change in the group code though (aside from removing the caching). Can you clarify? ContentGeneratorDescriptor.getMarkerGroups() is supposed to be supported too?

[travkin79]

Hi @trancexpress, Thank you for looking deeper into my changes and for improving the code. I think, your solution of clearing caches as soon as extensions change works better than calculating all collection always on demand.

@travkin79 feel free to do code clean-ups after we've merged the PR, I've adjusted it to make the review as easy as possible.

I guess, you mean, before merging the PR, right? I think, I can only commit changes to my fork. I think, I'll make a few minor improvements in the fork / PR today.

@travkin79 , you mention groups in the original commit message. I didn't notice a change in the group code though (aside from removing the caching). Can you clarify? ContentGeneratorDescriptor.getMarkerGroups() is supposed to be supported too?

You're right. I wanted to support groups, fields (all and the initially visible ones), and types. There were no significant changes on the groups code.

[trancexpress]

@travkin79 feel free to do code clean-ups after we've merged the PR, I've adjusted it to make the review as easy as possible.

I guess, you mean, before merging the PR, right? I think, I can only commit changes to my fork. I think, I'll make a few minor improvements in the fork / PR today.

No, I do mean after the PR is merged. Please keep clean-ups separate of this change, unless you mean to do clean-ups in the added code.

@travkin79 , you mention groups in the original commit message. I didn't notice a change in the group code though (aside from removing the caching). Can you clarify? ContentGeneratorDescriptor.getMarkerGroups() is supposed to be supported too?

You're right. I wanted to support groups, fields (all and the initially visible ones), and types. There were no significant changes on the groups code.

Why is there no change in the groups code? Shouldn't the groups also take groups of extensions? I.e. how its done for fields and marker types?

[travkin79]

Why is there no change in the groups code? Shouldn't the groups also take groups of extensions? I.e. how its done for fields and marker types?

Yes, you're right. But the code from master already does that. It's just not that obvious. The method addDefinedGroups(Collection<MarkerGroup>) does run through all extensions and adds the groups accordingly. The new test case ensures that groups from extensions are loaded, too.

No, I do mean after the PR is merged. Please keep clean-ups separate of this change, unless you mean to do clean-ups in the added code.

Ok. I guess that's for keeping the PRs as simple as possible to simplify reviews.

[travkin79]

There's another thing left open: We have to decide whether to change the default of making new marker fields invisible by default. I think, we can safely do that since the marker content generator extensions didn't work before. Thus, no fields would suddenly disappear because of our change.

I'm not sure if we should also change the extension point, as suggested by @iloveeclipse, in order to make clear, in which views the fields will be visible. I tend to leave it as it is, assuming that new fields will be invisible by default and a user would decide whether to add them to certain views (e.g. problems view, markers view).

[trancexpress]

There's another thing left open: We have to decide whether to change the default of making new marker fields invisible by default. I think, we can safely do that since the marker content generator extensions didn't work before. Thus, no fields would suddenly disappear because of our change.

I'm not sure if we should also change the extension point, as suggested by @iloveeclipse, in order to make clear, in which views the fields will be visible. I tend to leave it as it is, assuming that new fields will be invisible by default and a user would decide whether to add them to certain views (e.g. problems view, markers view).

From my POV this PR is a bug fix, adding missing functionality. If any client code unintentionally adds visible fields (either due to the default when not specifying visibility, or due to explicitly specifying a visible field), that client will have to be adjusted to work with latest the platform (i.e. set the visibility as desired). So from my side, leave as is - the change follows the API documentation.

@iloveeclipse ?

[trancexpress]

Will merge the PR tomorrow. Let me know if there is any feedback that has not been addressed.