fix: restoring widgets from saved state is broken since v8 in Lab

[mariobuikhuizen]

In 9d999d7 waiting for sessionContext.ready was removed, this caused _loadNotebook() not to work, because it was called before the widget metadata was available.

This commit restores waiting for sessionContext.ready. This has to be before _loadFromKernel(), otherwise this.kernel is not present, causing an exeption, skipping _loadNotebook().

Fixes: jupyterlab/jupyterlab#15361

[github-actions]

👈 Launch a binder notebook on branch mariobuikhuizen/ipywidgets/fix_restore_widgets_form_saved_state

[mariobuikhuizen]

Also related to #3823

[maartenbreddels]

@vidartf and @jasongrout do you remember why this line was removed in #2532 ?

Also, the failure on the visual test is odd, since it's a font that was already used. Could it be that we don't pin a particular library? I do see we use 'ubuntu-latest' which can lead to changes in visual results.

@mariobuikhuizen can you first open an 'empty' PR to see if this is unrelated to this PR?

[ibdafna]

I'm re-running the tests to see if we can figure out the visual regression test failure. Once we get to the bottom of that, we should merge this as the saved widget state functionality is important.

[ibdafna]

Looking at the failed regression tests, I'm not seeing any material difference between the two rendered items. The failure is triggered by a few changed pixels, but not a real regression.

[gutow]

Looking at the failed regression tests, I'm not seeing any material difference between the two rendered items. The failure is triggered by a few changed pixels, but not a real regression.

If this really is the only problem, I strongly urge this to be pushed ahead quickly. The broken saved widget state is a big deal that prevents me from being willing to work on moving my projects to jupyter lab from nbclassic.

Thanks.

[martinRenou]

I just rebased, it is still failing.

@mariobuikhuizen @ibdafna @maartenbreddels should we merge event though the visual regression test fails? Given the diff I don't think this is caused by this PR. We should fix the regression separately

[maartenbreddels]

Indeed, this seems unrelated, and it also happens for cell 41, so it's not likely to be a timing issue due to loading CSS slowly.
Let's do a few restarts to see if it's flakey or consistently failing.

[pplonski]

Can I help somehow with this issue, to have it merged?

[maartenbreddels]

We also really want to have this released soon. I'm gonna push the visual changes as a separate commit, with an explanation that we do not fully understand why this visual diff happens.

[maartenbreddels]

sidenote: images 40 to 44 have changes, but the visual report only shows 1 image at a time. To make sure we really commit small changes if have to push 1 image at a time. I'm not sure if this is a galata setting, since it will speed things up in the future?

[krassowski]

I'm not sure if this is a galata setting, since it will speed things up in the future?

Since you have a number of expects in a loop, you probably want to switch to expect.soft here:

ipywidgets/ui-tests/tests/widgets.test.ts

Lines 41 to 44 in b253179

	for (let i = 0; i < cellCount; i++) {
	const image = `widgets-cell-${i}.png`;
	expect(captures[i]).toMatchSnapshot(image);
	}

this will still mark the test as failed, but will run through all of them (and generate images for all).

[maartenbreddels]

Leaving this for future reference, in case we want to review this again, image 42:

Actual 42:

Expected 42:

Diff 42:

[maartenbreddels]

this will still mark the test as failed, but will run through all of them (and generate images for all).

Many thanks 🤗
Pushed a commit to this PR, hope it works!

[maartenbreddels]

Looking at a recent run of #3880 it seems the visual test are also failing in that PR (I've checked before that other PR's were green, maybe a caching 'luck'). Also, the docs fail in #3880 so I thin we can merge now.

Many thanks to the people who helped with this and the patience of our users; I'll work on a release now!

[kolibril13]

Great to hear! Looking forward to testing this new feature!

[martinRenou]

Thanks!!

[maartenbreddels]

We have the first 2024 release 🥳
Release: ipywidgets 8.1.2, widgetsnbextension 4.0.10, jupyterlab_widgets 3.0.10

[kolibril13]

Awesome!
I've just tested this with

python3.11 -m venv .venv && source .venv/bin/activate
pip install jupyterlab==4.1.0
pip install ipywidgets==8.1.2

and Save -> Restart -> Close -> Reopen Works now! 🎉

Screen.Recording.2024-02-09.at.11.28.24.mov

I've also tried another order (Restart before save)
Restart -> Save -> Close -> Reopen

That one still gives the error:

Screen.Recording.2024-02-09.at.11.37.10.mov

[kolibril13]

and here's another observation when I do the following steps:

1. Reopening a notebook with a saved widget state
2. Not changing anything in the notebook
3. Closing the notebook

it will ask me if I want to save changes before closing. That's unexpected, because I did not make changes.
When I click on "Discard changes", I can simply reopen the notebook again and see the widget.

However, when I click on "Save changes" and reopen the notebook, the error of not rendering the widget occurs again:

Screen.Recording.2024-02-09.at.11.42.18.mov

[mariobuikhuizen]

I've also tried another order (Restart before save)
Restart -> Save -> Close -> Reopen

That one still gives the error:

Ipywidgets v7 has the same behaviour and this also happens in the classic notebook, so this might be a different issue.

(edit: forgot to turn on save widget state, it works in classic notebook with v7)