Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slider presenter #1613

Closed
tinchodias opened this issue Oct 3, 2024 · 12 comments · Fixed by #1619
Closed

Slider presenter #1613

tinchodias opened this issue Oct 3, 2024 · 12 comments · Fixed by #1619

Comments

@tinchodias
Copy link
Contributor

  • 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
Copy link
Contributor Author

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
Copy link
Contributor Author

tinchodias commented Oct 3, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

Ducasse commented Oct 3, 2024

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

@tinchodias
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@tinchodias
Copy link
Contributor Author

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
Copy link
Contributor Author

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 tinchodias mentioned this issue Oct 7, 2024
@tinchodias
Copy link
Contributor Author

Not included in #1619:

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

@estebanlm
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

So, not yet included in #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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants