[CLOSED] integration of code-folding extension into brackets

[core-ai-bot]

Issue by thehogfather
Wednesday Mar 25, 2015 at 09:39 GMT
Originally opened as adobe/brackets#10792

---

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

---

thehogfather included the following code: https://github.com/adobe/brackets/pull/10792/commits

[core-ai-bot]

Comment by Florian-R
Wednesday Mar 25, 2015 at 09:47 GMT

---

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

[core-ai-bot]

Comment by prksingh
Wednesday Mar 25, 2015 at 09:58 GMT

---

@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

[core-ai-bot]

Comment by abose
Wednesday Mar 25, 2015 at 10:04 GMT

---

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

[core-ai-bot]

Comment by Florian-R
Wednesday Mar 25, 2015 at 10:09 GMT

---

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

[core-ai-bot]

Comment by thehogfather
Wednesday Mar 25, 2015 at 14:32 GMT

---

@abose sure thing.

[core-ai-bot]

Comment by ryanstewart
Wednesday Mar 25, 2015 at 18:25 GMT

---

@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.

[core-ai-bot]

Comment by ryanstewart
Thursday Mar 26, 2015 at 04:26 GMT

---

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?

[core-ai-bot]

Comment by MiguelCastillo
Friday Mar 27, 2015 at 00:51 GMT

---

@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.

[core-ai-bot]

Comment by thehogfather
Friday Mar 27, 2015 at 23:36 GMT

---

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

[core-ai-bot]

Comment by MarcelGerber
Sunday Mar 29, 2015 at 18:48 GMT

---

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

[core-ai-bot]

Comment by thehogfather
Sunday Mar 29, 2015 at 22:35 GMT

---

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?

[core-ai-bot]

Comment by nethip
Monday Mar 30, 2015 at 06:00 GMT

---

@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.

[core-ai-bot]

Comment by abose
Monday Mar 30, 2015 at 19:24 GMT

---

@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

[core-ai-bot]

Comment by MarcelGerber
Monday Mar 30, 2015 at 20:40 GMT

---

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.

[core-ai-bot]

Comment by thehogfather
Tuesday Mar 31, 2015 at 00:37 GMT

---

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

[core-ai-bot]

Comment by abose
Tuesday Mar 31, 2015 at 12:40 GMT

---

@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).

[core-ai-bot]

Comment by thehogfather
Tuesday Mar 31, 2015 at 16:05 GMT

---

@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+]?

[core-ai-bot]

Comment by abose
Wednesday Apr 01, 2015 at 09:10 GMT

---

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

[core-ai-bot]

Comment by abose
Thursday Apr 02, 2015 at 06:58 GMT

---

@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?

[core-ai-bot]

Comment by abose
Thursday Apr 02, 2015 at 07:05 GMT

---

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?

[core-ai-bot]

Comment by thehogfather
Thursday Apr 02, 2015 at 12:01 GMT

---

@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.

[core-ai-bot]

Comment by le717
Friday Apr 03, 2015 at 23:02 GMT

---

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

[core-ai-bot]

Comment by abose
Saturday Apr 04, 2015 at 04:39 GMT

---

@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.

[core-ai-bot]

Comment by abose
Sunday Apr 05, 2015 at 10:32 GMT

---

@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.

[core-ai-bot]

Comment by thehogfather
Sunday Apr 05, 2015 at 12:57 GMT

---

@abose no problem. Happy to help out anytime.

[core-ai-bot]

Comment by MiguelCastillo
Sunday Apr 05, 2015 at 14:17 GMT

---

@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.

[core-ai-bot]

Comment by abose
Monday Apr 06, 2015 at 10:55 GMT

---

@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.

[core-ai-bot]

Comment by MiguelCastillo
Monday Apr 06, 2015 at 14:13 GMT

---

@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 :)

[core-ai-bot]

Comment by thehogfather
Monday Apr 06, 2015 at 15:19 GMT

---

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

[core-ai-bot]

Comment by abose
Tuesday Apr 07, 2015 at 07:38 GMT

---

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