introduce static helper method GC#drawOn #1063
Conversation
tobiasmelcher
requested review from
niraj-modi and
iloveeclipse
as code owners
|
@HannesWell : could you please take a look at this change? I had to introduce a new Runnable interface which passes the GC as argument. Is there already an existing interface I could re-use? Do we have to introduce the static method GC#drawOn in all platform specific GC implementations? |
|
Hello @tobiasmelcher, thank you for this PR. Sorry for my late reply but I just started my almost three week of vacation when you created this PR and when I came back a full TODO list was ready to be worked off.
Yes we do, but you can just import/open the fragments for the other platforms and they will compile in your workspace. For you it is probably sufficient to only have one arch per OS since they only differ in the embedded native binaries, but the code is the same for all archs on one OS.
Yes there is already |
tobiasmelcher
force-pushed
the
GC.drawOn
branch
from
d4fb76f
to
37519e2
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
Outdated
Show resolved
Hide resolved
...g.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/AnimatedProgress.java
Outdated
Show resolved
Hide resolved
...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolder.java
Outdated
Show resolved
Hide resolved
|
Thanks a lot @HannesWell for all the feedback. Could you please take a look and review my latest change eb1dc57 ? As already mentioned, I had some troubles with the formatting; I executed then the format action only on the changed coding parts. Is that ok? |
There was a problem hiding this comment.
As already mentioned, I had some troubles with the formatting; I executed then the format action only on the changed coding parts. Is that ok?
Yes, that's IMO the perfect decision.
Could you please also update the doc of the GC class to mention the new GC.create() factory (for all platforms).
With that I think this change is complete (I'll mark it as ready for review), but I would like give others some time to review.
| @@ -59,7 +59,22 @@ | |||
| * @see <a href="http://www.eclipse.org/swt/examples.php">SWT Examples: GraphicsExample, PaintExample</a> | |||
| * @see <a href="http://www.eclipse.org/swt/">Sample code and further information</a> | |||
| */ | |||
| public final class GC extends Resource { | |||
| public sealed class GC extends Resource { | |||
There was a problem hiding this comment.
It would be good to also update the doc of this class (where it mentions GC.dispose()) and recommend to use the new factory, ideally with the little example also used in the Image class.
And I think Windows95 and Windows98 are not the most relevant target OS anymore. Maybe that just should be generalized.
| * try (var gc = GC.create(i)) { | ||
| * gc.drawRectangle(0, 0, 50, 50)); | ||
| * } |
There was a problem hiding this comment.
Please apply this doc update also to the Image implementations for Linux and Mac.
which takes care of creation and disposal of the GC object Fixes: eclipse-platform#955
- format the changed coding parts - remove the GC.create calls at locations where the diff gets too large
| } | ||
| } | ||
|
|
||
| public static GC.Closeable create(Drawable drawable) { |
There was a problem hiding this comment.
The new method needs some java-doc on all platforms. The existing single argument constructor of GC is probably a good source of inspiration.
The existing GC constructor could also be adjusted to recommend this method for safety.
What's important is to add the tag @since 3.126 in the Java-doc in order to make the API-tools aware this method was just added.
There was a problem hiding this comment.
Thanks for the hint. I hope it is fine with 6443eca
| public final class GC extends Resource { | ||
| public sealed class GC extends Resource { | ||
|
|
||
| public static final class Closeable extends GC implements AutoCloseable { |
There was a problem hiding this comment.
This new class needs some java-doc on all platforms. I think a short text that mentions that this is an auto-closable extension of the GC class is sufficient. The create method could be linked with @see.
What's important is to add the tag @since 3.126 in the Java-doc in order to make the API-tools aware this method was just added.
tobiasmelcher
force-pushed
the
GC.drawOn
branch
2 times, most recently
from
eb1dc57
to
6443eca
Compare
which takes care of creation and disposal of the GC object
Fixes: #955