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

Handle inspection on cursor movement #5906

Merged
merged 6 commits into from Jan 28, 2019
Merged

Conversation

@ntdef
Copy link
Contributor

@ntdef ntdef commented Jan 25, 2019

Resolves #5899

@ntdef
Copy link
Contributor Author

@ntdef ntdef commented Jan 25, 2019

There's a lot of duplication in this PR but I wanted to put this out there to make sure I'm on the right track first before working on consolidating.

Loading

@ntdef
Copy link
Contributor Author

@ntdef ntdef commented Jan 25, 2019

A test failed in @jupyterlab/test-outputarea but it doesn't seem related to this PR.

Loading

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks @ntdef! This approach is sensible, though I agree there is a fair amount of duplication.

What do you think about adding the ability for the ActivityMonitor to connect to more than one signal, with a single activityStopped emitter? In that case we could just connect both signals to the same monitor. I';m unsure whether it's a good idea, just musing...

Loading

packages/inspector/src/handler.ts Outdated Show resolved Hide resolved
Loading
timeout: 1000
});
this._valueMonitor.activityStopped.connect(
this.onEditorChange,
this
);
}
Copy link
Member

@ian-r-rose ian-r-rose Jan 25, 2019

Choose a reason for hiding this comment

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

It is possible to select a new cell/editor without changing the value or cursor position, in which case the inspector won't update. I think we should call onEditorChange() here as well to catch that case.

Loading

Copy link
Contributor Author

@ntdef ntdef Jan 26, 2019

Choose a reason for hiding this comment

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

Gotchu. Would that not trigger a editor.model.selections.changed? Is there another event that I should use?

Loading

Copy link
Member

@ian-r-rose ian-r-rose Jan 26, 2019

Choose a reason for hiding this comment

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

If the selection of the new cell doesn't move the cursor, it won't trigger a change signal (I was able to verify by clicking on the same cursor position in a cell repeatedly). But you needn't listen to another event for that, we can just to call the onEditorChange() function in this setter directly.

Loading

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 25, 2019

Also cc @saulshanabrook who has been doing some thinking about signal composition.

Loading

@ntdef
Copy link
Contributor Author

@ntdef ntdef commented Jan 26, 2019

What do you think about adding the ability for the ActivityMonitor to connect to more than one signal, with a single activityStopped emitter? In that case we could just connect both signals to the same monitor. I';m unsure whether it's a good idea, just musing...

Yeah, I agree, I think that would be tidy way to handle signals.

Loading

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 26, 2019

Yeah, I agree, I think that would be tidy way to handle signals.

Happy to keep this as is, however, and revisit it when we start implementing some of these signal-composing ideas down the line.

Loading

@ntdef
Copy link
Contributor Author

@ntdef ntdef commented Jan 27, 2019

Ok, this last commit should handle a cell change.

Loading

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 28, 2019

Looks good to me, thanks @ntdef!

Loading

@ian-r-rose ian-r-rose merged commit c372251 into jupyterlab:master Jan 28, 2019
3 checks passed
Loading
@jasongrout jasongrout added this to the 1.0 milestone Feb 2, 2019
@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