integration of code-folding extension into brackets #10792

Merged
merged 13 commits into from Apr 7, 2015

Projects

None yet
@thehogfather
Contributor

Integration of core code-folding extension files into brackets' default extension folder.

@Florian-R

Out of curiosity, any reason to not keep this as an extension?

@prksingh
Collaborator

@Florian-R There has been demand to have code folding in the default user experience for brackets. It made sense to reuse an already popular extension to provide this support in core

@abose
Contributor
abose commented Mar 25, 2015

@thehogfather Thanks 👍
The Travis CL builds are failing mostly due to JSLint related errors. Could you fix the lint errors and update?

@Florian-R

@prksingh I had hoped that Brackets has kept some sort of Node philosophy, "small core, vibrant userland" but anyway thanks for your input.

@thehogfather
Contributor

@abose sure thing.

@ryanstewart
Member

@Florian-R in general, that's our philosophy, but code folding has been on our list as "core" for a very long time. Most major editors have it out of the box. It's also far and away one of the most popular extensions, so it was clear that users were missing it.

@ryanstewart
Member

A couple of things we want to do as part of the merge:

  • Double check the shorts that are used. I think they're probably fine but we should take a look at that.
  • Move the preferences into our regular preferences system (brackets.json)
  • Create an option to disable code folding as preference

Open question here: Do we allow separate code folding preferences per project? or just globally? or per file?

@MiguelCastillo MiguelCastillo commented on an outdated diff Mar 27, 2015
src/extensions/default/CodeFolding/DefaultSettings.js
+ * JSON values for default settings
+ * @author Patrick Oladimeji
+ * @date 8/23/14 15:45:35 PM
+ */
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define*/
+define(function (require, exports, module) {
+ "use strict";
+
+ module.exports = {
+ minFoldSize: 2,
+ saveFoldStates: true,
+ alwaysUseIndentFold: true,
+ enableRegionFolding: true,
+ fadeFoldButtons: false,
+ maxFoldLevel: 2 // this value is only used when fold all is called
@MiguelCastillo
MiguelCastillo Mar 27, 2015 Member

NIT: indentation

@MiguelCastillo MiguelCastillo and 2 others commented on an outdated diff Mar 27, 2015
src/extensions/default/CodeFolding/Prefs.js
+ * @author Patrick Oladimeji
+ * @date 3/22/14 20:39:53 PM
+ */
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, brackets*/
+define(function (require, exports, module) {
+ "use strict";
+ var PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
+ stateManager = PreferencesManager.stateManager.getPrefixedSystem("code-folding"),
+ DefaultSettings = require("DefaultSettings"),
+ store = {},
+ settings = {},
+ folds = "folds";
+
+ function simplify(folds) {
+ if (!folds) { return folds; }
@MiguelCastillo
MiguelCastillo Mar 27, 2015 Member

Tweak this to

if (!folds) {
    return folds;
}
@le717
le717 Mar 29, 2015 Contributor

But how can you return folds if it not passed?

@thehogfather
thehogfather Mar 29, 2015 Contributor

That's equivalent to returning undefined. I've updated for clarity :).

@MiguelCastillo MiguelCastillo commented on an outdated diff Mar 27, 2015
src/extensions/default/CodeFolding/DefaultSettings.js
@@ -0,0 +1,19 @@
+/**
+ * JSON values for default settings
+ * @author Patrick Oladimeji
+ * @date 8/23/14 15:45:35 PM
+ */
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define*/
+define(function (require, exports, module) {
@MiguelCastillo
MiguelCastillo Mar 27, 2015 Member

Files are generally mixed tabs/spaces. For consistency, we should stick to 4 spaces.

@MiguelCastillo MiguelCastillo and 1 other commented on an outdated diff Mar 27, 2015
src/extensions/default/CodeFolding/Prefs.js
+ */
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, brackets*/
+define(function (require, exports, module) {
+ "use strict";
+ var PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
+ stateManager = PreferencesManager.stateManager.getPrefixedSystem("code-folding"),
+ DefaultSettings = require("DefaultSettings"),
+ store = {},
+ settings = {},
+ folds = "folds";
+
+ function simplify(folds) {
+ if (!folds) { return folds; }
+ var res = {}, range;
+ Object.keys(folds).map(function (line) {
@MiguelCastillo
MiguelCastillo Mar 27, 2015 Member

Why call map when you are not really transforming data and creating a new array form it? A forEach would suffice.

@thehogfather
thehogfather Mar 27, 2015 Contributor

Indeed that is a typo. Should be a forEach.

@MiguelCastillo MiguelCastillo commented on an outdated diff Mar 27, 2015
src/extensions/default/CodeFolding/Prefs.js
+ store = {},
+ settings = {},
+ folds = "folds";
+
+ function simplify(folds) {
+ if (!folds) { return folds; }
+ var res = {}, range;
+ Object.keys(folds).map(function (line) {
+ range = folds[line];
+ res[line] = Array.isArray(range) ? range : [[range.from.line, range.from.ch], [range.to.line, range.to.ch]];
+ });
+ return res;
+ }
+
+ function inflate(folds) {
+ if (!folds) { return folds; }
@MiguelCastillo
MiguelCastillo Mar 27, 2015 Member

Expand this out to multiple lines

if (!folds) {
    return folds;
}
@MiguelCastillo MiguelCastillo commented on an outdated diff Mar 27, 2015
src/extensions/default/CodeFolding/Prefs.js
+ }
+
+ function inflate(folds) {
+ if (!folds) { return folds; }
+ //transform the folds into objects with from and to properties
+ var ranges = {}, obj;
+ Object.keys(folds).forEach(function (line) {
+ obj = folds[line];
+ ranges[line] = {from: {line: obj[0][0], ch: obj[0][1]}, to: {line: obj[1][0], ch: obj[1][1]}};
+ });
+
+ return ranges;
+ }
+
+ module.exports = {
+ get: function (id) {
@MiguelCastillo
MiguelCastillo Mar 27, 2015 Member

Common style is to make these named functions that you assign to the module.exports. E.g.

function get(id) {
}

module.exports.get = get;

We probably also benefit from some jsdocs :)

@MiguelCastillo MiguelCastillo commented on an outdated diff Mar 27, 2015
src/extensions/default/CodeFolding/Prefs.js
+ });
+
+ return ranges;
+ }
+
+ module.exports = {
+ get: function (id) {
+ store = (stateManager.get(folds) || {});
+ return inflate(store[id]);
+ },
+ set: function (id, value) {
+ store[id] = simplify(value);
+ stateManager.set(folds, store);
+ stateManager.save();
+ },
+ getSetting: function (key) {
@MiguelCastillo
MiguelCastillo Mar 27, 2015 Member

Indentation here wont be a problem once they are moved from here to named functions.

@MiguelCastillo MiguelCastillo commented on an outdated diff Mar 27, 2015
...xtensions/default/CodeFolding/foldhelpers/foldcode.js
+ * @last modified 20 April 2014
+ */
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, brackets, document*/
+define(function (require, exports, module) {
+ "use strict";
+ var CodeMirror = brackets.getModule("thirdparty/CodeMirror2/lib/codemirror"),
+ prefs = require("Prefs");
+
+ module.exports = function () {
+ function doFold(cm, pos, options, force) {
+ force = force || "fold";
+ if (typeof pos === "number") {
+ pos = CodeMirror.Pos(pos, 0);
+ }
+ var finder = (options && options.rangeFinder) || CodeMirror.fold.auto;
@MiguelCastillo
MiguelCastillo Mar 27, 2015 Member

instead of checking is options exists in each line that uses options, you can have give it a default value.

options = options || {};
@MiguelCastillo
Member

@thehogfather It is awesome that you are brining this into brackets!! High-5.

I did not realize how big your extension was! But I did a quick pass and it seems that one glaring inconsistency is the mix of spaces/tabs and that lots of the function are defined right in the module.exports. I am guessing it will be a couple of rounds of tweaks/refactorings to get your extension in line with the rest of brackets. :)

Really happy this is coming in.

@thehogfather
Contributor

@MiguelCastillo No problem. I'll work on these fixes and update. :)

@MarcelGerber
Member

You should probably merge your translations (at least the English ones) into our main strings.js as other default extensions do the same.

thehogfather added some commits Mar 29, 2015
@thehogfather thehogfather Merge commit 'e56192e09ceed72834985f2483ed70e339e5c519' into codefolding
* commit 'e56192e09ceed72834985f2483ed70e339e5c519':
  Logos are now perfectly centered
  Whitespace trimming suggested by @MarcelGerber
  Fix some moved APIs that were left over from PR #10641
  Text is now perfectly centered
985fe8a
@thehogfather thehogfather Merged english translation strings into nls/root/strings.js 81cb981
@thehogfather
Contributor

I've tidied up and refactored a bit based on the suggestions (thanks all). I've moved preferences to brackets.json and merged the english strings into brackets main strings. There is also an option to disable the extension in the settings dialog (should this be on the menu instead - or as well?).

I have translations for fi, fr, ja, nl, pt-br and ru but haven't merged yet. Shall I go ahead and merge those?

@nethip
Contributor
nethip commented Mar 30, 2015

@thehogfather Please go ahead and merge all the translations for this, barring fr and ja. The fr and ja will need to go through our translation process.

@abose abose was assigned by nethip Mar 30, 2015
@abose
Contributor
abose commented Mar 30, 2015

@thehogfather As i understand, the translations, the HTML dialogs and the menu entries are for the settings dialog for code folding. But as @ryanstewart pointed out in an earlier comment, Brackets doesn't set app preferences with dialogs; But customizable options are set in the preerences file and the default options are set in the 'brackets.json' file in the repo. As the code folding extension is directly integrated with brackets, i think it would be best if it confirms to brackets preferences settings.
So i think we should

delete all the preferences dialogs and related strings and move all configurable settings to brackets.json.
Remove the menu entry for code folding settings
Code folding by default will be enabled for all users. The option to disable code folding can be set as a preferences entry.
@peterflynn @ryanstewart

@MarcelGerber
Member

It's probably easier to have a common prefix for all string names, like CODE_FOLDING_*.
Also, you can now remove your old nls files.

Furthermore, you should call definePreference(name, type, default) before using your preferences. That way, you can also get rid of DefaultSettings.js.

@thehogfather
Contributor

@abose thanks for the clarification. I'd misunderstood @ryanstewart's reference to brackets.json. Shall update.

@abose
Contributor
abose commented Mar 31, 2015

@thehogfather I see a console error message "Cannot assign Ctrl-Alt-X to e4b.TOGGLE_PANEL. It is already assigned to codefolding.expand"- when we run brackets+extract builds. Could we change the expand and collapse shortcuts to something like "ctrl + shift + [" and "ctrl + shift + ]" (like in sublime).

@thehogfather
Contributor

@abose we can certainly change the shortcut keys - although probably not to Ctrl+Shift+[ since that clashes with navigate.prevDoc. May I suggest Ctrl+Alt+[ and Ctrl+Alt+]?

@thehogfather thehogfather Removed htmltemplates related to dialog boxes for code-folding settings.
Updated shortcut to Ctrl+Alt+[ and Ctrl+Alt+] for expand and collapse
b341c37
@thehogfather thehogfather and 1 other commented on an outdated diff Mar 31, 2015
src/extensions/default/CodeFolding/Prefs.js
+
+ /**
+ Saves the line folds specified
+ */
+ function set(path, folds) {
+ store[path] = simplify(folds);
+ prefs.set(foldsKey, store);
+ }
+
+ /**
+ Get the code folding setting with the specified key from the store
+ @param {!string} key The key for the setting to retrieve
+ @returns {string} the setting with the specified key
+ */
+ function getSetting(key) {
+ return prefs.get(key, PreferencesManager.CURRENT_PROJECT);
@thehogfather
thehogfather Mar 31, 2015 Contributor

@abose @ryanstewart I need some clarification on how to retrieve preferences from the correct scope. For the sake of argument, the above line is attempting to retrieve code-folding settings that have been entered at the project level (i.e., using .brackets.json in a project dir). But it appears that the values in that file are actually not being returned in this instance. In fact after a little debugging, the project scope does not appear in the scope order used in the _getContext function defined in PreferencesManager (perhaps because there is no path property in the context sent from PreferencesManager.CURRENT_PROJECT). What is the correct way to use this api?

@MiguelCastillo
MiguelCastillo Apr 1, 2015 Member

@thehogfather prefs.get(key) is supposed to return the value by first searching the project .brackets.json, and if the key isn't found there, it will search the use (global) brackets.json. You don't need the second parameter. But it seems like you are after something a bit different. Can explain on what you are trying to achieve?

@thehogfather
thehogfather Apr 1, 2015 Contributor

@MiguelCastillo thanks, that was how I thought it worked. All I am trying to do is to override any code-folding preferences set at the user or default level with one set at the project level. For instance placing "code-folding.enabled": false in .brackets.json file to disable code-folding. I couldn't get this to work consistently yesterday so I thought I'd 'force' it to look at the project scope using the second parameter as suggested by the wiki. At the moment, when I put a breakpoint in developer console to debug, it works as expected. Without the breakpoint it doesn't seem to be reading the correct preference value.
Any insights or questions would be useful. I'll also have a closer look later today.
Cheers.

@thehogfather
thehogfather Apr 1, 2015 Contributor

Actually all is working consistently and as expected now.

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 1, 2015
src/extensions/default/CodeFolding/Prefs.js
+ @param {!{number: {from: {ch, line}, to: {ch, line}} folds the raw fold ranges indexed by line numbers
+ @returns {number: Array<Array<number>>} an object whose keys are line numbers and the values are array
+ of two 2-element arrays. First array contains [from.line, from.ch] and the second contains [to.line, to.ch]
+ */
+ function simplify(folds) {
+ if (!folds) {
+ return;
+ }
+ var res = {}, range;
+ Object.keys(folds).forEach(function (line) {
+ range = folds[line];
+ res[line] = Array.isArray(range) ? range : [[range.from.line, range.from.ch], [range.to.line, range.to.ch]];
+ });
+ return res;
+ }
+ /**
@MiguelCastillo
MiguelCastillo Apr 1, 2015 Member

add a courtesy new line right here :D

@abose
Contributor
abose commented Apr 1, 2015

shortcuts Ctrl+Alt+[ and Ctrl+Alt+] 👍

@abose abose commented on the diff Apr 2, 2015
src/nls/ru/strings.js
// extensions/default/WebPlatformDocs
- "DOCS_MORE_LINK" : "Подробнее\u2026"
+ "DOCS_MORE_LINK" : "Подробнее\u2026",
+
+ //extensions/default/CodeFolding
+ "COLLAPSE_ALL" : "Свернуть все",
@abose
abose Apr 2, 2015 Contributor

As we are not using the preferences dialog, do we need these translations now? We could just use the root English string as it used only for setting the preferences variable.

@abose abose commented on the diff Apr 2, 2015
src/nls/fi/strings.js
@@ -624,7 +624,25 @@ define({
"CMD_TOGGLE_RECENT_PROJECTS" : "Viimeisimmät projektit",
// extensions/default/WebPlatformDocs
- "DOCS_MORE_LINK" : "Lue lisää"
+ "DOCS_MORE_LINK" : "Lue lisää",
+
+ //extensions/default/CodeFolding
+ "COLLAPSE_ALL" : "Pienennä kaikki",
@abose
abose Apr 2, 2015 Contributor

As we are not using the preferences dialog, do we need these translations now? We could just use the root English string as it used only for setting the preferences variable.

@thehogfather
thehogfather Apr 2, 2015 Contributor

@abose 5 of the translations are for the menu items under View i.e., 'collapse all', 'expand all', 'collapse current', 'expand current', 'collapse custom regions'. I think it is good idea to keep these menu items (as they also show the shortcut keys). If there are strong reasons why you think we should remove them then we can discuss them.

As for the other preference strings, you are absolutely right. I've only kept them to use in the options parameter (name and description) when defining preferences. So they can be taken out of the non root strings.

@abose
abose Apr 4, 2015 Contributor

What does collapse custom regions do?
Also putting in all the code folding options in the top level view menu would increase the menu real estate. It would be ideal to move it into a top level 'code folding' entry in the view/edit menu(as in sublime ) and then put the folding options as submenus. The string freeze is by 6'th, so we would need to put in all strings into brackets before that.

@thehogfather
thehogfather Apr 4, 2015 Contributor

@abose collapse custom regions closes all custom defined region blocks in the current editor. These are defined using comments of the current mode with the keywords region & endregion e.g.,:

\\region
var point = {x: 1, y: 2};
\\endregion

With respect to menus, as far as I know, brackets does not implement submenus yet. Hence I'm not sure how to go about creating the sublime-like sub-menu for code-folding. So did you mean a top level "code folding" entry (just like view/edit), whose menu items are the folding options?

And as for the strings, I have no more changes to make. Are you happy with the code-folding strings present in the root string? In addition, do let me know if you would like me to remove all code-folding related translations I have merged into non-root strings. These can certainly be sorted promptly.

@abose
abose Apr 5, 2015 Contributor

I failed to notice that brackets didn't have submenus; i guess this is the way for now.

@peterflynn
peterflynn Apr 8, 2015 Member

@abose See https://trello.com/c/EwLGRkYe/541-native-submenus. The only reason we don't have it is that it's a sizeable chunk of work: we'd have to decide on the JS Menus API, implement submenus natively on Mac and Win, and implement them HTML for Linux / in-browser before the feature could land in master.

@abose
abose Apr 8, 2015 Contributor

OK, got it. we'll have to address it some day.

@abose
Contributor
abose commented Apr 2, 2015

@thehogfather How would it behave if the user had already installed the extension from the registry. I did a basic round of testing and coudnt find any issues in the case. But will this 'double loading' of the same extension source cause any problems? How do we handle this case?

@abose
Contributor
abose commented Apr 2, 2015

The package.json file which has the details for the extension description,author,Contributors etc.. is missing. Could you put that file back in?
Also need to change the name to "name": "code-folding" from "name": "brackets-code-folding" - to differentiate between the default code folding extension and the code folding extension in the registry. Should the version number be retained to get which version of the extension we integrated?

@MarcelGerber MarcelGerber commented on the diff Apr 2, 2015
src/extensions/default/CodeFolding/Prefs.js
+ * This is to make it easy for the user who edits their preferences file to easily manage the potentially numerous lines of preferences generated by the persisting code-folding state.
+ * @author Patrick Oladimeji
+ * @date 3/22/14 20:39:53 PM
+ */
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, brackets*/
+define(function (require, exports, module) {
+ "use strict";
+ var PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
+ prefs = PreferencesManager.getExtensionPrefs("code-folding"),
+ strings = brackets.getModule("strings"),
+ store = {},
+ foldsKey = "folds";
+
+ //default preference values
+ prefs.definePreference("enabled", "boolean", true,
@MarcelGerber
MarcelGerber Apr 2, 2015 Member

I don't think any other Brackets code uses the fourth parameter options just yet.

@thehogfather
thehogfather Apr 2, 2015 Contributor

No I didn't think so either. But I thought if the API exists why not use it?

@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

I like the extra meta data in there :)

@thehogfather
Contributor

@abose at the moment what happens is that the first version of the extension loaded takes precedence. However event handlers on the EditorManager, DocumentManager and ProjectManager may be handled twice. The only drawback I can see is that persistence of folded states is done twice when closing the application or switching projects. In both cases the integrity of the fold states is preserved.

I can update the version on the registry so that it is only initialised if code-folding doesnt already exist (e.g. by inspecting properties on the CodeMirror object). For those who do not update the extension, the menu items under View will still show an entry for the Settings Dialog.

@thehogfather thehogfather added package.json
removed deprecated warnings on event handlers
88261fc
@le717 le717 commented on an outdated diff Apr 3, 2015
src/extensions/default/CodeFolding/main.js
+ Expands the code region at the current cursor position.
+ */
+ function expandCurrent() {
+ var editor = EditorManager.getFocusedEditor();
+ if (editor) {
+ var cursor = editor.getCursorPos(), cm = editor._codeMirror;
+ cm.unfoldCode(cursor.line);
+ }
+ }
+
+ /**
+ Collapses all foldable regions in the current document. Folding is done up to a level 'n'
+ which is defined in the preferences. Levels refer to fold heirarchies e.g., for the following
+ code fragment, the function is level 1, the if statement is level 2 and the forEach is level 3
+
+ function sample() {
@le717
le717 Apr 3, 2015 Contributor

If this (sample()) is dead code, can you remove it? :)

@le717 le717 commented on an outdated diff Apr 3, 2015
src/extensions/default/CodeFolding/main.js
+ }
+ } else {
+ if (event.altKey) {
+ var rf = CodeMirror.fold.auto;
+ range = rf(cm, pos);
+ if (range) {
+ CodeMirror.commands.foldToLevel(cm, range.from.line, range.to.line);
+ }
+ } else {
+ cm.foldCode(line);
+ }
+ }
+ }
+
+ /**
+ Collapses all custom regions defined in the current editor
@le717
le717 Apr 3, 2015 Contributor

I think there's a small indentation issue here.

@le717
Contributor
le717 commented Apr 3, 2015

One thing I'm noticing is lack of JSDocs. I find the FuncDocr and Reasonable Comments extensions help make that easier. :)

@abose
Contributor
abose commented Apr 4, 2015

@thehogfather Yes it would be good if the extension is aware that there could be code folding extension by default and do the loading changes accordingly. You could push in a later update to the original extension.

@abose
Contributor
abose commented Apr 5, 2015

@thehogfather Will be merging this by tomorrow 👍 Thanks for the help. Will have to write the unit tests and some other changes, but once its in master we could help with those.

@thehogfather
Contributor

@abose no problem. Happy to help out anytime.

@MiguelCastillo
Member

@thehogfather sorry i disappeared. Let me take another pass at the code later this evening.

My take on the code folding extension in the registry is that once this is merged in there isn't really a need for the one in the registry, especially if it is just duplicate functionality. In general, I am not too concerned if someone decides to install an extension for functionality that already exists natively in brackets. However, we should be good citizens and make sure that the integrated extension does not use the same settings as the one that already exists in the registry - to avoid collision. Currently both use "code-folding", so we would probably pick another name for this integrated extension so that people with the extension already installed don't have conflicts.

Also, I am thinking that this will be announced as a feature for a future release, so lots of people will be aware that code folding is native in brackets :D

@abose default extensions don't need a package.json and the couple that have it are theme extensions because the theme field is needed by the extension manager. So package.json file should not come in - to stay consistent with the other default extensions. I don't believe there is a convention where default extensions need to have a name that starts with "brackets" - I don't think it is a requirement either.

@abose
Contributor
abose commented Apr 6, 2015

@MiguelCastillo The name for the extension in the registry is 'brackets-code-folding' and the one that is integrated is 'code-folding' to prevent the conflicts as you pointed out. Since the extension is used by a large number of users, I was hoping that @thehogfather would update the extension in the registry to consider the case where brackets already have the extension as default[detected using the extension name from package.json?]. If the extension is pulled out of the registry, wouldn't It affect the users who have the extension installed. That was why I thought to put in the package.json file in; apart from the dev attributions. Also most of the default extension (except themes) were developed inside the brackets source base itself; that may be why the package details are missing in them.

Also did you get a chance to look at the code, I was planning to integrate it today for kicking off the string translations and giving sufficient window for unit tests from our side as the 1.3 release is very close now.

@MiguelCastillo MiguelCastillo commented on the diff Apr 6, 2015
src/extensions/default/CodeFolding/Prefs.js
@@ -0,0 +1,114 @@
+/**
+ * Wrapper around brackets pref system to ensure preferences are stored in in one single object instead of using multiple keys.
+ * This is to make it easy for the user who edits their preferences file to easily manage the potentially numerous lines of preferences generated by the persisting code-folding state.
+ * @author Patrick Oladimeji
+ * @date 3/22/14 20:39:53 PM
+ */
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, brackets*/
+define(function (require, exports, module) {
+ "use strict";
+ var PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
+ prefs = PreferencesManager.getExtensionPrefs("code-folding"),
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Any objection to renaming code-folding to maybe brackets-code-folding? Simply to helps everyone keep settings separate.

@thehogfather
thehogfather Apr 6, 2015 Contributor

in principle no objection. But I would suggest that I change the prefs in the version in the registry to "brackets-code-folding" for consistency - to match the name in the package.json.

@abose
abose Apr 6, 2015 Contributor

That would be great. It would also help existing users keep saved settings from the extension.

@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

@thehogfather Are you proposing that the integrated and the one in the registry should have the same preferences name? Or are you saying that instead of changing this integrated version we change the one in the registry?

What I expect is that users will remove the code folding extension when the new version of brackets comes out. At which point it doesn't really matter what we call the preferences. But if the plan is to continue to move the extension in the registry forward, then giving this integrated version the new name will let the users of the extension continue to use it without needing to migrate their settings.

Maybe I am lacking some understanding. @thehogfather is your plan to continue to develop the extension in the registry and have competing functionality with what we are integrating? Because I am not sure I see the purpose of having this integrated if the one in the registry will continue to provide the same functionality.

@thehogfather
thehogfather Apr 6, 2015 Contributor

@MiguelCastillo no. Just proposing that the preference name should match the name in the package.json. So currently the version in the registry stores preferences using the "code-folding" key. I'll update it to "brackets-code-folding" and leave the version integrated into brackets as "code-folding". This also means any preferences saved in the old extension will be kept in the integrated version.

@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Gotcha. That makes sense. I was just curious if we wanted users to just transparently use the settings they have. In which case I expect them to remove the extension.

@MiguelCastillo MiguelCastillo and 1 other commented on an outdated diff Apr 6, 2015
src/extensions/default/CodeFolding/Prefs.js
+ prefs.definePreference("minFoldSize", "number", 2,
+ {name: strings.MIN_FOLD_SIZE, description: strings.MIN_FOLD_SIZE_HELP});
+ prefs.definePreference("saveFoldStates", "boolean", true,
+ {name: strings.SAVE_FOLD_STATES, description: strings.SAVE_FOLD_STATES_HELP});
+ prefs.definePreference("alwaysUseIndentFold", "boolean", true,
+ {name: strings.ALWAYS_USE_INDENT_FOLD, description: strings.ALWAYS_USE_INDENT_FOLD_HELP});
+ prefs.definePreference("enableRegionFolding", "boolean", true,
+ {name: strings.ENABLE_REGION_FOLDING, description: strings.ENABLE_REGION_FOLDING});
+ prefs.definePreference("fadeFoldButtons", "boolean", false,
+ {name: strings.FADE_FOLD_BUTTONS, description: strings.FADE_FOLD_BUTTONS_HELP});
+ prefs.definePreference("maxFoldLevel", "number", 2,
+ {name: strings.MAX_FOLD_LEVEL, description: strings.MAX_FOLD_LEVEL_HELP});
+ prefs.definePreference("folds", "object", {});
+
+ /**
+ Simplifies the fold ranges into an array of pairs of numbers.
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

@thehogfather - do you think we can get all the jsdocs to follow the conventions followed in Brackets?

/**
  *  Simplifies the fold ranges into an array of pairs of numbers.
  *
  * @param {!{number: {from: {ch, line}, to: {ch, line}} folds the raw fold ranges indexed by line numbers
  * @returns {number: Array<Array<number>>} an object whose keys are line numbers and the values are array
  *        of two 2-element arrays. First array contains [from.line, from.ch] and the second contains [to.line, to.ch]
  */

This is a good example.

@thehogfather
thehogfather Apr 6, 2015 Contributor

@MiguelCastillo not a problem. Thanks for the pointer.

@MiguelCastillo MiguelCastillo and 1 other commented on an outdated diff Apr 6, 2015
src/extensions/default/CodeFolding/Prefs.js
+ //transform the folds into objects with from and to properties
+ var ranges = {}, obj;
+ Object.keys(folds).forEach(function (line) {
+ obj = folds[line];
+ ranges[line] = {from: {line: obj[0][0], ch: obj[0][1]}, to: {line: obj[1][0], ch: obj[1][1]}};
+ });
+
+ return ranges;
+ }
+
+ /**
+ Gets the line folds saved for the specified path.
+ @param {string} path the document path
+ @returns {number: {from: {line, ch}, to: {line, ch}}} the line folds for the document at the specified path
+ */
+ function get(path) {
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Could we rename this get and set to getFolds and setFolds? It improves intent and readability. :)

@thehogfather
thehogfather Apr 6, 2015 Contributor

Sure - I agree.

@MiguelCastillo MiguelCastillo and 1 other commented on an outdated diff Apr 6, 2015
...xtensions/default/CodeFolding/foldhelpers/foldcode.js
+ return widget;
+ }
+
+ var range = getRange(true);
+ if (options.scanUp) {
+ while (!range && pos.line > cm.firstLine()) {
+ pos = CodeMirror.Pos(pos.line - 1, 0);
+ range = getRange(false);
+ }
+ }
+ if (!range || range.cleared || force === "unfold" || range.to.line - range.from.line < minSize) {
+ if (range) { range.cleared = false; }
+ return;
+ }
+
+ var myWidget = makeWidget();
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Convention in Brackets is that variables are declared/defined at the beginning of the method scoping them.

@peterflynn
peterflynn Apr 8, 2015 Member

@MiguelCastillo Fwiw, not necessarily true. Because of JSLint, it's safe (often actually safer) to declare vars further down in a function at the point when they're initialized. We do have plenty of code in core where vars are not all grouped at the top of the scope.

@MiguelCastillo
MiguelCastillo Apr 8, 2015 Member

Hmmm, the only reason I say this is because JSLint would complain if your vars werent declared at the top of your functions. I am guessing this changed. Good to know though. :)

@peterflynn
peterflynn Apr 8, 2015 Member

We use the vars JSLint option, which lets you declare vars anywhere in the scope -- as long as they're not duplicated or referenced before the declaration. It's sort of a best of both worlds, because it effectively makes var declarations work more like Java or C++ (assuming you don't ignore lint errors).

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 6, 2015
...ensions/default/CodeFolding/foldhelpers/foldgutter.js
+ return elt;
+ }
+
+ /**
+ Updates the gutter markers for the specified range
+ @param {!CodeMirror} cm the CodeMirror instance for the active editor
+ @param {!number} from the starting line for the update
+ @param {!number} to the ending line for the update
+ */
+ function updateFoldInfo(cm, from, to) {
+ var minFoldSize = prefs.getSetting("minFoldSize") || 2;
+ var opts = cm.state.foldGutter.options;
+ var fade = prefs.getSetting("fadeFoldButtons");
+ var gutter = $(cm.getGutterElement());
+ var i = from;
+ var isFold = function (m) {
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Can we expand isFold and clear function expressions to function declarations? I havent spotted more, but function expressions aren't really used in Brackets much.

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 6, 2015
...ensions/default/CodeFolding/foldhelpers/foldgutter.js
+ function updateFoldInfo(cm, from, to) {
+ var minFoldSize = prefs.getSetting("minFoldSize") || 2;
+ var opts = cm.state.foldGutter.options;
+ var fade = prefs.getSetting("fadeFoldButtons");
+ var gutter = $(cm.getGutterElement());
+ var i = from;
+ var isFold = function (m) {
+ return m.__isFold;
+ }, clear = function (m) {return m.clear(); };
+
+ /**
+ helper function to check if the given line is in a folded region in the editor.
+ @param {Number} line the
+ @return {from: {ch: Number, line: Number}, to: {ch: Number, line: Number}} the range that hides the specified line or undefine if the line is not hidden
+ */
+ function _isCurrentlyFolded(line) {
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Functions that start with _ are conventionally private... Can we add @private to the jsdoc?

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 6, 2015
...ensions/default/CodeFolding/foldhelpers/foldgutter.js
+ var minFoldSize = prefs.getSetting("minFoldSize") || 2;
+ var opts = cm.state.foldGutter.options;
+ var fade = prefs.getSetting("fadeFoldButtons");
+ var gutter = $(cm.getGutterElement());
+ var i = from;
+ var isFold = function (m) {
+ return m.__isFold;
+ }, clear = function (m) {return m.clear(); };
+
+ /**
+ helper function to check if the given line is in a folded region in the editor.
+ @param {Number} line the
+ @return {from: {ch: Number, line: Number}, to: {ch: Number, line: Number}} the range that hides the specified line or undefine if the line is not hidden
+ */
+ function _isCurrentlyFolded(line) {
+ var keys = Object.keys(cm._lineFolds), i = 0, r;
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Can we please name r and i to a little more descriptive name? It helps us mere humans read the code a bit better :)

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 6, 2015
...ensions/default/CodeFolding/foldhelpers/foldgutter.js
+ }
+ } else {
+ if (range && range.to.line - range.from.line >= minFoldSize) {
+ mark = marker(opts.indicatorOpen);
+ }
+ }
+ }
+ cm.setGutterMarker(i, opts.gutter, mark);
+ i++;
+ }
+ }
+ }
+
+ function updateInViewport(cm, from, to) {
+ var vp = cm.getViewport(), state = cm.state.foldGutter;
+ from = !isNaN(from) ? from : vp.from;
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

NIT: double negation tends to be harder to read.

from = isNaN(from) ? vp.from : from;
to = isNan(to) ? vp.to : to;
@MiguelCastillo MiguelCastillo commented on the diff Apr 6, 2015
...ensions/default/CodeFolding/foldhelpers/foldgutter.js
+ var folds = cm.getValidFolds(cm._lineFolds);
+ cm._lineFolds = folds;
+ Object.keys(folds).forEach(function (line) {
+ cm.foldCode(+line);
+ });
+ } else {
+ var state = cm.state.foldGutter;
+ var lineChanges = changeObj.text.length - changeObj.removed.length;
+ //update the lineFolds cache
+ updateFoldsCache(cm, changeObj.from.line, lineChanges);
+ if (lineChanges !== 0) {
+ updateFoldInfo(cm, changeObj.from.line + lineChanges, changeObj.from.line + lineChanges + 1);
+ }
+ state.from = changeObj.from.line;
+ state.to = 0;
+ window.clearTimeout(state.changeUpdate);
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

If we use lodash we can clean this setting/clearing timeout with the debounce function. Very nifty little function. https://lodash.com/docs#debounce

@peterflynn
peterflynn Apr 8, 2015 Member

@MiguelCastillo Though I wonder if it'd be better to try to keep this as close as possible to the original CM code, with the goal of getting our few changes merged back upstream so we don't have to maintain a fork of it...

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 6, 2015
...ensions/default/CodeFolding/foldhelpers/foldgutter.js
+ state.from = changeObj.from.line;
+ state.to = 0;
+ window.clearTimeout(state.changeUpdate);
+ state.changeUpdate = window.setTimeout(function () {
+ updateInViewport(cm);
+ }, prefs.getSetting("foldOnChangeTimeSpan") || 600);
+ }
+ }
+
+ /**
+ Triggered on viewport changes e.g., user scrolls or resizes the viewport.
+ @param {!CodeMirror} cm the CodeMirror instance for the active editor
+ */
+ function onViewportChange(cm) {
+ var state = cm.state.foldGutter;
+ window.clearTimeout(state.changeUpdate);
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Same debounce suggestion as above

@MiguelCastillo MiguelCastillo commented on the diff Apr 6, 2015
src/extensions/default/CodeFolding/main.js
+ },
+ locale: brackets.getLocale()
+});
+
+define(function (require, exports, module) {
+ "use strict";
+ var CodeMirror = brackets.getModule("thirdparty/CodeMirror2/lib/codemirror"),
+ Strings = brackets.getModule("strings"),
+ AppInit = brackets.getModule("utils/AppInit"),
+ CommandManager = brackets.getModule("command/CommandManager"),
+ DocumentManager = brackets.getModule("document/DocumentManager"),
+ EditorManager = brackets.getModule("editor/EditorManager"),
+ ProjectManager = brackets.getModule("project/ProjectManager"),
+ KeyBindingManager = brackets.getModule("command/KeyBindingManager"),
+ ExtensionUtils = brackets.getModule("utils/ExtensionUtils"),
+ Menus = brackets.getModule("command/Menus"),
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

NIT: A little indentation detail

@MiguelCastillo MiguelCastillo commented on the diff Apr 6, 2015
src/extensions/default/CodeFolding/main.js
+ EXPAND = "codefolding.expand",
+ EXPAND_ALL = "codefolding.expand.all",
+ gutterName = "CodeMirror-foldgutter",
+ COLLAPSE_CUSTOM_REGIONS = "codefolding.collapse.customregions";
+
+ ExtensionUtils.loadStyleSheet(module, "main.less");
+
+ //load code mirror addons
+ brackets.getModule(["thirdparty/CodeMirror2/addon/fold/brace-fold"]);
+ brackets.getModule(["thirdparty/CodeMirror2/addon/fold/comment-fold"]);
+ brackets.getModule(["thirdparty/CodeMirror2/addon/fold/markdown-fold"]);
+
+ //still using slightly modified versions of the foldcode.js and foldgutter.js since we
+ //need to modify the gutter click handler to take care of some collapse and expand features
+ //e.g. collapsing all children when 'alt' key is pressed
+ var foldGutter = require("foldhelpers/foldgutter"),
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Move these require statements along with the other one in line 51.

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 6, 2015
src/extensions/default/CodeFolding/main.js
+ }
+ if (previous) {
+ saveLineFolds(previous);
+ }
+ }
+ }
+
+ function saveBeforeClose() {
+ saveLineFolds(EditorManager.getCurrentFullEditor());
+ }
+
+ /**
+ Initialise the extension
+ */
+ function init() {
+ if (!CodeMirror.fold.combine && _prefs.getSetting("enabled")) {
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

In the name of readability, can we make this if statement be an early return? This is merely to get rid of the deep indentation in the body of the if statement.

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 6, 2015
src/extensions/default/CodeFolding/main.js
+ locale: brackets.getLocale()
+});
+
+define(function (require, exports, module) {
+ "use strict";
+ var CodeMirror = brackets.getModule("thirdparty/CodeMirror2/lib/codemirror"),
+ Strings = brackets.getModule("strings"),
+ AppInit = brackets.getModule("utils/AppInit"),
+ CommandManager = brackets.getModule("command/CommandManager"),
+ DocumentManager = brackets.getModule("document/DocumentManager"),
+ EditorManager = brackets.getModule("editor/EditorManager"),
+ ProjectManager = brackets.getModule("project/ProjectManager"),
+ KeyBindingManager = brackets.getModule("command/KeyBindingManager"),
+ ExtensionUtils = brackets.getModule("utils/ExtensionUtils"),
+ Menus = brackets.getModule("command/Menus"),
+ _prefs = require("Prefs"),
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Why underscore this _prefs but not in an other file/module?

@MiguelCastillo MiguelCastillo commented on an outdated diff Apr 6, 2015
src/extensions/default/CodeFolding/main.js
+ }
+
+ /**
+ Expands all folded regions in the current document
+ */
+ function expandAll() {
+ var editor = EditorManager.getFocusedEditor();
+ if (editor && editor._codeMirror) {
+ var cm = editor._codeMirror;
+ CodeMirror.commands.unfoldAll(cm);
+ }
+ }
+
+ function createGutter(editor) {
+ var cm = editor._codeMirror;
+ if (cm) {
@MiguelCastillo
MiguelCastillo Apr 6, 2015 Member

Can we also do an early return here?

@MiguelCastillo
Member

@thehogfather this code is definitely landable right now and it is looking very good!

There are a few changes that seem manageable to make the code fit better with brackets style. I think we are almost there :)

thehogfather added some commits Apr 6, 2015
@thehogfather thehogfather updated JSDocs and general code clean up. a366170
@thehogfather thehogfather Merge commit 'fd6c8acca823e3cf197e6d3ac0ab9adea190843e' into codefolding
* commit 'fd6c8acca823e3cf197e6d3ac0ab9adea190843e': (31 commits)
  Added a newline
  Adding newline
  Updating the comments
  Updating the comments
  Removing this preference as it should be set to true by default.
  Changing function name.
  Decreasing timeout to 4 sec.
  Adding unit test cases. Submitting changes suggested in PR review.
  Setting ONE_DAY to correct value. Initially value used for testing was left by mistake. Removing serverTestURL as it is not needed now.
  Moving lastTimeSentData before making AJAX call to make sure data is not sent before ONE_DAY as setting it to always() will be asynchronous
  Move the implementation to the extensions. Data will be send to the server as soon as user has launched the Brackets and dialog box prompting for user to opt-in and opt-out is closed. Rework the comments and defects. Test cases has not been added yet. Will be adding to the next PR.
  Updated by ALF automation.
  Updated by ALF automation.
  Extended {APP_NAME}
  Correcting jshint errors.
  Deleting duplicate string.
  Synchronized with latest root commit
  "developer" -> "pengembang"
  Fixed text wrapping
  Add comments..
  ...

Conflicts:
	src/nls/root/strings.js
16c4483
@thehogfather
Contributor

@MiguelCastillo just pushed an update for all those fixes + merged with the latest commit from master.

@abose abose merged commit 16c4483 into adobe:master Apr 7, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abose abose added a commit that referenced this pull request Apr 7, 2015
@abose abose Merge Code Folding pull request 10792
Merge pull request #10792
Code Folding by Patrick Oladimeji; Github:@thehogfather
d6262f1
@abose
Contributor
abose commented Apr 7, 2015

@thehogfather Thanks for the help. Merged into master.
Any suggestion on the unit tests that needs to be added?

@MiguelCastillo
Member

@thehogfather yeah man, your last changes look very good. Thank you, this is a great addition to brackets! Now you gotta come up with another really cool extension :)

@abose I really like the idea of adding the package.json for attribution :)

@le717
Contributor
le717 commented Apr 7, 2015

Dancing banana

Congrats on the merge, @thehogfather! 😄

@thehogfather
Contributor

👍 thanks all. Glad I could help.

@abose I guess I would mainly check that folded states are persisted and restored correctly, and that the preference options (enabled, minFoldSize etc.) work as intended.

@peterflynn peterflynn commented on the diff Apr 8, 2015
...xtensions/default/CodeFolding/foldhelpers/foldcode.js
@@ -0,0 +1,268 @@
+/**
+ * Based on http://codemirror.net/addon/fold/foldcode.js
+ * @author Patrick Oladimeji
@peterflynn
peterflynn Apr 8, 2015 Member

I think we still need to keep the original copyright from CodeMirror here

@peterflynn peterflynn commented on the diff Apr 8, 2015
src/extensions/default/CodeFolding/main.js
+*/
+/**
+ * Code folding extension for brackets
+ * @author Patrick Oladimeji
+ * @date 10/24/13 9:35:26 AM
+ */
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, require, $, brackets*/
+
+require.config({
+ paths: {
+ "text" : "lib/text",
+ "i18n" : "lib/i18n"
+ },
+ locale: brackets.getLocale()
+});
@peterflynn
peterflynn Apr 8, 2015 Member

This should be unnecessary now that this is a part of core

@peterflynn peterflynn commented on the diff Apr 8, 2015
src/extensions/default/CodeFolding/strings.js
+
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define */
+
+/**
+ * This file provides the interface to user visible strings in Brackets. Code that needs
+ * to display strings should should load this module by calling var Strings = require("strings").
+ * The i18n plugin will dynamically load the strings for the right locale and populate
+ * the exports variable. See src\nls\strings.js for the master file of English strings.
+ */
+define(function (require, exports, module) {
+ "use strict";
+
+ module.exports = require("i18n!nls/strings");
+
+});
@peterflynn
peterflynn Apr 8, 2015 Member

Ditto -- I think this whole file is no longer used

@peterflynn peterflynn commented on the diff Apr 8, 2015
...ensions/default/CodeFolding/foldhelpers/latex-fold.js
@@ -0,0 +1,59 @@
+/**
+ * Fold latex code regions
@peterflynn
peterflynn Apr 8, 2015 Member

I wonder if a better approach would be to make this more extensible -- an important goal of the LanguageManager work is to make it so any feature in Brackets can be enabled for additional languages just by installing an extension. Latex and the custom regions feature both seem like potential candidates to move out into extensions. (Especially since the Latex CM mode isn't even turned on in Brackets core...)

Custom regions also adds a menu item that I suspect few users will understand, and that's concerning to me. Since refactoring this to be more extensible is probably a post-1.3 thing, I wonder if it'd be better to just disable the custom regions feature for now? Or have the menu item hidden by default, and accessible only by setting a preference?

@thehogfather
thehogfather Apr 8, 2015 Contributor

@peterflynn I agree - and I've already tested moving the latex-fold function into the latex extension and that is works as expected. They can be safely removed from the main.js file (if you haven't sealed changes to 1.3 yet).
I also think the custom region should be disabled by default (and the menu option hidden).

@peterflynn peterflynn commented on the diff Apr 8, 2015
src/extensions/default/CodeFolding/Prefs.js
+ * Gets the line folds saved for the specified path.
+ * @param {string} path the document path
+ * @return {Object} the line folds for the document at the specified path
+ */
+ function getFolds(path) {
+ store = (prefs.get(foldsKey) || {});
+ return inflate(store[path]);
+ }
+
+ /**
+ * Saves the line folds for the specified path
+ * @param {!string} path the path to the document
+ * @param {Object} folds the fold ranges to save for the current document
+ */
+ function setFolds(path, folds) {
+ store[path] = simplify(folds);
@peterflynn
peterflynn Apr 8, 2015 Member

This really should be in view state and not the main preferences file -- it's exactly the kind of thing view state was intended for. This seems like something we should fix for 1.3 lest we start polluting everyone's preferences file (which should be easily human-readable) with folding state.

@peterflynn
peterflynn Apr 8, 2015 Member

Filed as #10846 to make sure we track this

@abose
abose Apr 8, 2015 Contributor

@thehogfather #10846 Instead of using the prefs , could we migrate the folding state to state.json?

@thehogfather
thehogfather Apr 8, 2015 Contributor

@abose yes - for the folds. I suspect the other settings would need to be in the regular prefs (brackets.json).

@abose
abose Apr 8, 2015 Contributor

@thehogfather could use PreferencesManager.getViewState("codefolding.states"); and PreferencesManager.setViewState("codefolding.states", state) to put to the state.json file.

@peterflynn peterflynn commented on the diff Apr 8, 2015
src/extensions/default/CodeFolding/Prefs.js
+ }
+
+ /**
+ * Get the code folding setting with the specified key from the store
+ * @param {!string} key The key for the setting to retrieve
+ * @return {string} the setting with the specified key
+ */
+ function getSetting(key) {
+ return prefs.get(key);
+ }
+
+ /**
+ * Clears all the saved line folds for all documents.
+ */
+ function clearAllFolds() {
+ prefs.set(foldsKey, {});
@peterflynn
peterflynn Apr 8, 2015 Member

This function appears to be unused, but I think it also has a bug: this doesn't reset the store var used elsewhere, so any call to setFolds() later seems like it would immediately restore all the folds that had been cleared previously.

It's probably safest just to re-get the pref before setting it each time. There may be other bugs lurking with the current approach too (e.g. if you edit the prefs file... does it blow those changes away?)

@thehogfather
thehogfather Apr 8, 2015 Contributor

@peterflynn yes editing the prefs file for documents that are currently open in brackets (has no effect - the changes are restored one the document becomes the active editor). This is a bug - that your safe approach should fix.

@peterflynn peterflynn commented on the diff Apr 8, 2015
src/extensions/default/CodeFolding/Prefs.js
+ strings = brackets.getModule("strings"),
+ store = {},
+ foldsKey = "folds";
+
+ //default preference values
+ prefs.definePreference("enabled", "boolean", true,
+ {name: strings.ENABLE_CODE_FOLDING, description: strings.ENABLE_CODE_FOLDING});
+ prefs.definePreference("minFoldSize", "number", 2,
+ {name: strings.MIN_FOLD_SIZE, description: strings.MIN_FOLD_SIZE_HELP});
+ prefs.definePreference("saveFoldStates", "boolean", true,
+ {name: strings.SAVE_FOLD_STATES, description: strings.SAVE_FOLD_STATES_HELP});
+ prefs.definePreference("alwaysUseIndentFold", "boolean", true,
+ {name: strings.ALWAYS_USE_INDENT_FOLD, description: strings.ALWAYS_USE_INDENT_FOLD_HELP});
+ prefs.definePreference("enableRegionFolding", "boolean", true,
+ {name: strings.ENABLE_REGION_FOLDING, description: strings.ENABLE_REGION_FOLDING});
+ prefs.definePreference("fadeFoldButtons", "boolean", false,
@peterflynn
peterflynn Apr 8, 2015 Member

I wonder if this preference should be on by default.

I can't actually get it to work though -- when I set it to true, it seems to have no effect (at least on JS or LESS files). Will file a bug.

@peterflynn
peterflynn Apr 8, 2015 Member

Ok, false alarm with the preferences issue.

I wonder if the best UX is actually in between this setting (hidden until mouseover) and the default (always visible) -- having the triangles be fainter until you mouse over the gutter, then getting darker. I may mock that up and see how it feels...

@peterflynn peterflynn commented on the diff Apr 8, 2015
src/extensions/default/CodeFolding/Prefs.js
+ "use strict";
+ var PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
+ prefs = PreferencesManager.getExtensionPrefs("code-folding"),
+ strings = brackets.getModule("strings"),
+ store = {},
+ foldsKey = "folds";
+
+ //default preference values
+ prefs.definePreference("enabled", "boolean", true,
+ {name: strings.ENABLE_CODE_FOLDING, description: strings.ENABLE_CODE_FOLDING});
+ prefs.definePreference("minFoldSize", "number", 2,
+ {name: strings.MIN_FOLD_SIZE, description: strings.MIN_FOLD_SIZE_HELP});
+ prefs.definePreference("saveFoldStates", "boolean", true,
+ {name: strings.SAVE_FOLD_STATES, description: strings.SAVE_FOLD_STATES_HELP});
+ prefs.definePreference("alwaysUseIndentFold", "boolean", true,
+ {name: strings.ALWAYS_USE_INDENT_FOLD, description: strings.ALWAYS_USE_INDENT_FOLD_HELP});
@peterflynn
peterflynn Apr 8, 2015 Member

What does it mean to have this enabled? What is the behavior e.g. in a JS file if this conflicts with the standard brace-fold/comment-fold helpers?

@thehogfather
thehogfather Apr 8, 2015 Contributor

It means that line indentation is used as a suggestion for fold regions. Although this shouldn't take priority over range finders based on the current mode. So where there is conflict, the brace-fold-finder should take precedence. Having said that this might be another pref that could be better off disabled by default.

@peterflynn peterflynn commented on the diff Apr 8, 2015
src/extensions/default/CodeFolding/main.js
+ }
+ }
+ }
+
+ /**
+ * Saves the line folds in the current full editor before it is closed.
+ */
+ function saveBeforeClose() {
+ saveLineFolds(EditorManager.getCurrentFullEditor());
+ }
+
+ /**
+ Initialise the extension
+ */
+ function init() {
+ if (CodeMirror.fold.combine || !prefs.getSetting("enabled")) {
@peterflynn
peterflynn Apr 8, 2015 Member

When would CodeMirror.fold.combine be true?

@thehogfather
thehogfather Apr 8, 2015 Contributor

It would be truthy if the code-folding plugin is already loaded. This is mainly for the extension in the registry- to prevent registering key bindings twice etc. I could not find an api to check that an extension has loaded. That code fragment can be removed without affecting the logic of initialisation.

@peterflynn
peterflynn Apr 8, 2015 Member

Ah, in that case -- core extensions are always loaded before user-installed extensions, so I think this logic is only needed in the registry copy, not here.

@abose
Contributor
abose commented Apr 8, 2015

@thehogfather I was verifying the unit tests and some of the integration tests in the unit test setup is failing.
Most is with this message- Cannot read property 'lastIndexOf' of undefined
Could you take a look at debug>run tests>integration>all .

@abose
Contributor
abose commented Apr 8, 2015

Code folding is enabled in all files by default, and in some files like txt files it looks a bit odd.
Would it be a good thing to enable code folding in only HTML,XML,js Etc. and disabled in all other file types?

@thehogfather
Contributor

@abose wrt unit tests, I am unable to run them completely. They freeze with about 125 tests to go. Can you tell me the file throwing the error or show a stack trace? I remember spotting one a while back in one of the imported codeMirror range finders.

@abose
Contributor
abose commented Apr 9, 2015

There is a memory issue with the unit tests currently, so we won't be able to run all tests in a single go.
So in order to run it, we have to the tests in individual sections.
First restart brackets; then open test suite, click on integration tab, click on all; it would start running integration tests. If brackets slows down, Restart brackets again to run tests.
In integration test, specifically the following test are failing[ FindInFiles(3 test cases),FindReplace - Integration(1), InlineEditorProviders(3), QuickOpen(3)].
I am checking out these errors and the stack.

But Issue#10857 seems to be much more important than this. My understanding of the extension is limited; it might take a while for me to fix it. Could you take a look at the issue?

@peterflynn
Member

@abose I took a quick look at those unit tests and it seems like the Health Report notification dialog might be causing problems in some of them (you can see it popping up in each test run, even though there's code that should suppress it).

The 3 QuickOpen failures might be different -- I can repro them on my old smart-autocompelte branch (which doesn't have code folding or Health Report), so those failures might be caused by my recent PR there. I can try to take a look tomorrow -- lmk if you find anything out before then.

@abose
Contributor
abose commented Apr 9, 2015

The health notifications dialogs will be disabled when unit tests are nunning, but i guess that PR is still under review.
For the time being, it can be disabled by going to HealthDataNotification.js > AppInit.appReady()>
params.parse();if (params.get("testEnvironment")) return;

@redmunds
Contributor
redmunds commented Apr 9, 2015

There's a wiki page with Tips and Tricks for Running Brackets Unit Tests so just add any new tips there and share this link.

@Pooria-H

Why there is no option to disable this feature? It's ugly.

@le717
Contributor
le717 commented May 28, 2015

@Pooria-H There is an option. In your brackets.json file, set code-folding.enabled to false. You can get more details on the wiki.

@ryanstewart
Member

Thanks @le717 !

@Pooria-H

@le717 Oh I didn't noticed. thanks

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