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.

Sign up for GitHub

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

Jump to bottom

edit text cells on double-click instead of single-click #1224

Merged
merged 2 commits into from Jan 6, 2012
Merged

edit text cells on double-click instead of single-click #1224

merged 2 commits into from Jan 6, 2012

Conversation

minrk
Copy link
Member

@minrk minrk commented Jan 3, 2012

Single-click to edit interferes with using interactive elements (e.g. non-flash videos), and select/copy of the rendered HTML. Switching to double-click makes the edit action more intentional.

@ellisonbg
Copy link
Member

I am really torn by this one. On one hand I see that that single click could/should be used for other purposes. This would also enable us to implement cell sorting by dragging more easily. On the other hand, I think new users will find the behavior very confusing ("how the hell do I edit a cell"). The notebook is right at the boundary between being a document (where users will absolutely expect single click to enter edit mode) and an spreadsheet type environment (where single click selects, double click edits). I would like to see what others think and also see what we could do to mitigate that confusion for new users.

@minrk
Copy link
Member Author

minrk commented Jan 4, 2012

I appreciate the dilemma. But if you have anything other than plaintext in a markdown cell, it's essentially nonfunctional. I find single click to edit extremely jarring. I also think that new users double click on stuff all the time.

An alternative to double-click would be to put single-click to edit on a delay, so it will happen anyway, just not immediately. Of course, we could do both.

-MinRK

On Jan 4, 2012, at 11:11, "Brian E. Granger"reply@reply.github.com wrote:

I am really torn by this one. On one hand I see that that single click could/should be used for other purposes. This would also enable us to implement cell sorting by dragging more easily. On the other hand, I think new users will find the behavior very confusing ("how the hell do I edit a cell"). The notebook is right at the boundary between being a document (where users will absolutely expect single click to enter edit mode) and an spreadsheet type environment (where single click selects, double click edits). I would like to see what others think and also see what we could do to mitigate that confusion for new users.

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

@fperez
Copy link
Member

fperez commented Jan 5, 2012

Color me torn as well... Just joining the dilemma/discussion, not that I have any clear criterion to help us decide right now...

@ellisonbg
Copy link
Member

Hmm, I tested this branch out on Mac/Chrome and single click is still triggering edit. What am I missing?

@minrk
Copy link
Member Author

minrk commented Jan 5, 2012

Did you refresh to get the new js?

-MinRK

On Jan 5, 2012, at 8:06, "Brian E. Granger"reply@reply.github.com wrote:

Hmm, I tested this branch out on Mac/Chrome and single click is still triggering edit. What am I missing?

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

@ellisonbg
Copy link
Member

Yep

On Thu, Jan 5, 2012 at 1:55 PM, Min RK
reply@reply.github.com
wrote:

Did you refresh to get the new js?

-MinRK

On Jan 5, 2012, at 8:06, "Brian E. Granger"reply@reply.github.com wrote:

Hmm, I tested this branch out on Mac/Chrome and single click is still triggering edit.  What am I missing?

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

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

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

@minrk
Copy link
Member Author

minrk commented Jan 5, 2012

Strange. I can confirm that it works as expected (single-click does not edit) on current Chrome, Safari, and FF. I think your refresh did not take, or you are not actually running from this branch.

@minrk minrk mentioned this pull request Jan 5, 2012
Merged
@ellisonbg
Copy link
Member

OK let me go back and double check.

On Thu, Jan 5, 2012 at 2:06 PM, Min RK
reply@reply.github.com
wrote:

Strange.  I can confirm that it works as expected (single-click does not edit) on current Chrome, Safari, and FF.  I think your refresh did not take, or you are not actually running from thins branch.

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

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

@fperez
Copy link
Member

fperez commented Jan 5, 2012

I can confirm that on linux (ff and chrome) it works as expected once I refresh the JS.

I wonder if, usability-wise, it wouldn't be good to match this change with not rendering the text cells as soon as the window loses focus, but only when some other element actively get it. Basically, this change would make it a little harder (double-click) to 'get in' to a text cell for editing, but if matched with also being harder to lose the editing state, it could overall be an improvement. Right now I find that often I'm editing a text cell and switch over to another window to copy some information, and it's frustrating to have to click around to edit again. Thoughts?

@minrk
Copy link
Member Author

minrk commented Jan 5, 2012

That makes some sense. I've just pushed a change where unselect() triggers render, not focusout. This means that rendering will only be triggered by the focusing of another cell, but not the focusing of other elements on the page, or loss of focus due to app switching.

(note: Enter starts edit, so you don't have to click around, but it's still annoying)

@fperez
Copy link
Member

fperez commented Jan 6, 2012

I like it with this last change. I still see the dilemma, but in it's current state I'm now +1 for making these changes. @ellisonbg, unless you still see some objections, I'd say we move forward on this one.

@fperez
Copy link
Member

fperez commented Jan 6, 2012

Actually, I take that back, a couple of issues remain. If you make a new markdown cell (the standard way to get one is to get a new cell and do C-m-m, then:

  • the Type *Markdown* and LaTeX: $\alpha^2$ stuff is now visible immediately. I personally think we should
  • I see (on chrome) no way to start editing right away. I must go to the mouse and click. Something is wrong with the keyboard focus after C-m-m.

@ellisonbg
Copy link
Member

I will look at this later tonight. One thing that I do think needs to
work is being able to go from one cell to another editing along with
way without the mouse. The logic for focusing/editing/rendering is
quite subtle and will have to be tested extremely well.

On Thu, Jan 5, 2012 at 6:19 PM, Fernando Perez
reply@reply.github.com
wrote:

Actually, I take that back, a couple of issues remain. If you make a new markdown cell (the standard way to get one is to get a new cell and do C-m-m, then:

  • the Type *Markdown* and LaTeX: $\alpha^2$ stuff is now visible immediately.  I personally think we should
  • I see (on chrome) no way to start editing right away.  I must go to the mouse and click.  Something is wrong with the keyboard focus after C-m-m.

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

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

@ellisonbg
Copy link
Member

@fperez: your first point got cut off. Were you going to say that th
new markdown cell should be empty so you can type immediately.

I do see the same behavior on Chrome that a new Markdown cell is not
ready to edit without clicking. That actually might be a bug in the
CM version we are using.

On Thu, Jan 5, 2012 at 6:46 PM, Brian Granger ellisonbg@gmail.com wrote:

I will look at this later tonight.  One thing that I do think needs to
work is being able to go from one cell to another editing along with
way without the mouse.  The logic for focusing/editing/rendering is
quite subtle and will have to be tested extremely well.

On Thu, Jan 5, 2012 at 6:19 PM, Fernando Perez
reply@reply.github.com
wrote:

Actually, I take that back, a couple of issues remain. If you make a new markdown cell (the standard way to get one is to get a new cell and do C-m-m, then:

  • the Type *Markdown* and LaTeX: $\alpha^2$ stuff is now visible immediately.  I personally think we should
  • I see (on chrome) no way to start editing right away.  I must go to the mouse and click.  Something is wrong with the keyboard focus after C-m-m.

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

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

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

@minrk
edit text cells on double-click instead of single-click
bcd0fff 
Single-click to edit gets in the way of using interactive elements (e.g. non-flash videos),
and select/copy of the rendered HTML.  Switching to double-click makes the edit action more intentional.
@minrk
trigger textcell render on unselect instead of focusout
b209473
@minrk
Copy link
Member Author

minrk commented Jan 6, 2012

Sorry - I removed the trigger("focusout"), which caused a render event, when I should have replaced it with an explicit call to render. Should be fixed now.

I seem to be able to write/convert/edit plenty of text cells with no use of the mouse.

@ellisonbg
Copy link
Member

One important point. I originally thought this would also affect code
cells as well. I am fine with markdown cells having this behavior -
previously it was too easy to enter edit mode of markdown cells by
accident. I will test your recent changes.

On Thu, Jan 5, 2012 at 7:21 PM, Min RK
reply@reply.github.com
wrote:

Sorry - I removed the trigger("focusout"), which caused a render event, when I should have replaced it with an explicit call to render.  Should be fixed now.

I seem to be able to write/convert/edit plenty of text cells with no use of the mouse.

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

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

@minrk
Copy link
Member Author

minrk commented Jan 6, 2012

It only affect text cells, because text cells are the only ones with a 'render' step, that causes problems with interaction. You can interact with code cells without any transformation, so there is no cost. It is distressing to try to select rendered content, and discover that it is in fact impossible.

Single-click on a code cell simply places a cursor (as far as the user is concerned), and is logical. edit() on a text cell, on the other hand, is a significant and disruptive action, which does not correspond to what single-clicks should do on areas that are not buttons.

@fperez
Copy link
Member

fperez commented Jan 6, 2012

On Thu, Jan 5, 2012 at 7:11 PM, Brian E. Granger
reply@reply.github.com
wrote:

@fperez: your first point got cut off.  Were you going to say that th
new markdown cell should be empty so you can type immediately.

Yes, precisely.

I do see the same behavior on Chrome that a new Markdown cell is not
ready to edit without clicking.  That actually might be a bug in the
CM version we are using.

But without this PR, it's OK: in master, I can C-m-m a cell to get
it to be text, and type away. With this PR, it loses focus and I have
to click. So there's definitely a focus issue introduced by this
particular PR.

BTW, Brian: I don't know if you're aware of the 'git-mrb' script in
tools/. It's super useful for rapidly testing branches from people
whose remotes you track (I track those of all the core committers for
ease of merging). I wrote it recently and it really simplifies/speeds
up the testing of PRs a lot. Let me know if you need any pointers.

@fperez
Copy link
Member

fperez commented Jan 6, 2012

Tested with latest focus fixes and it looks good to me now. I'm OK with this as it is now, let's wait for Brian's testing later.

@ellisonbg
Copy link
Member

Ok this looks good.

minrk added a commit that referenced this pull request Jan 6, 2012
@minrk
Merge pull request #1224 from minrk/dblclick
f68e34b 
* edit text cells on double-click instead of single-click
* render text cells on unselect instead of focusout.
  Only renders when selecting another cell, and prevents rendering when 
  switching applications/windows/tabs.
@minrk minrk merged commit f68e34b into ipython:master Jan 6, 2012
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@minrk
Merge pull request ipython#1224 from minrk/dblclick
0ff90de 
* edit text cells on double-click instead of single-click
* render text cells on unselect instead of focusout.
  Only renders when selecting another cell, and prevents rendering when 
  switching applications/windows/tabs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@minrk @ellisonbg @fperez