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

Notify command changed #4533

Merged
merged 7 commits into from May 16, 2018
Merged

Conversation

@ian-r-rose
Copy link
Member

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

Fixes #4524, by calling commands.notifyCommandChanged in most of the extensions. As discussed in that issue, this is the intended way for rendered commands to know when they should be updated. I am not crazy about this solution since it results in a lot of redundant notifyCommandChanged, but it should remove edge cases like #4524.

It also involves a bit of an abuse of Typescript in a bunch of places, since I am iterating over the keys of the CommandIDs namespaces in the extensions (see also this comment). I am happy to entertain other ways to approach that.

@ian-r-rose ian-r-rose added this to the Beta 3 milestone May 8, 2018
@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 8, 2018

Rather than notifying for all commands in a namespace, shouldn't you only notify for the ones which are actually invalid due to the change?

@ian-r-rose
Copy link
Member Author

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

For the ones where I notified all of them, it was usually when an instance tracker was involved. In those cases, isEnabled depends on the current widget, and will essentially always need updating when the tracker currentWidget updates.

There are a couple of unnecessary ones that have snuck through (like "Create New"), but I thought that in the interests of maintainability this was the preferable choice.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 8, 2018

Do you think it would make sense to make the command id to notifyCommandChanged be optional, and if not provided, the downstream consumer should treat it as a wildcard, and assume that all rendered commands should be refreshed?

The original intent of the signal was to allow fine-grained control of what to re-render on the consumer's end. But if the consumers are typically always going to be vdom-ish, then the point might be moot, and we can get away with a coarse grained "registry changed in some unspecified fashion" signal.

@ian-r-rose
Copy link
Member Author

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

From the perspective of just having implemented this, I think the level of granularity required by notifyCommandChanged is more of a pain than an asset at the moment, especially with the knowledge that most renderers will use a vdom approach (as you point out).

So something like an optional argument to notifyCommandChanged sounds like it would be useful, and simplify the code here.

@ian-r-rose
Copy link
Member Author

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

Also note: the application-extension calls notifyCommandChanged whenever the layout is modified. With a big-hammer notifyCommandChanged, this can take over all the places where more specific instance trackers call that, and they can all be removed.

@vidartf
Copy link
Member

@vidartf vidartf commented May 8, 2018

So if someone triggers a notifyCommandChanged() without command ID, all commands of all extensions get updated? This seems like a possible performance issue in the future if several "not-perfect" extensions are loaded (e.g. one or more extensions that never pass command id + one extension that performs an expensive operation in a command callback).

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 8, 2018

Misbehaving extensions can always kill the app. We can't protect ourselves from that. As it currently stands, I don't know of any code that is actually looking at the command id which changed anyway. The widget just calls .update() whenever any command changes. That update is conflated, and the widget uses vdom to render anyway.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 8, 2018

And I think in all likelihood, the widget's which actually render commands will be core-maintained extensions.

@vidartf
Copy link
Member

@vidartf vidartf commented May 8, 2018

Misbehaving extensions can always kill the app.

Sure, I'm just trying to ensure that it is harder to write a bad extension by accident.

As it currently stands, I don't know of any code that is actually looking at the command id which changed anyway.

So, whenever anyone calls notifyCommandChanged, all of the isEnabled callbacks etc. on all commands will be called? These callbacks are the ones I'm concerned about. Consider e.g. this code: https://github.com/jupyter/nbdime/blob/master/packages/labextension/src/plugin.ts#L176-L208 It has some reasonable balances, but it should probably be cleaned up a lot better if this is going to be called a lot.

@vidartf
Copy link
Member

@vidartf vidartf commented May 8, 2018

PS:

if (args.id !== id) {
return; // Not our command
}

@vidartf
Copy link
Member

@vidartf vidartf commented May 8, 2018

Looking at things closer, I guess the toolbar code is the only that does things that way (and I was the one who originally contributed that code, before I was very comfortable with phosphor). I assume that code needs a new looking at.

@ian-r-rose
Copy link
Member Author

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

@sccolbert Do you think we can do a patch release of @phosphor/widgets and @phosphor/commands with those updates, or should we proceed with this PR as is?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 8, 2018

Looking at things closer, I guess the toolbar code is the only that does things that way (and I was the one who originally contributed that code, before I was very comfortable with phosphor). I assume that code needs a new looking at.

Also, @gnestor's work on a react-based toolbar might obsolete it?

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 8, 2018

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 15, 2018

With new versions of phosphor widgets out, @ian-r-rose needs to update this PR. Should make it simpler. @afshin can you review this after those changes?

@ian-r-rose
Copy link
Member Author

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

Okay, this is ready for a look. A lot of the logic that was in this PR has now been moved upstream into phosphor, so this is a net simplification of the codebase.

afshin
afshin approved these changes May 16, 2018
Copy link
Member

@afshin afshin left a comment

Awesome, thanks @ian-r-rose, @sccolbert for both the upstream API change and this PR.

👍

@afshin afshin merged commit 18035d8 into jupyterlab:master May 16, 2018
2 checks passed
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants