Bug 242719 add SpinnerFieldEditor #2179
Conversation
sebthom
force-pushed
the
spinner-field-editor
branch
from
2c95380
to
a11af47
Compare
sebthom
changed the title
sebthom
force-pushed
the
spinner-field-editor
branch
5 times, most recently
from
ba50234
to
6d7d98c
Compare
sebthom
force-pushed
the
spinner-field-editor
branch
4 times, most recently
from
da489ce
to
f03795a
Compare
sebthom
force-pushed
the
spinner-field-editor
branch
from
f03795a
to
eef38e7
Compare
sebthom
changed the title
sebthom
force-pushed
the
spinner-field-editor
branch
2 times, most recently
from
2b2fed3
to
2d0a7af
Compare
HannesWell
force-pushed
the
spinner-field-editor
branch
from
2d0a7af
to
d60b19f
Compare
|
Thank you for this enhancement. Can anyone of the committers that is more involved into UI topics review this? |
There was a problem hiding this comment.
Thank you for your contribution!
Since adding this PR means increasing API, I think it is extra important to make sure the comments for all methods are speaking.
In particular, many of the javadoc-comments refer to another method instead of "just telling me what the method does".
The code looks sound, so do the test.
Is there any use for this componet? It would be great to see this component as a real component used in some follow-up-down-the-line-PR
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Contributors: | ||
| * IBM Corporation - initial API and implementation |
There was a problem hiding this comment.
Is this file copied from somewhere? The copyright header looks off somehow.
| public class SpinnerFieldEditor extends FieldEditor { | ||
|
|
||
| /** | ||
| * Value that will feed {@link Spinner#setDigits(int)}. |
There was a problem hiding this comment.
These comments don't help me understand the code.
| * Value that will feed {@link Spinner#setDigits(int)}. | |
| * amount of digits to display (?) by default (?) |
| * @param min the value used for {@link Spinner#setMinimum(int)}. | ||
| * @param max the value used for {@link Spinner#setMaximum(int)}. | ||
| */ | ||
| public SpinnerFieldEditor(final String name, final String labelText, final Composite parent, final int min, |
There was a problem hiding this comment.
This feels like an oddly specific constructor. Maybe consider providing a builder here?
| return incrementValue; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
A lot of these javadoc comments seem redundant. In particular, I am talking about comments referring to the setter of a getter.
| /** | |
| /** {@return the maximum value this spinner may take on (10 by default) } |
| } | ||
|
|
||
| /** | ||
| * Set the value to be used for Spinner.setDigits(int) and update the spinner. |
There was a problem hiding this comment.
| * Set the value to be used for Spinner.setDigits(int) and update the spinner. | |
| * Sets the number of decimal places used by the receiver. | |
| * @see Spinner#setDigits(int) |
This is a follow-up PR for https://bugs.eclipse.org/bugs/show_bug.cgi?id=242719
Similar to the approach in that issue I created the SpinnerFieldEditor based on the latest code of ScaleFieldEditor.