-
Notifications
You must be signed in to change notification settings - Fork 185
Revert "Replace usages of new Image(device, width, height)..." #2162 #2163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "Replace usages of new Image(device, width, height)..." #2162 #2163
Conversation
|
Since a proper fix is not trivial, I will simply revert the commit and create a follow-up PR where 807b159 is improved and reintroduced. The follow-up PR will be targeted for M1. |
HeikoKlare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The revert is fine, as the original change obivous introduced a bug which we need to revert for the upcoming release.
But the provided regression test needs to be revised:
- It does not test anything specifically regarding transparent style of the Composite. It only tests that one specific kind of regression does not occur in the sense of not leading to an exception in case a specific setup for shell and composite around is used, but that has nothing to do with testing transparent style for composite itself but some very specific regression to composite.
- It creates a new shell even though the test class already provides infrastructure including a shell (and even a composite). In case another shell is required (which does not seem to be the case, as I can simply remove the additional shell), that one would at least need to be safely disposed after the test execution.
- Why is setting the layout to the shell necessary here? If the test is about transparent style, it should have nothing to do with the layout of the containing shell. If it has some relevance, it needs to become clear by proper test naming, commenting or the like.
- Same as for the point above applies to the paint listener to the composite.
417c37e to
bff30ab
Compare
|
I adapted the name of the test and used the The listener and the layout are necessary for the use case, otherwise the test doesn't run into the issue. Since this is a very complicated scenario and even getting a reproducer is not trivial, I simply took the reproducer from #2162 and stripped it down to the bare minimum. |
|
Check failures (tests in Linux are unrelated: I see the same for all 3 platforms. Very likely a follow-up of this failure: |
…e-platform#2162 This reverts commit 807b159. Add regression test. Fixes eclipse-platform#2162
bff30ab to
e4e5eec
Compare
HeikoKlare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the test improvement!
This reverts commit 807b159.
Add regression test.
Fixes #2162