search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] integration of code-folding extension into brackets #9494

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by thehogfather Wednesday Mar 25, 2015 at 09:39 GMT Originally opened as https://github.com/adobe/brackets/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

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

core-ai-bot commented 3 years ago

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 commented 3 years ago

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

@abose sure thing.

core-ai-bot commented 3 years ago

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 commented 3 years ago

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

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

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

core-ai-bot commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

shortcuts Ctrl+Alt+[ and Ctrl+Alt+] :+1:

core-ai-bot commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

@thehogfather Will be merging this by tomorrow :+1: 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 commented 3 years ago

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

@abose no problem. Happy to help out anytime.

core-ai-bot commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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?