Only the first connected slot on a Signal is invoked about lumino HOT 14 CLOSED

Would it be possible for the demo to be lumino only, without any of the jupyterlab packages?

from lumino.

The dockpanel example might be relevant: https://github.com/jupyterlab/lumino/tree/master/examples/example-dockpanel

from lumino.

Perhaps something is responding to the tab close request by disposing the document, which is cascading into disconnecting the rest of the signal handlers? If so, perhaps whatever is disposing the document should do so asynchronously so that the other signal handlers can run first?

from lumino.

@vidartf I didn't know that existed, I took some time working in that and managed to really whittle down my example. You can add the following chunk of code at the end of the main() function in https://github.com/jupyterlab/lumino/tree/master/examples/example-dockpanel and recreate the issue.

  let dockPanel = new DockPanel();
  dockPanel.addWidget(new ContentWidget('Black'), { mode: 'split-bottom' });

  let myTabBar = dockPanel.tabBars().next();

  if (myTabBar) {
    let connect = myTabBar.tabCloseRequested.connect((sender, args) => {
      console.log('tabClose callback run');
    });
    console.log('tabClose calBack connected: ' + connect);
  }

  Widget.attach(dockPanel, document.body);

By adding that code it will put a tab at the bottom of the page. By looking at the console you'll see that the callback function successfully connects to the tabCloseRequested signal on page load. But when you close the tab you will not see the console log that is inside the callback function (ie the callback was not run).

from lumino.

@jasongrout I'll look at the lumino code to see if I can track something like that down

from lumino.

I forgot to click send on this comment this afternoon, so I'll comment it as is:

Ok I have some update to this, as well as an even more streamlined example.

Add just to following to the end of main() instead and the callback will be added to the closing of any tab on the example.

  let myTabBar = dock.tabBars().next();

  if (myTabBar) {
    let connect = myTabBar.tabCloseRequested.connect((sender, args) => {
      console.log('tabClose callback run');
    });
    console.log('tabClose calBack connected: ' + connect);
  }

This more streamlined version has allowed me to more accurately trace the issue. Specifically the callback is successfully run as long as at least one tab in the section is still there, but if you close the last tab it doesn't run. This is because Widget.dispose() runs Signal.clearData()

from lumino.

So I've spent most of the day doing intricate stack tracing and logging to trace exactly what's happening here. Essentially this is a really specific edge case and annoying as it is it is behaving exactly as it should imho.

When a tab is closed on a tab bar a function called _removeWidget() in dock panel.ts is eventually run, in this function there is a check if the tab being closed is the last tab in the tab bar, if it's not it breaks and things work as I expected. But if it's the last tab in the tab bar it disposes of the tabbar itself, which then clears the signals related to it. This mean that the tabCloseRequested signal will be removed and further connections to it will never run when closing the final tab in a tab bar.

@jasongrout @vidartf if this makes sense to you and you agree this annoying edge case should be left as is feel free to close this issue.

from lumino.

But if it's the last tab in the tab bar it disposes of the tabbar itself, which then clears the signals related to it.

Ah, makes sense. Perhaps that tab bar disposal should be done asynchronously, like setTimeout(...,0) or maybe even better a promise resolution, to allow other tab close request handlers to run?

from lumino.

That would complicate the logic to introduce asynchronicity, but at the same time it would make this corner case maybe a bit better?

from lumino.

Now that I'm aware of this edge case I've discovered an easy work-around to it. Instead of connecting my callback to the tabCloseRequested signal I've instead attached it to the disposed signal. This work-around may be a little much if I wanted it to run on tab close for all tabs including the last one (since I'd have to attach it to both) but in my case the final tab was the one I wanted to catch to begin with.

As for the asynchronicity idea, I'm not sure it would be possible, or at least very difficult, since the line where the tabbar is disposed is deep in the call stack and much code after it requires it is disposed first.

from lumino.

As for the asynchronicity idea, I'm not sure it would be possible, or at least very difficult, since the line where the tabbar is disposed is deep in the call stack and much code after it requires it is disposed first.

I agree it's difficult to think through the corner cases. To be clear, my suggestion was to consider putting all of these lines in a setTimeout or promise resolution so they are executed after the js frame has finished:

It's great that you have a workaround, and I think you're right to argue that the current behavior is also correct, and simpler to reason about as well since it is all synchronous.

from lumino.

@jasongrout @vidartf if this makes sense to you and you agree this annoying edge case should be left as is feel free to close this issue.

Sounds good.

from lumino.

If someone wants to tweak this, I see two possible ways:

• Always fire all signals "in-flight" if an object is disposed when emitting, e.g. by copying the relevant data while in the emission process. This might have a long tail though, so it might not be straight forward.
• Have the dispose call in docklayout (

  lumino/packages/widgets/src/docklayout.ts

  Line 651 in 9ea670f

  tabNode.tabBar.dispose();

  ) be called in the next animation frame / timeout of 0 (whatever is the current pattern in the repo). Likely to be ok, but some care would have to be taken that we do not create other corner cases due to the changes in timing.

from lumino.

(here is a pattern of delayed dispose)

from lumino.