Multifix resolution for removal of bundle/package from Quick Fix #685

[lathapatil]

Fixes #685

[github-actions]

Test Results

   291 files  ±0     291 suites  ±0   1h 4m 34s ⏱️ + 12m 28s
 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 53b1d4d. ± Comparison against base commit e39979b.

♻️ This comment has been updated with latest results.

[gireeshpunathil]

@lathapatil - when I tested your patch I did not see any difference unfortunately! it may be that the bundle references are removed rom the Require-Bundle header actually, but not reflected in the manifest editor.

I remember there is a way to sync the manifest content in the view and the in-memory copy; but forgot which one. can you pls check if there is one such thing?

[lathapatil]

@lathapatil - when I tested your patch I did not see any difference unfortunately! it may be that the bundle references are removed rom the Require-Bundle header actually, but not reflected in the manifest editor.

I remember there is a way to sync the manifest content in the view and the in-memory copy; but forgot which one. can you pls check if there is one such thing?

It is reflecting in manifest file as well when I test from my workspace / branch. How do I reproduce this?

[gireeshpunathil]

• add two new, bogus bundles against Require-Bundle:

 Require-Bundle: org.eclipse.ui,
 foo,
 bar

• select quick fix
• select both the errors
• apply

I see only one getting removed (the pre-existing behaviour).

[gireeshpunathil]

@lathapatil - I don't want to block this PR just because I am unable to see the change - may be I am doing some silly mistake. So I request @HannesWell to have a look and advise.

[HannesWell]

@lathapatil - I don't want to block this PR just because I am unable to see the change - may be I am doing some silly mistake. So I request @HannesWell to have a look and advise.

I'll have a look at it as soon as possible, but I'm quite busy this week, so I probably won't have the time before the weekend.

[lathapatil]

@lathapatil - I don't want to block this PR just because I am unable to see the change - may be I am doing some silly mistake. So I request @HannesWell to have a look and advise.

@gireeshpunathil I've observed the same behavior you mentioned when unresolved bundles are added at the end, as in the example you provided. However, when we add them in between, it seems to work fine. I'll need to debug this case further. I'll keep you updated on my findings.

[lathapatil]

@gireeshpunathil When I debugged this case, I found that while saving the removal of the first marker into the manifest file, the other markers were not updated properly; instead, they were deleted.

Working case -
Require-Bundle: bcpg;bundle-version="1.77.0",dummy1,dummy2,
bcprov.source;bundle-version="1.77.0"

Non-working case -
Require-Bundle: bcpg;bundle-version="1.77.0",
dummy1,dummy2,
bcprov.source;bundle-version="1.77.0"

After removal of the first bundle manifest file is being saved and when the file commit happens updateMarkers method of AbstractMarkerAnnotationModel is called and here I see the difference with annotationMap being populated in working case and empty in non-working case hence markers are not updated properly.

But I do not have deeper knowledge of these annotation models. It might take more time for me to check on this.. If you already know what's happening here you can help me out !

[gireeshpunathil]

@lathapatil - it is possible that we (not your changes, but the original design itself) are missing to connect all the markers that are relevant, that appear before the current line being altered.

for example:

• line x is being quick-fixed, markers for lines x+ are scanned and updated: working case
• line x is being quick-fixed, markers for lines x- are not scanned so not updated: failing case

so looks like you are in the right direction, focus on how the markers are selected for update - if we are moving forward in the document for searching of markers, we need to do it in both directions - at least in this case.

does it make sense at all?