Provide editor options "line numbers", "active line" and "word wrap" as ... #3097

RaymondLim · 2013-03-11T16:53:32Z

...new menu items under View menu.

…as new menu items under View menu.

…to rlim/word-wrap

Conflicts: src/editor/Editor.js

…to rlim/word-wrap

RaymondLim · 2013-03-11T20:49:55Z

src/styles/brackets_theme_default.less

@@ -101,6 +101,9 @@
 @selection-color-focused: #d2dcf8;
 @selection-color-unfocused: #d9d9d9;

+/* background color of the line that has the cursor */
+@activeline-bgcolor: #e8f2ff;


@GarthDB -- see if you have any comment on this color.

TomMalbran · 2013-03-13T20:41:36Z

Now that there is an EditorOptions file, auto close brackets could be moved there too, since is another Editor option command.

jasonsanjose · 2013-03-14T17:26:04Z

Reassigned to me. Raymond, please fix the merge conflicts. Thanks.

Conflicts: src/command/Commands.js src/nls/root/strings.js

RaymondLim · 2013-03-14T18:01:25Z

Done fixing merge conflicts. Ready for review.

jasonsanjose · 2013-03-14T20:28:53Z

Reviewing

jasonsanjose · 2013-03-14T20:32:58Z

I'm seeing 6 console errors when starting brackets on this branch. I cleaned my cache and removed all extensions to be sure:

Attempting to register a command with a missing name, id, or command function:undefined view.toggleLineNumbers command/CommandManager.js:182
Attempting to register a command with a missing name, id, or command function:undefined view.toggleActiveLine command/CommandManager.js:182
Cannot assign Cmd-G to navigate.gotoLine. It is already assigned to edit.findNext command/KeyBindingManager.js:405
addMenuItem(): commandID not found: view.toggleLineNumbers command/Menus.js:485
addMenuItem(): commandID not found: view.toggleActiveLine command/Menus.js:485
Exception when calling a 'brackets done loading' handler: 
TypeError: Cannot call method 'setChecked' of undefined
    at _init (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/editor/EditorOptionHandlers.js:75:58)
    at _callHandler (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/utils/AppInit.js:59:13)
    at Object._dispatchReady (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/utils/AppInit.js:74:13)
    at Object.<anonymous> (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/brackets.js:320:13)
    at Object.g.s.newContext.h.execCb (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:30:455)
    at Object.g.s.newContext.U.check (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:20:302)
    at Object.g.s.newContext.U.enable.c (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:24:331)
    at file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:7:527
    at g.s.newContext.U.emit (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:25:174)
    at t (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:7:178)
    at Object.g.s.newContext.U.emit (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:25:145) utils/AppInit.js:62

jasonsanjose · 2013-03-14T20:40:26Z

Ignore the edit.findNext key binding error. That's a known issue.

jasonsanjose · 2013-03-14T20:54:33Z

src/nls/root/strings.js

@@ -213,6 +213,7 @@ define({
    "CMD_RESTORE_FONT_SIZE"               : "Restore Font Size",
    "CMD_SCROLL_LINE_UP"                  : "Scroll Line Up",
    "CMD_SCROLL_LINE_DOWN"                : "Scroll Line Down",
+    "CMD_TOGGLE_WORD_WRAP"                : "Enable Word Wrap",


CMD_TOGGLE_LINE_NUMBERS and CMD_TOGGLE_ACTIVE_LINE are missing, causing the console errors I noted earlier.

Somehow I didn't get them in when resolving the conflicts in merging master to my branch.

jasonsanjose · 2013-03-14T21:12:32Z

I'm seeing 2 issues with word wrap that I've filed:

codemirror/codemirror5#1361 With word wrap enabled, new trailing spaces are not wrapped
codemirror/codemirror5#1362 With word wrap enabled, spaces inside a tag are not rendered

I haven't filed new tracking bugs in Brackets. @RaymondLim can you do that? Thanks.

jasonsanjose · 2013-03-14T21:14:24Z

src/editor/Editor.js

@@ -1373,6 +1385,60 @@ define(function (require, exports, module) {
        return _closeBrackets;
    };

+      /**


Fix indentation

Good catch! Not sure how I got this in accidentally.

jasonsanjose · 2013-03-14T21:26:23Z

I'm seeing failures in the new unit tests:

should NOT style active line after turning it off
should NOT style the active line when opening another document with show active line off

jasonsanjose · 2013-03-14T21:29:05Z

test/spec/EditorOptionHandlers-test.js

+            });
+
+            runs(function () {
+                var openAndSelect = function (path) {


openAndSelect matches line 176. I think you intended to refactor this into a higher scope and reuse it, right?

Oops. Not really I wanted to have it in a higher scope. I was just copying from other unit test and then duplicate the same code for testing active line.

RaymondLim · 2013-03-18T23:56:05Z

Ready for third round of review unless @redmunds says I need to do something with the default prefs.

jasonsanjose · 2013-03-19T17:50:24Z

I'm still not comfortable with not assigning new default preferences, whether it's during migration or during normal startup. I'll take a look at #3101 and see if we can fix the problem there before merging this pull. Sound like a plan @RaymondLim?

RaymondLim · 2013-03-19T18:37:46Z

I totally agree with you. I was shocked when I noticed this yesterday morning. So it is definitely not ok for our end users even if we tell them to delete preference cache.

redmunds · 2013-03-19T19:05:21Z

The preference should be reversed so undefined defaults to what was there before. So, change showLineNumbers to hideLineNumbers.

jasonsanjose · 2013-03-19T22:12:05Z

@RaymondLim, @TomMalbran's fix got merged. Looks like there's an conflict in Editor.js. I'm pretty sure the change should now be: PreferencesManager.getPreferenceStorage(PREFERENCES_CLIENT_ID, defaults);

TomMalbran · 2013-03-19T22:17:32Z

Yes, and it should also be PreferencesManager.handleClientIdChange(_prefs, "com.adobe.brackets.Editor");

RaymondLim · 2013-03-19T22:19:18Z

Thanks. I'll fix the conflict soon.

Conflicts: src/editor/Editor.js

TomMalbran · 2013-03-19T22:52:43Z

src/editor/Editor.js

-    var defaultPrefs = { useTabChar: false, tabSize: 4, indentUnit: 4, closeBrackets: false };
-
+        defaultPrefs = { useTabChar: false, tabSize: 4, indentUnit: 4, closeBrackets: false,
+                         showLineNumbers: true, styleActiveLine: true, wordWrap: true };


Missing the var now. Seems like this is what made the build fail.

Thanks. I was unable to launch and still looking for the culprit.

… conflict.

jasonsanjose · 2013-03-20T03:10:28Z

I'm testing preferences as follows

by clearing out my cache
running sprint 21 (using tools/restore_installed_build.sh) to create an initial set of prefs
quitting brackets
setup this branch (using tools/setup_for_hacking.sh)
launch brackets again

The prefs seem to work correctly, but I'm seeing an error in EditorOptionsHandler when launching:

TypeError: Cannot call method 'setChecked' of undefined
    at _init (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/editor/EditorOptionHandlers.js:78:64)
    at _callHandler (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/utils/AppInit.js:59:13)
    at Object._dispatchReady (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/utils/AppInit.js:74:13)
    at Object.<anonymous> (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/brackets.js:321:13)
    at Object.g.s.newContext.h.execCb (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:30:455)
    at Object.g.s.newContext.U.check (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:20:302)
    at Object.g.s.newContext.U.enable.c (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:24:331)
    at file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:7:527
    at g.s.newContext.U.emit (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:25:174)
    at t (file://localhost/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/require.js:7:178)

TomMalbran · 2013-03-20T04:00:01Z

src/editor/EditorOptionHandlers.js

+        CommandManager.get(Commands.TOGGLE_LINE_NUMBERS).setChecked(Editor.getShowLineNumbers());
+        CommandManager.get(Commands.TOGGLE_ACTIVE_LINE).setChecked(Editor.getShowActiveLine());
+        CommandManager.get(Commands.TOGGLE_WORD_WRAP).setChecked(Editor.getWordWrap());
+        CommandManager.get(Commands.CMD_TOGGLE_CLOSE_BRACKETS).setChecked(Editor.getCloseBrackets());


This should be Commands.TOGGLE_CLOSE_BRACKETS

RaymondLim · 2013-03-20T04:00:31Z

Good catch! It's my mistake when copying the string from other place.

jasonsanjose · 2013-03-20T16:41:24Z

@TomMalbran and @RaymondLim and maybe @redmunds, I think I found the issue with the preferences migration. See #3185.

This pull looks good, but I would like to merge #3185 and re-test before pulling this in.

jasonsanjose · 2013-03-20T21:24:33Z

Looks good. Scenario tested sprint 21 to sprint 22 and the preferences migrate properly and the new defaults are added as expected. Merging.

Provide editor options "line numbers", "active line" and "word wrap" as ...

TomMalbran · 2013-03-21T22:00:53Z

I saw that there are tests here for word wrap and line highlight. I was thinking that I could add tests for auto close brackets, but wasn't sure how much to test. Should be something like it should auto close when active only or more complex adding the backspace function and others?

RaymondLim · 2013-03-21T22:27:43Z

@TomMalbran -- Yes, you're welcome to add unit tests for auto close brackets. Start it with simple ones and then extend it to cover complex ones for backspace handling and not auto close when typing on or inside an existing word later.

TomMalbran · 2013-03-22T18:45:12Z

Working on them. I started adding tests for Toggle Line Numbers.

I have the problem of trying to mock a key input. Is there anyway to insert some text to an editor and make it treat it as a key input?

peterflynn · 2013-03-22T19:24:58Z

@TomMalbran: SpecRunnerUtils.simulateKeyEvent() may be of some help, although editor input is a tricky case. If you look at CodeHint-test there are several different approaches it uses in various tests.

TomMalbran · 2013-03-23T01:42:38Z

Great. Will look into that. Thanks

Provide editor options "line numbers", "active line" and "word wrap" …

72536cf

…as new menu items under View menu.

ghost assigned peterflynn Mar 11, 2013

RaymondLim added 5 commits March 11, 2013 13:21

Provide editor options "line numbers", "active line" and "word wrap" …

8f35d64

…as new menu items under View menu.

Merge branch 'rlim/word-wrap' of https://github.com/adobe/brackets in…

cf60f0c

…to rlim/word-wrap

Merge remote-tracking branch 'origin' into rlim/word-wrap

ca752e8

Conflicts: src/editor/Editor.js

Merge remote-tracking branch 'origin' into rlim/word-wrap

3d34f38

Conflicts: src/editor/Editor.js

Merge branch 'rlim/word-wrap' of https://github.com/adobe/brackets in…

6ece1e2

…to rlim/word-wrap

RaymondLim reviewed Mar 11, 2013
View reviewed changes

Add unit tests for word-wrap and active line options.

5f5ef09

RaymondLim mentioned this pull request Mar 13, 2013

Feature Request: Drag highlighted text. #3125

Closed

ghost assigned jasonsanjose Mar 14, 2013

Merge remote-tracking branch 'origin' into rlim/word-wrap

97a9774

Conflicts: src/command/Commands.js src/nls/root/strings.js

Move auto close brackets to EditorOptionHandlers.js file.

47a3be3

jasonsanjose reviewed Mar 14, 2013
View reviewed changes

jasonsanjose mentioned this pull request Mar 19, 2013

Preferences ID cleanups - Part II #3101

Merged

Merge remote-tracking branch 'origin' into rlim/word-wrap

9c336f5

Conflicts: src/editor/Editor.js

TomMalbran reviewed Mar 19, 2013
View reviewed changes

Add back "var" that was accidentally removed when resolving the merge…

29e28f8

… conflict.

Fix a typo that is causing auto close brackets command to be undefined.

9996e84

TomMalbran reviewed Mar 20, 2013
View reviewed changes

jasonsanjose mentioned this pull request Mar 20, 2013

fix merging of old prefs with new prefs #3185

Merged

jasonsanjose added a commit that referenced this pull request Mar 20, 2013

Merge pull request #3097 from adobe/rlim/word-wrap

0ca4305

Provide editor options "line numbers", "active line" and "word wrap" as ...

jasonsanjose merged commit 0ca4305 into master Mar 20, 2013

jasonsanjose deleted the rlim/word-wrap branch March 20, 2013 21:24

This was referenced Mar 25, 2013

Dragging a file from Windows Explorer and dropping it onto a Brackets window is broken. #3239

Closed

Fix bug #3239 (Dragging a file onto Brackets to open it is broken on Win) #3242

Merged

core-ai-bot mentioned this pull request Aug 29, 2021

[CLOSED] Provide editor options "line numbers", "active line" and "word wrap" as ... brackets-archive/bracketsIssues#2915

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide editor options "line numbers", "active line" and "word wrap" as ... #3097

Provide editor options "line numbers", "active line" and "word wrap" as ... #3097

RaymondLim commented Mar 11, 2013

RaymondLim Mar 11, 2013

TomMalbran commented Mar 13, 2013

jasonsanjose commented Mar 14, 2013

RaymondLim commented Mar 14, 2013

jasonsanjose commented Mar 14, 2013

jasonsanjose commented Mar 14, 2013

jasonsanjose commented Mar 14, 2013

jasonsanjose Mar 14, 2013

RaymondLim Mar 14, 2013

jasonsanjose commented Mar 14, 2013

jasonsanjose Mar 14, 2013

RaymondLim Mar 15, 2013

jasonsanjose commented Mar 14, 2013

jasonsanjose Mar 14, 2013

RaymondLim Mar 15, 2013

RaymondLim commented Mar 18, 2013

jasonsanjose commented Mar 19, 2013

RaymondLim commented Mar 19, 2013

redmunds commented Mar 19, 2013

jasonsanjose commented Mar 19, 2013

TomMalbran commented Mar 19, 2013

RaymondLim commented Mar 19, 2013

TomMalbran Mar 19, 2013

RaymondLim Mar 19, 2013

jasonsanjose commented Mar 20, 2013

TomMalbran Mar 20, 2013

RaymondLim commented Mar 20, 2013

jasonsanjose commented Mar 20, 2013

jasonsanjose commented Mar 20, 2013

TomMalbran commented Mar 21, 2013

RaymondLim commented Mar 21, 2013

TomMalbran commented Mar 22, 2013

peterflynn commented Mar 22, 2013

TomMalbran commented Mar 23, 2013

Provide editor options "line numbers", "active line" and "word wrap" as ... #3097

Provide editor options "line numbers", "active line" and "word wrap" as ... #3097

Conversation

RaymondLim commented Mar 11, 2013

Choose a reason for hiding this comment

TomMalbran commented Mar 13, 2013

jasonsanjose commented Mar 14, 2013

RaymondLim commented Mar 14, 2013

jasonsanjose commented Mar 14, 2013

jasonsanjose commented Mar 14, 2013

jasonsanjose commented Mar 14, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonsanjose commented Mar 14, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonsanjose commented Mar 14, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaymondLim commented Mar 18, 2013

jasonsanjose commented Mar 19, 2013

RaymondLim commented Mar 19, 2013

redmunds commented Mar 19, 2013

jasonsanjose commented Mar 19, 2013

TomMalbran commented Mar 19, 2013

RaymondLim commented Mar 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonsanjose commented Mar 20, 2013

Choose a reason for hiding this comment

RaymondLim commented Mar 20, 2013

jasonsanjose commented Mar 20, 2013

jasonsanjose commented Mar 20, 2013

TomMalbran commented Mar 21, 2013

RaymondLim commented Mar 21, 2013

TomMalbran commented Mar 22, 2013

peterflynn commented Mar 22, 2013

TomMalbran commented Mar 23, 2013