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

Overriding the command registry keydown handler causes shortcut execution errors. #55

Open
afshin opened this issue Feb 16, 2021 · 1 comment

Comments

@afshin
Copy link

afshin commented Feb 16, 2021

There is a spot where we monkey-patch the Lumino CommandRegistry's processKeydownEvent method and it is occasionally brittle, causing command invocations to fail. This seems like an anti-pattern for two reasons:

  1. We are overriding the entire application's command registry and can cause other extensions to fail.
  2. We are relying on an object we've attached to the window object instead of a local variable.
app.commands.processKeydownEvent = (event) => {
  if (window.beakerx.tableFocused) {
    return false;
  }

  return originalProcessFn.call(app.commands, event);
};

https://github.com/twosigma/beakerx_widgets/blob/master/beakerx_widgets/js/src/lab/BeakerxWidgetExtension.ts#L67-L69

This problem is sometimes triggered when a user changes the kernel on an active notebook multiple times. One user reported that after switching the kernel a second or third time, the window.beakerx object is undefined so the handler fails while trying to access the tableFocused attribute.

@afshin
Copy link
Author

afshin commented Apr 1, 2021

We should author a test that captures failures of window.beakerx as well ... unless we refactor it out entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant