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

NotebookWidgetFactory add toolbarItems option (#5363) #5370

Merged
merged 6 commits into from Sep 27, 2018

Conversation

@futurist
Copy link
Contributor

@futurist futurist commented Sep 24, 2018

Fixes #5363

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 24, 2018

Thanks for this, @futurist! Looking at your code, it makes me think that we should enable this for all document widgets, not just the notebook panel. What that would mean is extending this interface:

export interface IWidgetFactory<T extends IDocumentWidget, U extends IModel>

And this interface:

export interface IOptions<

The ABCWidgetFactory would have a toolbarItems property, and the NotebookWidgetFactory would set its default to ToolbarItems.defaultItems: IToolbarItem[]. We would add those toolbar items after creating the widget:

let widget = this.createNewWidget(context);

Note, the notebook toolbar items would have to be refactored to take the full panel instead of just the session.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 24, 2018

Link for ABCWidgetFactory:

export abstract class ABCWidgetFactory<

@futurist
Copy link
Contributor Author

@futurist futurist commented Sep 25, 2018

Great feature! How about the idea to add toolbarItems into DocumentRegistry.IContext:

export interface IContext<T extends IModel> extends IDisposable {

Since every DocumentWidget.IOpitons has an context.

context: DocumentRegistry.IContext<U>;

The idea is context reflect the model, and the toolbarItems can also reflect to a model, like Code model and Markdown model and DSVModel can use different toolbarItems.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 25, 2018

I'm torn on this one, toolbars could be considered a view item or a model item. My initial inclination is that they are a view item.

@futurist
Copy link
Contributor Author

@futurist futurist commented Sep 25, 2018

I pushed new commit, please review

Copy link
Member

@blink1073 blink1073 left a comment

Excellent! Just a few nits/suggestions.

/**
* toolbar items to be added when createNew
*/
toolbarItems?: DocumentRegistry.IToolbarItem[];
Copy link
Member

@blink1073 blink1073 Sep 25, 2018

I think this should always be available, and be an empty list by default.

Copy link
Contributor Author

@futurist futurist Sep 25, 2018

can be empty here is to want distinguish the absence of toolbar or an empty toolbar.

new NotebookWidgetFactory({name: 'notebook', fileTypes: ['notebook']})

Will give an default toolbar, whereas

new NotebookWidgetFactory({name: 'notebook', fileTypes: ['notebook'], toolbarItems: []})

Will give an empty toolbar.

But if there's a better way then good to go!

Copy link
Member

@blink1073 blink1073 Sep 25, 2018

What I mean is that it should be optional in IOptions but concrete on ABCWidgetFactory, where we set it to options.toolbarItems || [] in the constructor.

Copy link
Contributor Author

@futurist futurist Sep 25, 2018

I tried to put the code this.toolbarItems = options.toolbarItems || [] in ABCWidgetFactory.constructor, but then later in NotebookWidgetFactory.createNewWidget the below code:

if (!this.toolbarItems) {
      this.toolbarItems = ToolbarItems.getDefaultItems(widget);
}

cannot work, since the options.toolbarItems is lost, if this.toolbarItems is [], we don't know whether it's user intended or just empty to want to fallback to default toolbar.

I agree with you that it's better to have a solid type for this.toolbarItems, the null is evil.

One way I can image is to save another state that whether fallback to default toolbar? Or just provide a way to let user populate default toolbar?

Copy link
Member

@blink1073 blink1073 Sep 25, 2018

Hmm, okay, I see your point. Maybe it should be a function that we pass in as the option instead: (widget: T) => IToolbarItems. The NotebookWidgetFactory would default to ToolbarItems.getDefaultItems.

Copy link
Contributor Author

@futurist futurist Sep 26, 2018

The NotebookWidgetFactory.IOptions has toolbarItems too, need to store this then in createNewWidget to decide whether to populate default items, maybe in ABCWidgetFactory

protected _defaultToolbarItems: DocumentRegistry.IToolbarItem[] | undefined;

is better?

Copy link
Member

@blink1073 blink1073 Sep 26, 2018

How about private _overrideToolbarItems: DocumentRegistry.IToolbarItem[] | undefined and protected defaultToolbarItems: DocumentRegistry.IToolbarItem[]?

Copy link
Contributor Author

@futurist futurist Sep 26, 2018

sorry I don't get your point... I think the use case can make me more clear:

new NotebookWidgetFactory({name: 'notebook', fileTypes: ['notebook'], toolbarItems: [A, B]})
// 'default-toolbar.ts':  getDefault: [X, Y, Z]

What will be the value of _overrideToolbarItems and defaultToolbarItems ?

new NotebookWidgetFactory({name: 'notebook', fileTypes: ['notebook'], toolbarItems: undefined})

This one should set widget.toolbar to [X, Y, Z]
What will be the value of _overrideToolbarItems and defaultToolbarItems ?

Copy link
Contributor Author

@futurist futurist Sep 26, 2018

I've pushed new commit to reflect the latest discussion.

Copy link
Member

@blink1073 blink1073 Sep 26, 2018

I think we're over-complicating this. Looking back at your original ask, all we wanted was the ability to override the default via IOptions. In light of that, I propose:

  1. IOptions.overrideToolbarItems?: IToolbarItem
  2. protected overrideToolbarItems: IToolbarItem[] | undefined`
  3. In createWidget(), add _overrideToolbarItems if it is defined.
  4. In NotebookWidgetFactory.createNewWidget(), if overrideToolbarItems is undefined, use ToolbarItems.getDefaultItems() to populate.

@@ -190,6 +192,37 @@ export namespace ToolbarItems {
Toolbar.createKernelStatusItem(panel.session)
);
}

export function getDefaultItems(
Copy link
Member

@blink1073 blink1073 Sep 25, 2018

Needs a docstring 😄

@@ -83,7 +86,10 @@ export class NotebookWidgetFactory extends ABCWidgetFactory<
let content = this.contentFactory.createNotebook(nbOptions);

let widget = new NotebookPanel({ context, content });
ToolbarItems.populateDefaults(widget);
// ToolbarItems.populateDefaults(widget);
Copy link
Member

@blink1073 blink1073 Sep 25, 2018

Please remove rather than comment out

/**
* Array of toolbar items to be added to NotebookPanel.
*/
toolbarItems?: IToolbarItem[];
Copy link
Member

@blink1073 blink1073 Sep 25, 2018

We should use an interface from DocumentRegistry here rather than redefining it locally.

@futurist futurist force-pushed the customize-toolbar branch from 15fa122 to 519e807 Sep 26, 2018
@futurist futurist force-pushed the customize-toolbar branch from 79d12eb to fb67bfc Sep 26, 2018
@futurist
Copy link
Contributor Author

@futurist futurist commented Sep 26, 2018

I've pushed new commit according to your 4 step proposal, just one little request:

const factory = new WidgetFactory({
          name: 'test',
          fileTypes: ['text'],
          overrideToolbarItems: [
            {
              name: 'foo',
              widget: new Widget()
            },
            {
              name: 'bar',
              widget: new Widget()
            }
          ]
});

The word overrideToolbarItems in options is very long, can it be simplified?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 27, 2018

Yeah, that's fair to make the IOptions name shorter.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 27, 2018

I think just toolbarItems is enough.

@futurist
Copy link
Contributor Author

@futurist futurist commented Sep 27, 2018

Here we go, please wait for the green!

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 27, 2018

Looks great, thanks for tolerating my nit-picking!

@futurist
Copy link
Contributor Author

@futurist futurist commented Sep 27, 2018

That's perfectly OK! No Steve, No iPhone!

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 27, 2018

Ha! 1) Hopefully we're friendlier than Steve. 2) Hopefully we're as successful as the iPhone. Thanks again for the PR!

@blink1073 blink1073 merged commit 98b4e6a into jupyterlab:master Sep 27, 2018
1 of 2 checks passed
@blink1073 blink1073 added this to the 0.35 milestone Sep 27, 2018
jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Sep 28, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

Because a phosphor widget can only have one parent, it doesn't work to pass in specific widgets that will be used in multiple panels created. This means that if I pass in some toolbar items to a widget factory, then create two widgets, the second widget will effectively steal the toolbar items from the first widget.

#5395 contains some tests that illustrate this issue. Let's continue the conversation over there.

@blink1073 blink1073 mentioned this pull request Sep 28, 2018
31 tasks
@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.

3 participants