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 _SelectionModel and _MultipleSelectionModel to sync using indices #1262

Merged
merged 12 commits into from Apr 10, 2017

Conversation

Projects
None yet
2 participants
@jasongrout
Member

jasongrout commented Apr 6, 2017

Fixes #1091

  • Fix javascript side
  • Close #880 when this is merged.

CC @maartenbreddels

@maartenbreddels

This comment has been minimized.

Member

maartenbreddels commented Apr 6, 2017

If we sync label too, that would be useful in some cases, what do you think?

jasongrout added some commits Apr 6, 2017

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 6, 2017

@maartenbreddels, can you give this a try and see how it works?

If we sync label too, that would be useful in some cases, what do you think?

Possibly, though I don't like having two possibly conflicting sources of truth in the protocol, and it becomes confusing on who sets it (frontend or backend? Both I think have a valid usecase).

Clean up python selection side extensively.
Make selection slider have a nonempty selection.

@jasongrout jasongrout force-pushed the jasongrout:selections branch from f4f89d8 to 47ad6fb Apr 6, 2017

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 6, 2017

If we sync label too, that would be useful in some cases, what do you think?

I'd much rather have the label derived from the index independently on the frontend and backend.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 7, 2017

Two design decisions:

(a) in this implementation, the default selection is no selection (except for the selection slider, which must have something selected

(b) We always coerce the options list to a ((label, value), (label, value)...) tuple. I think before it kept whatever object was originally assigned. I like this better - it emphasizes the ordered aspect of the options. However, for convenience, we still allow (and even expand) possible input types to include any mapping or iterable.

Thoughts?

jasongrout added some commits Apr 7, 2017

Select the first entry by default
We can still explicitly select no entry by having an empty options list or by explicitly setting the index, value, or label to None.
@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 7, 2017

(a) in this implementation, the default selection is no selection (except for the selection slider, which must have something selected

I went ahead and changed this to always select the first item (if there is a first item) on initialization or when changing the options. That seems to work better with interact, for example, where you just want some value by default.

However, I left the multiple select to still unselect all items when changing the options or setting it initially. Thoughts?

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 7, 2017

However, I left the multiple select to still unselect all items when changing the options or setting it initially. Thoughts?

(This is a change - before, if the options changed in a multiple select, it would try to match up the previous selected labels to attempt to select the same items after the change. I think that is very error-prone.)

@jasongrout

This comment has been minimized.

Member

jasongrout commented Apr 10, 2017

Merging...let's iterate in another PR.

@jasongrout jasongrout merged commit c9d8088 into jupyter-widgets:master Apr 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasongrout jasongrout referenced this pull request Apr 18, 2017

Closed

Update Changelog #1279

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment