ping @jdfreder, @ellisonbg, @jasongrout for widget review

Looking at the code, but I definitely think this is better than the multiple=True version in #5327

I am +1 on this feature and this type of implementation as a separate class. Some feedback:

• I tried using it and found the python API to be confusing. I think we will need to rethink how the different attributes are named. For example, having value_name be a list was confusing. Can you summarize the public attributes, what they do and if they are r/w/rw to help us design the right API and naming?

Heh, well was just working with what was there from _Selection: just copied that API. I guess calling the options values kind of sets this off on the wrong foot. In an ideal world, we'd have more distinction between the "possible" and the "current": Django, for example, calls this kind of thing choices

I'll add some more docstring, explaining this choice: I feel being consistent is quite important.

More doc added, but this doesn't fix the underlying problem, I suppose.

If renaming was going to happen:

• as mentioned, django has choices, and doesn't actually name the given value...
• plain old javascript, of course, has options and selectedIndex... but is awkward for multiple values
• in jquery, a select with multiple will give back a list in val, while the selector is :selected
• qt has items and selectedItem

So I guess changing value[_name]s to item[_name]s or choice[_name]s or option[_name]s would be the least surprising, which value[_name] seems fine, and there is probably no way to make it clearer.

Ned rebase apparently. Sorry.

will look into...

On Thu, Dec 4, 2014 at 10:15 AM, Matthias Bussonnier <
notifications@github.com> wrote:

Ned rebase apparently. Sorry.

—
Reply to this email directly or view it on GitHub
#6890 (comment).

rebased!

On Thu, Dec 4, 2014 at 10:16 AM, Nicholas Bollweg nick.bollweg@gmail.com
wrote:

will look into...

On Thu, Dec 4, 2014 at 10:15 AM, Matthias Bussonnier <
notifications@github.com> wrote:

Ned rebase apparently. Sorry.

—
Reply to this email directly or view it on GitHub
#6890 (comment).

I have this on my review list for today. @bollwyvl it looks like you didn't push the rebase, it's not showing on GH.

@jdfreder Right you are! Sorry! I had another branch with a deceptively similar name.

This will create a merge conflict with #6747

calls this kind of thing choices

I like choices, @ellisonbg ?

It is hard to evaluate the name "choices" outside of the context of the other names in the API. Can you summarize the different names and what they are for?

#6747 was merged and it doesn't look like it caused conflicts, I'm surprised. I haven't looked, but it may not behave as expected anymore.

@jdfreder thanks, in fact needed a rebase... it was still using traitlets.List, which is a recipe for trouble, anyway, and now even SelectMultiple.value is a tuple.

I also added some tests for SelectMultiple backend widgets... are there front end tests I should add?

As to the values/value/choices API question: value is a good and usable name for the thing that is selected, for its compatibility with all the other widgets. For widgets that can take multiple independent values, it should be pluralized. However, values is taken.

Here are some options for (the things that can be chosen), (the thing that is chosen), (the things that are chosen):

• values/value_names, value_name/value... ????
  • confusing, even for non-multi-select. When value is usually to be a list, there's just no good way to name them, if the stem is the same.
• choices/choice_name, value_name/value... value_names/values
  • could be construed as I made this choice, but its prevalence in other APIs suggests this isn't a common interpretation.
• items/item_names, value_name/value... value_names/values
  • shortest! is what Qt uses.
• options/option_names, value_name/value... value_names/values
  • probably the clearest, and most accurate given its HTML intent, but options has connotations outside of a list of discrete things that could be the value, but aren't neccessarily, i.e. settings, configuration.

For the naming, what about something along the lines of options, selected_option, selected_options?

+1 to @takluyver 's suggestion

+1 - I like that too

On Wed, Jan 21, 2015 at 1:37 PM, Jonathan Frederic <notifications@github.com

wrote:

+1 to @takluyver https://github.com/takluyver 's suggestion

—
Reply to this email directly or view it on GitHub
#6890 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

Sorry, a bit out of the loop the last few days :)
Just to clarify, are we talking changing the name of value to selected_option in _Selection and all subclasses, or just for the new _MultipleSelection?

I prefer the consistency across all of _Selction widgets.

Does that conflict with things that expect widgets to have a value trait representing the user's input?

I don't have strong feelings either way, just thinking through the implications.

Changed values to options throughout, and the family of _dict, _labels, _values that go with it: this included tests, example notebooks, etc. Hopefully that's as far as the creep goes 🐙

Also, i left selected_label for SelectMultiple... since value stayed the same, it seemed more natural to leave it be instead of pluralizing.

I think I'd be inclined to pluralise even so, and just have value as a
special case. Jon, thoughts?
On 27 Jan 2015 18:24, "bollwyvl" notifications@github.com wrote:

Also, i left selected_label for SelectMultiple... since value stayed the
same, it seemed more natural to leave it be instead of pluralizing.

—
Reply to this email directly or view it on GitHub
#6890 (comment).

I agree with @takluyver - I think value should be the only exception, and other variables should be named logically (selected_labels here).

okiedoke. I thought it would cause more problems that it did on the front end, but it appears to work just fine with only minor modification. rebased and changed.

also updated the example notebook.

Thanks @bollwyvl , I'm taking a look at this now.

Hm... as shift/ctrl is part of the gimmick of mutliple select (potentially with the keyboard), the handle_click is a little insufficient... is there a reason to not use on("input", ...) on the listbox itself? I've added it to SelectMultipleView, but it might be appropriate on SelectView as well...

It looks to me like on('change', ... instead of on('input', ... is what we want to use.

@jdfreder, up to you whether we accept this with the whitespace changes or ask @bollwyvl to refactor them out, since it's most likely to cause conflicts with stuff you know about.

gah, sorry about the whitespace, bane of all developers. switched to atom
when i switched machines, and it strips by default. would a squash merge,
then interactive merge without whitespace work? is there a magic rebase
configuration to not add whitespace changes?

as to events, looks like change and input both fire: (events logged in
console)

http://codepen.io/anon/pen/PwKBjz

i'm used to change being pretty late-binding on text inputs (basically
blur) which seems un-widgetlike, but for selects, each atomic click
appears to be counted, so it should indeed be fine.

On Thu, Jan 29, 2015 at 2:16 PM, Thomas Kluyver notifications@github.com
wrote:

It looks to me like on('change', ... instead of on('input', ... is what
we want to use.

@jdfreder https://github.com/jdfreder, up to you whether we accept this
with the whitespace changes or ask @bollwyvl https://github.com/bollwyvl
to refactor them out, since it's most likely to cause conflicts with stuff
you know about.

—
Reply to this email directly or view it on GitHub
#6890 (comment).

No worries, these things happen. git rebase does have a --ignore-whitespace flag, maybe that will do the trick.

I only see 'change' firing in your example, with Firefox on Linux, not 'input'. It seems to be firing at all the right times, both with mouse and keyboard input, though.

That was really unpleasant 👊 reminds me of the bad old days of big CVS merges! Does anyone in the project have a semi-official .editorconfig?

Changed the event to change for both, and yes: seeing the same thing on FF, though chromium is giving me input.

I've taken screenshots, and still can't figure why the tests are failing, as the one right after the rebase worked... will keep investigating...

The tests were using $el.click, which doesn't actually fire the change event. Learn something every day. We could use casper.sendEvent to be a little more realistic, but adding this case seemed reasonable for now.

Now handling that little gem, I think this is basically ready to go... unless I should do yet more event investigating. Is keyboard support a thing that needs testing?

Is keyboard support a thing that needs testing?

It's not tested now so I wouldn't worry about it. In the future we may be changing test frameworks anyways. I restarted Travis, because it looked like the last failure was sporadic.

Thanks @bollwyvl ! I'm sorry this took so long to merge. Tests are passing, I looked over it one more time, and it looks good to me.