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.

Sign up for GitHub

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

Register Context Commands with already added types #399

Merged
merged 3 commits into from
Nov 11, 2020
Merged

Register Context Commands with already added types #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4fa0860
Register Context Commands with already added types
mnowacki-b Nov 5, 2020
e586f91
Add registerAdapterType to LSPExtension
mnowacki-b Nov 5, 2020
4a95fd1
Merge remote-tracking branch 'origin/master' into context-menu
mnowacki-b Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/jupyterlab-lsp/src/adapter_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export class WidgetAdapterManager implements ILSPAdapterManager {
type.tracker.widgetAdded.connect((tracker, widget) => {
this.connectWidget(extension, widget, type);
});
extension.registerAdapterType(this, type);
}

public registerExtension(extension: LSPExtension) {
Expand Down
29 changes: 17 additions & 12 deletions packages/jupyterlab-lsp/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
import { ICommandPalette } from '@jupyterlab/apputils';
import { ISettingRegistry } from '@jupyterlab/settingregistry';
import { IDocumentManager } from '@jupyterlab/docmanager';
import { IDocumentWidget } from '@jupyterlab/docregistry';
import { Signal } from '@lumino/signaling';
import { LanguageServerManager } from './manager';
import '../style/index.css';
Expand All @@ -13,6 +14,7 @@ import { IStatusBar } from '@jupyterlab/statusbar';
import { LSPStatus } from './components/statusbar';
import { DocumentConnectionManager } from './connection_manager';
import {
IAdapterTypeOptions,
ILSPAdapterManager,
ILSPCodeExtractorsManager,
ILSPFeatureManager,
Expand Down Expand Up @@ -123,7 +125,7 @@ export class LSPExtension implements ILSPExtension {
constructor(
public app: JupyterFrontEnd,
private setting_registry: ISettingRegistry,
palette: ICommandPalette,
private palette: ICommandPalette,
documentManager: IDocumentManager,
paths: IPaths,
status_bar: IStatusBar,
Expand Down Expand Up @@ -167,19 +169,22 @@ export class LSPExtension implements ILSPExtension {
});

adapterManager.registerExtension(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the ordering of this line and the connect below doesn't matter, perhaps we could:

  • connect first
  • resigterExtension
    • have registerExtension (which I assume can only be called once?) do this.types.foreach((t) => this.adapterTypeAdded.emit(t))

Copy link
Member

Choose a reason for hiding this comment

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

I like this idea! In addition, it is worth noting that registerExtension already does two loops (one to connect to already registered adapters and second to watch for future registrations), so it seems like a natural place to address this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both for your quick response!
It sounds like a good idea; but I do have concerns (maybe they are unfounded).
If we do reemit adapterTypeAdded signal won't we cause potential problems if another extension decides to connect to it and then starts to receive multiple events with the same type?
Also, can we really assume that resigterExtension can be called only once? If so should we have something in place to enforce that?
Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bollwyvl @krassowski
I am more then happy to make the requested changes; but I thought I will make a small proposal first :)
I made some alterations please have a look and let me know what you think.
Thank you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gah, lost what i was typing. So it goes.

So: it looks like registerExtension is expecting an LSPExtension, rather than an ILSPExtension. I think this kinda signals that it's meant for in-house usage, or would be entirely replaced, rather than having multiple. Perhaps @krassowski can suggest some tactical docstrings. If it can be multiple, then registerAdapterType probably needs to be added to the interface...

Copy link
Member

Choose a reason for hiding this comment

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

it looks like registerExtension is expecting an LSPExtension, rather than an ILSPExtension. I think this kinda signals that it's meant for in-house usage

yes, this is an accurate description.

}

adapterManager.adapterTypeAdded.connect((manager, type) => {
let command_manger = new ContextCommandManager({
adapter_manager: adapterManager,
app: app,
palette: palette,
tracker: type.tracker,
suffix: type.name,
entry_point: type.entrypoint,
...type.context_menu
});
this.feature_manager.registerCommandManager(command_manger);
registerAdapterType(
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 to extraction to a method.

adapterManager: ILSPAdapterManager,
type: IAdapterTypeOptions<IDocumentWidget>
): void {
let command_manger = new ContextCommandManager({
adapter_manager: adapterManager,
app: this.app,
palette: this.palette,
tracker: type.tracker,
suffix: type.name,
entry_point: type.entrypoint,
...type.context_menu
});
this.feature_manager.registerCommandManager(command_manger);
}

get foreign_code_extractors() {
Expand Down