refix: quick-fix for extraneous empty line(s) in manifest editor

[gireeshpunathil]

Extraneous lines are erroneous to the manifest compiler, but it does not recommend / provide a resolution. Add a quick fix to remove extraneous line. takes care of the empty valid empty lines at the end of manifests.

Fixes: #563
Refs: #564
Refs: #572

/cc @iloveeclipse @vik-chand - please review. The issue with the previous one was that I was checking whether the pertinent line is the last one or not (in which case we ignore it, as the last line can be empty as per the spec), but that logic did not cater to multiple last lines.

The new version addresses this by getting the rest of the content in the file and trim it to see if those are empty. I tested it and looks like it works.

Also, I converted this as a warning, and removed the return statement, so that this does not block the build.

[github-actions]

Test Results

   291 files  ±0     291 suites  ±0   56m 58s ⏱️ - 4m 40s
 3 526 tests ±0   3 468 ✅ ±0   58 💤 ±0  0 ❌ ±0 
10 875 runs  ±0  10 698 ✅ ±0  177 💤 ±0  0 ❌ ±0 

Results for commit 4430347. ± Comparison against base commit 692af00.

♻️ This comment has been updated with latest results.

[HannesWell]

Since this change seems to be non trivial could you please add some tests for it?
Maybe one for the simple case and one for the one that caused problemlos in the first attempt?

[gireeshpunathil]

Since this change seems to be non trivial could you please add some tests for it?
Maybe one for the simple case and one for the one that caused problemlos in the first attempt?

I was looking for prior arts in manifest compiler test cases. I came across 2 types and both pose challenges to my scenario, so I thought I will ask here:

1. tests in https://github.com/eclipse-pde/eclipse.pde/tree/c835eb17aa9248cffad5beb47d367424c3e7f571/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/model/bundle: these validate the correctness of manifest format to great extent, however, none of those exercise the manifest parser.

2. https://github.com/eclipse-pde/eclipse.pde/blob/c835eb17aa9248cffad5beb47d367424c3e7f571/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/ee/ExecutionEnvironmentTests.java : performs full-blown build of a project, and requires the manifests and other artifacts in the disk (I guess).

Are there better options that does these?

• able to create a manifest in memory like string buffers
• able to compile it and collect the errors and warnings

[vik-chand]

Since this change seems to be non trivial could you please add some tests for it?
Maybe one for the simple case and one for the one that caused problemlos in the first attempt?

I was looking for prior arts in manifest compiler test cases. I came across 2 types and both pose challenges to my scenario, so I thought I will ask here:

1. tests in https://github.com/eclipse-pde/eclipse.pde/tree/c835eb17aa9248cffad5beb47d367424c3e7f571/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/model/bundle: these validate the correctness of manifest format to great extent, however, none of those exercise the manifest parser.

2. https://github.com/eclipse-pde/eclipse.pde/blob/c835eb17aa9248cffad5beb47d367424c3e7f571/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/ee/ExecutionEnvironmentTests.java : performs full-blown build of a project, and requires the manifests and other artifacts in the disk (I guess).

Are there better options that does these?

* able to create a manifest in memory like string buffers

* able to compile it and collect the errors and warnings

I will propose to do this (PDE compiler errors/warning) via a separate issue. API tools do have test case for API tool scenarios ( however there too , we don't have tests for quickfixes). In JDT, we have tests for JDT compiler errors and warnings. It will be good to have a set of test case for PDE compiler problems as well( not only extra lines but all of the other ones) ( we could look at both JDT as well as API tool test scenarios to get started with this work).

[vik-chand]

@gireeshpunathil Can you please update this patch wrt master?
@iloveeclipse Can you please review post the update from Gireesh?

[gireeshpunathil]

@vik-chand @HannesWell @iloveeclipse - pushed a change to improve the parsing, pls have a look. I have tested locally and everything looks ok; will continue to look for ways to add test scenarios in subsequent PR(s).

[iloveeclipse]

OK, last state is definitely better.

Please don't create multiple commits for this feature and squash previous commits with the next commit that would handle requested changes in one commit.

[vik-chand]

@gireeshpunathil For multi-fix, we may need to override public IMarker[] findOtherMarkers(IMarker[] markers) {
See AddAutomaticModuleResolution or ConfigureProblemSeverityForPDECompilerResolution as an example

[gireeshpunathil]

ok - multi-fix is new to me.

when I debugged, what I could see is:

• the parser builds a list of markers, with a set of line numbers.
• the resolver is called for each marker
• a resolution here means deleting a line and updating the doc
• this means subsequent markers are invalid, as the liner numbers referred by those are now different in the doc

any suggestions?

[laeubi]

the resolver is called for each marker

If I remember correctly, you can override a method to get all markers at once somehow to handle them as one batch.

[vik-chand]

For multi-fix of adding since tag ( in API Tools), after adding each since tag, the line number does change. It will be worth seeing what is done there to overcome this issue.

[gireeshpunathil]

did not find another resolver that does a similar thing.

I tried a hack by persisting the state across createChange calls, but soon realized that it wont work: the document in question is then subjected for race condition. The document modification in one thread fires an event in another, which in turn causes re-parsing of the manifest and potential generation of new markers.

so one reasonable approach I see is to aggregate all the warnings in one marker, and resolve it atomically. Might look little odd from the ui perspective, but I think it is the right thing to do from a use case point of view too: no one would want to fix one extraneous line and leave another untouched, do they?

WDYT?

[iloveeclipse]

Would work, sure. Just create a marker pointing to very first line. In theory one could simply avoid storing any line info (except first) and simply go over file and remove every empty line.

[gireeshpunathil]

Would work, sure. Just create a marker pointing to very first line. In theory one could simply avoid storing any line info (except first) and simply go over file and remove every empty line.

@iloveeclipse - pushed a change to this effect. please have a look!

[gireeshpunathil]

Please don't create multiple commits for this feature and squash previous commits with the next commit that would handle requested changes in one commit.

sure, will follow that in future. (in the previous project I worked, we leave the review commits separate for sometime, so that the reviewers can visually see the delta changes; and squash into one once things are ready to merge. I am still learning new practices everyday!)

[iloveeclipse]

Should be mostly OK now, just few smaller nitpicks.

[gireeshpunathil]

@iloveeclipse - I have been on vacation for almost a month, will address the review comments sometime this week.

[gireeshpunathil]

@iloveeclipse - apologies for the delay, was stuck with split priorities. I have addressed all the comments, pls have a look. thanks!

[gireeshpunathil]

I also found a strange thing while testing this. If I add empty lines between Bundle-SymbolicName: el;singleton:=true and Bundle-Version: 1.0.0.qualifier lines in the manifest, then it produces lot of errors like this:

I tested with master and can see it is a pre-existing scenario. Looks like the first fatal error caused skipping of the rest of the parsing, including the require-bundle validation. Will investigate outside of this PR. Just mentioning here to avoid confusion.

[gireeshpunathil]

@jukzi - did you add / modify any commits to this PR? I do see some actions above but do not see any files changed, so just wondering what it could have been! thanks.

@iloveeclipse - when you get some time, pls review this PR, or else let me know if you still have request changes.

[jukzi]

i only rebased

[gireeshpunathil]

I don't know what is the usual practices in this situation for this project; this PR is waiting for more than a month for review or cancellation of previously set X mark.

@iloveeclipse @HannesWell @vik-chand - pls advise.

[vik-chand]

I don't know what is the usual practices in this situation for this project; this PR is waiting for more than a month for review or cancellation of previously set X mark.

@iloveeclipse @HannesWell @vik-chand - pls advise.

@iloveeclipse Can you please review the latest change?

I have no time for re-review

[jukzi]

Would be more clear to add "empty" before "lines". That's even more explanatory then "extraneous"

[jukzi]

Please add a Juni Test that shows removing multiple lines works as expected.

[vik-chand]

Please add a Juni Test that shows removing multiple lines works as expected.

None of the quickfixes in Manifest file has JUnit. It will be a good idea to create a separate issue to create Junit test for quickfixes.

vik-chand doesn't require junit

[jukzi]

None of the quickfixes in Manifest file has JUnit.

@vik-chand then please you finish review.

[vik-chand]

None of the quickfixes in Manifest file has JUnit.

@vik-chand then please you finish review.

Sure, I will review this

[vik-chand]

My review comments -

1. For multiple empty lines, we just have 1 error marker. If we look at any other error marker and its behaviour, we generally have 1 error marker per problem. It is not obvious that the 2nd empty line is also an error in itself.

2. Multi-fix removes all lines not marked with error which again is confusing and I haven't seen such behavior in any pde or jdt or other quick fix. We should strive to be consistent. However trying multifix would require a preference value (which may or may not be present in the UI). Already we have too many preference options. Also it will require to deal with the line change number.

Based on this, either
A) We should not do multi-fix with this PR ( and make another issue out of it). I don't anticipate lot of work for that multifix issue.
B) We should align the multi-fix in this PR itself which is consistent with other multi-fixes in PDE and JDT.

JUnit for quickfixes of PDE ( on similar lines of JDT UI - I believe JDT UI has one) is a good idea. In any case, we should also create an issue for this so as to not lose this work item.

[gireeshpunathil]

thanks @vik-chand for the review. IMO, the problem is with the current design of quick-fix resolution framework in the empty lines context. we could very well attach one marker per error. But when it comes to the resolution site, applying of the fix poses challenge. When one fix is applied, it renders the rest of the markers invalid - due to the fact that the line numbers stored in those markers are changed due to the first resolution. Depending on relative position of the marker that is being resolved, the other markers may contain valid or invalid data.

I can go back to single-fix (from where we started originally), but that again may look a weak feature from end user perspective? let me dig a little more to see if there is a way to synchronize multiple markers w.r.t resolutions. If it proves to be impossible, I am happy to fall back to single fix. Ideas welcome!

[gireeshpunathil]

ok, I was doing a little digging around how to develop multi-fix in a fool-proof manner, and looked at how similar quick-fix implementations are doing.

• RemoveUnknownExecEnvironments: shows only one error in the first fault line, but resolution applies to all applicable lines.
• RemoveRequireBundleResolution: shows errors in all the relevant lines, but multi-fix leads to resolution to only the first item.
• RemoveImportPackageResolution: shows errors in all the relevant lines, quick-fix initially shows only on line, but upon selecting the resolution other lines appear in the view, but upon selecting multi-fix, only the first one gets removed.

I stopped looking further after these 3. In short, the multi-fix is already inconsistent, and looks like we went to and fro a couple of times in this PR in search of ideal behaviour.

I can invest more time on this, but would like to know if it makes sense to land this as is, given the prior arts. I can then spend time on implementing a marker aggregator function in the AbstractManifestMarkerResolution class that will help adjusting all the affected markers in-line with a single change. That will help me addressing all the above issues one and for all. WDYT?

@vik-chand @HannesWell

[vik-chand]

• RemoveImportPackageResolution

You can open a bug for RemoveRequireBundleResolution & RemoveImportPackageResolution

So with this PR we are mimicking this behavior ( RemoveUnknownExecEnvironments). I think in the quickfix if you make make it explicit( via text) that all extraneous lines will be deleted.

What do you think @HannesWell ?

[HannesWell]

I haven't follow the discussion closely, but in general I agree that it would be good if the quick-fix removes all extraneous empty lines on the first run. And I also think that it should be consistent and other quick-fixes in a similar situation should behave the same. IMHO it would be worth to invest some time into this. :)

[vik-chand]

Also please open an issue for junits for quickfixes

[HannesWell]

Just tried out this change and I get errors when there are entries outside the main section (which is valid):

Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: Host
Bundle-SymbolicName: host;singleton:=true
Bundle-Version: 1.0.0.qualifier
Bundle-RequiredExecutionEnvironment: JavaSE-11

Name: com/ibm/icu/text/TitlecaseTransliterator.class
SHA-256-Digest: bGXIJlHHRm7FZikT6z4B1DZw87oPajgDwhHlAojb5a0=

You can see these extra entries for example in all jar-signed artifacts in the TP. So I think an error in this case is a false-positive.

[gireeshpunathil]

@HannesWell - pushed a change to fix this issue, can you please have a look? thanks!

[laeubi]

@HannesWell anything left here?

[HannesWell]

@HannesWell anything left here?

I'll have to check it again in the next days, have to familiaries myself again with this change.