Skip to content
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

Add busy favicon when kernels are running #4361

Merged
merged 12 commits into from May 25, 2018

Conversation

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Apr 11, 2018

Fixes #3957 by changing the favicon when kernels are running.

As implemented, this uses the session manager to update, but this is a bit slow. It isn't refreshed as soon as the kernel status changes, but it does seem to work.

It would might be quicker if we subscribe to the ISession.statusChanged or the IKernel.statusChanged signals instead of the SessionManager.runningChanged signal.

However, I am not sure where the best place to react to an updated global list of ISession's or IKernel's is.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Another option would be to react to the kernel of the currently active document (if it has one). Then you could directly subscribe to the kernel.statusChanged signal.

let i = 0;

app.serviceManager.sessions.runningChanged.connect((_, sessions) => {
const newIsBusy = sessions.map(s => s.kernel.execution_state).indexOf('busy') !== -1;
Copy link
Member

@ian-r-rose ian-r-rose Apr 17, 2018

Since we have a type definition for Kernel.Status, can we just check for identity === 'busy', rather than using indexOf?

const newIsBusy = sessions.map(s => s.kernel.execution_state).indexOf('busy') !== -1;
if (newIsBusy !== isBusy) {
isBusy = newIsBusy;
console.log({isBusy});
Copy link
Member

@ian-r-rose ian-r-rose Apr 17, 2018

Superfluous console log.

console.log({isBusy});
if (isBusy && !interval) {
interval = window.setInterval(() => {
favicon.href = `/static/base/images/${icons[i++ % 3]}`;
Copy link
Member

@ian-r-rose ian-r-rose Apr 17, 2018

Are you animating the favicon here? What does the classic notebook use the other busy indicators for? I only see references to favicon-busy-1.ico.

Copy link
Member Author

@saulshanabrook saulshanabrook Apr 19, 2018

Ah, they used to use the animated favicon, but then they removed it: https://github.com/jupyter/notebook/pull/2675/files For some reason I was found the commit adding the animated state and didn't see that they undid it.

I will change it not to animate.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 19, 2018

@ian-r-rose I think that's a good idea. I changed it to reflect the current document. However, I put that logic in @jupyterlab/application. It now has a dependency on notebook and console, which doesn't seem ideal. It has the dependency because it is checking if the current widget is one of those instances, and if so grabbing the session off of it.

I could remove that dependency and just use the session property of the active widget (if it is defined). But that would break if some widget defined a session that wasn't an IClientSession.

So maybe it would be better to create an abstraction so any widget can update if it's busy or not, to update the favicon. Any ideas on the best way to do this?

@flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Apr 19, 2018

I think this should be merged first, and another PR/discussion should be opened about supporting widgets.

That way we have the busy icon in the next release, no matter how long the discussion and implementation of the rest lasts

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 19, 2018

I could remove that dependency and just use the session property of the active widget (if it is defined). But that would break if some widget defined a session that wasn't an IClientSession.

I went ahead with this.

@saulshanabrook saulshanabrook changed the title WIP: Add busy favicon when kernels are running Add busy favicon when kernels are running Apr 19, 2018
@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 25, 2018

@ian-r-rose this is ready for another review whenever you have time

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Apr 25, 2018

Can you post a screenshot?

@@ -657,6 +664,25 @@ class ApplicationShell extends Widget {
}
this._currentChanged.emit(args);
this._onLayoutModified();

this._onCurrentSession(args.newValue ? (args.newValue as any).session : undefined);
Copy link
Member

@ian-r-rose ian-r-rose Apr 26, 2018

Do we have a more type-safe way of getting at whether there is an IClientSession associated with a widget? This works for notebooks and consoles, but more-or-less by accident, in that they both have the session available by that name on the top-level widget. If another widget comes along with a different API, but still uses an IClientSession this will not work.

I can think of how we would do it for documents: we could use the DocumentManager.contextForWidget function to get the context, which has a session object on it. I don't currently know how to do it for a more general widget...

Copy link
Member

@ian-r-rose ian-r-rose Apr 26, 2018

Ah, sorry, I missed that you addressed this in the above comment. Yeah, I am not sure about this just now (though I think the docmanager solution would work for documents).

Copy link
Member

@ian-r-rose ian-r-rose Apr 26, 2018

If you moved this logic into @jupyterlab/application-extension, we can be less conservative about the dependencies and then import the IConsoleTracker and INotebookTracker. But then it would not provide a way for other session-using-widgets to hook into the same logic.

Copy link
Member

@ian-r-rose ian-r-rose Apr 26, 2018

Or we could do a hybrid of the two approaches: use the DocManager/context approach to cover all documents that might use a session, and also check the IConsoleTracker to cover consoles. This would be type-safe, and cover most of the things that might be consumers of kernels (missing non-document, non-console widgets).

Copy link
Member Author

@saulshanabrook saulshanabrook Apr 26, 2018

Is there some way for a widget to register a isBusy for itself? So not base it on the session object at ll but make it more generic? Like there is the setDirty method on the application that widgets can use to mark if they are unsaved. The simplest way I can think of is storing a WeakMap in the application mapping widgets to isBusy signals. Any widget could add itself and a signal to this map when it is created.

Copy link
Member

@ian-r-rose ian-r-rose Apr 27, 2018

I like this idea! I think it could follow a very similar logic to the setDirty setup (which doesn't use a WeakMap, but instead a DisposableDelegate with an internal count of the dirty widgets.

/**
* Handle a change to the session of the current widget.
*/
private _onCurrentSession(session?: IClientSession) {
Copy link
Member

@ian-r-rose ian-r-rose Apr 26, 2018

Can we change these to _onCurrentSessionChanged and _onCurrentStatusChanged, or similar? I found the names here to be kind of confusing.

Copy link
Member Author

@saulshanabrook saulshanabrook Apr 26, 2018

Yep, sounds good, will do.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 27, 2018

@ellisonbg Here is a quick gif showing how it changes based on selected widget and busy status:

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 27, 2018

I am thinking of just implementing this as a global check if any kernel is busy, like the isDirty flag, since that's a bit simpler. Please weigh in if you feel strongly that it should show the specific kernel of the open window.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

I think it is more understandable and less confusing if it is just a global check.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 30, 2018

I have updated it to be a global check.

How?

It does this by combining the /api/kernels API call with the frontend kernel status changes. This let's us refresh from server status every couple of seconds and live update with the local changes.

I added this functionality to the KernelManager and now expose this manager on the serviceManager.

UI

It handles updates from notebooks:

And updates from consoles:

It also takes into account the status of kernels after a reload, for kernels that aren't on screen:

I think there is currently a bug when a window is reloaded with a kernel currently executing, where it is seen as not busy on reload. So this affects the favicon as well:

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 30, 2018

Why did you not do the isBusy approach that we talked about earlier? We are trying to be as conservative as possible with @jupyterlab/services, not changing the API surface area unless we really need to. Another thing that is nice about the isBusy approach is that it is usable by other extensions (even ones that have concepts of "busy" that are not necessarily tied to a given kernel).

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 30, 2018

This makes use of the backend response, so if we have a kernel running that isn't displayed it will still register as busy.

I supposed that could be added with the previous approach though. I thought this was cleaner, so then we don't have to have special cases for notebooks and consoles.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 30, 2018

All this back and forth makes me realize I should have properly scoped out the design requirements for this feature before implementing it in different ways!

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 30, 2018

I think that showing a busy signal for a running kernel that is not tied to a given activity might be kind of confusing.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 30, 2018

Yes, all of these decisions that the classic notebook has made suddenly become a lot subtler when you can have many activities on the same page! What do you think @jasongrout?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 30, 2018

All this back and forth makes me realize I should have properly scoped out this feature before implementing it in different ways!

Part of the back-and-forth and part of the implementation experimentation is figuring out the scope, so I'm not sure the time was for nought.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 30, 2018

Part of the back-and-forth and part of the implementation experimentation is figuring out the scope, so I'm not sure the time was for nought.

And also helpful for myself to learn about the codebase

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 30, 2018

Yes, all of these decisions that the classic notebook has made suddenly become a lot subtler when you can have many activities on the same page! What do you think @jasongrout?

How about we answer some usecase-based questions, maybe on the original thread where we have outside engagement.

  1. Why do I care if something is "busy"? How will this information affect my actions? How will knowing the "busy" state when I am not in the browser tab help me?
  2. (implicit in the above) - what components do I consider as "busy"

@saulshanabrook - do you want to take the lead in posing questions like these on the original thread and gathering more information from users?

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented Apr 30, 2018

@jasongrout that would be great. Thank you.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 15, 2018

In our dev call today:

  • We like the idea of having an application wide a isBusy attribute handled similarly to isDirty
  • We like the idea of a global notion of being busy.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 15, 2018

And the notebook/console extensions would use this...

@jzf2101
Copy link
Contributor

@jzf2101 jzf2101 commented May 18, 2018

I pulled this PR and tried running and wasn't able to run in editable or dev mode. Could this be related to my other installation issues?

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 22, 2018

@jzf2101 Could you try again? I just rebased and implemented the changes we discussed last week, i.e. changing to using a setBusy function on the application. I ended up calling that in the clientsession, instead of in the console panel and notebook panel, since then we just have to write the logic once.

Copy link
Contributor

@jzf2101 jzf2101 left a comment

LGTM 👍

@jzf2101
Copy link
Contributor

@jzf2101 jzf2101 commented May 22, 2018

@tgeorgeux do you want to look at it before we merge?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 22, 2018

I'd still like to look over the most recent version.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

I like this implementation a lot! I have one minor comment. This will also likely conflict with the context refactor of @jasongrout that is in review. I wonder which of them we should merge first?

} else {
if (this._busyDisposable) {
this._busyDisposable.dispose();
delete this._busyDisposable;
Copy link
Member

@ian-r-rose ian-r-rose May 22, 2018

I don't think there is any need to delete the disposable here. The GC should be able to clean it up once you set it to a new value.

Copy link
Member Author

@saulshanabrook saulshanabrook May 22, 2018

I delete it so that I don't dispose it twice. I only have it set on the object, when it hasn't been disposed yet, after we have gotten a busy status.

Copy link
Member Author

@saulshanabrook saulshanabrook May 22, 2018

For example, when the session first starts there is no _busyDisposable, so if we get some status, we shouldn't try to dispose of it. We should only dispose of it when it exists.

Copy link
Member

@ian-r-rose ian-r-rose May 22, 2018

It should be safe to dispose of it twice, I think. But if we are worried about that, can we explicitly set it to null instead?

Copy link
Member

@ian-r-rose ian-r-rose May 22, 2018

Ah, I see what you mean. Yeah, I think it would be more consistent with the rest of the codebase to set the class member to null (we currently are only using the delete operator on plain Objects, rather than ES6 classes).

Copy link
Member Author

@saulshanabrook saulshanabrook May 22, 2018

Sure, just pushed.

@saulshanabrook
Copy link
Member Author

@saulshanabrook saulshanabrook commented May 22, 2018

@ian-r-rose Thanks for the review. I am inclined to let his merge first, to make it easiest for him to get that in ASAP. There isn't a ton here, so rebasing shouldn't be that bad.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Great! I am happy with this, thanks for all the back and forth @saulshanabrook. Let's hold off until @jasongrout weighs in on what he wants to do, but as far as I'm concerned we can merge at any time.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 22, 2018

I would love to merge #4453 first, if possible.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 25, 2018

Looks like no merge conflicts, so full steam ahead!

@ian-r-rose ian-r-rose merged commit 9cdc90c into jupyterlab:master May 25, 2018
2 checks passed
@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@saulshanabrook saulshanabrook mentioned this pull request Sep 8, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.