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

Disable ForeignHandler by default and add command console:toggle-echo to enable ForeighHandler through context menu #4503

Merged
merged 11 commits into from May 2, 2018

Conversation

@BoPeng
Copy link
Collaborator

@BoPeng BoPeng commented May 1, 2018

This patch disables ForeignHandler in console window by default so expressions evaluated in notebooks will not be echoed in associated console window. A command console:toggle-echo is added to the context menu to allow echo.

See details in #4424

Copy link
Contributor

@jasongrout jasongrout left a comment

Thanks! I've noted some things inline. This was surprisingly straightforward - a testament to @afshin's design.

@@ -399,6 +402,17 @@ function activateConsole(app: JupyterLab, mainMenu: IMainMenu, palette: ICommand
isEnabled
});

commands.addCommand(CommandIDs.toggleEcho, {
label: 'Toggle echo…',
Copy link
Contributor

@jasongrout jasongrout May 1, 2018

Choose a reason for hiding this comment

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

We reserve the ellipsis for commands that require further confirmation, like a dialog box. Since this doesn't require further confirmation, we shouldn't have the ellipsis here.

@@ -148,7 +148,7 @@ describe('@jupyterlab/console', () => {
describe('#enabled', () => {

it('should default to `true`', () => {
expect(handler.enabled).to.be(true);
expect(handler.enabled).to.be(false);
Copy link
Contributor

@jasongrout jasongrout May 1, 2018

Choose a reason for hiding this comment

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

Test label is now 'should default to `false`'

Copy link
Contributor

@jasongrout jasongrout May 1, 2018

Choose a reason for hiding this comment

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

The test below should probably explicitly set handler.enabled = true, right?

Copy link
Collaborator Author

@BoPeng BoPeng May 1, 2018

Choose a reason for hiding this comment

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

Yes, you are right. handler.enabled = true; is added below.

Copy link
Contributor

@jasongrout jasongrout left a comment

Sorry, somehow I didn't submit my biggest comment.

* Toggle ForeignHandler
*/
toggleForeignHandler() {
this._foreignHandler.enabled = ! this._foreignHandler.enabled;
Copy link
Contributor

@jasongrout jasongrout May 1, 2018

Choose a reason for hiding this comment

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

Instead of a toggle, can we just expose this boolean attribute using a get/set pair? Maybe it could be named .echoForeignData or simply just .echo - not sure about a good name?

I think the toggling logic should be in the command, and at this level, it makes more sense to have the strictly more general get/set pair creating a new attribute exposing the enabled attribute.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 1, 2018

PR revised according to @jasongrout 's comments. Tests failed due to some sort of timeout error, which I do not know how to address.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 1, 2018

I slept on the name - how about .showAllActivity instead of .echoEnabled? I think it conveys what is happening - echo often means sending something right back, but we aren't sending anything back. Similarly, we can call the menu item Toggle showing all kernel activity or something.

Even better would be calling the menu item "Show all [kernel?] activity", but I think the command palette doesn't support having a checkmark showing state next to an item? @sccolbert, @ian-r-rose?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 1, 2018

@jasongrout That's right. Since the command palette doesn't render check marks, we have used that pattern in a bunch of places. It is accomplished by passing in an isPalette arg to the command, and conditionally rendering the label:

command = CommandIDs.toggleLeftArea;
app.commands.addCommand(command, {
label: args => args['isPalette'] ?
'Toggle Left Area' : 'Show Left Area',
execute: () => {

I like Show all kernel activity / Toggle showing all kernel activity.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 1, 2018

Note that the isPalette pattern can cause problems with the rendering of keyboard shortcuts (phosphorjs/phosphor#325)

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 1, 2018

Show all kernel activity is good, but Toggle showing all kernel activity is kind of long. How does Show only console activity sound?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 1, 2018

Hmmm - then it sounds to me like the default should be showing all activity, and you should check the box to narrow to just this console. And it's a bit confusing if you have two consoles sending things to the same kernel - it sounds a bit like it should show all activity from any console.

The other thing I thought of was something along the lines of "Show other activity" - but I like your concentration on showing just my activity language over the word "other".

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 1, 2018

@sccolbert - how hard would it be to show the state as a checkmark in the command palette, like in the menus?

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 1, 2018

The issue I see with the phrasing that emphasizes "my" or "this" is that a user probably won't have any idea what the "other" is. Even "other activity" isn't really descriptive as to what that "other" thing is. I would prefer something more descriptive to the user that also addresses the usability of the changed default, like "show all kernel activity from all attached notebooks/consoles." I know that is too long...

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 1, 2018

"Show all other kernel activity"?

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 1, 2018

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 1, 2018

I do not see a need to change the label of menu item. Is the following good enough?

Initially state:

image

After turning on show all other kernel activity:

image

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 1, 2018

I am personally not a fan of "other", since it is not especially clear what that is opposed to, but I don't consider that blocking. @BoPeng, can you capitalize the words in the label for consistency with the other menu items?

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 1, 2018

Changed but I would vote for "Show All Kernel Activity".

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 1, 2018

I agree with that.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 1, 2018

Done again. Hopefully this is acceptable to everyone.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 1, 2018

Note that I cannot make change of label work with the following change. args would get {'isPalette': true} the first time and then {} afterwards. Something is missing here although I am ok with a fixed label with check mark.

diff --git a/packages/console-extension/src/index.ts b/packages/console-extension/src/index.ts
index 6835ebae5..3d3b3cf0b 100644
--- a/packages/console-extension/src/index.ts
+++ b/packages/console-extension/src/index.ts
@@ -403,7 +403,8 @@ function activateConsole(app: JupyterLab, mainMenu: IMainMenu, palette: ICommand
   });

   commands.addCommand(CommandIDs.toggleShowAllActivity, {
-    label: args => 'Show All Kernel Activity',
+    //label: args => 'Show All Kernel Activity',
+    label: args => args['isPalette'] ? 'Show All Kernel Activity' : 'Show Only Console Activity',
     execute: args => {
       let current = getCurrent(args);
       if (!current) {

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 1, 2018

You can make it so that the label is context dependent: the arg isPalette get's passed when you add it to the command palette, and it gets rendered with that argument every time the command palette is rendered:
e.g.:
https://github.com/jupyterlab/jupyterlab/blob/master/packages/application-extension/src/index.tsx#L296

This is only relevant if you are actually adding the command to the command palette, which, as far as I cant tell, you have not done here.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 2, 2018

Line 430 adds toggleShowAllActivity (line 428) to the palette...

image

When I trace the label function, it was called with args={'isPalette': true} for a while but when I actually right-click, only args={} is passed.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 2, 2018

Ah, yes, so it does. If args={} is passed to the command palette, it should show the wrong label. Do you see that happening?

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 2, 2018

I see the wrong label (the last part of ? : ) all the time because {} is always passed when I right-click.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 2, 2018

I think we are talking past each other: the command palette (the panel in the the left area) is the part that gets the args isPalette: true. This is not the same as the context menu, to which you are not sending any args. So my question is:

  1. What do you want the label for the command palette to be?
  2. What do you want the label for the context menu to be?

If those two things are different, you can try to distinguish between them by passing different args. If they are not, then you don't need to do that.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 2, 2018

Phew, I see what is going on now. We currently have a fixed label Show All Kernel Activity with a checkmark showing the status. I am fine with that but I thought you also suggested the option to show Show All Kernel Activity initially, and change the label to something like Show Only Console Activity after the status is changed to show all activity...

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 2, 2018

Now I see that you were not talking about that behavior. Rather to show different labels in context menu and command palette, and there is a problem with showing the status in command palette because the checkmark is not rendered there.

Are we on the same pace now?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 2, 2018

Right, that is what I was referring to. Since the command palette does not render checkmarks (at least for now, I would like to change that), the label there would not be really correct. So I am proposing

  1. In the context menu have the label be Show All Kernel Activity with/without a check
  2. In the command palette have the label be Toggle Show All Kernel Activity

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 2, 2018

Done. I suppose that is no remaining issue with this PR.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 2, 2018

Inspired by the confusion here, and the workarounds that @ian-r-rose has had to do as well, perhaps we should merge @ian-r-rose's PR at #4510 first that simplifies the commands added to the palette, and then update this PR.

On the other hand, if this passes tests now, I'm happy to merge it and update the palette command later along with a lot of other commands.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 2, 2018

I am happy to do either. If we merge this first, I can update the content here in my PR.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 2, 2018

The tests should pass because the Travis test that failed last time has passed and appvoyer passed last time.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 2, 2018

Long delay at AppVoyer but all tests have passed now.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 2, 2018

Thanks @BoPeng, I think this is a nice improvement on the default behavior.

@ian-r-rose ian-r-rose merged commit b36f970 into jupyterlab:master May 2, 2018
2 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 3, 2018

Thanks again!

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented May 3, 2018

You all are very welcome!

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.

None yet

4 participants