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

Use CMs electric chars #9387

Merged
merged 1 commit into from Apr 8, 2015

Conversation

Projects
None yet
4 participants
@MarcelGerber
Contributor

MarcelGerber commented Oct 1, 2014

This fixes multiple issues:
#1324, #8434, #8723, #8980, #9120

I verified #8395 (fixed by #8439) is still fixed.

There's one behaviour change (tracked via codemirror/CodeMirror#2812):
In a switch statement (JS), the cursor automatically indents 2 units, but back-indents as soon as you type "case".

@peterflynn You once noted (on IRC) you saw flaws with CM's implementation, could you please take a look?
cc @JeffryBooher

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 1, 2014

Member

Two quick thoughts:

  1. IIRC one of the reasons we forked this in the first place was indentation jumping around when typing ":" in object literals. We'll need to make sure that stays fixed, and if not we should look into merging our fix for that issue upstream to CM so our code can stay more in sync.
  2. It seems worth doing a little more verification on #8395, since looking at the CM implementation it's pretty clear they don't have any of the code that was added in #8439 -- they always call indentLine() immediately and directly. If #8395 is indeed still fixed, it would be good to understand why.
Member

peterflynn commented Oct 1, 2014

Two quick thoughts:

  1. IIRC one of the reasons we forked this in the first place was indentation jumping around when typing ":" in object literals. We'll need to make sure that stays fixed, and if not we should look into merging our fix for that issue upstream to CM so our code can stay more in sync.
  2. It seems worth doing a little more verification on #8395, since looking at the CM implementation it's pretty clear they don't have any of the code that was added in #8439 -- they always call indentLine() immediately and directly. If #8395 is indeed still fixed, it would be good to understand why.
@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 1, 2014

Member

Oh, IIRC another bug was that typing ":" inside a comment would cause the indentation to jump also...

Member

peterflynn commented Oct 1, 2014

Oh, IIRC another bug was that typing ":" inside a comment would cause the indentation to jump also...

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Oct 1, 2014

Contributor

Well, #8395 is still fixed as I left the code in (see diff) :)

Contributor

MarcelGerber commented Oct 1, 2014

Well, #8395 is still fixed as I left the code in (see diff) :)

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 1, 2014

Member

Oh I see -- didn't realize you found a way to keep that. Looks like that code needs to be made multi-cursor-aware though, otherwise #8434 isn't really fully fixed.

Member

peterflynn commented Oct 1, 2014

Oh I see -- didn't realize you found a way to keep that. Looks like that code needs to be made multi-cursor-aware though, otherwise #8434 isn't really fully fixed.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Oct 4, 2014

Contributor

I just did some further tests and couldn't find any issues with ":", at least in JS mode.
I'd still be happy if you could test it yourself.

Contributor

MarcelGerber commented Oct 4, 2014

I just did some further tests and couldn't find any issues with ":", at least in JS mode.
I'd still be happy if you could test it yourself.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Oct 4, 2014

Contributor

@peterflynn FYI, I fixed the anti-orphan code to support multiple cursors.

Contributor

MarcelGerber commented Oct 4, 2014

@peterflynn FYI, I fixed the anti-orphan code to support multiple cursors.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Oct 5, 2014

Member

Cool, thanks for making that update! I don't have a lot of cycles next week to do any testing, but hopefully someone else on the team can pick this up sooner.

Member

peterflynn commented Oct 5, 2014

Cool, thanks for making that update! I don't have a lot of cycles next week to do any testing, but hopefully someone else on the team can pick this up sooner.

@pthiess

This comment has been minimized.

Show comment
Hide comment
@pthiess

pthiess Oct 27, 2014

Member

@MarcelGerber Hey Marcel, we are locking down for 1.0 but it will go into the next release.

Member

pthiess commented Oct 27, 2014

@MarcelGerber Hey Marcel, we are locking down for 1.0 but it will go into the next release.

@prafulVaishnav

This comment has been minimized.

Show comment
Hide comment
@prafulVaishnav

prafulVaishnav Mar 30, 2015

Contributor

@MarcelGerber Picking this for review.

Contributor

prafulVaishnav commented Mar 30, 2015

@MarcelGerber Picking this for review.

/**
* @private
* Handle CodeMirror key events.
* @param {!Event} event
*/
Editor.prototype._handleKeypressEvents = function (event) {
this._checkElectricChars(event);
var keyStr = String.fromCharCode(event.which || event.keyCode);

This comment has been minimized.

@prafulVaishnav

prafulVaishnav Apr 7, 2015

Contributor

@MarcelGerber Hii.. Can you please write this in separate function as it is specific to electricChars handling.

@prafulVaishnav

prafulVaishnav Apr 7, 2015

Contributor

@MarcelGerber Hii.. Can you please write this in separate function as it is specific to electricChars handling.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 7, 2015

Contributor

@prafulVaishnav Done. Please let me know when you'd like to merge so I can squash commits.

Contributor

MarcelGerber commented Apr 7, 2015

@prafulVaishnav Done. Please let me know when you'd like to merge so I can squash commits.

@prafulVaishnav

This comment has been minimized.

Show comment
Hide comment
@prafulVaishnav

prafulVaishnav Apr 8, 2015

Contributor

@MarcelGerber You can squash commit. I will merge it then.

Contributor

prafulVaishnav commented Apr 8, 2015

@MarcelGerber You can squash commit. I will merge it then.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber
Contributor

MarcelGerber commented Apr 8, 2015

Show outdated Hide outdated src/editor/Editor.js
var keyStr = String.fromCharCode(event.which || event.keyCode);
if (/[\]\{\}\)]/.test(keyStr)) {
this._removeWhitespaceAfterBrace();

This comment has been minimized.

@prafulVaishnav

prafulVaishnav Apr 8, 2015

Contributor

@MarcelGerber Please update the function name to _handleWhitespaceForElectricChars

@prafulVaishnav

prafulVaishnav Apr 8, 2015

Contributor

@MarcelGerber Please update the function name to _handleWhitespaceForElectricChars

This comment has been minimized.

@MarcelGerber

MarcelGerber Apr 8, 2015

Contributor

Oops. Fixed.

@MarcelGerber

MarcelGerber Apr 8, 2015

Contributor

Oops. Fixed.

prafulVaishnav added a commit that referenced this pull request Apr 8, 2015

@prafulVaishnav prafulVaishnav merged commit 97ed87b into adobe:master Apr 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@prafulVaishnav

This comment has been minimized.

Show comment
Hide comment
@prafulVaishnav

prafulVaishnav Apr 8, 2015

Contributor

@MarcelGerber Thanks.. It must fix various issues. I will check and close them all.

Contributor

prafulVaishnav commented Apr 8, 2015

@MarcelGerber Thanks.. It must fix various issues. I will check and close them all.

@MarcelGerber MarcelGerber deleted the MarcelGerber:cm-electric-chars branch Apr 8, 2015

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 8, 2015

Contributor

In the description above, I listed some issues that should be fixed now.

Contributor

MarcelGerber commented Apr 8, 2015

In the description above, I listed some issues that should be fixed now.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 8, 2015

Contributor

I've just had a look at all the referenced issues myself - they are indeed all fixed.
Note for #8723: Checkout 7c7d429 so the line numbers are correct.

Contributor

MarcelGerber commented Apr 8, 2015

I've just had a look at all the referenced issues myself - they are indeed all fixed.
Note for #8723: Checkout 7c7d429 so the line numbers are correct.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 8, 2015

Contributor

I also filed codemirror/CodeMirror#3188, suggesting an event on electric char indent, so we can call _handleWhitespaceForElectricChars every time an electric char is handled, to also handle languages with more special electric chars like XML, Verilog, Ruby, or even JavaScripts case and default properly.

Contributor

MarcelGerber commented Apr 8, 2015

I also filed codemirror/CodeMirror#3188, suggesting an event on electric char indent, so we can call _handleWhitespaceForElectricChars every time an electric char is handled, to also handle languages with more special electric chars like XML, Verilog, Ruby, or even JavaScripts case and default properly.

@prafulVaishnav

This comment has been minimized.

Show comment
Hide comment
@prafulVaishnav

prafulVaishnav Apr 9, 2015

Contributor

@MarcelGerber Thanks.. We will track it and once it is there we will make the change.

Contributor

prafulVaishnav commented Apr 9, 2015

@MarcelGerber Thanks.. We will track it and once it is there we will make the change.

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