Don't remove parts from part stack on DnD drop #2812
Conversation
There was a problem hiding this comment.
Thank you for this PR, the code change looks perfectly fine and with the example-project and the instructions you gave in #2771 (comment) I was able to reproduce the problem and verify the fix.
The only request I have is to migrate the test to JUnit-5 as we have discussed in the dev-call.
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; |
There was a problem hiding this comment.
Could you please update this and the containing test-suite, to use JUnit-5 as you mentioned in the dev-call last week that migrating this test class to Junit-5 isn't trivial.
There was a problem hiding this comment.
@HannesWell thank you for your comment. As we discussed I migrated the tests from JUnit4 to JUnit5.
Unfortunately there were ugly warnings regarding JUnit4 access. I could not get rid of them and removed JUnit4 completely. That resulted in refactoring 2 'parameterized tests' to JUnit5 and therefore I have more changes as expected.
Although the tests run successfully on Friday I want to take a look at them again today and will push them today or tomorrow.
I could also create another PR for migrating the tests from JUnit 4->5 (b/c of the many changes in 2 test classes). Just let me know what you prefer.
There was a problem hiding this comment.
That resulted in refactoring 2 'parameterized tests' to JUnit5 and therefore I have more changes as expected.
Even better, great! :)
I could also create another PR for migrating the tests from JUnit 4->5 (b/c of the many changes in 2 test classes).
Please create a dedicated PR just for that migration and once that is merged, this can be submitted afterwards.
FYI, I'll be on vacation now, but either somebody else will review that or I'll have a look at it when I'm back.
There was a problem hiding this comment.
Sounds good :) So I leave this one as it is for now and update it after the "JUnit migration" PR has been merged.
| @@ -11,6 +11,7 @@ | |||
| * Contributors: | |||
| * IBM Corporation - initial API and implementation | |||
| * Patrik Suzzi <[email protected]> - Bug 497348 | |||
| * Oliver Lins <[email protected]> - Issue 2771 | |||
There was a problem hiding this comment.
It's better to explain the change in a half-sentence here. If you really want to reference an issue, it's better to use the full GH URL as issue-ids are not as unique anymore as they used to be in Eclipse Bugzilla.
There was a problem hiding this comment.
Thank you for the hint. If I mention an issue I will use a short description or the full URL.
To be honest, I'm not sure if I should mention myself at all. I tried to follow the "contribution.md" and the style I found. Is it common practice to mention the author of changes?
There was a problem hiding this comment.
In the distant past it was required to list the contributors, but given that the very same information is well tracked in Git itself it's no longer necessary.
(I generally don't do this when I contribute and prefer other people don't do it either. But that's my personal approach and opinion.)
There was a problem hiding this comment.
@merks Thank you for your approach/opinion. It sounds logical to me.
There was a problem hiding this comment.
I usually do it only if I make significant changes to a class, e.g. did a major rework or added a major new feature.
E.g. if a file is part of a mass refactoring or if the change is small I usually don't do that.
But what I do more often (and what you could definitively do) is update the second/later year in the copyright notice to 2025.
p-O-q
force-pushed
the
issue-2771
branch
2 times, most recently
from
fb5afb3 to
a9e331e
Compare
Currently when dropping elements onto a part stack MStackElements are removed if their element ids are equal. This leads to disappearing parts in case of - at least one part created using part descriptors allowing multiple instances is present in the part stack - another such part is dropped onto the part stack Following the comments/notes the intention is to remove placeholder parts with the same element id as the element's id being dropped. This fix ensures that only instances of type MPlaceholder are removed from the part stack. Fixes eclipse-platform#2771
|
@merks Should I request a new review using the above button? Do you have time to review the code? |
|
I see the tests look very nice and modern and are running in the verification build. 🏆 |
|
Excellent work. Thanks! ❤️ |
|
@merks Thank you for accepting and merging this PR :) |
Currently when dropping elements onto a part stack MStackElements are removed if their element ids are equal. This leads to disappearing parts in case of
Following the comments/notes the intention is to remove place holder parts with the same element id as the element's id being dropped.
This fix ensures that only instances of type MPlaceholder are removed from the part stack.
Fixes #2771