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

Refactor launcher to use CommandRegistry. #4757

Merged
merged 3 commits into from Jun 27, 2018

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 20, 2018

Fixes #4397.

This is not a huge simplification, overall, but it does push some logic from the launcher into the commands that already existed in the various extensions.

One drawback is that we are being less strict about the type signature of launcher commands. Previously, they were expected to take a cwd and a name, and resolve with a Widget. This does the same, but the type signature for commands.execute cannot check for cwd, and it cannot ensure that a widget is returned. Therefore, consumers of the launcher are expected to be good citizens, as tsc cannot catch errors there.

One side effect of this is that icons have been added to the New submenu:
image
I can revert that change by only sometimes returning an iconClass from the creation commands, but maybe it looks kind of nice?

@afshin
Copy link
Member

@afshin afshin commented Jun 26, 2018

Hm. Is the callback even necessary? Perhaps there should be a courtesy behavior, if the promise that your command returns resolves into a Widget then the launcher automatically adds it to the main area. If it returns anything else, the launcher just assumes you know what you're doing and does nothing.

Or perhaps, the launcher is not responsible for adding anything to the main area at all and in fact doesn't even create an onclick handler ... it could just use the command linker and rely on each command to do whatever it wants, whether that's adding something to the main area, or launching a modal, or whatever other kind of activity you might ostensibly want to launch.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 26, 2018

The callback I am referring to is being replaced by the command execute function, and that is necessary. There is another callback which is used to replace the launcher widget with whatever is created by the command.

That first function is the trickier one: we want to be able to pass in a cwd so that the command can create the activity in a directory if it needs to. But it makes the type signature looser to use a command, so we can do less type checking on extensions that add to the launcher that way.

@@ -379,14 +366,16 @@ function Card(kernel: boolean, item: ILauncherItem, launcher: Launcher, launcher
return;
}
launcher.pending = true;
let callback = item.callback as any;
let value = callback(launcher.cwd, item.name);
let value = commands.execute(command, {
Copy link
Member

@afshin afshin Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value is a Promise, so it should probably be called something else, maybe promise, and the Promise.resolve(value).then(...) line that follows should probably be promise.then(...)

Copy link
Member Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right you are. For some reason I had it in my head that commands could return a value or a promise for a value, and was therefore promisifying the return value.

afshin
afshin approved these changes Jun 27, 2018
Copy link
Member

@afshin afshin left a comment

Awesome. Thanks, @ian-r-rose!

@afshin afshin merged commit 23ff0c1 into jupyterlab:master Jun 27, 2018
2 checks passed
@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@timkpaine
Copy link
Member

@timkpaine timkpaine commented Aug 6, 2018

Launcher items had a displayName, what is the equivalent? I thought it would be label but apparently not

@timkpaine
Copy link
Member

@timkpaine timkpaine commented Aug 6, 2018

is it caption?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Aug 6, 2018

It is label: https://github.com/jupyterlab/jupyterlab/blob/master/packages/notebook-extension/src/index.ts#L485-L491

Edit: label on the command, rather than the ILauncherItemOptions

@timkpaine
Copy link
Member

@timkpaine timkpaine commented Aug 6, 2018

hmm, mine is blank for some reason. I see an icon on the launcher but no label, and from the file->new menu I see a label but no icon

@timkpaine
Copy link
Member

@timkpaine timkpaine commented Aug 6, 2018

@timkpaine
Copy link
Member

@timkpaine timkpaine commented Aug 6, 2018

weird, I get different behavior on different browsers (my company computer vs my laptop)

@timkpaine
Copy link
Member

@timkpaine timkpaine commented Aug 6, 2018

ah its a function?

@timkpaine
Copy link
Member

@timkpaine timkpaine commented Aug 6, 2018

label: args => (args['isPalette'] ? 'New Terminal' : 'Terminal'),

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Aug 6, 2018

It should be able to be either a function or a string.

@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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants