Prevent jumping of window to input when output is clicked. #1858

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

mcelrath commented Jun 5, 2012

When the user clicks on the output portion of a cell, the window will scroll to the corresponding input portion of the cell. This is very disruptive when you have multi-page output and makes examining or cutting/selecting output very tedious.

This patch fixes that by explicitly calling calling code_mirror.focus() when it is necessary (on keypress up/down and execute) and removing the corresponding call from CodeCell.protototype.select().

I also removed the code_mirror.refresh() calls, as they are unnecessary.

(This is the same code as #1857, I have reorganized my repository to have a branch 'jumpy_output' following @fperez 's recommendation. Sorry for being a git neophyte...)

mcelrath commented Jun 7, 2012

FYI, as @ellisonbg probably suspects, this patch introduced a number of focus related issues. I'm testing an improved that works as follows:
A method is added notebook.prototype.focus_selected_cell() that is called judiciously after notebook operations, alongside Notebook.prototype.select().

After each operation that modifies the notebook, the Notebook must decide 1) what to select and 2) what to focus. So each operation (e.g. run, cut/paste, add cell, etc) a cell will be focused and selected as appropriate. #1 could be multiple cells. Consider typing shift-down when no cell is focused, to select two cells but focus neither.

There currently is no real infrastructure for handling clicks in either Notebook or any of the Cell classes. CodeMirror has an onMouseDown handler, which focuses a CodeMirror element if it is clicked. The net effect is that with my new patch, clicking anywhere except on the CodeMirror element will only select, but never focus a cell.

I think handling of clicks by the Notebook and Cell should be more explicit, to enable things like multi-select and interactive plots. So Notebook should have an onMouseDown handler that can handle multi-select, and Cell classes should also to handle self-focusing. The latter requires that Cell inform Notebook that the cell has been focused, so that Notebook can de-focus the previously focused cell. This introduces a dependency on Notebook for *Cell, which is something I gather from the comments that you want to avoid. But I see no way for Notebook to decide whether a clicked cell should be focused, since the Notebook shouldn't have detailed knowledge of different cell types. e.g. did the user click on the input area, or an interactive plot that may read keystrokes, or did he click on In[30]: or on CodeMirror's gutter?

Thoughts? Maybe there's a way to avoid the dependency this using chained handlers...

P.S. There's also lots of jumping associated with executing the contents of a cell that I intend to address...

Owner

fperez commented Jun 8, 2012

Thanks for improving your git workflow quickly, I'm sure you'll also find it to provide a smoother experience on your side.

I'm going to have to leave the details of this one to @ellisonbg and others who have dug into the JS code more than me...

mcelrath commented Jun 8, 2012

No problem. I've been meaning to start using a SCM more heavily (including everyday work) for several years now. IPython gave me the excuse to take the plunge... I set up several private work repositories, and set up my bash prompt and vim to tell me the branch, etc. I like it!

Quick question: I find myself destroying and re-making commits. Is this advisable? Or should I request that upstream pull from a branch, and put incremental improvement commits into that branch?

Owner

fperez commented Jun 8, 2012

On Thu, Jun 7, 2012 at 9:29 PM, Bob McElrath
reply@reply.github.com
wrote:

No problem.  I've been meaning to start using a SCM more heavily (including everyday work) for several years now.  IPython gave me the excuse to take the plunge...  I set up several private work repositories, and set up my bash prompt and vim to tell me the branch, etc.  I like it!

Excellent. I've more or less mentally aliased mkdir to git init :)
Version control is a way of life for me: writing papers, grant
proposals, teaching classes, etc, I do it all via git. Email is for
textual communications, attachments should more or less be banned from
existence in collaborative endeavors. Once you get into the habit of
it, you'll wonder how you managed without.

Quick question: I find myself destroying and re-making commits.  Is this advisable?  Or should I request that upstream pull from a branch, and put incremental improvement commits into that branch?

I'm not quite sure I understand what you're saying, could you describe
the situation you're wondering about with a bit more detail?

mcelrath commented Jun 8, 2012

Fernando Perez
[reply@reply.github.com]
wrote:

Quick question: I find myself destroying and re-making commits.  Is this
advisable?  Or should I request that upstream pull from a branch, and put
incremental improvement commits into that branch?

I'm not quite sure I understand what you're saying, could you describe the
situation you're wondering about with a bit more detail?

e.g. I make and commit some changes (and maybe even push them to github). Then
I find they're incomplete and I want to add to it. I could:

  1. make a second commit
  2. do a 'git reset --soft HEAD~1' which reverts the last commit, leaving the
    changes as unstaged, make the changes, and commit again.

I've been doing #2, it seems to create a single self-contained idea as a single
commit.

But maybe no one cares about having everything in one commit and having
everything in one branch is enough?

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Owner

fperez commented Jun 9, 2012

Ah, got it. For cases like that, you can use the git commit --amend command, which lets you add new material and edit the commit message, without creating yet a new commit. If you haven't pushed yet, that's it. If you have already pushed the old commit, simply do a git push --force and the branch will update.

It's perfectly OK, and routine, to do forced pushes and rebases on branches that are only visible as PRs. The place where that is never done is on a branch that is already used by others to build new branches off, such as master.

I hope this is clear...

Owner

fperez commented Jun 13, 2012

@mcelrath, what are your thoughts on this one? I read the discussion above and it looks like this may need some further work, right? I'm trying to do some triage before our first beta, due tomorrow.

I have several more fixes in that seem quite stable.

One more major fix I wanted to add was when a cell is executed that already
contains long output: the browser scrolls the executed cell out of view. I
remember seeing possibly other work on this? I'll have to investigate.

I could push an update tomorrow morning (US Pacific) possibly including that fix
if that's not too late for you.

Fernando Perez [reply@reply.github.com] wrote:

@mcelrath, what are your thoughts on this one? I read the discussion above and it looks like this may need some further work, right? I'm trying to do some triage before our first beta, due tomorrow.


Reply to this email directly or view it on GitHub:

#1858 (comment)

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Owner

fperez commented Jun 13, 2012

Sounds good, go for it. BTW, are you subscribed to the dev mailing list? While these days most of the hands-on work happens on issues and PRs on github, the list is still a good idea to be subscribed for some of the more general project discussions. Just a suggestion...

BTW, have you thought any further about the dialog for the Courier New issue, after Ondrej's comments? It might be nice to tone that one down before the upcoming beta (I hope to cut it by tomorrow night).

Fernando Perez [reply@reply.github.com] wrote:

Sounds good, go for it. BTW, are you subscribed to the dev mailing list? While these days most of the hands-on work happens on issues and PRs on github, the list is still a good idea to be subscribed for some of the more general project discussions. Just a suggestion...

Yes, I'm there.

BTW, have you thought any further about the dialog for the Courier New issue,
after Ondrej's comments? It might be nice to tone that one down before the
upcoming beta (I hope to cut it by tomorrow night).

I put an alternative patch in my repo:
#1883 (comment)
which silently fixes the issue (no dialog). If you like this better feel free
to pull that.

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

The other issues that impact my solution on this one are #1553 and #1825. Am I
correct that these won't be merged for the beta? Or should I take them into
account?

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Owner

fperez commented Jun 13, 2012

Hi Bob,

On Wed, Jun 13, 2012 at 7:12 AM, Bob McElrath
reply@reply.github.com
wrote:

Fernando Perez [reply@reply.github.com] wrote:

Sounds  good, go for it.  BTW, are you subscribed to the dev mailing list?  While these days most of the hands-on work happens on issues and PRs on github, the list is still a good idea to be subscribed for some of the more general project discussions.  Just a suggestion...

Yes, I'm there.

Great, glad to hear that.

I put an alternative patch in my repo:
   #1883 (comment)
which silently fixes the issue (no dialog).  If you like this better feel free
to pull that.

Would you mind pushing a quick PR on that? I prefer if we follow the
review workflow even for small changes, it gives everyone a chance to
take a peek. Thanks!

Owner

fperez commented Jun 13, 2012

On Wed, Jun 13, 2012 at 7:41 AM, Bob McElrath
reply@reply.github.com
wrote:

The other issues that impact my solution on this one are #1553 and #1825.  Am I
correct that these won't be merged for the beta?  Or should I take them into
account?

We'll wait an extra day to give @ellisonbg a chance to review some of
the NB stuff: he's been offline for a couple of days and his eyes on
the NB material are really important. So you have an extra day or so
before the beta; I won't cut it before Thursday night (pacific) at the
earliest.

The overlapping focusing and scrolling capabilities of CodeMirror, jquery-ui, and browser forms are driving me nuts...still working on this. Each change seems to break 10 other focus-related behaviors.

I'm tired of having to grab the scroll bar and go hunting for the focused cell in the existing codebase...

Owner

ellisonbg commented Jun 16, 2012

I will try to help with the review of this soon. I know it is
important, but it is also subtle.

On Fri, Jun 15, 2012 at 4:48 PM, Bob McElrath
reply@reply.github.com
wrote:

The overlapping focusing and scrolling capabilities of CodeMirror, jquery-ui, and browser forms are driving me nuts...still working on this.  Each change seems to break 10 other focus-related behaviors.

I'm tired of having to grab the scroll bar and go hunting for the focused cell in the existing codebase...


Reply to this email directly or view it on GitHub:
#1858 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

fperez commented Jun 26, 2012

Question: we're getting down to the wire for releasing 0.13. What do you guys think of this one? From the sound of your discussion I'm thinking we should retarget this for 0.14, as it seems to be pretty subtle. Thoughts?

Fernando Perez [reply@reply.github.com] wrote:

Question: we're getting down to the wire for releasing 0.13. What do you guys
think of this one? From the sound of your discussion I'm thinking we should
retarget this for 0.14, as it seems to be pretty subtle. Thoughts?

I think re-targetting for 0.14 is a good idea.

There are some nasty interactions occurring between jQuery and CodeMirror, and
I've yet to fully disentangle that mess. This is even responsible for some of
the slowness I reported in my SVG mail.

I can provide a simple patch to fix the stated problem, but there are a very
large number of ways that the window "jumps" upon focus changes, clicks, output,
etc. and I was trying to identify and fix them all. I'd like to retarget my
branch for 0.14 to fix all these problems.

It's more complicated than one might expect because if you click on an output
cell, and don't want the page to scroll up or down when you do so, you end up
with a focused CodeMirror textarea off-screen, which the browser doesn't like --
so focus handing has to change. And fixing this leads down the rabbit hole...

Owner

fperez commented Jun 26, 2012

Great, I'll re-target now, and you'll have plenty of time to dig into those subtleties. Thanks for your patience with these issues, we're really glad to have you on board.

Owner

Carreau commented Aug 24, 2012

Hi,
This PR have no update for 2 month now.
What is it's status ?

As discussed on a list, we'd like to keep the PR list short.
The consensus on the list is that we can close a PR but open an Issue referencing it. It does not prevent the author or the dev to reopen the PR later.

I got mired in a lot of little changes, and didn't get this PR to a satisfactory
state. I decided to dump my changes and rewrite them.

Now, I think I may have lost them due to using rebase instead of merge. :(
(most changes were not pushed to github)

I think you should close this PR.

Bussonnier Matthias [notifications@github.com] wrote:

Hi,
This PR have no update for 2 month now.
What is it's status ?

As discussed on a list, we'd like to keep the PR list short.
The consensus on the list is that we can close a PR but open an Issue
referencing it. It does not prevent the author or the dev to reopen the PR
later.


Reply to this email directly or view it on GitHub.

*

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Owner

Carreau commented Aug 24, 2012

Now, I think I may have lost them due to using rebase instead of merge. :(
(most changes were not pushed to github)

Nothing is ever lost with git (almost).
with git reflog you can get a chronological order of all the commit you went through.
You then can checkout those commit by sha instead of branch name.

This is the occasion to try :-)

an example :

$ git reflog
2e2514a HEAD@{0}: checkout: moving from autohighlight to metaui
53af75a HEAD@{1}: reset: moving to github/autohighlight
75d6b39 HEAD@{2}: rebase finished: returning to refs/heads/autohighlight
75d6b39 HEAD@{3}: rebase: fix some whitespace
0928e9d HEAD@{4}: rebase: autochange highlight with cell magics  <---- lost commit 
9e3dc7f HEAD@{5}: checkout: moving from autohighlight to 9e3dc7f4054eff90360a62d08b62055bbd2
53af75a HEAD@{6}: checkout: moving from master to autohighlight  
9e3dc7f HEAD@{7}: checkout: moving from autohighlight to master
...

$ git checkout 0928e9d
Note: checking out '0928e9d'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at 0928e9d... autochange highlight with cell magics

$ git checkout -b saved_commit   
Switched to a new branch 'saved_commit'
Owner

ellisonbg commented Aug 24, 2012

Ok, but let's open an issue for this

Sent from my iPad

On Aug 24, 2012, at 10:29 AM, Bob McElrath notifications@github.com wrote:

I got mired in a lot of little changes, and didn't get this PR to a satisfactory
state. I decided to dump my changes and rewrite them.

Now, I think I may have lost them due to using rebase instead of merge. :(
(most changes were not pushed to github)

I think you should close this PR.

Bussonnier Matthias [notifications@github.com] wrote:

Hi,
This PR have no update for 2 month now.
What is it's status ?

As discussed on a list, we'd like to keep the PR list short.
The consensus on the list is that we can close a PR but open an Issue
referencing it. It does not prevent the author or the dev to reopen the PR
later.


Reply to this email directly or view it on GitHub.

*

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Reply to this email directly or view it on GitHub.

Ok, opening one.

I have a .ipynb notebook documenting undesirable notebook behavior and what is
(in my opinion) more desirable behavior. I've fixed about 3/4 of them, I will
have to review them and also look at what has changed with CodeMirror, as that
was a source of several bugs.

It would also be useful to have others look at it and agree/disagree on what is
"undesirable" behavior, and what behavior is desired.

Where could I upload such a file? IPython/testing? I don't see a way to upload
auxiliary files to github as is common with bugzilla.

Brian E. Granger [notifications@github.com] wrote:

Ok, but let's open an issue for this

Sent from my iPad

On Aug 24, 2012, at 10:29 AM, Bob McElrath notifications@github.com wrote:

I got mired in a lot of little changes, and didn't get this PR to a
satisfactory
state. I decided to dump my changes and rewrite them.

Now, I think I may have lost them due to using rebase instead of merge. :(
(most changes were not pushed to github)

I think you should close this PR.

Bussonnier Matthias [notifications@github.com] wrote:

Hi,
This PR have no update for 2 month now.
What is it's status ?

As discussed on a list, we'd like to keep the PR list short.
The consensus on the list is that we can close a PR but open an Issue
referencing it. It does not prevent the author or the dev to reopen the PR
later.


Reply to this email directly or view it on GitHub.

*

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

*

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Owner

minrk commented Aug 24, 2012

On Fri, Aug 24, 2012 at 1:26 PM, Bob McElrath notifications@github.comwrote:

Ok, opening one.

I have a .ipynb notebook documenting undesirable notebook behavior and
what is
(in my opinion) more desirable behavior. I've fixed about 3/4 of them, I
will
have to review them and also look at what has changed with CodeMirror, as
that
was a source of several bugs.

It would also be useful to have others look at it and agree/disagree on
what is
"undesirable" behavior, and what behavior is desired.

Where could I upload such a file? IPython/testing? I don't see a way to
upload
auxiliary files to github as is common with bugzilla.

We typically use gists for associated files, and then you can use
nbviewer.ipython.org to view the rendered notebook online.

Brian E. Granger [notifications@github.com] wrote:

Ok, but let's open an issue for this

Sent from my iPad

On Aug 24, 2012, at 10:29 AM, Bob McElrath notifications@github.com
wrote:

I got mired in a lot of little changes, and didn't get this PR to a
satisfactory
state. I decided to dump my changes and rewrite them.

Now, I think I may have lost them due to using rebase instead of
merge. :(
(most changes were not pushed to github)

I think you should close this PR.

Bussonnier Matthias [notifications@github.com] wrote:

Hi,
This PR have no update for 2 month now.
What is it's status ?

As discussed on a list, we'd like to keep the PR list short.
The consensus on the list is that we can close a PR but open an Issue
referencing it. It does not prevent the author or the dev to reopen
the PR
later.


Reply to this email directly or view it on GitHub.

*

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being
overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes
frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub.

*

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed
by
the tribe. If you try it, you will be lonely often, and sometimes
frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/1858#issuecomment-8013854.

Min RK [notifications@github.com] wrote:

We typically use gists for associated files, and then you can use
nbviewer.ipython.org to view the rendered notebook online.

You mean to cut and paste the .ipynb into gist.github.com? There does not seem
to be a way to upload a file there, I should cut and paste it?

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Owner

Carreau commented Aug 24, 2012

Yes, copy and past works.
https://github.com/defunkt/gist
Also to upload files.

nbviewer works with any url if you prefer other hosting platform. Just give it the url to a raw file.

Unfortunately this tool 'gist' does not work. I get a
SSL_connect returned=1 errno=0 state=SSLv3 read server certificate B: certificate verify failed
and upon using some experimental version referenced in their issue tracker I get:
Creating gist failed: 500 Internal Server Error

Anyway I cut and pasted. The document describing what PR #1858 is intended to
fix is:
git://gist.github.com/3455828.git
http://nbviewer.ipython.org/3455828

Bussonnier Matthias [notifications@github.com] wrote:

Yes, copy and past works.
https://github.com/defunkt/gist
Also to upload files.

nbviewer works with any url if you prefer other hosting platform. Just give it

the url to a raw file.

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

Owner

Carreau commented Sep 30, 2012

Hi,
This PR is without activities for 1 month now,
I'm going to close it and open an issue to reference it.
This does not mean than we refuse the code that is here, but that we try to keep the number of opened Pull request as low as possible to have better bird view of active code.

Fell free to reopen this PR whenever you want or contact us if you have any questions.

Thanks for contributing.

@Carreau Carreau closed this Sep 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment