Community
Participate
Working Groups
Bug 539014 introduce SWT builder classes in JFace and in Bug 546990 we started using it. No complains so far, time to release them as API.
Marcus, can you prepare a Gerrit?
New Gerrit change created: https://git.eclipse.org/r/148720
(In reply to Eclipse Genie from comment #2) > New Gerrit change created: https://git.eclipse.org/r/148720 Should we furthermore move the internal parts (basically everything object which does not end with *Factory) to an internal package, like o.e.jface.widgets.internal? Seems that internal packages are unusual in o.e.jface.
Getting a lot of errors like 08:24:03 * Marker [on: /org.eclipse.jface/src/org/eclipse/jface/bindings/BindingManager.java, id: 252, type: org.eclipse.pde.api.tools.api_usage, attributes: [apiMarkerID: 3, charEnd: 22737, charStart: 22727, lineNumber: 662, message: BindingManager illegally references method Tracing.printTrace(String, String), messagearguments: Tracing#BindingManager#printTrace(String, String), org.eclipse.jdt.internal.core.JavaModelManager.handleId: =org.eclipse.jface/src<org.eclipse.jface.bindings{BindingManager.java[BindingManager~computeBindings~QMap;~QMap;~QMap;~QMap;, problemTypeName: org.eclipse.jface.bindings.BindingManager, problemid: 640712815, severity: 1, sourceId: API Tools], created: 9/3/19 6:24 AM] 08:24:03 * Marker [on: /org.eclipse.jface/src/org/eclipse/jface/bindings/CachedBindingSet.java, id: 253, type: org.eclipse.pde.api.tools.api_usage, attributes: [apiMarkerID: 3, charEnd: 12388, charStart: 12378, lineNumber: 356, message: CachedBindingSet illegally references method Tracing.printTrace(String, String), messagearguments: Tracing#CachedBindingSet#printTrace(String, String), org.eclipse.jdt.internal.core.JavaModelManager.handleId: =org.eclipse.jface/src<org.eclipse.jface.bindings{CachedBindingSet.java[CachedBindingSet~setPrefixTable~QMap;, problemTypeName: org.eclipse.jface.bindings.CachedBindingSet, problemid: 640712815, severity: 1, sourceId: API Tools], created: 9/3/19 6:24 AM] 08:24:03 * Marker [on: /org.eclipse.jface/src/org/eclipse/jface/viewers/TreeSelection.java, id: 254, type: org.eclipse.pde.api.tools.api_usage, attributes: [apiMarkerID: 3, charEnd: 1017, charStart: 1004, lineNumber: 32, message: TreeSelection leaks APIs from the superclass StructuredSelection, messagearguments: StructuredSelection#TreeSelection, org.eclipse.jdt.internal.core.JavaModelManager.handleId: =org.eclipse.jface/src<org.eclipse.jface.viewers{TreeSelection.java[TreeSelection, problemTypeName: org.eclipse.jface.viewers.TreeSelection, problemid: 576778288, severity: 1, sourceId: API Tools], created: 9/3/19 6:24 AM] 08:24:03 * Marker [on: /org.eclipse.jface/src/org/eclipse/jface/viewers/deferred/AbstractConcurrentModel.java, id: 261, type: org.eclipse.pde.api.tools.api_usage, attributes: [apiMarkerID: 3, charEnd: 875, charStart: 852, lineNumber: 25, message: AbstractConcurrentModel implements non-API interface IConcurrentModel, messagearguments: IConcurrentModel#AbstractConcurrentModel, org.eclipse.jdt.internal.core.JavaModelManager.handleId: =org.eclipse.jface/src<org.eclipse.jface.viewers.deferred{AbstractConcurrentModel.java[AbstractConcurrentModel, problemTypeName: org.eclipse.jface.viewers.deferred.AbstractConcurrentModel, problemid: 576725006, severity: 1, sourceId: API Tools], created: 9/3/19 6:24 AM] I didn't change any of these classes. Any idea what this is about and how I can get rid of it?
(In reply to Marcus Höpfner from comment #4) > Getting a lot of errors like > > > 08:24:03 * Marker [on: > /org.eclipse.jface/src/org/eclipse/jface/bindings/BindingManager.java, id: > 252, type: org.eclipse.pde.api.tools.api_usage, attributes: [apiMarkerID: 3, > charEnd: 22737, charStart: 22727, lineNumber: 662, message: BindingManager > illegally references method Tracing.printTrace(String, String), > messagearguments: Tracing#BindingManager#printTrace(String, String), > org.eclipse.jdt.internal.core.JavaModelManager.handleId: > =org.eclipse.jface/src<org.eclipse.jface.bindings{BindingManager. > java[BindingManager~computeBindings~QMap;~QMap;~QMap;~QMap;, > problemTypeName: org.eclipse.jface.bindings.BindingManager, problemid: > 640712815, severity: 1, sourceId: API Tools], created: 9/3/19 6:24 AM] > 08:24:03 * Marker [on: > /org.eclipse.jface/src/org/eclipse/jface/bindings/CachedBindingSet.java, id: > 253, type: org.eclipse.pde.api.tools.api_usage, attributes: [apiMarkerID: 3, > charEnd: 12388, charStart: 12378, lineNumber: 356, message: CachedBindingSet > illegally references method Tracing.printTrace(String, String), > messagearguments: Tracing#CachedBindingSet#printTrace(String, String), > org.eclipse.jdt.internal.core.JavaModelManager.handleId: > =org.eclipse.jface/src<org.eclipse.jface.bindings{CachedBindingSet. > java[CachedBindingSet~setPrefixTable~QMap;, problemTypeName: > org.eclipse.jface.bindings.CachedBindingSet, problemid: 640712815, severity: > 1, sourceId: API Tools], created: 9/3/19 6:24 AM] > 08:24:03 * Marker [on: > /org.eclipse.jface/src/org/eclipse/jface/viewers/TreeSelection.java, id: > 254, type: org.eclipse.pde.api.tools.api_usage, attributes: [apiMarkerID: 3, > charEnd: 1017, charStart: 1004, lineNumber: 32, message: TreeSelection leaks > APIs from the superclass StructuredSelection, messagearguments: > StructuredSelection#TreeSelection, > org.eclipse.jdt.internal.core.JavaModelManager.handleId: > =org.eclipse.jface/src<org.eclipse.jface.viewers{TreeSelection. > java[TreeSelection, problemTypeName: > org.eclipse.jface.viewers.TreeSelection, problemid: 576778288, severity: 1, > sourceId: API Tools], created: 9/3/19 6:24 AM] > 08:24:03 * Marker [on: > /org.eclipse.jface/src/org/eclipse/jface/viewers/deferred/ > AbstractConcurrentModel.java, id: 261, type: > org.eclipse.pde.api.tools.api_usage, attributes: [apiMarkerID: 3, charEnd: > 875, charStart: 852, lineNumber: 25, message: AbstractConcurrentModel > implements non-API interface IConcurrentModel, messagearguments: > IConcurrentModel#AbstractConcurrentModel, > org.eclipse.jdt.internal.core.JavaModelManager.handleId: > =org.eclipse.jface/src<org.eclipse.jface.viewers. > deferred{AbstractConcurrentModel.java[AbstractConcurrentModel, > problemTypeName: org.eclipse.jface.viewers.deferred.AbstractConcurrentModel, > problemid: 576725006, severity: 1, sourceId: API Tools], created: 9/3/19 > 6:24 AM] > > > > I didn't change any of these classes. Any idea what this is about and how I > can get rid of it? I don't know. But let's see what jenkin tells us about the next patchset (i just uploaded).
I have some feedback about the widget factory functionality. I am sorry that I give this feedback at the last minute, when you are about to publish the API. ## The big thing: Does not cover much of SWT I think clients will find it weird and confusing that so few of the SWT widgets are included, and so few of their properties are supported. I think this feature isn't really finished until it covers more of SWT. I discovered that you already had a discussion about this at Bug 546227 and decided that it's better to publish the functionality first, and then later extend it. I understand your reasons, but I want to give my opinion anyway. 1. Only a small selection of widgets are included. It would be good to include factories also for the most of the other SWT widgets, including those in the custom package. It will be weird for clients if only some of the widgets are supported. For example: - Combo - CCombo - Scale - Slide - Etc. 2. Only some of the properties of the controls are available on the builders. It would be good to include support for most other widget properties. For example: - background - backgroundImage - foreground - visible - menu - different kinds of listener. ## Smaller practical things 3. It is not good to expose the `layout` property for many controls that are not logically composites: Tree, Table, Spinner. I think it would be better make those factories not extend AbstractCompositeFactory. 4. This method should be renamed, to avoid overloading on Object. If the argument lambda has a slightly wrong type then the wrong overload will be called: AbstractControlFactory#layoutData(Supplier<Object>) 5. This method can use a wildcard to be more flexible for clients: AbstractControlFactory#layoutData(Supplier<?>) 6. Are the factories meant to be extended by clients? It might be a good idea to disallow extension to make it easier to adjust the implementation, at least during an initial period. ## Small new features 7. It would be useful with some specialized factory methods for common cases. For example: WidgetFactory.newPushButton() // Short for newButton(SWT.PUSH) WidgetFactory.newButton() // Short for newPushButton(SWT.NONE)
(In reply to Jens Lideström from comment #6) > I have some feedback about the widget factory functionality. I am sorry that > I give this feedback at the last minute, when you are about to publish the > API. > > ## The big thing: Does not cover much of SWT > > I think clients will find it weird and confusing that so few of the SWT > widgets are included, and so few of their properties are supported. I think > this feature isn't really finished until it covers more of SWT. > > I discovered that you already had a discussion about this at Bug 546227 and > decided that it's better to publish the functionality first, and then later > extend it. I understand your reasons, but I want to give my opinion anyway. First of all thanks for your feedback. Please see my comments below. > > 1. Only a small selection of widgets are included. It would be good to > include factories also for the most of the other SWT widgets, including > those in the custom package. It will be weird for clients if only some of > the widgets are supported. > > For example: > - Combo > - CCombo > - Scale > - Slide > - Etc. You are right. It is only a few. You mentioned the bug where it was decided to go this way (start with a few, extend later). Any contributions are welcome. Please create commits for the controls/widgets you need. > 2. Only some of the properties of the controls are available on the > builders. It would be good to include support for most other widget > properties. > > For example: > - background > - backgroundImage > - foreground > - visible > - menu > - different kinds of listener. I thought these are special cases. At least I usually don't call this methods, at least not when constructing the widgets. I think this would overload the apis. > ## Smaller practical things > > 3. It is not good to expose the `layout` property for many controls that are > not logically composites: Tree, Table, Spinner. I think it would be better > make those factories not extend AbstractCompositeFactory. Tree, Table and Spinner extend Composite. The factories have the same hierarchy like the actual widgets/controls. So Tree extends Composite, then TreeFactory extends (Abstract)CompositeFactory. > 4. This method should be renamed, to avoid overloading on Object. If the > argument lambda has a slightly wrong type then the wrong overload will be > called: > > AbstractControlFactory#layoutData(Supplier<Object>) I do not get your point here. If the caller supplies a Supplier the above is called, when anything else is supplied #layoutData(Object layoutData) is called. > 5. This method can use a wildcard to be more flexible for clients: > > AbstractControlFactory#layoutData(Supplier<?>) Refer to Control#setLayoutData(Object layoutData). That's why Supplier<Object> is used. The factories can be seen as wrapper around the controls. Method signatures are kept as close as possible to the actual methods. > 6. Are the factories meant to be extended by clients? It might be a good > idea to disallow extension to make it easier to adjust the implementation, > at least during an initial period. You are right. GridDataFactory is final as well. I opened https://bugs.eclipse.org/bugs/show_bug.cgi?id=551375 for that. > > ## Small new features > > 7. It would be useful with some specialized factory methods for common > cases. For example: > WidgetFactory.newPushButton() // Short for newButton(SWT.PUSH) > WidgetFactory.newButton() // Short for newPushButton(SWT.NONE) That has been discussed in another bug and decided not do it. There are so many use cases. The next one wants to have a method like WidgetFactory.newPushButton(Sring text). It was decided to have atomic methods first. Shortcuts can be implemented by clients in own classes.
(In reply to Marcus Höpfner from comment #7) > > 3. It is not good to expose the `layout` property for many controls that are > > not logically composites: Tree, Table, Spinner. I think it would be better > > make those factories not extend AbstractCompositeFactory. > > Tree, Table and Spinner extend Composite. The factories have the same > hierarchy like the actual widgets/controls. So Tree extends Composite, then > TreeFactory extends (Abstract)CompositeFactory. Yes, understand that. My point is that it would be better to not let TreeFactory extend AbstractCompositeFactory, even if Tree extends Composite. Tree should not be used as a Composites by users. Users are not supposed to use the `setLayout` method on Tree. The factory API will be better if the factory for Tree doesn't have the `layout` method at all, given that it doesn't make sense to call that method. For example, the Javadoc on Tree says this: * Note that although this class is a subclass of <code>Composite</code>, * it does not normally make sense to add <code>Control</code> children to * it
(In reply to Marcus Höpfner from comment #7) > > 4. This method should be renamed, to avoid overloading on Object. If the > > argument lambda has a slightly wrong type then the wrong overload will be > > called: > > > > AbstractControlFactory#layoutData(Supplier<Object>) > > I do not get your point here. If the caller supplies a Supplier the above is > called, when anything else is supplied #layoutData(Object layoutData) is > called. My point is that when there is a overload with an Object argument then it is verify easy to call the wrong method by accident. For example, in this code the method layoutData(Object) is call with a Supplier argument, which will give an exception at runtime: Supplier<GridData> c = GridData::new; WidgetFactory.composite(SWT.NONE) .layoutData(c) .create(null); This can happen by accident when the code is initially written, but it's even more insidious when the code is refactored later. Because of this I think that the API would be slightly better if layoutData(Supplier) had a different name, such as `supplyLayoutData` or something similar.
(In reply to Marcus Höpfner from comment #7) > > 5. This method can use a wildcard to be more flexible for clients: > > > > AbstractControlFactory#layoutData(Supplier<?>) > > Refer to Control#setLayoutData(Object layoutData). That's why > Supplier<Object> is used. The factories can be seen as wrapper around the > controls. Method signatures are kept as close as possible to the actual > methods. The point of using layoutData(Supplier<?>) is that it is a little more convenient for users if they have a Supplier with a specific type already. For example, if they user happen to have a supplier like this: Supplier<GridData> s = GridData::new; The user should be able to directly use that supplier with the factory API. But currently only Supplier<Object> is allowed, not Supplier<GridData>. If we instead user Supplier<?> as the parameter that this becomes possible. This is not a common case, but I think this change makes the API a little bit better in some cases. And given there these is no disadvantage to it I think it should be done.
New Gerrit change created: https://git.eclipse.org/r/150011
I created the following change to address issues 3-6 in my post above: https://git.eclipse.org/r/#/c/150011/ * TreeFactory, SpinnerFactory and TableFactory extends AbstractControlFactory instead of AbstractCompositeFactory. * Concrete factories are made final * Extension from abstract factories is block by using package private constructors. * layoutData(Supplier<Object>) is renamed and given a wildcard.
I'd like to just say that I think the widget factories look very well designed generally, and that they will be verify useful. I don't know if I've made that clear. The feedback and suggestions I have is mainly about details.
(In reply to Jens Lideström from comment #13) > I'd like to just say that I think the widget factories look very well > designed generally, and that they will be verify useful. > > I don't know if I've made that clear. The feedback and suggestions I have is > mainly about details. Thanks a lot for making that clear :) Your proposals are very helpful to make the api better. I appreciate it. Let's wait for the commits to be merged. Afterwards we can merge https://git.eclipse.org/r/148720
New Gerrit change created: https://git.eclipse.org/r/150052
New Gerrit change created: https://git.eclipse.org/r/150053
8. This is purely a style issue (and you might have discussed it already): The package name org.eclipse.jface.widgets is not very descriptive. I think it would be easier for readers if the package is called org.eclipse.jface.widgetsfactory. (Or widgetfactories.) I know that the corresponding package for layout factories is call org.eclipse.jface.layout. I don't think that is a good name either, and I think it would be better not to repeat that mistake.
Gerrit change https://git.eclipse.org/r/150052 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=03a4b239e89afe5cefc59e7928724836506504fe
Gerrit change https://git.eclipse.org/r/150053 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=90b26aa21d4455c0950826c85d452d7e48438747
What else is to be done? - javadoc? @Jens: did you create a bug for that? - package name? Bug? Although I have create gerrits for further controls I'd say we could release the API with the current set of controls. We can create bugs for concrete widgets that are missing.
(In reply to Marcus Höpfner from comment #20) > What else is to be done? > > - javadoc? @Jens: did you create a bug for that? #52065 > - package name? Bug? I didn't create any bug. Do it if you want do; this is just a suggestion.
(In reply to Jens Lideström from comment #21) > (In reply to Marcus Höpfner from comment #20) > > What else is to be done? > > > > - javadoc? @Jens: did you create a bug for that? > > #52065 > > > - package name? Bug? > > I didn't create any bug. Do it if you want do; this is just a suggestion. Ok. Then I'd suggest to wait for https://bugs.eclipse.org/bugs/show_bug.cgi?id=552065 and then close this bug and release the widget factories.
Gerrit change https://git.eclipse.org/r/148720 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=46f5465e354e632c5b9eae2d39279cc4deea7f36
Thanks you Marcus, Matthias and Jens. I'm looking forward to use this new API to build Uis. Please add to the N&N.
(In reply to Lars Vogel from comment #24 > Please add to the N&N. Marcus?
