Spec icon indicating copy to clipboard operation
Spec copied to clipboard

Slider presenter

Open tinchodias opened this issue 1 year ago • 12 comments

  • beHorizontal (default): doesn't work except when min=0 and max=1.
  • beVertical doesn't work
  • SpSliderPresenterBackendTest: only basic smoke tests + should end with "AdapterTest"

tinchodias avatar Oct 03 '24 13:10 tinchodias

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 avatar Oct 03 '24 13:10 tinchodias

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 avatar Oct 03 '24 15:10 tinchodias

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.

tinchodias avatar Oct 03 '24 15:10 tinchodias

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

Ducasse avatar Oct 03 '24 19:10 Ducasse

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

  • Remove beVertical and beHorizontal from the slider presenter API, and use the automatic orientation from the Morphic widget (and make other backends have this behavior)
  • Remove the automatic orientation from Morphic widget and make beVertical and beHorizontal work.

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

@estebanlm ?

tinchodias avatar Oct 03 '24 22:10 tinchodias

More in the presenter API to be fixed: Screenshot 2024-10-03 at 20 26 53

tinchodias avatar Oct 03 '24 23:10 tinchodias

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 avatar Oct 07 '24 19:10 tinchodias

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 avatar Oct 07 '24 21:10 tinchodias

Not included in #1619:

  • SpSliderPresenter>>color:
  • SpSliderPresenter's marks API
  • vertical/horizontal property (the widget determines the orientation in base on width/height ratio)

tinchodias avatar Oct 07 '24 22:10 tinchodias

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.

estebanlm avatar Oct 08 '24 09:10 estebanlm

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 avatar Oct 08 '24 15:10 tinchodias

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

  • SpSliderPresenter's marks API --> I can't find any traces in the slider Morph of these marks
  • vertical/horizontal property (the widget determines the orientation in base on width/height ratio)

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

tinchodias avatar Oct 08 '24 16:10 tinchodias