Community
Participate
Working Groups
The current implementation of Color in SWT does not actually require disposal of the Color instances in practice as no OS resources are allocated, so nothing to dispose. I believe the Color requiring disposal was an artifact of limited color depth displays that no longer exist in practice anymore. This appears to be what https://www.eclipse.org/articles/Article-SWT-Color-Model/swt-color-model.htm written in 2001 says. I think SWT has exceeded the original use cases envisioned in that design document, specifically: "a certain number of fixed colors are decided upon for usage in the plug-in". Would SWT team accept a patch that removed the dispose requirement from the API doc? And/or add setBackground(RGB)/setForeground(RGB) to GC? This came up while I was working on Bug 540737 which is to add 24-bit color support to the Terminal. Managing the resources that I am supposed to dispose of is non-trivial and in practice unneeded.
I think this is fine idea. Sravan, what do you think?
(In reply to Alexander Kurtakov from comment #1) > I think this is fine idea. Sravan, what do you think? I Think its a very good idea. If a patch is provided I will review it for Mac.
I am working on a gerrit.
I pushed a series of gerrits (162993-162998) The first one (https://git.eclipse.org/r/#/c/162993/) covers just javadoc changes to document that disposal is not required. The rest add new constructors and do other documentation fixes. I will apply the changes to all three platforms for the rest of the commits once someone has had a first look at them to make sure I am on the correct track.
I suggest to add this also to the N&N. Obviously we did not know about this, as we use dispose almost everywhere for colors
A consequence of this change is that it will never be possible in the future to change the implementation so that Color once again requires disposal, should such a need arise. That would break backwards compatibility. Is this something that has been considered and deemed to be an acceptable? Also, is there any chance that there are SWT implementations for non-standard platforms, implemented by third parties, which require Color to be disposed, and would break with this change? If these issues have been considered and accepted I think it sounds like a nice simplification.
(In reply to Jonah Graham from comment #4) > I pushed a series of gerrits (162993-162998) I'd just like to give you a tip about the "topic" feature in Gerrit: https://gerrit-review.googlesource.com/Documentation/user-upload.html#topic It might be useful to mark all Gerrit changes with the same topic in situations like this: git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental -o topic=color-dispose
Thanks, Jonah
Thanks Lars for merging. What should I do with the remaining commits, should I create a separate bug?
(In reply to Jonah Graham from comment #9) > Thanks Lars for merging. What should I do with the remaining commits, should > I create a separate bug? Please cc me in the Gerrits
(In reply to Jens Lideström from comment #6) > A consequence of this change is that it will never be possible in the future > to change the implementation so that Color once again requires disposal, > should such a need arise. That would break backwards compatibility. > > Is this something that has been considered and deemed to be an acceptable? > > Also, is there any chance that there are SWT implementations for > non-standard platforms, implemented by third parties, which require Color to > be disposed, and would break with this change? > > If these issues have been considered and accepted I think it sounds like a > nice simplification. Thanks for raising these questions. I don't know answers myself.
(In reply to Lars Vogel from comment #10) > (In reply to Jonah Graham from comment #9) > > Thanks Lars for merging. What should I do with the remaining commits, should > > I create a separate bug? > > Please cc me in the Gerrits Done. Note that the remaining Gerrits are not ready, see comment 4.
(In reply to Jonah Graham from comment #11) > (In reply to Jens Lideström from comment #6) > > A consequence of this change is that it will never be possible in the future > > to change the implementation so that Color once again requires disposal, > > should such a need arise. That would break backwards compatibility. > > > > Is this something that has been considered and deemed to be an acceptable? > > > > Also, is there any chance that there are SWT implementations for > > non-standard platforms, implemented by third parties, which require Color to > > be disposed, and would break with this change? > > > > If these issues have been considered and accepted I think it sounds like a > > nice simplification. > > Thanks for raising these questions. I don't know answers myself. In trying to answer some of these questions, I can start with what was said in Bug 546835 that removed the last of the actual need for disposal: (Nikita Nemkin from Bug 546835 comment #8) > On 256-bit color displays (which are completely obsolete), everything will > be drawn with a reduced palette. > On 16-bit and 32-bit color displays (which is the norm), nothing will change. As for other implementations of SWT, is this a theoretical problem, or are there any known implementations of SWT, and if so are they active?
Added missing xrefs to gerrit (see Bug 563149 for why it wasn't automatic)
(In reply to Jens Lideström from comment #7) > I'd just like to give you a tip about the "topic" feature in Gerrit: Thanks for the tip - https://git.eclipse.org/r/#/q/topic:color-dispose+(status:open+OR+status:merged)
(In reply to Jonah Graham from comment #13) > As for other implementations of SWT, is this a theoretical problem, or are > there any known implementations of SWT, and if so are they active? I remember that I have at some point heard about such things, but I don't know. I don't have much knowledge about this, but I don't think it will be an issue. I don't think there are any active such projects. I guess that the regular SWT developers will know if it where a problem. I found this project after a quick net search: http://swtswing.sourceforge.net/main/index.html It implements SWT with a Swing backend. It hasn't been updated since 2007 though.
Reopening, since there are 5 outstanding patches linked to this bug. I think we shouldn't deprecate old Color constructors, that's a lot of new warnings for little gain. We can't stop inheriting from Resource and can't deprecate dispose() anyway. Also, to preserve existing behavior, any Colors created and returned by SWT should have a Device reference (i.e. use an old constructor). (In reply to Jens Lideström from comment #6) > A consequence of this change is that it will never be possible in the future > to change the implementation so that Color once again requires disposal, > should such a need arise. That would break backwards compatibility. I doubt it will ever come to this, but finalizers and reference queues are always an option. > Also, is there any chance that there are SWT implementations for > non-standard platforms, implemented by third parties, which require Color to > be disposed, and would break with this change? 3rd party implementations (of which there are none in use) shouldn't prevent SWT evolution.
(In reply to Jens Lideström from comment #16) > (In reply to Jonah Graham from comment #13) > > As for other implementations of SWT, is this a theoretical problem, or are > > there any known implementations of SWT, and if so are they active? > > I remember that I have at some point heard about such things, but I don't > know. > > I don't have much knowledge about this, but I don't think it will be an > issue. I don't think there are any active such projects. I guess that the > regular SWT developers will know if it where a problem. > > I found this project after a quick net search: > > http://swtswing.sourceforge.net/main/index.html > > It implements SWT with a Swing backend. It hasn't been updated since 2007 > though. The other SWT ports were listed in the SWT website, but that content has been archived now as the ports are no longer active - https://www.eclipse.org/swt/archived_content.html
New Gerrit change created: https://git.eclipse.org/r/163204
(In reply to Nikita Nemkin from comment #17) > Reopening, since there are 5 outstanding patches linked to this bug. > > I think we shouldn't deprecate old Color constructors, that's a lot of new > warnings for little gain. We can't stop inheriting from Resource and can't > deprecate dispose() anyway. > > Also, to preserve existing behavior, any Colors created and returned by SWT > should have a Device reference (i.e. use an old constructor). I believe I have addressed the above concerns. Once the patches[1] have been reviewed I will apply the same changes to the other ports. [1] https://git.eclipse.org/r/q/topic:%2522color-dispose%2522+status:open
(In reply to Nikita Nemkin from comment #17) > Reopening, since there are 5 outstanding patches linked to this bug. > > I think we shouldn't deprecate old Color constructors, that's a lot of new > warnings for little gain. We can't stop inheriting from Resource and can't > deprecate dispose() anyway. > > Also, to preserve existing behavior, any Colors created and returned by SWT > should have a Device reference (i.e. use an old constructor). I the past all Color instances have returned a Device instance from Resource#getDevice. After this change that method might will throw SWTException(SWT.ERROR_GRAPHIC_DISPOSED). Is that a problem? It is probably rare for someone to call getDevice on a Color. But there might be some framework somewhere that does. I don't know if this is a problem that is worth taking into consideration, I'm just raising the question. One alternative is to give each Color a standards Device, like this: https://git.eclipse.org/r/#/c/162994/5/bundles/org.eclipse.swt/Eclipse+SWT/gtk/org/eclipse/swt/graphics/Color.java@53
New Gerrit change created: https://git.eclipse.org/r/163388
New Gerrit change created: https://git.eclipse.org/r/163389
Please fix for RC1.
(In reply to Dani Megert from comment #24) > Please fix for RC1. I will split most of the remaining work into Bug 563560 - e.g. the new API seems to require additional discussion/review. The main thing left on this bug is N&N entry and making sure existing API documentation is correct. I'll provide a further comment with the gerrits that are still relevant and require review shortly.
Removed gerrits that are not relevant from See Also. They have been moved to Bug 563018. The gerrit that still needs review & merge for 4.16 is https://git.eclipse.org/r/#/c/163204/ - Improve description of Color.dispose - which was created as a result of a comment in another review. The N&N gerrit will be here soon.
New Gerrit change created: https://git.eclipse.org/r/163555
Gerrit change https://git.eclipse.org/r/163204 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=68dac2b31c95dd12c485fe519da1f04ed284ff0b
Gerrit change https://git.eclipse.org/r/163555 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=90195681f221d0cc2dd3bb1106aedf68031ac6b5
(In reply to Jonah Graham from comment #26) > Removed gerrits that are not relevant from See Also. They have been moved to > Bug 563018. ^ this points to the current bug. Do you mean some other bug? > > The gerrit that still needs review & merge for 4.16 is > https://git.eclipse.org/r/#/c/163204/ - Improve description of Color.dispose > - which was created as a result of a comment in another review. > > The N&N gerrit will be here soon. Both merged.
(In reply to Alexander Kurtakov from comment #30) > (In reply to Jonah Graham from comment #26) > > Removed gerrits that are not relevant from See Also. They have been moved to > > Bug 563018. > > ^ this points to the current bug. Do you mean some other bug? Yes. Bug 563560. > > > > > The gerrit that still needs review & merge for 4.16 is > > https://git.eclipse.org/r/#/c/163204/ - Improve description of Color.dispose > > - which was created as a result of a comment in another review. > > > > The N&N gerrit will be here soon. > > Both merged. Thank you. I think that means we are done with this bug and can continue in 4.17 timeframe in Bug 563560
