Ensures fold-gutter is always inserted after the line-number gutter (if it exists) #12673

thehogfather · 2016-08-14T11:09:23Z

Addresses issue where line numbers appears after fold gutter if the line number visibility is toggled in the View Menu.

Uses MutationObserver to watch changes to the brackets code mirror gutters.

@ficristo I noticed that globals are no longer placed in files, I've used globals here for MutationObserver (since it isn't used anywhere else) - but open to moving to a more central location if needed.

…ine number visibility is toggled in the View Menu. Uses MutationObserver (https://developer.mozilla.org/en/docs/Web/API/MutationObserver) to watch changes to the brackets code mirror gutters. Was PR adobe#11593 Addresses adobe#11577, adobe#10864

…r/brackets into fold-gutter-position # Conflicts: # src/extensions/default/CodeFolding/main.js

ficristo · 2016-08-14T12:36:23Z

@thehogfather thank you for the PR.
In general, from now onwards, the inline directives are mostly forbidden (there are still some exceptions).
In this case you can put MutationObserver after ArrayBuffer both in .eslintrc.json and .brackets.json.

ficristo · 2016-08-14T12:41:16Z

I think @marcelgerber or @petetnt are better to review this one.

marcelgerber · 2016-08-14T15:35:41Z

Looks like #10864 is fixed now, but I still see the other issue with the active line highlight

(This is Brackets running without extensions on the preinstalled Brackets Dark theme)

thehogfather · 2016-08-14T16:44:05Z

@marcelgerber could you share a link to the active line highlight issue? This PR only addresses the issues mentioned in the description. Neither of those have anything to do with active line highlights.

marcelgerber · 2016-08-14T16:49:16Z

Ah, I see now. #11577 mentions the issue, but I thought it's all about it.
This may not even be the fault of this extension.

marcelgerber · 2016-08-14T17:00:21Z

src/extensions/default/CodeFolding/main.js

-            var lnIndex = gutters.indexOf("CodeMirror-linenumbers");
+            var lineNumberIndex = gutters.indexOf("CodeMirror-linenumbers");
+            if (lineNumberIndex < 0) {
+                $(editor.getRootElement()).addClass("linenumber-disabled");


This might also go well here:

brackets/src/editor/Editor.js

Lines 2324 to 2328 in 7814da1

} else if (prefName === SHOW_LINE_NUMBERS) {

Editor._toggleLinePadding(!newValue);

this._codeMirror.setOption(cmOptions[SHOW_LINE_NUMBERS], newValue);

this.refreshAll();

} else {

That way, stylesheets and other extension have an easy way to reuse this state.
cc @nethip

thehogfather · 2016-08-14T17:02:26Z

No I don't think the extension introduced the issue - although to be honest I am not actually sure what the expectation is here - so a little difficult to explore a solution.

petetnt · 2016-08-14T20:09:10Z

src/extensions/default/CodeFolding/main.js

+                lineNumberIndex,
+                foldGutterIndex,
+                cm = editor._codeMirror,
+                guttersContainer = $(".CodeMirror-gutters", editor.getRootElement()),


editor.getRootElement() is used here, on L342, L344 and L346 so it could / should be saved to an variable

petetnt · 2016-08-14T20:13:16Z

Small general nit: Would be nice if all the magic strings would be moved to constants in the file, would make modifications somewhat easier / safer (can be done in another PR too though)!

Good job @thehogfather once again 👍

thehogfather · 2016-08-15T22:14:25Z

src/extensions/default/CodeFolding/main.js

-            gutters.splice(lnIndex + 1, 0, GUTTER_NAME);
+            var lineNumberIndex = gutters.indexOf(LINE_NUMBER_GUTTER);
+            if (lineNumberIndex < 0) {
+                $(rootElement).addClass("linenumber-disabled");


@marcelgerber originally wrote :

This might also go well here:

brackets/src/editor/Editor.js

Lines 2324 to 2328 in 7814da1

} else if (prefName === SHOW_LINE_NUMBERS) {

Editor._toggleLinePadding(!newValue);

this._codeMirror.setOption(cmOptions[SHOW_LINE_NUMBERS], newValue);

this.refreshAll();

} else {

That way, stylesheets and other extension have an easy way to reuse this state.
cc @nethip

zaggino · 2016-08-25T22:32:25Z

Is this waiting for anything @petetnt ?

petetnt · 2016-08-26T07:26:00Z

I'll try to test this out once more and run the tests ASAP and merge this in if all is good.

petetnt · 2016-08-27T17:33:01Z

There are some failing Code Folding tests which are unrelated to this PR which are being tracked in #12724.

This PR LGTM, merging.

zaggino · 2016-08-27T23:47:03Z

Getting back to this, related to #12726 (comment) I believe this should be reverted, using MutationObserver is not a good thing. Especially if some other extensions tries to do the same thing, I expect we start to get infinite loops?

cc @marcelgerber @petetnt @thehogfather

thehogfather · 2016-08-28T07:36:38Z

This is a symptom of an underlying limitation (within the core libraries CodeMirror and brackets) - i.e., the absence of a rich API for manipulating the gutter in the editor.

At the moment, every extension (that requires the gutter) splices and directly manipulates the gutter without regard for how any other extension might be using the gutter wrt relative position / placement etc. Indeed it is difficult to preempt how many different ways the gutter might be used by other extensions.

What might be immediately useful in a hypothetical API would be the notion of an anchor gutter or gutters (say the line numbers gutter) such that placement of other gutters are requested relative to said anchor gutter(s). So one might say insert the code-folding gutter after the line-numbers, insert the git gutter before the line-numbers, insert theseus-gutter after git, before line numbers etc.

If such an API (or a much refined variation) existed, then there wouldn't be need for the MutationObserver (since all requests to manipulate would go through said API).

Re: infinite loops,
Totally possible if different extensions decided to use mutationobservers directly as well. The implementation in this PR itself should not cause infinite loops as it disconnects the observer while updating the gutter.

cc @zaggino @marcelgerber @petetnt
ref #12726

zaggino · 2016-08-28T09:17:40Z

We should definitely set up an API in brackets, make this work with that API and let authors fix their extensions to use the API. Current state is not maintenable, and if we add this, another authors will do the same for their purposes and then we'll have a hell around a simple feature.

petetnt · 2016-08-28T15:29:25Z

Haven't/didn't run to #12726 but let's revert this one and let's think of a nicer API for doing this.

thehogfather added 3 commits August 14, 2016 11:56

Merge branch 'fold-gutter-position' of https://github.com/thehogfathe…

a2e5dbc

…r/brackets into fold-gutter-position # Conflicts: # src/extensions/default/CodeFolding/main.js

Added mutation observer to .brackets.json and .eslintrc.json

ad95fd2

marcelgerber reviewed Aug 14, 2016
View reviewed changes

petetnt reviewed Aug 14, 2016
View reviewed changes

Refactored to address code review comments

35eba9e

thehogfather reviewed Aug 15, 2016
View reviewed changes

ficristo added the F Code Folding label Aug 16, 2016

petetnt merged commit 5727863 into adobe:master Aug 27, 2016

This was referenced Aug 27, 2016

CodeFolding should be centralized. #11577

Open

Code folding doesn't play well with View > Line Numbers toggle #10864

Closed

petetnt mentioned this pull request Aug 28, 2016

Revert "Ensures fold-gutter is always inserted after the line-number gutter (if it exists)" #12727

Merged

ficristo mentioned this pull request Oct 13, 2016

APIs for manipulating gutters #12742

Merged

	} else if (prefName === SHOW_LINE_NUMBERS) {
	Editor._toggleLinePadding(!newValue);
	this._codeMirror.setOption(cmOptions[SHOW_LINE_NUMBERS], newValue);
	this.refreshAll();
	} else {

Ensures fold-gutter is always inserted after the line-number gutter (if it exists) #12673

Ensures fold-gutter is always inserted after the line-number gutter (if it exists) #12673

Uh oh!

Conversation

thehogfather commented Aug 14, 2016

Uh oh!

ficristo commented Aug 14, 2016

Uh oh!

ficristo commented Aug 14, 2016

Uh oh!

marcelgerber commented Aug 14, 2016

Uh oh!

thehogfather commented Aug 14, 2016

Uh oh!

marcelgerber commented Aug 14, 2016

Uh oh!

marcelgerber Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

thehogfather commented Aug 14, 2016

Uh oh!

petetnt Aug 14, 2016

Choose a reason for hiding this comment

Uh oh!

petetnt commented Aug 14, 2016

Uh oh!

thehogfather Aug 15, 2016

Choose a reason for hiding this comment

Uh oh!

zaggino commented Aug 25, 2016

Uh oh!

petetnt commented Aug 26, 2016

Uh oh!

petetnt commented Aug 27, 2016

Uh oh!

zaggino commented Aug 27, 2016

Uh oh!

thehogfather commented Aug 28, 2016

Uh oh!

zaggino commented Aug 28, 2016

Uh oh!

petetnt commented Aug 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants