[CLOSED] Reduce number of places we reference CodeMirror (Editor._codeMirror) directly

brackets-archive / bracketsIssues

Archive of issues in brackets.

0 stars 0 forks source link

[CLOSED] Reduce number of places we reference CodeMirror (Editor._codeMirror) directly #423

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by peterflynn Thursday Mar 08, 2012 at 22:55 GMT Originally opened as https://github.com/adobe/brackets/issues/425

I don't know that it's worth trying to eliminate references 100%. IMHO, code that truly depends on CodeMirror-specific implementation details (such as CodeHintUtils, which relies on the tokenizer) should continue to just talk to CodeMirror APIs directly. It's probably ok for EditorManager to continue referencing _codeMirror too, since it's tightly bound to the UI layout requirements of CodeMirror's DOM nodes.

But there are other places that definitely should move away from referencing _codeMirror: for example, the usage in InlineEditorProviders. It feels especially important for code that we might want to recast as a plugin at some point (like the editor provider example).

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Mar 16, 2012 at 05:55 GMT

Although it's probably less gross for EditorManager to look at _codeMirror compared to other modules, NJ and I had a few ideas for easy ways to clean up most of those cases:

All the references to getWrapperElement() could get replaced by an Editor.setVisible() method (or show()/hide())
All the references to getScrollerElement() could get replaced by an Editor.setHeight() method
The references to getGutterElement() are all for syncing gutter widths between editors. We should probably move all that functionality into Editor -- or at the very least, move the logic for getting/setting gutter width on an individual Editor down into Editor.

core-ai-bot commented 3 years ago

Comment by njx Tuesday Mar 27, 2012 at 17:36 GMT

QRB reviewed. Marking code cleanup--most of these would be straightforward to implement and won't involve major refactoring.

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday May 01, 2012 at 05:47 GMT

Situation is now improved in some respects, but much worse in others:

CSSInlineEditor._ensureCursorVisible()'s reference to Editor.getRootElement() seems like it could get replaced with Editor.hasFocus()
Most references to Editor.getScrollerElement() could still get replaced by an Editor.setHeight()
Gutter-width syncing (with many references to _codeMirror) still lives outside Editor (in InlineTextEditor now)
Live development code has numerous references to _codeMirror, mainly for finding what's under the cursor. Some references are actually unused (looks like copy-pasted boilerplate code).
Unit tests contain dozens of references to _codeMirror, almost all to call replaceRange(). This should become a Document API sooner rather than later!

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Jun 14, 2012 at 22:31 GMT

Fixing #803 has improved this quite a bit -- many of the references in unit tests and all the references in EditorCommandHandlers are now gone.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Mar 28, 2013 at 23:44 GMT

The 'token stream' abstraction I'm working on this sprint will help here too.

core-ai-bot commented 3 years ago

Comment by WebsiteDeveloper Sunday Sep 15, 2013 at 11:21 GMT

@peterflynn is it worth keeping this bug open?

core-ai-bot commented 3 years ago

Comment by peterflynn Sunday Sep 15, 2013 at 14:17 GMT

@WebsiteDeveloper Probably not -- thanks for the suggestion :-) I've added this to the API improvement user story. Closing as MTB.

core-ai-bot commented 3 years ago

Comment by peterflynn Sunday Sep 15, 2013 at 14:23 GMT

From a quick skim of the code, the main causes for _codeMirror references nowadays are tokens (would be cleaned up by "token stream"), marks (limited to the HTML live dev code), legacy references in the old live dev code (easy to fix), and char coordinates/metrics.