Add preference to change severity for missing project specific encoding

ingomohr commented 2 years ago

Introduces a new preference that enables to specify if and using which severity markers shall be added in case of missing project-specific encoding settings.

This is the PR of the fork for issue #166

Todo

@merks I could use some help/hints w/ further unit tests. ** i.e. where am I supposed to integrate tests for this? Can I add an extra test class just for this?
@merks Maybe you have an idea for this: I cannot seem to delete the "project encoding" markers. If I delete them, the pref change listener re-creates them right away, so that to the user this has the effect of "not deleted at all" * I checked the event type, the build kind and the delta type - but they're all exactly the same for both cases: ** I delete the marker from the Problems view *** I re-build (or clean and rebuild) the projects on the workspace What I want is to be able to delete the markers - and then have a project-rebuild re-add the markers again. Turning off "on access refresh" had no effect here. I'm out of ideas atm.

Here's what I did

Change

Added the preference with a default value (which is WARN (as it was the severity before)
Added a pref change listener as a service to remove/add/update the markers accordingly
Added a severity provider class b/c I need access to the severity in both the new pref change listener I added and the existing ValidateProjectEncoding class
Tested that provider using Mockito (which I added to the dependencies of the resources test fragment) ** Since I cannot mock a static constant (static methods can be mocked w/ Mockito, but constants apparently can't), I decided to use the same package for the test class so that I can override the "package-protected" getter for the InstanceScope.

Manual Integration Test

Tested w/ my fork of eclipse.platform.ui
Modifying the severity (except "ignore") updated the markers as expected
Modifying the severity to "ignore" removed the markers as expected
Modifying the severity from "ignore" to something else added the markers were needed

Thank you in advance for having a look at the PR!

Kind regards Ingo

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/1/

merks commented 2 years ago

It generally looks good. The corresponding nature test is here:

org.eclipse.core.tests.resources.NatureTest

I wonder if the ProjectEncodingPreferenceSeverityProvider is a bit overkill because I don't see a corresponding thing for the Nature checking; I think it's using package protected helper methods.

I'm not sure I understand the problems with deleting the markers. is the behavior you see different from deleting the marker for a missing nature?

ingomohr commented 2 years ago

Hello @merks . Sorry for the delay, and thank you for the response.

Apparently, you can't remove the "unknown nature" marker, either. Since this (to me) is inconsistant with the standard Eclipse behavior, in which you can remove problem markers (and that will turn up again once you've built the project again), I'll give it another round and see whether I can fix this for the new preference.

Thanks for the hint w/ the NatureTest, too.

ingomohr commented 2 years ago

Change After 1st Review

(amended the one commit in the PR)
Added unit tests (they all passed on the local setup; CI will give final answer)
Removed ProjectEncodingPreferenceSeverityProvider and used static-"package protected"-method approach instead ** (consistency w/ the other impl)

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/4/

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/5/

merks commented 2 years ago

Note that the tests that are failing all appear to pass on master:

https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/master/lastBuild/testReport/org.eclipse.core.tests.resources/

ingomohr commented 2 years ago

Note that the tests that are failing all appear to pass on master:

https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/master/lastBuild/testReport/org.eclipse.core.tests.resources/

Good morning, @merks . I'm on it. Those regression tests still boggle my mind atm, but this should be resolvable. :)

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/6/

ingomohr commented 2 years ago

Update After Fixing Failing Regression Tests

The problem was that my preference change listener actually also did things when the resource changed
- That, however, was already the job of ValidateProjectEncoding.
- After removing the unnecessary code CI went "green".
Problem is now:
- My pref change listener technically still is an IResourceChangeListener simply because I cannot register IPreferenceChangeListeners via OSGi service.
- I could register it at the activator - but instead I would like to add support for adding pref change listeners via service, too. However, I don't want to mix that into this PR.

What do you think, @merks ?

merks commented 2 years ago

@ingomohr Good question. I assume you'd need to listen to this one node in particular so the service would need to provide a fair bit of information to get the right preference node:

InstanceScope.INSTANCE.getNode(ResourcesPlugin.PI_RESOURCES);

@laeubi is likely to have some insights/thoughts about registering a preference listener as a service.

This area in the code accesses the instance preferences so that's probably the appropriate place to register a listener programmatically:

https://github.com/eclipse-platform/eclipse.platform.resources/blob/625a25fa7752eb3cfe33216d314ad82501a38c74/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java#L2242-L2246

laeubi commented 2 years ago

Problem is now:

I'm not sure if I fully understand what the problem is, but here is an example registering the listeners via DS:

https://github.com/eclipse-platform/eclipse.platform.resources/blob/master/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/CheckMissingNaturesListener.java

ingomohr commented 2 years ago

Problem is now:

I'm not sure if I fully understand what the problem is, but here is an example registering the listeners via DS:

https://github.com/eclipse-platform/eclipse.platform.resources/blob/master/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/CheckMissingNaturesListener.java

Hi @laeubi Thanks for the link. The problem is that the service accepts IResourceChangeListeners. What I want to register, however, is not an IResourceChangeListener but an IPreferenceChangeListener.

At the moment, I add that listener via service as resource change listener and don't implement the resourceChangedmethod - but that feels quite "hacky". :)

So, what we imho need to decide (for the ticket #166 to which this PR refers):

Do we keep the impl the way it is and create another issue to add the feature to add IPreferenceChangeListeners via service, too
or do I register the pref change listener for this PR at the Activator class instead ( ResourcesPlugin)?
or do we do something else?

merks commented 2 years ago

My 2 cents... Programmatically register to get this PR to closure. Then, if someone wants to work on being able to register a preference listener as a service, and that's feasible, they could do so and update this code accordingly.

ingomohr commented 2 years ago

My 2 cents... Programmatically register to get this PR to closure. Then, if someone wants to work on being able to register a preference listener as a service, and that's feasible, they could do so and update this code accordingly.

Thanks @merks, I'll do that.

ingomohr commented 2 years ago

Change

Pref change listener is no hacked resource change listener anymore
Registered the pref change listener at the suggested location when the Workspace opens up
Deregistered the pref change listener when the Workspace closes
Performed manual integration test w/ UI fork: still works as expected
Executed new unit tests and all before-failing unit tests locally: they all still pass (CI will tell, however)
Tested deregistering pref change listener via debugging. (I'm not sure whether this is supposed to covered w/ a unit test - or if so: which is the best location for that).

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/8/

laeubi commented 2 years ago

Please don't register resourcechange listeners in the activator, this will most likely fail if the workspace is not ready yet. You don't need to implement any service when you create a component so something like:

@Component(service = {})
public class MyPreferenceChangeListener implements IPreferenceChangeListener {

    @Reference
    private IWorkspace workspace;

    @Reference
    private ILog log;

    @Reference(target = IScopeContext.BUNDLE_SCOPE_FILTER)
    IScopeContext bundleScope;

    private IEclipsePreferences eclipsePreferences;

    @Activate
    public void register() {
        eclipsePreferences = bundleScope.getNode(""); //$NON-NLS-1$
        eclipsePreferences.addPreferenceChangeListener(this);
    }

    @Deactivate
    public void unregister() {
        eclipsePreferences.removePreferenceChangeListener(this);
    }
}

should work here.

merks commented 2 years ago

It's being done in the Workspace.open not in the activator... Other things access the INSTANCE scope preferences there already...

laeubi commented 2 years ago

It's being done in the Workspace.open not in the activator... Other things access the INSTANCE scope preferences there already...

Yes and too much work is there already done, actually I won't add more things here if not absolutely necessary, registering a resource change listener is not such thing that requires early workspace access...

ingomohr commented 2 years ago

Thanks for the feedback. It's not a resource change listener, though. It's a prefs change listener. How do you suggest to register it?

ingomohr commented 2 years ago

Sorry, I missed the snippet above. Will try that approach.

ingomohr commented 2 years ago

Change

Workspace class is now untouched again
Pref change listener is registered/deregistered via OSGi service
- Thanks for the hint! I didn't know this would work.
- Both registration and deregistration were called as expected
Performed manual integration tests w/ my UI fork: still worked as expected (/)

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/9/

ingomohr commented 2 years ago

Change

Removed unwanted line in ProjectEncodingPreferenceChangeListener that initialized the workspace that was actually already injected.

Performed manual integration tests w/ UI fork again. Still worked as expected.

ingomohr commented 2 years ago

@merks @laeubi : Quick question: From the old Gerrit-workflow I remember that the Eclipse projects were interested in having only one commit per issue. That's why I amended my commit for this PR all the time. Is this still what you want? (Amending commits typically makes re-reviews harder - at least that's my take-away from working w/ Bitbucket Server).

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/10/

merks commented 2 years ago

@merks @laeubi : Quick question: From the old Gerrit-workflow I remember that the Eclipse projects were interested in having only one commit per issue. That's why I amended my commit for this PR all the time. Is this still what you want? (Amending commits typically makes re-reviews harder - at least that's my take-away from working w/ Bitbucket Server).

Yes, this is typically what people do. You can also do a chain of commits, and then squash and merge at the end:

merks commented 2 years ago

@iloveeclipse Do you have any outstanding concerns? This looks good to go now...

iloveeclipse commented 2 years ago

@iloveeclipse Do you have any outstanding concerns? This looks good to go now...

I haven't tested/reviewed, but it looks like the new test isn't added to the test suite and so it will be not executed.

iloveeclipse commented 2 years ago

@ingomohr: it would be easier to validate this change for reviewers if you could push a PR for platform UI with the preference page change, even if it is not ready yet.

ingomohr commented 2 years ago

@iloveeclipse Do you have any outstanding concerns? This looks good to go now...

I haven't tested/reviewed, but it looks like the new test isn't added to the test suite and so it will be not executed.

@iloveeclipse You mean org.eclipse.core.tests.resources.AllTests, yes? I would have added the test if I had known the test should be added there.

iloveeclipse commented 2 years ago

Just look around where other tests are included and add it. I have no IDE right now, but most of Eclipse tests are referenced by some testsuite.

ingomohr commented 2 years ago

Change

Added ProjectEncodingTest to suite org.eclipse.core.tests.resources.AllTests.
- (Thought: Maybe moving to JUnit5 and getting rid of extra-steps like adding tests to suites would be a good idea in the future)
Fixed ValidateProjectEncoding to update existing markers's severity
Removed DRY violating content from ProjectEncodingPreferenceChangeListener

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/11/

iloveeclipse commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/11/

Now you see +10 tests: https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/11/testReport/org.eclipse.core.tests.resources/

Fine. Next, please throw away most listener related code as explained before.

ingomohr commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/11/

Now you see +10 tests: https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/11/testReport/org.eclipse.core.tests.resources/

Fine. Next, please throw away most listener related code as explained before.

Yup. Seen those test results on Jenkins. DRY violating code is gone.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/12/

ingomohr commented 2 years ago

@ingomohr: it would be easier to validate this change for reviewers if you could push a PR for platform UI with the preference page change, even if it is not ready yet.

I created PR 220 for this.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/13/

merks commented 2 years ago

What @iloveeclipse suggested does sound like a simplification. That being said, I do understand that it's starting to seem like each person that is asked for an opinion pushes in a different direction. That doesn't make contributing feel very rewarding but more of a gauntlet. I think we can all recognize that a lot of time has been invested here and if focused feedback had been provided early, the process would have been shorter.

ingomohr commented 2 years ago

What @iloveeclipse suggested does sound like a simplification. That being said, I do understand that it's starting to seem like each person that is asked for an opinion pushes in a different direction. That doesn't make contributing feel very rewarding but more of a gauntlet. I think we can all recognize that a lot of time has been invested here and if focused feedback had been provided early, the process would have been shorter.

Thank you for the update/feedback, @merks. That matches my feeling, too. So, I'll drop the pref-change-listener-via-service approach and add that listener to the CharsetManager. I'll do that tonight.

iloveeclipse commented 2 years ago

Please compare https://github.com/eclipse-platform/eclipse.platform.resources/pull/170 with current code. That does exactly same with less code / less complexity.

ingomohr commented 2 years ago

Please compare #170 with current code. That does exactly same with less code / less complexity.

Thanks for the hint, @iloveeclipse. The only thing I'm not keen on in your PR is the inner class for the preference change listener. I used a lambda implementation instead b/c it's much smaller. If you specifically want that inner class to happen, just let me know.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/14/

iloveeclipse commented 2 years ago

@trancexpress : Simeon, as you've originally wrote the code, could you please also look at the PR please? The corresponding part for the UI with new preference is https://github.com/eclipse-platform/eclipse.platform.ui/pull/220.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/15/

trancexpress commented 2 years ago

I tested the latest changes here and https://github.com/eclipse-platform/eclipse.platform.ui/pull/220/commits/39b6cd3edb7708d23798a2ee76dbcabbcbd5cf59, LGTM.

The plug-in customization line to disable the marker (I assume the only customization of interest):

org.eclipse.core.resources/missingEncodingMarkerSeverity=-1

Please mention this in the commit message.

(plug-in customization is specified with command line argument for Eclipse: -pluginCustomization /.../plugin_customization.ini)

ingomohr commented 2 years ago

I tested the latest changes here and eclipse-platform/eclipse.platform.ui@39b6cd3, LGTM.

The plug-in customization line to disable the marker (I assume the only customization of interest):
org.eclipse.core.resources/missingEncodingMarkerSeverity=-1
Please mention this in the commit message.

(plug-in customization is specified with command line argument for Eclipse: -pluginCustomization /.../plugin_customization.ini)

I'm not sure I fully understand you here. I never heard of plug-in customization for preferences before. I updated the commit message (I hope) accordingly.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-167/17/

eclipse-platform / eclipse.platform.resources