[notebook] Make pager resizable, and remember size... #1705

Merged
merged 5 commits into from May 25, 2012

Projects

None yet

5 participants

@Carreau
Member
Carreau commented May 6, 2012

Resizing to small collapse the pager keeping the size to at least 20%
height

(trying to) resize a collapsed pager to more than 10% "expand" it.

Pager can remember it size when toggling by clicking.

@Carreau Carreau Make pager resizable, and remember size...
Resizing to small collapse the pager keeping the size to at least 20%
height

(trying to) resize a collapsed pager to more than 10% "expand" it.

Pager can remember it size when toggling by clicking.
f581afa
@fperez
Member
fperez commented May 7, 2012

While you're working on the pager, how easy do you think it would be to also make it so q and Esc close it when it's focused? That would match the keyboard workflow of the terminal and qt console: type something that activates pager, use arrow keys to navigate for reading, use q or Esc when finished. Just a suggestion, if it looks tricky don't worry.

Sorry that I won't be able to review much for a few days, I'm at a conference...

@Carreau
Member
Carreau commented May 7, 2012

Collapsing on esc is easy (and done) but as the pager does not get 'focus', and can't with current implementation, it is not easy to add navigation with arrow keys.

If we ever give the pager ability to get focus, we should implement the ability to switch focus on code-cell again without closing the pager, and have a clear indication that the pager is in focus. Right know I would wait for #1697 (js refactor) and #1509 (new tooltip and completer) to be merged. then we could make the pager a js 'widget' that allows to search for help, navigate with keys, talk to the kernel to render rest docstring...etc.

@fperez
Member
fperez commented May 7, 2012

On Mon, May 7, 2012 at 3:43 AM, Bussonnier Matthias
reply@reply.github.com
wrote:

Collapsing on esc is easy (and Done) but as the pager does not get 'focus', and can't with current implementation, it is not easy.

Excellent, thanks. That's already good progress.

If we ever give the pager ability to get focus, we should implement the ability to switch focus on code-cell  again without closing the pager, and have a clear indication that the pager is in focus. Right know I would wait for #1697 (js refactor) and #1509 (new tooltip and completer) to be merged. then we could make the pager a js 'widget' that allows to search for help, navigate with keys, talk to the kernel to render rest docstring...etc.

OK, sounds like a good plan.

@ellisonbg
Member

I am not quit clear on what this PR implements. I was expecting to be able to resize the pager area by dragging the divider. But when I try this on Chrome, it doesn't drag. Furthermore, there is a glitch when I single click to collapse the pager: it first gets bigger for a split second and then collapses. Am I misunderstanding what this is supposed to do or am I hitting a bug?

@Carreau
Member
Carreau commented May 8, 2012

Le 8 mai 2012 07:22, "Brian E. Granger"
a écrit :

I am not quit clear on what this PR implements. I was expecting to be
able to resize the pager area by dragging the divider. But when I try this
on Chrome, it doesn't drag. Furthermore, there is a glitch when I single
click to collapse the pager: it first gets bigger for a split second and
then collapses. Am I misunderstanding what this is supposed to do or am I
hitting a bug?

That's what it's supposed to do ....
Strange thing for me on chrome osx it works fine ...

I'll investigate ...


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

@Carreau
Member
Carreau commented May 8, 2012

Well it works ok for me on OS-X on chrome 20.xx safari 5.1.1 and (now) in FF 12.0... I had to clear the cache though.

I also see the fact that it gets 'slightly bigger' (~5-10px) pixels when collapsing, i'm not sure it's pretty fast, but it does not look different from master.

@ellisonbg
Member

Hmm, can others try? I tried clearing my browser cache and still got
nothing. Also tried on FF and didn't work there.

On Tue, May 8, 2012 at 12:58 AM, Bussonnier Matthias
reply@reply.github.com
wrote:

Well it works ok for me on OS-X on chrome 20.xx safari 5.1.1 and (now) in FF 12.0... I had to clear the cache though.

I also see the fact that it gets 'slightly bigger' (~5-10px) pixels when collapsing, i'm not sure it's pretty fast, but it does not look different from master.


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

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

@Carreau
Member
Carreau commented May 8, 2012

Works on FF an debian (about tells me FF7, but I doubt it...), I really don't understand why it does not works for you... There is I think around 30 pixels snap to differentiate 'click' from 'drag', could it be it ?

@ellisonbg
Member

I will check that.

On Tue, May 8, 2012 at 1:16 PM, Bussonnier Matthias
reply@reply.github.com
wrote:

Works on FF an debian (about tells me FF7, but I doubt it...), I really don't understand why it does not works for you... There is I think around 30 pixels snap to differentiate 'click' from 'drag', could it be it ?


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

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

@ellisonbg
Member

OK it is working for me now. It was the fact that I had an older version that was using document.height. In general, it is very important to use the jQuery height()/outerHeight() when computing the size of elements. You fixed this in a later commit and it works great.

@ellisonbg ellisonbg commented on an outdated diff May 8, 2012
IPython/frontend/html/notebook/static/js/pager.js
};
Pager.prototype.bind_events = function () {
var that = this;
- this.pager_element.bind('collapse_pager', function () {
- that.pager_element.hide('fast');
+ this.pager_element.bind('collapse_pager', function (event,extrap) {
@ellisonbg
ellisonbg May 8, 2012 IPython member

Even though this is JavaScript and not Python, I try to follow PEP 8 coding conventions, so 1 space after the comma in function argument please. Here and elsewhere.

@ellisonbg ellisonbg commented on an outdated diff May 8, 2012
IPython/frontend/html/notebook/static/js/pager.js
this.percentage_height = 0.40;
+ this.pager_splitter_element = $(pager_splitter_selector)
+ .draggable({
+ containment: 'window',
+ axis:'y',
+ helper: null ,
+ drag: function(event,ui){
+ // recalculate the amount of space the pager should take
+ var pheight =($(body).height()-event.clientY-4);
+ var downprct = pheight/IPython.layout_manager.app_height();
+ downprct = Math.min(0.9,downprct);
+ if(downprct < 0.1) {
@ellisonbg
ellisonbg May 8, 2012 IPython member

Space after the if before the (.

@ellisonbg ellisonbg commented on an outdated diff May 8, 2012
IPython/frontend/html/notebook/static/js/pager.js
this.percentage_height = 0.40;
+ this.pager_splitter_element = $(pager_splitter_selector)
+ .draggable({
+ containment: 'window',
+ axis:'y',
+ helper: null ,
+ drag: function(event,ui){
@ellisonbg
ellisonbg May 8, 2012 IPython member

Space between the ) and {.

@ellisonbg ellisonbg commented on an outdated diff May 8, 2012
IPython/frontend/html/notebook/static/js/pager.js
this.percentage_height = 0.40;
+ this.pager_splitter_element = $(pager_splitter_selector)
+ .draggable({
+ containment: 'window',
+ axis:'y',
+ helper: null ,
+ drag: function(event,ui){
+ // recalculate the amount of space the pager should take
+ var pheight =($(body).height()-event.clientY-4);
+ var downprct = pheight/IPython.layout_manager.app_height();
+ downprct = Math.min(0.9,downprct);
+ if(downprct < 0.1) {
+ that.percentage_height = 0.1;
+ that.collapse({'duration':0});
+ } else if(downprct > 0.2) {
@ellisonbg
ellisonbg May 8, 2012 IPython member

Again space after if.

@ellisonbg
Member

Minor changes to be made, but this is a great implementation of something that has been sorely needed. I am quite impressed at how easy the jQuery draggable stuff made this. Should be merge ready soon.

@Carreau Carreau pep 8 and js
space after comma, space around equal, space before if and curly bracket
e56bcf4
@Carreau
Member
Carreau commented May 9, 2012

fixed.

@ellisonbg
Member

So I have been thinking about making the notebook keyboard shortcuts modal. What I mean by this is that I want to make it so that selecting a cell does not focus the code mirror editor. This will enable us to implement things like multiple selection and other cell+notebook level keyboard shortcuts. In a modal editor you need a keyboard shortcut to initiate editing the selected cell. For that I am thinking about using RETURN, which will focus the code mirror editor. To stop editing, I think that ESC makes the most sense because it doesn't mean anything when you are editing a cell. BUT, if we use ESC for that, I don't think we should use ESC for collapsing the pager. Thoughts on this @fperez, @minrk, @takluyver ?

Other than this point, I think this PR is ready to merge, unless others want to test it more.

@minrk
Member
minrk commented May 23, 2012

Can we use q to collapse the pager? I think ESC is what ~100% of people will expect in a general browser context, but existing IPython users will likely be familiar with q to dismiss the pager.

@ellisonbg
Member

Don't we have to pick something that is not something valid to type
into a code mirror cell. Most of the time the pager is open, a code
mirror cell is in focus on receiving keyboard events.

On Tue, May 22, 2012 at 9:30 PM, Min RK
reply@reply.github.com
wrote:

Can we use q to collapse the pager?  I think ESC is what ~100% of people will expect in a general browser context, but existing IPython users will likely be familiar with q to dismiss the pager.


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

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

@Carreau
Member
Carreau commented May 23, 2012

sorry, I have been away for a few, trying to catch up.
qis a valid shortcut if the pager take focus and prevent from typing, but as it is now, the pager does not prevent from writing.

@ellisonbg, we can still have esc collapsing the pager only when cell are not selected, so that escwill collapse the pager, then leave edition mode (or other way around). It will just require some more thinking os esc key handeling.

@fperez
Member
fperez commented May 23, 2012

My thought would be that both esc and q should close the pager when focused. Ideally, the workflow would be:

  • user types foo?? and the pager opens.
  • Focus shifts automatically to the pager! So up/down arrows can be used to navigate it.
  • Esc and q both close the pager
  • User is back at the prompt, ready to continue working.

And the mouse is never needed for the above. Can we get that?

@takluyver
Member

Fernando's idea seems pretty sound, but should we also include an
obvious shortcut to go back to the cell without closing the pager? I
guess this could be the same key used to initiate editing from
selection mode.

@Carreau
Member
Carreau commented May 23, 2012

Pager is not focusable right now. It could be made focusable with some trick, hidden form etc, but IMHO we should wait until JS refactor is merged.

Then we could make it a separate widget, with much more interactivity, like searching, browsing help...

@ellisonbg
Member

Because the pager is not focusable right now, can we simply wait before we add a keyboard shortcut to collapse it. When we rework the keyboard shortcuts for cells, that will give us the opportunity to work in the pager shortcuts as well.

On Wed, May 23, 2012 at 3:06 AM, Bussonnier Matthias
reply@reply.github.com
wrote:

Pager is not focusable right now. It could be made focusable with some trick, hidden form etc, but IMHO we should wait until JS refactor is merged.

Then we could make it a separate widget, with much more interactivity, like searching, browsing help...


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

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

@Carreau
Member
Carreau commented May 23, 2012

That's fine for me, but we can also remove the shortcut while working on cell shortcut, because esc to collapse pager is really usefull to avoid reaching the mouse... it depends on when the code for the cell will arrive.

I'll revert c8e47d4 then, can I merge when it is done ?
We can carry on discussing after put it will be one PR merged at least.

@ellisonbg
Member

Yes I am fine with you merging this after reverting that commit.
Great work, can't wait to begin using this capability.

On Wed, May 23, 2012 at 11:34 AM, Bussonnier Matthias
reply@reply.github.com
wrote:

That's fine for me, but we can also remove the shortcut while working on cell shortcut, because esc to collapse pager is really usefull to avoid reaching the mouse... it depends on when the code for the cell will arrive.

I'll revert c8e47d4 then, can I merge when it is done ?
We can carry on discussing after put it will be one PR merged at least.


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

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

@Carreau Carreau merged commit fa31376 into ipython:master May 25, 2012
@Carreau
Member
Carreau commented May 25, 2012

thanks, merged.

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