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

Fix slider #1619

Merged
merged 3 commits into from
Oct 13, 2024
Merged

Fix slider #1619

merged 3 commits into from
Oct 13, 2024

Conversation

tinchodias
Copy link
Contributor

@tinchodias tinchodias commented Oct 7, 2024

  • Fixes in slider's Morphic adapter
    • Value: presenter and widget were not syncing correctly
    • Horizontal slider didn't work except when min=0 and max=1.
    • Vertical slider was wrong (not it becomes an ignored presenter property)
    • Value was supporting (unexpectedly) parsing from string, with special adhoc support for fractions
    • Label didn't update when change after opened
  • Fixes in slider presenter
    • Delete color:, which doesn't work and anyway it should by set via styles
  • Fixes on test case for slider's adapter
    • Rename SpSliderPresenterBackendTest -> SpSliderAdapterTest
    • Add tests (only 2 smoke tests before)

Partially fixes #1613

- Value: presenter and widget were not syncing correctly
- Horizontal slider didn't work except when min=0 and max=1.
- Vertical slider was wrong (not it becomes an ignored presenter property)
- Value was supporting (unexpectedly) parsing from string, with special adhoc support for fractions
- Label didn't update when change after opened
- Rename SpSliderPresenterBackendTest -> SpSliderAdapterTest
- Add tests (only 2 smoke tests before)
@tinchodias tinchodias mentioned this pull request Oct 7, 2024
- This kind of visual property needs to be defined through styles.
- It does not work (self does not understand #widget), so there are no users
@Ducasse Ducasse merged commit fed1607 into pharo-spec:Pharo13 Oct 13, 2024
2 checks passed
@astares
Copy link
Contributor

astares commented Oct 14, 2024

Will this also fix #1410 and #899
(which is related to SpSliderPresenter as well)?

@tinchodias
Copy link
Contributor Author

Yes, thank you for the references @astares. I commented in both issues now. I can't close them, don't have permissions.

@astares
Copy link
Contributor

astares commented Oct 14, 2024

@tinchodias
I checked in a fresh P13 image as of a few minutes ago:

Pharo13.0.0SNAPSHOT
Build information: Pharo-13.0.0+SNAPSHOT.build.292.sha.4b69f175d6873a03ffbad68a892e671a6ad69ef1 (64 Bit)

and it is not fixed there. But the code is not in yet.

I think the merge in the Spec repo does not yet trigger an image build to include the Pharo13 branch again into standard image. We have to wait for a merge in the standard repo so that the image build is triggered.

If you have the code loaded you can try with

SpDemo open

and see if the slider on "Forms" dialog with the "Scale:" label behaves correctly.

@tinchodias
Copy link
Contributor Author

I've worked on P12(*) but you can see it working:

Oct-14-2024 14-13-14

(*): More exactly:
Pharo 12.0.0
Build information: Pharo-12.0.0+SNAPSHOT.build.1530.sha.b368d31b34edb6e488d36358613c2eae00c2f495 (64 Bit)

@tinchodias
Copy link
Contributor Author

For the record: I just realized I didn't mention that @tesonep worked on these changes. We spotted together the issues in a pair-programming session, and performed the main fixes. Then I refactored and fixed some more, before creating the PR.
Cheers

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 this pull request may close these issues.

Slider presenter
3 participants