pharo-spec / Spec

Spec is a framework in Pharo for describing user interfaces.
MIT License
62 stars 65 forks source link

Slider presenter #1613

Closed tinchodias closed 1 month ago

tinchodias commented 1 month ago
tinchodias commented 1 month ago

Like this, it works:

s := SpSliderPresenter new.
s min: 0.
s max: 1.
s quantum: 0.1.
s value: 0.3.
s open.

Like this, it is broken:

s := SpSliderPresenter new.
s min: 0.
s max: 10.
s quantum: 1.
s value: 3.
s open.

giff

tinchodias commented 1 month ago

The effect of beVertical is really weird:

s := SpSliderPresenter new.
s min: 0.
s max: 10.
s quantum: 0.2.
s beVertical.
s open.

giff2 this is related to these sentences in SpMorphicSliderAdapter>>#buildWidget:

    self presenter isHorizontal ifFalse: [ 
        preWidget := TransformationMorph new asFlexOf: preWidget.
        preWidget transform withAngle: 90 degreesToRadians negated ].

Nevertheless, the Morphic slider changes its orientation automatically by comparing width/height, as shown in this capture: giff3 So, the presenter's orientation is ignored.

tinchodias commented 1 month ago

Another sub-issue: the adapter tolerates string values, including a special care about fractions. See SpMorphicSliderAdapter>>#value::

value: aValue

    | value |   
    value := aValue isNumber
        ifTrue: [ aValue ] 
        ifFalse: [ 
            (aValue includes: $/)
                ifTrue: [ (NumberParser on: aValue) nextFraction ]
                ifFalse: [ aValue asNumber ] ].

    ^ self presenter value: value asFloat

This is candidate to be removed. Just accept numbers is a better option.

Ducasse commented 1 month ago

Yes please what a bad idea. May be it should also accept XML, YAML, Java and more! What a strange idea.

tinchodias commented 1 month ago

@Ducasse What is your opinion about orientation? 2 options:

Both presenter API orientation and automatic orientation are not possible at the same time.

@estebanlm ?

tinchodias commented 1 month ago

More in the presenter API to be fixed:

Screenshot 2024-10-03 at 20 26 53
tinchodias commented 1 month ago

Another bug: SpSliderPresenter has this API, but has no effect on the widget:

addMark: aString at: aValue
    "Add the mark `aString` at `aValue` ."

    ^ self marks: (self marks
        add: (SpSliderMark new
            value: aValue;
            text: aString;
            yourself);
        yourself)

at least it has no senders, and I tested it with:

s := SpSliderPresenter new.
s min: 0.
s max: 100.
s quantum: 1.
s value: 100.
s addMark: 'mid' at: 50.
s open.
tinchodias commented 1 month ago

Additionally, a slider can have a label. This works when set before open, but needs a manual action to trigger a refresh to show thie label when set after open: Oct-07-2024 18-08-38

s := SpSliderPresenter new.
s min: 0.
s max: 100.
s quantum: 1.
s value: 100.
s open.
s label: 'Label of Slider'.
tinchodias commented 1 month ago

Not included in #1619:

estebanlm commented 1 month ago

hi, @tinchodias no, color: should not be fixed, it should be removed. color and that stuff needs to be defined through styles, not hardcoding it.

tinchodias commented 1 month ago

Thanks @estebanlm good point, I agree. I'm going to delete the color: method from SpSliderPresenter. No need to deprecate it as it is overriding an accessor from SpAbstractWidgetPresenter.

tinchodias commented 1 month ago

So, not yet included in https://github.com/pharo-spec/Spec/pull/1619:

I propose, for both cases, to remove the API. The alternatives would be a) to just deprecate instead of removing or b) implement it