Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Integrate latest CodeMirror for sprint 36 #6268

Merged
merged 4 commits into from
Jan 8, 2014
Merged

Integrate latest CodeMirror for sprint 36 #6268

merged 4 commits into from
Jan 8, 2014

Conversation

njx
Copy link

@njx njx commented Dec 18, 2013

Update: Removed "[discussion only]" tag since we think we've addressed the issues and could merge this.

In the latest CM (as of codemirror/codemirror5@d13582f), the CSS mode has been refactored substantially. The good news is that LESS is now supported by CSS mode, meaning that we can much more easily make basic code hinting work for LESS files (similar to what we've already done for SCSS files). The less good news is that the internals of the mode have changed significantly. In particular:

  • There's no longer a stack member on the state object, but instead a context member which seems to fulfill the same role (using a linked list instead of a stack).
  • Some of the token types appear to have changed (specifically, propertyValue now seems to be prop).

This PR contains a quick attempt to make the existing CSS code hinting work properly with the changes to CSS mode. It also hooks up LESS files to the CSS mode (instead of the old LESS mode) and attempts to turn on code hinting in those files.

With these changes, it looks like almost all the existing CSS code hinting unit tests pass, except for one in CSS Context Info. However, I haven't done much manual testing beyond verifying that basic property and value hints seem to work in CSS and LESS files. Also, this was based on only a very cursory understanding of the existing CSSUtils code and of the changes to the CM CSS mode, so it's quite likely that we need to do more work and testing to make sure everything is working properly.

@RaymondLim - would you mind taking a look at the code changes and seeing what you think? I'm wondering if we need to add a story for this (to ensure we get adequate testing, etc.), which might mean pushing it to the next sprint.

Additionally, there were a number of other smaller changes since our last CM merge that are probably worth testing around. From looking at the commit log, these are the areas we'd want to do some sanity testing around:

  • Inline editor line hiding (esp at beginning/end of doc) - make fix first
  • LESS - using CSS mode now! - include that and test
  • XML-mode
  • Auto-closing of HTML tags
  • Look for gutter issues
  • Hit up arrow while selection exists
  • Scroll into view
  • newline and indent
  • electric chars
  • CSS mode changes, incl. tokenization (test quick edit, quick open symbol, etc.)
  • JS mode - indentation around for / forEach
  • Check undo/redo/dirty bit esp. around quick typing

Finally, this PR also contains the small update required by #6204.

@njx
Copy link
Author

njx commented Dec 19, 2013

Just realized that codemirror/codemirror5@fad3f49, where Marijn updated his own CSS code hinting add-on to work with the new mode, might give us clues as to the right way to fix our code hinting. In particular, it looks like he uses token.state.state instead of token.state.context.type (seems like the two might be equivalent). You probably still need to use context.prev.type to get the previous state though.

@njx
Copy link
Author

njx commented Dec 19, 2013

Also, I just realized that I don't really know what the difference is between token.type and token.state.state (or token.state.context.type). If the two are always equivalent, maybe we should just be checking token.type?

@ghost ghost assigned RaymondLim Dec 19, 2013
@RaymondLim
Copy link
Contributor

@njx I'm getting the following unit tests failure on Windows.

  • Units suite
    • 1 failure in CSS Context (fixed)
    • 10 failures in EditorCommandHandler (fixed)
  • Integration suite
    • 4 failures in EditorCommandHandler (fixed)
  • Extensions suite
    • 1 failure in Bower-install
    • 1 failure in HTML Code Hints (temporarily disable the failing test since we cannot integrate the recent fix from CM yet.)
    • 1 or 2 in JS Code Hints (no failure if running only JS Code Hints unit tests)
    • 9 failures in URL Hinting (fixed)

Update: I'll be investigating CSS Context, HTML Code HInts and URL Hinting failures.

@njx
Copy link
Author

njx commented Dec 20, 2013

Good to know. I didn't run all the unit tests myself so it's not too surprising there are other failures. I have a task on the Sprint 36 CM Merge card to look into these, but I think we're waiting until Jonathan makes a call on whether to try to pull this work in. Until then, maybe we should just put this on hold.

@njx
Copy link
Author

njx commented Jan 7, 2014

Reviewed the most recent commit and added some comments. Let me know when there are more changes you want me to review.

…rCommandHandler unit test failures by adjusting third argument of cm.indentLine() call to "true".
@RaymondLim
Copy link
Contributor

I think I've fixed all unit test failures. The remaining failures -- 1 in Bower-install and 1 or 2 in JS code hints -- have nothing to do with CM update and can be reproduced in master as well. I filed an issue for the HTML Code hints unit test failure and Marijn already fixed it in codemirror/codemirror5@08d5cab, but I don't think we can include the fix in this pull request. So I just disable the unit test for now.

@njx
Copy link
Author

njx commented Jan 8, 2014

Makes sense. However, we should file a bug on the case related to the disabled unit test. It looks like it causes an obvious regression in the code coloring (as in the bug you filed), but I wasn't able to actually reproduce any problems with code hints in that case. It's a bit of a bummer to ship with the code-coloring regression, but we can release note it.

I did notice that the fix for the issue is only in the v4 branch of CodeMirror for some reason - I left him a comment asking about that.

@njx
Copy link
Author

njx commented Jan 8, 2014

Merging.

njx pushed a commit that referenced this pull request Jan 8, 2014
Integrate latest CodeMirror for sprint 36
@njx njx merged commit d6eacd6 into master Jan 8, 2014
@njx njx deleted the nj/cm-sprint36 branch January 8, 2014 01:36
@njx
Copy link
Author

njx commented Jan 8, 2014

Filed #6400 on the regression above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants