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

Should we expose IShell.activeWidget, or short-circuit in IShell.activateById? #14883

Open
krassowski opened this issue Jul 30, 2023 · 1 comment
Labels
api-change A change that should be accompanied by a major version increase enhancement status:Needs Discussion

Comments

@krassowski
Copy link
Member

Problem

Extensions may want to activate a widget if it is not active. This is because activating a widget may have side effects. In case of notebooks the side effect of ensuring focus can lead to problematic behaviour in CodeMirror editor extensions, such as jupyterlab-vim. In jupyterlab-contrib/jupyterlab-vim#85 I workaround it by comparing the widget with IShell.currentWidget but it feels like we should be comparing against IShell.activeWidget; however that is not exposed as public API.

Proposed Solution

a) expose IShell.activeWidget,
b) short-circuit in IShell.activateById if provided id is equal IShell.activeWidget.id

Downsides:

  • (a) increases API surface
  • (b) may break existing code if it is relying on side effects always being invoked

Additional context

Implementation has activateById, activeWidget, and currentWidget:

/**
* The active widget in the shell's main area.
*/
get activeWidget(): Widget | null {
return this._tracker.activeWidget;
}

/**
* The current widget in the shell's main area.
*/
get currentWidget(): Widget | null {
return this._tracker.currentWidget;
}

/**
* Activate a widget in its area.
*/
activateById(id: string): void {

Public API only exposes activateById and currentWidget:

/**
* A minimal shell type for Jupyter front-end applications.
*/
export interface IShell extends Widget {
/**
* Activates a widget inside the application shell.
*
* @param id - The ID of the widget being activated.
*/
activateById(id: string): void;

/**
* The focused widget in the application shell.
*
* #### Notes
* Different shell implementations have latitude to decide what "current"
* or "focused" mean, depending on their user interface characteristics.
*/
readonly currentWidget: Widget | null;

@krassowski krassowski added enhancement status:Needs Discussion api-change A change that should be accompanied by a major version increase status:Needs Triage Applied to new issues that need triage labels Jul 30, 2023
@fcollonval
Copy link
Member

I support you with this proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change A change that should be accompanied by a major version increase enhancement status:Needs Discussion
Projects
None yet
Development

No branches or pull requests

3 participants