APIs for manipulating gutters #12742

thehogfather · 2016-08-31T21:44:28Z

(Review and test) @zaggino @marcelgerber @petetnt

This introduces 5 public apis on the Editor instance for manipulating gutters in an editor.
The intended workflow is that extension authors use this api to manipulate the gutter within the editor. Typically onActiveEditorChange, register the gutter if not already registered. Use unregisterGutter if the extension is disabled and use setGutterMarker to draw things on the gutter.

None of the apis could go on the EditorManager since EditorManager already depends on Editor.js and we would have needed to make Editor depend on EditorManager to be aware of registered gutters when clearing gutters or setting the gutter markers.

As it stands, this will not play nicely with the current ways extensions are written since they are not been registered first and there is no clean way to allow backward compatibility for the old method of manipulation of gutters. Things appear to work correctly, until the line-number visibility is toggled (then external extension gutter disappears until it is re-rendered).

@zaggino it would be nice to know how well this plays with your extensions. I've only been able to test with two things on the gutter.

    /**
     * Clears all marks from the gutter with the specified name.
     * @param {string} name The name of the gutter to remove.
     */
    Editor.prototype.clearGutter = function (name);

    /**
     * Sets the marker for the specified gutter on the specified line number
     * @param   {string}   lineNumber The line number for the inserted gutter marker
     * @param   {string}   gutterName The name of the gutter
     * @param   {object}   marker     The jquery object representint the marker to the inserted in the gutter
     */
    Editor.prototype.setGutterMarker = function (lineNumber, gutterName, marker);

    /**
     * Returns the list of gutters current registered on this editor.
     * @return {!Array.<{name: string, priority: number}>}
     */
    Editor.getRegisteredGutters = function ();

    /**
     * Registers the gutter with the specified name at the given priority.
     * @param {string} name    The name to register with the quarter
     * @param {number} priority  A number denoting the priority of the gutter. Priorities higher than 100 appear after the line numbers. Priority less than 100 appear before.
     * @param {?Array<number>} languageIds A list of language ids that this gutter is valid for. If no language ids are passed, then the gutter is valid in all languages.
     */
    Editor.registerGutter = function (name, priority, languageIds);

    /**
     * Unregisters the gutter with the specified name and removes it from the UI.
     * @param {string} name The name of the gutter to be un registered.
     */
    Editor.unregisterGutter = function (name);

relates to #12726

…urrent editor

zaggino · 2016-08-31T22:57:47Z

src/editor/Editor.js

@@ -2330,6 +2341,11 @@ define(function (require, exports, module) {
            } else if (prefName === SHOW_LINE_NUMBERS) {
                Editor._toggleLinePadding(!newValue);
                this._codeMirror.setOption(cmOptions[SHOW_LINE_NUMBERS], newValue);
+                if (newValue) {
+                    this.registerGutter(LINE_NUMBER_GUTTER, 100);


magic number 100, make a constant please (probably exportable)

zaggino · 2016-08-31T23:07:41Z

Initial review, it's be worth exploring if registerGutter, unregisterGutter & getRegisteredGutters would not be better placed on Editor instead of Editor.prototype OR removing the language parameters and related code.

ficristo · 2016-09-01T18:59:51Z

Just some thoughts.
Editor is becoming a bit big: is it worth to put this on a new file?
Can we enforce the codefolding arrows to always be the rightmost thing in the gutter?

registering/unregistering/getting registered gutters are now `static` objects on the editor class. modified code folding gutter to use new api.

thehogfather · 2016-09-03T10:51:35Z

@ficristo I could explore the consequences of a refactor which introduces a Gutter abstraction for manipulating gutter elements. Hypothetically, Gutter instances would be composed as properties of Editor instances. Registering/unregistering/listing gutters would be static properties of the Gutter class. The Gutter class would also need a reference to the Editor instance (since any modifications to the gutter are still done via the _codeMirror editor property). This could probably be spiked in more detail in a separate issue. But there is usually a tradeoff in a refactor such as this. Javascript is not strongly typed so it is would become more difficult to navigate the project structure, while still having this "central" dependency (i.e., editor._codeMirror) that more and more files get access to.

I'm not 100% in agreement with enforcing that the code-folding arrows are the rightmost in the gutter. I think we should provide a flexible API with as little exceptions and restrictions as possible. For instance a very creative designer might conjure up a wireframe where a very thin gutter element is used to render the range (i.e., start and end) that a foldable region spans. I'm sure people can come up with other examples. For now I have given the priority what I feel is a sufficiently high number (500) to push the folding gutter to the rightmost gutter. I think this is fine as long as we make the gutter priorities and how they work clear to the extension developers.

I am yet to write tests for these and I will as soon as I can get the tests to run. Anyone else having issues running tests? All I see is a white screen when I try. Any known workarounds?

ficristo · 2016-09-03T12:03:58Z

For the first point I was looking both at the length of Editor and what is happening in CodeMirror where it is split in multiple files. So I was thinking to put the gutter in a new file with the idea to put/move more things in the future.
For the second I thought the core extensions should be have the most priority. But you are right it would limit extension author.
Thank you for the explanations, if necessary we can think more about these things in future releases.

For the tests I don't have problems on my Windows10 PC. Are you saying as soon as you open the test window you see only a white screen?

thehogfather · 2016-09-03T13:06:43Z

@ficristo regarding tests, yes. That is correct. I only see a white screen as soon as the test windows are opened. I am on OSX El Capitan. I was just wondering if anyone has ever experienced this issue and if there were known work arounds.

zaggino · 2016-09-29T06:20:38Z

@thehogfather can you run tests now?

thehogfather · 2016-09-29T21:09:45Z

@zaggino yes i can now run tests (at least selective tests) attempting to run all tests eventually gets to a white screen but I think this has always been a problem for me - even from a couple of years ago. Let me know if there are any strategies/workarounds for running all tests.

zaggino · 2016-09-29T23:29:02Z

Yeah, I don't know of any. If you build your own shell, try using the release one from pre-release. If you don't try building your own. That's pretty much all advice I have (which I know it's not a lot).

…ather/brackets into thehogfather/gutter_apis finished gutter api tests for editor

thehogfather · 2016-10-02T18:31:41Z

ok - building my own shell did not make a huge difference with regards to running All tests. Presumably someone is able to. I've updated the PR with additional tests to Editor-tests to cover the gutter api functionality.

zaggino · 2016-10-13T01:12:11Z

@ficristo second look?

ficristo · 2016-10-13T05:27:34Z

If someone else of @adobe/brackets-committers can review would be better, maybe @petetnt or @marcelgerber.
@zaggino my only question, more for curiosity than anything else, have you tryed it with one of your extensions?

swmitra · 2016-10-13T05:46:29Z

As a whole the idea of adding gutter manipulation using well defined public apis sounds quite promising.
The only obvious issue what I see ( an existing one ) is the CM native "lineNumber" gutter hide and show which would shuffle the gutter sequence. To resolve that issue I would like to have one more option as an abstracted logic from the extensions, where we have a gutter management module that registers a DOM mutation listener ( preferably a MutationObserver setup on the CM gutter wrapper ) , that would enforce the gutter markers sequence when required.

I am not quite clear about the usage of language ID though as a parameter.

zaggino · 2016-10-13T05:53:41Z

@ficristo no, I haven't tried modifying any of my extensions to use the API yet

ficristo · 2016-10-13T19:26:33Z

@swmitra initially there was this #12673 which used MutationObserver. Since it caused some problems this was opened.
@thehogfather can comment more.

thehogfather · 2016-10-13T22:08:18Z

@swmitra @ficristo Indeed one of the issues (#10864) that triggered this was discovered as a 'Codefolding' issue. Consequently, the original fix involved using the MutationObserver in the code-folding module to listen to changes to the gutter (e.g., when line numbers are hidden/shown) and making sure the codefolding gutter is always inserted after that. The fix was shortsighted and did not fix the underlying lack of API. Moreover, it also gave rise to at least one bug with scroll states of the editor - see #12726.

With the new API, I don't think there is need to watch changes to the gutter - to detect if lineNumbers are present or not. Brackets already watches changes to the preferences (one of which is whether or not to show line numbers) and the new API uses this information to ensure that the state of the gutters rendered in the editor is consistent with that of the external lineNumber preferences -- see the diffs in Editor._updateOption and Editor._renderGutters (comments on those would be particularly welcome). There is also the notion of gutter priority, which is used to sort and render the gutters in a consistent order - even when they are unregistered and then later re-registered. This should prevent any shuffling that might potentially occur. I have fixed the priority of the lineNumber gutter at an arbitrary value of 100. Gutters with less priority appear to the left and higher priorities appear to the right of the line numbers. Gutters with equal priorities are rendered in the order they were registered. When a gutter is shown/hidden, it is simply added/removed from the registered gutters list with the requisite priority value.

Perhaps one would want these level api changes in CodeMirror itself as it does not provide an API rich enough for these use cases?

Finally, the usage of languageId is to allow flexibility in specifying that an extension developer would like their gutter design only for specific languages. For instance, I remember there is/was a nice extension that showed a colored square in the gutter of stylesheet files right next to lines where colors have been used. Such a gutter need not exist in javascript files. So on initialisation, the extension editor could then register gutter for specific languages. Default is all languages.

ficristo · 2016-10-31T12:12:47Z

src/editor/Editor.js


+   /**


Nit: indent.

ficristo · 2016-10-31T12:13:13Z

src/editor/Editor.js

+     * The line number gutter is defined as { name: LINE_NUMBER_GUTTER, priority: 100 }
+     * @type {Array.<{name: string, priority: number, languages: Array}}
+     */
+    var registeredGutters = [];
    var cmOptions         = {};


Add a new line before cmOptions.

ficristo · 2016-10-31T12:14:54Z

src/editor/Editor.js

@@ -110,7 +115,8 @@ define(function (require, exports, module) {
        DEFAULT_SPACE_UNITS     =  4,
        DEFAULT_TAB_SIZE        =  4,
        MAX_SPACE_UNITS         = 10,
-        MAX_TAB_SIZE            = 10;
+        MAX_TAB_SIZE            = 10,
+        LINE_NUMBER_GUTTER_PRIORITY    = 100;


Can you instead create a new var group here with LINE_NUMBER_GUTTER?

var LINE_NUMBER_GUTTER = "CodeMirror-linenumbers", LINE_NUMBER_GUTTER_PRIORITY = 100;

ficristo · 2016-10-31T12:15:07Z

src/editor/Editor.js

@@ -414,13 +421,16 @@ define(function (require, exports, module) {
            readOnly                    : isReadOnly
        });

+


ficristo · 2016-10-31T12:15:15Z

src/editor/Editor.js

@@ -449,6 +459,7 @@ define(function (require, exports, module) {
        this._resetText(document.getText());
        this._duringSync = false;

+


ficristo · 2016-10-31T12:26:35Z

src/editor/Editor.js

+        }
+
+        if (!name || typeof name !== "string") {
+            throw new Error("The name of the registered gutter must be a string.");


I'm not sure: @zaggino do we throw exceptions on wrong parameters?
Maybe a console.error followed by a return is better?

ficristo · 2016-10-31T12:28:20Z

src/extensions/default/CodeFolding/main.js

@@ -73,6 +75,7 @@ define(function (require, exports, module) {

    /** Set to true when init() has run; set back to false after deinit() has run */
    var _isInitialized = false;


Add a new line before initialisedFiles. Can you add a comment too?

ficristo · 2016-10-31T12:29:34Z

src/extensions/default/CodeFolding/main.js

@@ -383,6 +395,7 @@ define(function (require, exports, module) {
        foldCode.init();
        foldGutter.init();

+


ficristo · 2016-10-31T12:31:35Z

src/editor/Editor.js

+
+        // If the line numbers gutter has not been explicitly registered and the CodeMirror lineNumbes option is
+        // set to true, we explicitly add the line  numbers gutter. This case occurs the first time the editor loads
+        // and showwLineNumbers is set to true in preferences


Nit: showLineNumbers, only one w

ficristo · 2016-10-31T12:32:02Z

src/editor/Editor.js

+            rootElement = this.getRootElement();
+
+        // If the line numbers gutter has not been explicitly registered and the CodeMirror lineNumbes option is
+        // set to true, we explicitly add the line  numbers gutter. This case occurs the first time the editor loads


Nit: add the line numbers gutter, a single space between words.

ficristo · 2016-10-31T12:39:31Z

I left only nits, from the API perspective I cannot really say much.

A general rule: do not touch whitespaces on code you don't touch.

Maybe, as already said by @zaggino, we should exports LINE_NUMBER_GUTTER_PRIORITY and CODE_FOLDING_GUTTER_PRIORITY.
Worth adding a test that manipulate the gutter after the code folding markers?

@thehogfather if we can upstream some changes to CodeMirror, to me would be really nice.

petetnt · 2016-11-03T23:08:04Z

src/editor/Editor.js

+     * @param {string} name The name of the gutter to be un registered.
+     */
+    Editor.unregisterGutter = function (name) {
+        var i, gutter;


Maybe would prefer Array.prototype.filter for simplicity over couple of possible cycles saved 🤔 😄

petetnt · 2016-11-03T23:18:11Z

src/editor/Editor.js

+
+    /**
+     * Unregisters the gutter with the specified name and removes it from the UI.
+     * @param {string} name The name of the gutter to be un registered.


Nitpick: "unregistered"

…gfather/gutter_apis

swmitra

Looks good to me and ready to be merged.

ficristo · 2017-02-13T19:26:46Z

@petetnt do you want to review? Otherwise I'll merge this.

petetnt · 2017-02-13T22:21:16Z

LGTM too, merging this.

thehogfather added 3 commits August 29, 2016 23:12

Added apis for manipulating gutters in Editor

d7d9497

fix bug with line number initialisation

8323052

enabled filtering the gutter displayed based on the language of the c…

aa7ab6e

…urrent editor

zaggino reviewed Aug 31, 2016
View reviewed changes

zaggino added this to the Release 1.8 milestone Aug 31, 2016

zaggino added the F Editor label Aug 31, 2016

Addressed CR comments.

d1f5877

registering/unregistering/getting registered gutters are now `static` objects on the editor class. modified code folding gutter to use new api.

ficristo modified the milestones: Release 1.9, Release 1.8 Sep 5, 2016

thehogfather added 2 commits October 2, 2016 18:51

added tests for gutter apis

5584b55

Merge branch 'thehogfather/gutter_apis' of https://github.com/thehogf…

8d98567

…ather/brackets into thehogfather/gutter_apis finished gutter api tests for editor

zaggino approved these changes Oct 13, 2016

View reviewed changes

ficristo suggested changes Oct 31, 2016

View reviewed changes

thehogfather added 2 commits November 3, 2016 22:43

addressing code review comments

1f84de1

revert space

c71542d

petetnt reviewed Nov 3, 2016

View reviewed changes

ficristo approved these changes Nov 4, 2016

View reviewed changes

thehogfather added 2 commits December 3, 2016 16:20

Merge branch 'master' of https://github.com/adobe/brackets into theho…

a11c18f

…gfather/gutter_apis

addressed more missed code review comments

66c233e

swmitra self-requested a review February 10, 2017 07:53

swmitra approved these changes Feb 10, 2017

View reviewed changes

petetnt approved these changes Feb 13, 2017

View reviewed changes

petetnt merged commit 40f96c3 into adobe:master Feb 13, 2017

ficristo mentioned this pull request Mar 15, 2017

Scroll state not remembered in some cases #12726

Closed

This was referenced Aug 30, 2021

[CLOSED] Scroll state not remembered in some cases brackets-archive/bracketsIssues#10844

Open

[CLOSED] APIs for manipulating gutters brackets-archive/bracketsIssues#10859

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APIs for manipulating gutters #12742

APIs for manipulating gutters #12742

thehogfather commented Aug 31, 2016 •

edited

Loading

zaggino Aug 31, 2016

zaggino commented Aug 31, 2016

ficristo commented Sep 1, 2016

thehogfather commented Sep 3, 2016

ficristo commented Sep 3, 2016

thehogfather commented Sep 3, 2016

zaggino commented Sep 29, 2016

thehogfather commented Sep 29, 2016

zaggino commented Sep 29, 2016

thehogfather commented Oct 2, 2016

zaggino commented Oct 13, 2016

ficristo commented Oct 13, 2016

swmitra commented Oct 13, 2016

zaggino commented Oct 13, 2016

ficristo commented Oct 13, 2016

thehogfather commented Oct 13, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo Oct 31, 2016

ficristo commented Oct 31, 2016

petetnt Nov 3, 2016

petetnt Nov 3, 2016

swmitra left a comment

ficristo commented Feb 13, 2017

petetnt commented Feb 13, 2017

		@@ -414,13 +421,16 @@ define(function (require, exports, module) {
		readOnly : isReadOnly
		});

		@@ -449,6 +459,7 @@ define(function (require, exports, module) {
		this._resetText(document.getText());
		this._duringSync = false;

		@@ -73,6 +75,7 @@ define(function (require, exports, module) {

		/** Set to true when init() has run; set back to false after deinit() has run */
		var _isInitialized = false;

		@@ -383,6 +395,7 @@ define(function (require, exports, module) {
		foldCode.init();
		foldGutter.init();

APIs for manipulating gutters #12742

APIs for manipulating gutters #12742

Conversation

thehogfather commented Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

zaggino commented Aug 31, 2016

ficristo commented Sep 1, 2016

thehogfather commented Sep 3, 2016

ficristo commented Sep 3, 2016

thehogfather commented Sep 3, 2016

zaggino commented Sep 29, 2016

thehogfather commented Sep 29, 2016

zaggino commented Sep 29, 2016

thehogfather commented Oct 2, 2016

zaggino commented Oct 13, 2016

ficristo commented Oct 13, 2016

swmitra commented Oct 13, 2016

zaggino commented Oct 13, 2016

ficristo commented Oct 13, 2016

thehogfather commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swmitra left a comment

Choose a reason for hiding this comment

ficristo commented Feb 13, 2017

petetnt commented Feb 13, 2017

thehogfather commented Aug 31, 2016 •

edited

Loading