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

Fix codefolding gutter #977

Merged
merged 2 commits into from
Apr 30, 2017

Conversation

juhasch
Copy link
Member

@juhasch juhasch commented Apr 29, 2017

WIP: The gutter won't be added otherwise using notebbok 5.0.0 / Mac / Chrome.

[ gutters , "CodeMirror-foldgutter"] results in: [Array(0), "CodeMirror-foldgutter"]

gutters.push('CodeMirror-foldgutter'); results in ["CodeMirror-foldgutter"]

@jcb91
Copy link
Member

jcb91 commented Apr 29, 2017

This is my doing, I think, I noticed we were effectively setting a nested array, contrary to the spec, so altered it. However, I must be missing something here, why do we want a 0-length array as the first element? The CodeMirror manual says it should be an array of strings, and setting the option calls setGuttersForLineNumbers, which does something comparable (I think?!) to my gutters.push operation to ensure the linenumbers gutter is/isn't present as appropriate in src/display/gutters.js#L27-L33.
Also, do you mean that this works in 4.x, but breaks in 5.x? Is that maybe a CodeMirror version thing? 😕 ❔

@juhasch
Copy link
Member Author

juhasch commented Apr 29, 2017

It is a strange thing. Actually, I think what you did was correct.
I came across this when investigating the runtools gutter stuff.

I guess it could be a bug in CodeMirror. When you toggle line numbers from the menu (view->toggle line numbers), both gutters shows up correctly...

@jcb91
Copy link
Member

jcb91 commented Apr 29, 2017

When you toggle line numbers from the menu (view->toggle line numbers), both gutters shows up correctly...

Curiouser and Curiouser! Well, that sort of makes it sound like the setGuttersForLineNumbers isn't triggered by setting gutters, but it's right there, at least by CodeMirror 5.8.0 (which is what notebook has required since at least 4.1.0). However, I also note that 5.0.0 is now using a patched version of CodeMirror, which I think has also been backported to 4.3.2 or so, and which makes it rather more difficult to track anything down 😟. Oh well...

@juhasch
Copy link
Member Author

juhasch commented Apr 29, 2017

Yes, it is not easy to follow what is happening.
Actually I want to disentangle runtools from using the codefolding gutter. In theory it should be easy, in practice no so much. Also, the arrangement of the gutters changes each time, as it is never clear what is being loaded first. The line number gutter however works OK.

I will try to investigate more into how the line numbering gutter implementation gets things right.

@juhasch
Copy link
Member Author

juhasch commented Apr 29, 2017

This makes adding gutters work. Without this, references instead of individual arrays to specify gutters are used. This seems to trigger strange behavior in CodeMirror.

@jcb91
Copy link
Member

jcb91 commented Apr 30, 2017

It seems that the setOption call does a quick equality check, and doesn't call any handlers if newvalue == oldvalue, which I guess makes sense. Of course, if we just push into the same array, then the two variables end up referencing the same object, so they compare as equal. 👍

@juhasch juhasch merged commit dd7d29e into ipython-contrib:master Apr 30, 2017
@juhasch juhasch deleted the fix/codefolding_gutter branch April 30, 2017 21:33
@juhasch juhasch restored the fix/codefolding_gutter branch May 1, 2017 10:45
@juhasch juhasch deleted the fix/codefolding_gutter branch May 1, 2017 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants