Upgrade nouislider

[jasongrout]

As a follow-up to #3215, this PR upgrades nouislider. Turns out that the next major version of nouislider is in typescript, and thus we don't need the types package.

[jasongrout]

Looks like the relevant errors to fix are:

> @jupyter-widgets/[email protected] build:src /home/runner/work/ipywidgets/ipywidgets/packages/controls
> tsc --build

Error: src/widget_float.ts(161,9): error TS2322: Type '(value: number) => number' is not assignable to type '(value: string) => number | false'.
  Types of parameters 'value' and 'value' are incompatible.
    Type 'string' is not assignable to type 'number'.
Error: src/widget_selection.ts(763,9): error TS2322: Type '(value: number) => number' is not assignable to type '(value: string) => number | false'.
  Types of parameters 'value' and 'value' are incompatible.
    Type 'string' is not assignable to type 'number'.

I'm leaving this for now. If someone else wants to take this on, feel free! Please post here so we can coordinate work.

[ibdafna]

@jasongrout thanks for the help - LGTM!

[ibdafna]

@jasongrout the type conversion with Number() seems to throw off a specific case in FloatLogSlider. I'm investigating.

[jasongrout]

For the record, it does seem like the example in widget list.py works great with this PR:

[jasongrout]

However, this seems like a bug: Execute these three cells:

x = widgets.FloatLogSlider(
    value=10,
    base=10,
    min=-10, # max exponent of base
    max=10, # min exponent of base
    step=0.2, # exponent step
    description='Log Slider'
)
x

x.value = 25

x.value = 100

When I execute the second cell, I see the value set to approximately 25. When I execute the third cell, the value is not set to 100, but stays around 25 (both visually, as well as when I check with x.value). Executing that x.value = 100 a second time does set the slider to be 100.

[jasongrout]

Here is the current 8.0 alpha. Something does definitely seem wrong here - I'm setting the value to 10, but get back 10^10:

And here is setting the value to 9, again, very wrong:

I also checked with ipywidgets 7, and things seem to work correctly there (and consistent with this PR's behavior), so I think this PR is fixing things.

[ibdafna]

However, this seems like a bug: Execute these three cells:

x = widgets.FloatLogSlider(
    value=10,
    base=10,
    min=-10, # max exponent of base
    max=10, # min exponent of base
    step=0.2, # exponent step
    description='Log Slider'
)
x

x.value = 25

x.value = 100

When I execute the second cell, I see the value set to approximately 25. When I execute the third cell, the value is not set to 100, but stays around 25 (both visually, as well as when I check with x.value). Executing that x.value = 100 a second time does set the slider to be 100.

Just tested this case with the latest commit and it looks like that's still happening

[ibdafna]

@jasongrout debugging this, the value variable is taking some nonsensical values in the createSlider() method for FloatLogSlider. Let's hold off merging until I get to the bottom of this.

[jasongrout]

Two data points: assigning a value to .value works fine in 7.6.3, and at least is responsive in the latest 8.0 alpha.

[jtpio]

The Read The Docs failure (https://readthedocs.org/projects/ipywidgets/builds/14072469/) seems to be related to the latest ipykernel rc: ipython/ipykernel#694

[jasongrout]

A brief analysis of the problem with the setting not taking effect the first time (#3216 (comment)). First, there are two attributes at play here, the .changed attribute managed by backbone, which records any attribute that has changed since top-level call to set, and the _buffered_state_diff, managed by ipywidgets. The changed attribute is reset/emptied every time there is a new top-level call to set, so _buffered_state_diff is supposed to accumulate changes across successive calls to set until the changes can be propagated to the kernel.

1. We set the value to 25 in python. Note that 25 is not a valid value (given the step specified). This sends a comm message to the browser to set the value to 25.
2. The browser handles the message by calling the model set method (this is the top-level call). This in turn triggers a backbone event, which leads to the slider updating. However, since we have the step set:, the value is changed to approximately 25.1, and the model is set to this new value and this new value is sent to the backend using .touch(). This sync clears the _buffered_state_diff:

   ipywidgets/packages/base/src/widget.ts

   Line 519 in 4132414

   this._buffered_state_diff = {};

   but not the changed attribute.
3. Control returns to the top-level set call. Since the changed attribute is only reset on the next top-level set call, the changed dictionary is still there, and the _buffered_state_diff accumulates the value=25.1 setting (again) at

   ipywidgets/packages/base/src/widget.ts

   Lines 350 to 353 in 4132414

   	this._buffered_state_diff = utils.assign(
   	this._buffered_state_diff,
   	attrs
   	);

4. The next cell sets the value to 100. However, _buffered_state_diff is carried over from before (with value being 25.1), and that somehow messes things up.

[jasongrout]

In brief, the essential conflict here is the lifetimes of the .changed cache and the ._buffered_diff_state cache. .changed lasts from one top-level .set to the start of the next top-level .set call (so in particular, a recursive call to .set does not reset .changed, e.g., if a change event handler triggered by a top-level .set call also calls .set, like in the case here). The ._buffered_diff_state cache is populated from .changed at the end of every .set call (including recursive calls) and is reset when the state is saved to the kernel.

The conflict here is when state is .set, then saved to the kernel in the middle of a top-level .set call. In this case, we the recursive .set call sets .changed and ._buffered_diff_state, but the save to kernel resets ._buffered_diff_state (but not .changed. When the top-level .set call finishes, ._buffered_diff_state accumulates the .changed state a second time, not knowing that that state was already sent on to the kernel, but another save to the kernel is not called.

Some obvious thoughts, which are both basically just aligning when the two state stores are populated or reset:

1. Can we reset the .changed state when we sync to the kernel? The problem with this is that the changed handlers in the top-level .set like to have a consistent idea of what changed, so deleting things from the .changed cache can really confuse change handlers (because to some of them, it would look like there wasn't really a change).
2. Can we just accumulate ._buffered_diff_state state at the top-level .set call rather than every .set call, which essentially means that syncing to the kernel only happens after top-level .set calls and not during recursive .set calls? This would be a big change in behavior - right now syncing to the kernel happens immediately, but this would mean it would happen after the top-level .set call. We'd also have to deal with storing which view was claiming responsibility for the change for the eventual sync, since the top-level .set is a model-level thing. Or we could just ignore syncs that are happening in recursive sets, relying on whoever called the top-level set to sync changes, but that also can eliminate information about what view had made a change, and can be confusing about when a sync happened and when it was ignored (perhaps that can be a return value of the syncing function?)
3. For this specific case (of a handling a state change message from the kernel), we could check to see if we had state in _buffered_diff_state when a state sync message was finished being handled, and send an update to the kernel if so.
4. In a recursive .set call, we could keep track of state that was synced to the kernel, and remove this state from the ._buffered_diff_state at the end of a top-level .set call. Instead of changing the existing lifetimes, this ends up being a bridge between the lifetimes of the two state stores.

I think I prefer the last option the best for now as a backwards-compatible change that fixes this issue.

[jasongrout]

I implemented option 4 above in 6127306, and now the unresponsive set at #3216 (comment) seems to be fixed.

@ibdafna (or anyone else) - can you review?

[jasongrout]

Looks like there is a problem with the vertical slider (probably from b0251d2 ?). Hooray again for the ui tests!

[jasongrout]

Looks like there is a problem with the vertical slider (probably from b0251d2 ?). Hooray again for the ui tests!

Fixed in a975b57

I think this is ready for review.

[ibdafna]

@jasongrout LGTM. Also tested locally and everything is working as expected. Thank you!