Community
Participate
Working Groups
Hi rienas, in this bug i'll post the ScaleRidget, SpinnerRidget and SliderRidget. All they are ITraverseRidgets. It would be great to get some hints, if they are not riena like... ;-) All my todos are marked with @TODO. This points i'll implement in the next days. Best greetings, Flo
Created attachment 132133 [details] TraversalRidgets
For the creation of the ridgets i used the currently existing code. So i have analysed the existing ridgets and tried to create my one similar to them.
Flo, thanks for the contribution. I'll provide feedback after M7 (=next week).
Created attachment 136871 [details] Fragment v1 - Converted contribution to fragment (File > Import... > Existing Projects into Workspace > Archive file) - Added Tests - Minor fixes
Uploaded wrong file, update on monday...
Created attachment 137172 [details] Fragment v2
Hi Florian, thanks again for your contributions. Please see my comments below. I'm actually quite happy with it as I don't have any high-level concerns. The comments below should be easily addressable. Thanks and kind regards, Elias. Legal stuff 1. Please add EPL copyright headers on all .java files that have no header. See http://www.eclipse.org/legal/copyrightandlicensenotice.php for details. 2. Please add yourself as an author (under contributors) for files derived from riena. Coding Style - We always use { }, even for 1-statement ifs: if(control != null) { foo(); } - We try to have a few sentences of javadoc for every class (even private ones) just to have an idea of it's purpose - Interfaces and abstract protected methods should have javadoc Code Review EventObserver.java - class javadoc is not up-to-date - EventObserver: public _final_ class ? - fireAction: shouldn't it be private? - for constistency: bindToUIControl; unbindFromUIControl (i.e. UI not Ui) - constructor: to be safe you should make a copy int[] events in the constructor; otherwise callers can still change the internal events array of EventObserver: int[] foo = new int[] { SWT.Selection }; EventObserver obs = new EventObserver(foo); foo[0] = 4711; // bad >> better: this.event = new int[ events.length ]; System.arraycopy(events, 0, this.events, 0, this.events.length); ITraverseRidget.java - it would be good to shortly explain the terms increment, pageincrement, min, max in the class javadoc. Right know the javadoc is not a big help if somebody is not already familiar with these concepts from SWT. - getters: what is the expected range for each return value (0 - x?; >0?); - setters: - what is the valid value range for each parameter (min, max, selection, increment, etc). - what happens when I call the setter with an 'illegal' value - is there a relationship between min and max - i.e. can max be less or equal to min? can they be negative? - selection setter - is there a value to 'clear' the selection (-1 / null) - Wouldn't it make more sense to rename get/setSelection() to get/setValue() since the 'selection' is really the 'value' of the ridgets? It also avoids confusion with ISelectableRidget. - the method getIncrement() should be added to the interface - how does the maximum, minimum, selection interact? r.setSelection(0); r.setMinimum(10); r.getSelection() ? r.setSelection(10); r.setMaximum(5); r.getSelection(); ? r.setMaximum(5); r.setMinimum(10); ? r.setMinimum(-5); r.setMaximumum(-1); ? For me it seems a good ideam to document and follow these SWT conventions: * is the selection is outside of the minimum / maximum it gets adjusted * when setting max to max < min - the set operation is ignored * when setting min to min > max - the set operation is ignored * selection and/or minimum must be >= 0 - Personally am a bit confused by having both methods setToolTipText(String) and setToolTipTemplate(String) on the same ridget. I recommend overriding setToolTipText(..) to do what setToolTipTemplate(...) does now (i.e. support the pattern arg.) and remove the PROPERTY_TOOL_TIP_TEMPLATE / get/setToolTipTemplate(...) - setToolTip[Pattern](null) should clear the tooltip AbstractTraverseRidget.java - class javadoc is not up-to-date - should only have a 0-argument constructor - package public methods update* + replace* - I think they all could be protected? - bindUIControl() - why does it only call updateUISelection() ? I think it should invoke all updateMethods() so that all stored state from the ridget is applied to a new control (i.e. min, max, increment, etc.) If subclasses are adding additional state they propably have to implement bindUIControl() too, so they can also initialize the control See #testApplySettingsOnBind() in AbstractTraverseRidgetTest - added createMarkerSupportMethod() that returns BasicMarkerSupport (since it's a IBasicMarkableRidget): @Override protected AbstractMarkerSupport createMarkerSupport() { return new BasicMarkerSupport(this, propertyChangeSupport); } - added call to super.unbind(): protected void unbindUIControl() { super.unbindUIControl(); IEventListener.java - what is the difference to IActionListener? IScaleRidget.java - javadoc ISliderRidget.java - javadoc - PROPERTY_THUMB - should have a value (i.e. "thumb") - value ranges for setters and getters ISpinnerRidget.java - javadoc - setters: what is the valid range for the arguments 'digits' / 'textLimit' ? - getters: what is the valid range for the return value ScaleRidget / SliderRidget / SpinnerRidget - class javadoc is not up-to-date - should only have a 0-argument constructor - isDisableMandatoryMarker - not needed; already inherited Tests - I've also added some test cases to force me use / understand / review the code. Some are failing by design; please have a look ;-).
Hi, i started with the refactoring of the concerns above. Flo
I have a 2 questions: - EventObserver: public final class ? Flo: i am not sure. Do you think the class should be final? I had a look at ActionOberserver. The EventObserver looks the same. IEventListener.java - what is the difference to IActionListener? Flo: It is the same. I thought using a own Listener type for an EventObserver. Do you think using the IActionListener instead? Thanks, Flo
hi, coming week i am in my tirolyan office. So i will continue the week after.
Hi Elias, Ekke noticed that i have completely forgotten to tell you about the delay of my contributions. Due to some busy weeks with my real job, redview and one week of holiday i could not find time to finish it. Some points are already changed. But some are not fixed yet. Sorry for that... Next i will post delays earlier... And thanks again for the really good description. In about 2 weeks redview should have a state to publish it. So i have more time. Greetings, Flo
hi, the important redview things are nearly finished. On saturday i'll continue the refactoring of my contributions. Sorry again for the delay, It was a busy time. Greetings, Flo
Hi, a little progress information. The TraverseRidgets are grown. - I have written a detailed java doc. - Fixed the bugs. Currently i have implemented the tests for property changes based on the java doc. Next point is the implementation of the eventTests, which already have been prepared by elias too. After finishing the tests, the selectionPattern incl. tests will be done. I think if the tests and the selectionPattern works fine, i can post them next week. Greetings, Flo
Hi, i have finished this contribution. Currently there is one TODO in the code as a question according a copied java doc from swt. On different places i have copied the general description of the method from swt and changed it to match the ridget, because swt has a really good and clear description. I am not sure whether i have to document this for copyright purposes??!! Differing to the description above i have implemented the toolTipPattern as an internal propertyChangedListener, because the setToolTipText() is final and so not inheritable. This causes a double call of setToolTipText() and a doubled property changed event. I am not sure, whether this a riena like function. Greetings, Flo
Created attachment 143138 [details] The fragment
Changed copyright will follow on evening.
Created attachment 144117 [details] Fragment with new copyrights
Florian: Good job! Please review the attached patch, coming shortly. Here's what I changed: - Javadoc: <br> -> <p>, < -> < > -> >) - removed IScaleRidget (no methods, this does not make sense because we can not ever add methods to an interface without breaking compatibility) - removed EventObserver (too similar to action observer) - moved the specific documentation regarding the value adjustments into the ridgets. The reason is that it is Ridget the specific part (=implementation, close to the widgets). The interfaces should be quite generic. - after that, I removed the setMaxinimum, setMinimum, setPageIncrement, setValue methods from ISliderRidget - it is very uncommon for interfaces to override methods they have inherited from other interfaces. If you have Foo that is an ISliderRidget and an ITraverseRidget which method wins? Before we start the IP-review I need you to do the following things: 1. Please modify the contributors statement in all places where you have copied javadoc from SWT (I think ITraverse/Slider/Spinner-Ridget). It shoould have an entry like this: XYZ Corporation - javadoc; copied from XYZFile.java licensed under XYZ license and attach the updated patch to bugzilla. 2. A confirmation in bugzilla that: a. You authored 100% of the contribution (except where indicated otherwise) b. You have the rights to donate the content to Eclipse 3. About illegal values. Do you thing we could throw an IllegalArgumentException instead. This is more similar to what we do elsewhere. I can change this after the patch is reviewed. Kind regards, Elias.
Created attachment 146025 [details] Patch with traversal ridgets from Florian's fragment
hi, the points are nearly finished. One test case left. This evening i'll finish it. Greetings, Flo
Created attachment 146305 [details] The new patch Hi Elias, i have changed some links in the javadoc, which had a method as target, but required the class header. The copyright of the IBM Corporation was added. The InvalidArgumentException has been added. The test cases are up to date. Greetings, Flo
Created attachment 146306 [details] mylyn/context/zip
Hi Elias, I can confirm that: 1. I authored 100% of the contribution (except the test cases written by you) 2. I have the rights to donate the content to Eclipse Thanks for your help... Florian
Looks good! @Christian: please open a CQ. Thanks, Elias.
Thank you Florian! :-) The contribution is approved by the IP-team. I will commit it today.
Resolved in HEAD.
Hi Elias, you are welcome. ;-) Flo
