Opening preferences brackets.json in split view along with default preferences #11307

nethip · 2015-06-24T14:40:02Z

With this PR, upon choosing Debug > Open Preferences file, preferences file brackets.json will open in the second pane of split view while the default preferences will load in the left-pane. The default preferences are generated on the fly by iterating through all valid preferences and writing into the default preferences file, defaultSettings.json created inside brackets preferences folder. Here is the work card for the same

This is the sample screenshot of how the split view looks like.

I am going to add unit tests for these. Meanwhile I would like to get some eyes on the changes and also make some code changes based on the review comments.

@sprintr @marcelgerber @abose Please review this when you get a chance.Thanks!

cc @ryanstewart

Pending Tasks

~~Add unit tests~~: Adding unit tests to this PR is not straight forward as we are using shell apis for this PR and they are not working with our unit test framework. I have added a waffle card to track this specific task.

…the default settings preferences file"

…e left pane and user preferences file brackets.json in the right pane. Also moved the code from PreferencesManager to DebugCommands extension.

…f code

…xisting instances of defaultPreferences.json before deleting the existing instance of defaultPreferences.json."

…riable.

…false"

marcelgerber · 2015-06-24T14:50:02Z

src/extensions/default/DebugCommands/main.js

@@ -48,6 +51,12 @@ define(function (require, exports, module) {

    var KeyboardPrefs = JSON.parse(require("text!keyboard.json"));

+    // default preferences file name
+    var DEFAULT_SETTINGS_FILENAME = "defaultSettings.json";


We really only use the term "preferences", so we should do so here, too.

Sure! will do this change.

marcelgerber · 2015-06-24T15:05:49Z

When I just tried this branch, I noticed that Debug > Open preferences file no-ops when you're not in Split View and none of the files is already open.

nethip · 2015-06-24T15:48:39Z

@marcelgerber That was quick 😄 Thanks for starting to test this! I just checked. My bad. I pushed some bad change in one of the last commits that is causing this. For now to see what it looks like, be in the split view and then try Debug > Open Preferences File. I will check the code and push in a fix. Thanks!

… Changed typeof(var) == "undefined" var === undefined. And changed defaultSettings.json to defaultPreferences.json

nethip · 2015-06-24T18:58:56Z

@marcelgerber I have addressed the code review comments and updated the PR. Also I have fixed the bug with opening defaultPreferences and brackets.json in split view. Could you take a look at it and let me know if anything else needs to be changed?

sprintr · 2015-06-24T19:42:57Z

src/extensions/default/DebugCommands/main.js

+                prefType === "boolean" ||
+                prefType === "string"  ||
+                prefType === "array"   ||
+                prefType === "object") {


I think we should use an array of supported preference types here and just check whether or not prefType is in it.

Sure! will do that.

…e keys in a sorted order. Also added a new preference to give an opportunity to people to either display the user preferences on the right side or the left.

nethip · 2015-07-06T10:34:15Z

@sprintr I have changed the for() to forEach(). Also now I am sorting the keys.
@mackenza This PR is now updated with a new preference to display the preferences either on the right side or the left side. Please check it out and let me know if this looks fine.

@marcelgerber Let me know if the changes look good to you. I could then rebase this entire branch to a very limited no of commits before we merge this to master.

mackenza · 2015-07-06T16:52:36Z

@nethip I get an error trying to open up the split view with your last changes:

Console says:
Uncaught TypeError: Cannot read property 'removeView' of null in view/MainViewManager.js:810

sprintr · 2015-07-06T19:52:27Z

@nethip I am also facing the issue @mackenza reported above, so am not able to try it out. You may add default values of closeTags prefs as well here like you did for jslint.options

…t in with the preference change that allows users to specifiy the pane in which the user preferences need to be open. Also added default values to closeTag attribute.

nethip · 2015-07-06T23:55:51Z

@mackenza @sprintr Thanks a lot guys for trying out this PR and letting me know about the issue. This kind of crept in with the addition of the new preference to display the user preferences either in the first-pane or second-pane. I now have updated my branch with the fix. Also I have added default values of closeTags. Could you take the latest changes and checkout the feature and let me know if you guys see anything fishy. I really appreciate you guys taking some time in trying this feature out. Thanks!

sprintr · 2015-07-07T00:06:51Z

@nethip 🆒 Works just as expected. 👍 for making the new layout default. The opening/closing/restoring layout scheme issue can be skipped for the moment, it looks more like an extension idea.

nethip · 2015-07-09T06:26:16Z

Since this needs to make into 1.4 we would want to send the strings for translations. I would like to know if there any parts of this PR that needs to be worked on. Or are we good to go with this change, in which case I will go ahead and merge this change.

@marcelgerber @sprintr @abose @mackenza @

rroshan1 · 2015-07-10T11:23:53Z

src/editor/Editor.js

@@ -279,9 +283,11 @@ define(function (require, exports, module) {
     * @param {{startLine: number, endLine: number}=} range If specified, range of lines within the document
     *          to display in this editor. Inclusive.
     */


Please add "@ param options" in comments for the function.

rroshan1 · 2015-07-10T12:31:15Z

Currently, the comments at the beginning of defaultPreferences.json looks untidy when opened in split view,

I would suggest a cleaner

rroshan1 · 2015-07-10T12:45:49Z

@nethip Everything else looks good from my side. 👍
I have run unit tests and scenario tests and all have passed.
I can merge it once the comments are addressed.

mackenza · 2015-07-10T13:03:44Z

src/nls/root/strings.js

-    "DESCRIPTION_FONT_SIZE"                          : "Change font size; e.g, 13px"
+    "DESCRIPTION_FONT_SIZE"                          : "Change font size; e.g, 13px",
+    "DESCRIPTION_OPEN_PREFS_IN_SPLIT_VIEW"           : "False to disable opening preferences file in split view",
+    "DESCRIPTION_OPEN_DEFULT_PREFS_IN_FIRST_PANE"     : "False to open preferences in right pane/bottom pane",


spelling error... should be DESCRIPTION_OPEN_DEFAULT_PREFS_IN_FIRST_PANE

mackenza · 2015-07-10T14:27:46Z

@nethip I am really not crazy about the preference name for whether the editable window is left or right.

alwaysOpenUserPrefsinSecondPane is a real mouthful and semantically questionable. A simple fix might be to remove the "always". It's not needed. We don't have it in the other preference related to this: openPrefsInSplitView so I feel it's redundant.

So, in the interest of getting this feature merged, I propose we simply go with:

openUserPrefsInSecondPane (notice I also camel-cased the I in In

mackenza · 2015-07-10T14:30:44Z

src/nls/root/strings.js

+    "DESCRIPTION_FONT_SIZE"                          : "Change font size; e.g, 13px",
+    "DESCRIPTION_OPEN_PREFS_IN_SPLIT_VIEW"           : "False to disable opening preferences file in split view",
+    "DESCRIPTION_OPEN_DEFULT_PREFS_IN_FIRST_PANE"     : "False to open preferences in right pane/bottom pane",
+    "DEFAULT_PREFERENCES_JSON_HEADER_COMMENT"        : "/*\n * This is a read-only file with the preferences supported by brackets. \n * Use this as a reference to modify your own preferences file(brackets.json)\n * opened in the right-hand pane. \n * Refer to https://github.com/adobe/brackets/wiki/How-to-Use-Brackets#preferences \n * for more information on how to use preferences inside Brackets.\n */",


now that we have a preference for which pane the default prefs opens in, we cannot say opened in the right-hand pane. I suggest:

opened in the other pane.

@mackenza

…eview comments from @mackenza and @rroshan1

nethip · 2015-07-10T16:17:52Z

@mackenza I agree with you on changing the preference name. Just did it. Also changed the description variable name along with other string changes.

@rroshan1 I incorporated your changes as well.

marcelgerber · 2015-07-10T16:26:49Z

There's still a merge conflict.

nethip · 2015-07-10T16:43:51Z

@marcelgerber I am on it. Looking into the merge conflict!

nethip · 2015-07-10T16:51:35Z

@marcelgerber I have created another PR which is basically a rebase of this PR, as this PR has some 30 odd commits. Would you mind having a quick look and merging it. Thanks!

nethip · 2015-07-10T17:17:37Z

@marcelgerber Here is the new PR with the rebased commits.

nethip · 2015-07-10T18:24:05Z

Since there are other PRs that are dependent on this I have gone ahead and merged other PR. Please feel free to call out any specific pieces of code that need to be reworked on. I will make sure that is addressed.

A big thank you to @marcelgerber @sprintr @mackenza @abose @rroshan1 for providing valuable feedback because of which this PR is in a very good shape. Thanks!

nethip added 21 commits June 11, 2015 16:39

Open brackets.json in the second pane of the split view

8e2a5c3

Initial implementation for generating preferences file

c5de5d3

"Adding a read only mode to opening a file. We will use this to open …

b0bbb12

…the default settings preferences file"

Continuation of adding readOnly mode to Brackets

3afea16

This has the first cut which now opens default preferences file in th…

2810380

…e left pane and user preferences file brackets.json in the right pane. Also moved the code from PreferencesManager to DebugCommands extension.

Formatting object change

437652e

Added code to format nested object preferences. Also refactored lot o…

0d738b7

…f code

Code refactoring

c3a1e34

Adding a preference to enable/disable opening preferences in split view.

126b993

Improved object formatting

a133ed1

Fixed some unit testing defects

9e395d6

Merge branch 'master' into prashant/open-prefs-in-split-view

ae7aa26

Code cleanup and also added tests temporarily for unit tests

44548a3

"Fixed many unit testing defects as well as added code to close any e…

3aca4ec

…xisting instances of defaultPreferences.json before deleting the existing instance of defaultPreferences.json."

"Removing extra spaces across the changes made on this branch"

4a9df25

"More removals of white spaces"

4f391d3

"Changed the editor options from boolean to object"

b7c04e0

"Continuation of the older commit."

a466029

Fixed a unit testing defect caused by using the same editorOptions va…

8d8ea87

…riable.

"Changed the default values of some preferences default from true to …

0d92a2d

…false"

Merge branch 'master' into prashant/open-prefs-in-split-view

abb0f9e

marcelgerber reviewed Jun 24, 2015
View reviewed changes

"Fixed an issue which pops up when searching for default preferences.…

cbd9875

… Changed typeof(var) == "undefined" var === undefined. And changed defaultSettings.json to defaultPreferences.json

sprintr reviewed Jun 24, 2015
View reviewed changes

Changed for() to forEach() at two places and we are now generating th…

c5018f6

…e keys in a sorted order. Also added a new preference to give an opportunity to people to either display the user preferences on the right side or the left.

Fixing a unit test issue while opening default preferences. This crep…

e59bfdf

…t in with the preference change that allows users to specifiy the pane in which the user preferences need to be open. Also added default values to closeTag attribute.

rroshan1 reviewed Jul 10, 2015
View reviewed changes

mackenza reviewed Jul 10, 2015
View reviewed changes

nethip added 2 commits July 10, 2015 21:24

Some string changes along with preference name change got from code r…

7267458

…eview comments from @mackenza and @rroshan1

Adding documentation for additional parameters. Thanks to @rroshan1.

dba7ddd

nethip mentioned this pull request Jul 10, 2015

Opening preferences brackets.json in split view along with default preferences #11368

Merged

nethip mentioned this pull request Jul 10, 2015

Add unit tests to opening preferences files in split view. #11369

Open

nethip closed this Jul 10, 2015

nethip deleted the prashant/open-prefs-in-split-view branch May 31, 2016 06:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opening preferences brackets.json in split view along with default preferences #11307

Opening preferences brackets.json in split view along with default preferences #11307

nethip commented Jun 24, 2015

marcelgerber Jun 24, 2015

nethip Jun 24, 2015

marcelgerber commented Jun 24, 2015

nethip commented Jun 24, 2015

nethip commented Jun 24, 2015

sprintr Jun 24, 2015

nethip Jun 24, 2015

nethip commented Jul 6, 2015

mackenza commented Jul 6, 2015

sprintr commented Jul 6, 2015

nethip commented Jul 6, 2015

sprintr commented Jul 7, 2015

nethip commented Jul 9, 2015

rroshan1 Jul 10, 2015

rroshan1 commented Jul 10, 2015

rroshan1 commented Jul 10, 2015

mackenza Jul 10, 2015

mackenza commented Jul 10, 2015

mackenza Jul 10, 2015

nethip commented Jul 10, 2015

marcelgerber commented Jul 10, 2015

nethip commented Jul 10, 2015

nethip commented Jul 10, 2015

nethip commented Jul 10, 2015

nethip commented Jul 10, 2015

Opening preferences brackets.json in split view along with default preferences #11307

Opening preferences brackets.json in split view along with default preferences #11307

Conversation

nethip commented Jun 24, 2015

Pending Tasks

marcelgerber Jun 24, 2015

Choose a reason for hiding this comment

nethip Jun 24, 2015

Choose a reason for hiding this comment

marcelgerber commented Jun 24, 2015

nethip commented Jun 24, 2015

nethip commented Jun 24, 2015

sprintr Jun 24, 2015

Choose a reason for hiding this comment

nethip Jun 24, 2015

Choose a reason for hiding this comment

nethip commented Jul 6, 2015

mackenza commented Jul 6, 2015

sprintr commented Jul 6, 2015

nethip commented Jul 6, 2015

sprintr commented Jul 7, 2015

nethip commented Jul 9, 2015

rroshan1 Jul 10, 2015

Choose a reason for hiding this comment

rroshan1 commented Jul 10, 2015

rroshan1 commented Jul 10, 2015

mackenza Jul 10, 2015

Choose a reason for hiding this comment

mackenza commented Jul 10, 2015

mackenza Jul 10, 2015

Choose a reason for hiding this comment

nethip commented Jul 10, 2015

marcelgerber commented Jul 10, 2015

nethip commented Jul 10, 2015

nethip commented Jul 10, 2015

nethip commented Jul 10, 2015

nethip commented Jul 10, 2015