Community
Participate
Working Groups
Many API's from E4 are still marked as internal. This is annoying because: * We get warnings when programming * we can't distinguish API from non API Is there a reason to keep them internal or can I start pushing changes to make them official?
Which API?
Why did I think that so many E4 API is still internal? It is just a few packages. Anyway, I have an application that uses the following packages a lot and it is riddled with warnings because of it. * o.e.e4.core.commands ECommandService, EHandlerService * o.e.e4.core.services.log ILoggerProvider, ILogger * o.e.e4.core.services BasicResourceProvider, IResourcePool, etc.. * o.e.e4.ui.css.swt.theme IThemeManager, ITheme, IThemeEngine * o.e.e4.services.adapter Adapter
> Anyway, I have an application that uses the following packages a lot and it > is riddled with warnings because of it. > > * o.e.e4.core.commands > ECommandService, EHandlerService > > * o.e.e4.core.services.log > ILoggerProvider, ILogger > > * o.e.e4.core.services > BasicResourceProvider, IResourcePool, etc.. > > * o.e.e4.ui.css.swt.theme > IThemeManager, ITheme, IThemeEngine > > * o.e.e4.services.adapter > Adapter +1! Particularly for ECommandService, EHandlerService, IThemeManager, ITheme & IThemeEngine :)
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/175102
Beware, more API means much higher maintenance cost and technical debt, distracting from actual value creation... Before jumping to eagerly in publishing everything, we should audit was parts of the API are useful for external consumers and try to hide everything that's not obviously useful.
(In reply to Mickael Istria from comment #5) > Beware, more API means much higher maintenance cost and technical debt, > distracting from actual value creation... Before jumping to eagerly in > publishing everything, we should audit was parts of the API are useful for > external consumers and try to hide everything that's not obviously useful. +1. Looking at org.eclipse.e4.core.commands: it contains 4 classes with partly missing documentation, tags "@noimplement", "@noinstantiate" and "@noreference" on every second method, lot of warnings, missing java docs etc. It is not API, it is a bunch of quickly hacked *internal* code.
(In reply to Andrey Loskutov from comment #6) > (In reply to Mickael Istria from comment #5) > > Beware, more API means much higher maintenance cost and technical debt, > > distracting from actual value creation... Before jumping to eagerly in > > publishing everything, we should audit was parts of the API are useful for > > external consumers and try to hide everything that's not obviously useful. > > +1. Yes, I agree as well. I will be very prudent. > > Looking at org.eclipse.e4.core.commands: it contains 4 classes with partly > missing documentation, tags "@noimplement", "@noinstantiate" and > "@noreference" on every second method, lot of warnings, missing java docs > etc. > > It is not API, it is a bunch of quickly hacked *internal* code. I have added documentation for EHandlerService and ECommandService. These two classes are definitely API. The other two classes are not API. Usage seems limited to *internal*. Do you think I can move these to the internal package?
> I have added documentation for EHandlerService and ECommandService. These > two classes are definitely API. > > The other two classes are not API. Usage seems limited to *internal*. Do you > think I can move these to the internal package? no unless you want break every application because CommandServiceAddon is not available anymore.
(In reply to Thomas Schindl from comment #8) > > I have added documentation for EHandlerService and ECommandService. These > > two classes are definitely API. > > > > The other two classes are not API. Usage seems limited to *internal*. Do you > > think I can move these to the internal package? > > no unless you want break every application because CommandServiceAddon is > not available anymore. I think you can mark it with @noreference @noimplements to make it non API.
(In reply to Lars Vogel from comment #9) > (In reply to Thomas Schindl from comment #8) > > no unless you want break every application because CommandServiceAddon is > > not available anymore. I have updated the e4xmi's. Why would every application break? Now that we have you here Tom, what do you think about the API proposal of comment 2. > > I think you can mark it with @noreference @noimplements to make it non API. Ok, that is a lot less intrusive.
(In reply to Wim Jongman from comment #10) > (In reply to Lars Vogel from comment #9) > > (In reply to Thomas Schindl from comment #8) > > > > no unless you want break every application because CommandServiceAddon is > > > not available anymore. > > I have updated the e4xmi's. Why would every application break? which one? You can't update mine unless you add a preprocessor. > > Now that we have you here Tom, what do you think about the API proposal of > comment 2. o.e.e4.core.commands: I'm fine with makeing `EHandlerService` & `ECommandService` API but marking them @noimplement as they are today. o.e.e4.core.services.log: I don't think this a good logging API, one would implement today o.e.e4.core.services: I don't see any of those classes you mention, they are from tools o.e.e4.ui.css.swt.theme: Ok but need to be marked @noimplement o.e.e4.services.adapter: indifferent - IIRC I once had a bug filed on that topic that I think Adapters should work differently but I can't remember exactly what that was
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/175290
Lets focus in this bug on ECommandService and EHandlerService.
(In reply to Lars Vogel from comment #13) > Lets focus in this bug on ECommandService and EHandlerService. Sure, the change is done and can be merged.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/175290 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=1b4100d2faab4de8dbbcb36896f8761b4e675ae1
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/180413
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/180413 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d6a09ebba07ec95ac16aabbcddf8d8e0907e3e3a
Wim, could you add this to the N&N page for 4.20?
(In reply to Lars Vogel from comment #18) > Wim, could you add this to the N&N page for 4.20? Sure, but where is the repo that contains this again? Can't find it in the platform web repo.
(In reply to Wim Jongman from comment #19) > (In reply to Lars Vogel from comment #18) > > Wim, could you add this to the N&N page for 4.20? > > Sure, but where is the repo that contains this again? Can't find it in the > platform web repo. See https://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/180692
Gerrit change https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/180692 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=b1b65ca15b2505f7dfbb94fada0ee113c57cc13f
The result of this is that XWT and consequently Papyrus are broken. Surely the 'correct' way to promote xxx.internal.YYY to public API is to add xxx.YYY and to change xxx.internal.YYY to derive from xxx.YYY? xxx.internal.YYY can then be marked as deprecated giving a reasonable period for applications to adapt. After all this bug was triggered by annoyance that internal API had to be used, so it wasn't actually internal so it must undergo deprecation. REOPENing and CRITICAL since a significant SimRel contributor is unnecessarily broken.
(In reply to Ed Willink from comment #23) > The result of this is that XWT and consequently Papyrus are broken. > > Surely the 'correct' way to promote xxx.internal.YYY to public API is to add > xxx.YYY and to change xxx.internal.YYY to derive from xxx.YYY? > > xxx.internal.YYY can then be marked as deprecated giving a reasonable period > for applications to adapt. After all this bug was triggered by annoyance > that internal API had to be used, so it wasn't actually internal so it must > undergo deprecation. > > REOPENing and CRITICAL since a significant SimRel contributor is > unnecessarily broken. Package were not changed AFAICS. See https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/175290 Please clarify.
(In reply to Lars Vogel from comment #24) > Please clarify. Sorry Bugzilla often doesn't talk to me. Sorry again I don't understand or trust multi stage Gerrits. From my study of the code as exemplified by the content of an RC2.zip, the old ...internal... has changed to skip the internal package name, thereby breaking anyone who uses the old names.
Ed, please check the code directly and let us know what you think is wrong.
I checked also and the only commit for this package in the recent year was 1b4100d2faab4de8dbbcb36896f8761b4e675ae1 which did not change the package
Hi Ed, I see https://git.eclipse.org/r/c/xwt/org.eclipse.xwt/+/180977, which increases the ranges to: org.eclipse.e4.core.commands;bundle-version="[0.12.900,2.0.0)". Also XWT from https://download.eclipse.org/xwt/milestones-1.7.0/1.7.0-S/ seems to install correctly. Though the documentation does not include any quick example that can be used to quickly test it. Only problem is that XWT is still disabled in simrel, see the email from Wayne Beaton on the cross project issue list.
