Closed Bug 1058183 Opened 10 years ago Closed 10 years ago

Expose code folding as an option for all editor instances

Categories

(DevTools :: Source Editor, defect)

Product:
DevTools
Component:
Source Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 35

People

(Reporter: bgrins, Assigned: anaran, Mentored)

Details

Attachments

(1 file, 9 obsolete files)

0001-Bug-1058183-Expose-code-folding-as-an-option-for-all-20140912T223314+0200.patch
17.44 KB, patch
bgrins
: review+
Details | Diff | Splinter Review 
Right now we can toggle code folding in the scratchpad with devtools.scratchpad.enableCodeFolding, but we could rename the pref and make it apply to all editors.  This could also be offered in the options panel / webide options.
OK, so basically the plan is to move the option for scratchpad that sets code folding [0] into the editor.js file where other defaults are set [1].

In order to get this to auto reload when a pref changes, we could move the option setting [2] into reloadEditorPrefs, where other dynamic preferences are handled.

[0]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#1607
[1]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/editor.js#142
[2]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/editor.js#192
Assignee: nobody → adrian
Status: NEW → ASSIGNED
Let's dive right to the bottom of this little floe:

# Scratchpad (customizing Editor)

ENABLE_CODE_FOLDING
devtools.scratchpad.enableCodeFolding

SHOW_TRAILING_SPACE
devtools.scratchpad.showTrailingSpace

## Uses editor pref
    const tabsize = Services.prefs.getIntPref("devtools.editor.tabsize");

## Has its own autocomplete pref:

devtools.scratchpad.enableAutocompletion

# Editor

ENABLE_AUTOCOMPLETION
devtools.editor.autocomplete


# WebIDE

It reads and writes "devtools.editor" preferences via Project->Preferences, which is quite misleading.

These are actually **global** preferences (last WebIDE project wins, others, and Scratchpad lose).

# Page Source

toolkit/components/viewsource/content/viewSourceUtils.js uses its own "view_source" prefs.

I was curious because its display of "Line x, Column y" in the statusbar would be useful for Scratchpad too.

# Inconsistencies

## Browser Toolbox Options

... could be seen as the central UI for this.
Too bad its text cannot be selected and copied (usability issue).
It lists Editor Preferences
Detect indentation
Autoclose brackets
Indent using spaces
Tab size
Key binding

## WebIDE
(Hurrah, its text can be copied!)

Has the following, but really its an alternate (confusingly) interface for the global options!

It also invents its own terms "Soft tabs" "Autoindent".

Editor

    Keybindings
    Tab size
    Soft tabs
    Autoindent
    Autocomplete
    Autoclose brackets


# Questions

Does WebIDE need its own preferences per project?
E.g. jetpack add-ons use add-on id specific preferences.

Should scratchpad preference showTrailingSpace be moved up to devtools.editor.showTrailingSpace?

Should scratchpad use 
devtools.editor.autocomplete
instead of its own pref?

Should devtools.scratchpad.enableCodeFolding be renamed to
devtools.editor.enableCodeFolding
as it is moved?
See also

Bug 1062931 - WebIDE->Project->Preferences are not, they interact with global devtools.editor.* prefs instead
(In reply to adrian from comment #2)
> # Questions
> 
> Does WebIDE need its own preferences per project?
>
> E.g. jetpack add-ons use add-on id specific preferences.

No, webide shares prefs with the toolbox

> 
> Should scratchpad preference showTrailingSpace be moved up to
> devtools.editor.showTrailingSpace?
> 

Yes

> Should scratchpad use 
> devtools.editor.autocomplete
> instead of its own pref?
>

Probably, but not in this bug

> Should devtools.scratchpad.enableCodeFolding be renamed to
> devtools.editor.enableCodeFolding
> as it is moved?

Yes
(In reply to adrian from comment #2)
> ## Browser Toolbox Options
> 
> ... could be seen as the central UI for this.
> Too bad its text cannot be selected and copied (usability issue).
> It lists Editor Preferences
> Detect indentation
> Autoclose brackets
> Indent using spaces
> Tab size
> Key binding
> 
> ## WebIDE
> (Hurrah, its text can be copied!)
> 
> Has the following, but really its an alternate (confusingly) interface for
> the global options!
> 
> It also invents its own terms "Soft tabs" "Autoindent".

Yes, those were changed after landing (see bug 1045084). We should update terms in the toolbox options panel to match (in a different bug).
Autoindent is really confusing, as it might be mixed up with the auto indenting on next line feature that CM does.

This is what I am talking about:

I am here:

function () {| <--- cursor

I press enter

function () {
  | <-- automatically indentation added. Be it tab or spaces

Chrome also writes it has "Detect Indentation"


Also, are we sure "Soft Tabs" is a common term ? Cloud 9 IDE referenced in bug 1045084 is a very less known IDE nowadays.

Major IDE :

ST2: translate tab to spaces
IntelliJ: Gives an option b/w Tabs and Spaces for indentation
Chrome : Has a list to choose [2 spaces, 4 spaces, 8 spaces, tab character]
Eclipse: Insert spaces for tabs
Back from a mid-air collision...

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to adrian from comment #2)
> > # Questions
> > 
> > Does WebIDE need its own preferences per project?
> >
> > E.g. jetpack add-ons use add-on id specific preferences.
> 
> No, webide shares prefs with the toolbox

But I donn't like the duplicated preferences/options UIs then.

Can't one be a link to the other (DRY)?

> 
> > 
> > Should scratchpad preference showTrailingSpace be moved up to
> > devtools.editor.showTrailingSpace?
> > 
> 
> Yes
> 
> > Should scratchpad use 
> > devtools.editor.autocomplete
> > instead of its own pref?
> >
> 
> Probably, but not in this bug

What's the difference between this and showTrailingSpace (which should be changed) for my better understanding?

> 
> > Should devtools.scratchpad.enableCodeFolding be renamed to
> > devtools.editor.enableCodeFolding
> > as it is moved?
> 
> Yes

Do we need to do something about the scratchpad prefs move, like migration, for user profiles?

Does the move need to be covered in tests?
(In reply to adrian from comment #7)
> Back from a mid-air collision...
> 
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > (In reply to adrian from comment #2)
> > > # Questions
> > > 
> > > Does WebIDE need its own preferences per project?
> > >
> > > E.g. jetpack add-ons use add-on id specific preferences.
> > 
> > No, webide shares prefs with the toolbox
> 
> But I donn't like the duplicated preferences/options UIs then.
> 
> Can't one be a link to the other (DRY)?
> 

The Web IDE opens in a new window - linking to a toolbox in some other tab would be confusing UX.  That prefs page is meant to live within the Web IDE window, which is why there is duplication.

> > 
> > > 
> > > Should scratchpad preference showTrailingSpace be moved up to
> > > devtools.editor.showTrailingSpace?
> > > 
> > 
> > Yes
> > 
> > > Should scratchpad use 
> > > devtools.editor.autocomplete
> > > instead of its own pref?
> > >
> > 
> > Probably, but not in this bug
> 
> What's the difference between this and showTrailingSpace (which should be
> changed) for my better understanding?
> 

Sorry, that was a mistake - we should *not* move showTrailingSpace in this bug.  Let's just do code folding here, and handle cleanup / consistency issues in other bugs.

> > 
> > > Should devtools.scratchpad.enableCodeFolding be renamed to
> > > devtools.editor.enableCodeFolding
> > > as it is moved?
> > 
> > Yes
> 
> Do we need to do something about the scratchpad prefs move, like migration,
> for user profiles?
> 
> Does the move need to be covered in tests?

Good question, but I doubt hardly anyone has used this pref (searching for that string online only shows links to the source code and similar results).  And it will be visible in the UI now, so if someone has set it to false and it now defaults to true they could toggle it in the prefs panel.
(In reply to Girish Sharma [:Optimizer] from comment #6)
> Autoindent is really confusing, as it might be mixed up with the auto
> indenting on next line feature that CM does.
> 
> This is what I am talking about:
> 
> I am here:
> 
> function () {| <--- cursor
> 
> I press enter
> 
> function () {
>   | <-- automatically indentation added. Be it tab or spaces
> 
> Chrome also writes it has "Detect Indentation"
> 
> 
> Also, are we sure "Soft Tabs" is a common term ? Cloud 9 IDE referenced in
> bug 1045084 is a very less known IDE nowadays.
> 
> Major IDE :
> 
> ST2: translate tab to spaces
> IntelliJ: Gives an option b/w Tabs and Spaces for indentation
> Chrome : Has a list to choose [2 spaces, 4 spaces, 8 spaces, tab character]
> Eclipse: Insert spaces for tabs

We should prabaly move this discussion into Bug 1045084 where the strings were changed - I think we should use the same terms in both places, but definitely want to make sure we pick good ones.
(In reply to adrian from comment #7)
> Back from a mid-air collision...
> 
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > (In reply to adrian from comment #2)
> > > # Questions
> > > 
> > > Does WebIDE need its own preferences per project?
> > >
> > > E.g. jetpack add-ons use add-on id specific preferences.
> > 
> > No, webide shares prefs with the toolbox
> 
> But I donn't like the duplicated preferences/options UIs then.
> 
> Can't one be a link to the other (DRY)?

I'm not a favor of linking to the other, but it would be nice if these used the same localized strings, especially if we want to add this new option to both places.
(In reply to Brian Grinstead [:bgrins] from comment #10)

> > > No, webide shares prefs with the toolbox

So it's just wrong for WebIDE to call it Project Preferences.

And, frankly, without supporting project-specific preferences this will not be of much use than for small private projects where one can enforce one indentation style.

> > 
> > But I donn't like the duplicated preferences/options UIs then.
> > 
> > Can't one be a link to the other (DRY)?
> 
> I'm not a favor of linking to the other, but it would be nice if these used
> the same localized strings, especially if we want to add this new option to
> both places.

Yep, here's another reason for a unified approach.
(In reply to adrian from comment #11)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> 
> > > > No, webide shares prefs with the toolbox
> 
> So it's just wrong for WebIDE to call it Project Preferences.
> 
> And, frankly, without supporting project-specific preferences this will not
> be of much use than for small private projects where one can enforce one
> indentation style.
>

I don't think it is called Project Preferences anywhere in the UI is it?  Just 'Preferences', I think.  

I'd say let's start by making the pref / editor.js changes and add it to the options panel, then let's take a look at possibly sharing the strings between the two places next.
(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to adrian from comment #7)
> > Back from a mid-air collision...
> > 
> > (In reply to Brian Grinstead [:bgrins] from comment #4)
> > > (In reply to adrian from comment #2)
> > > > # Questions
> > > > 
> > > > Does WebIDE need its own preferences per project?
> > > >
> > > > E.g. jetpack add-ons use add-on id specific preferences.
> > > 
> > > No, webide shares prefs with the toolbox
> > 
> > But I donn't like the duplicated preferences/options UIs then.
> > 
> > Can't one be a link to the other (DRY)?
> > 
> 
> The Web IDE opens in a new window - linking to a toolbox in some other tab
> would be confusing UX.  That prefs page is meant to live within the Web IDE
> window, which is why there is duplication.
> 
> > > 
> > > > 
> > > > Should scratchpad preference showTrailingSpace be moved up to
> > > > devtools.editor.showTrailingSpace?
> > > > 
> > > 
> > > Yes
> > > 
> > > > Should scratchpad use 
> > > > devtools.editor.autocomplete
> > > > instead of its own pref?
> > > >
> > > 
> > > Probably, but not in this bug
> > 
> > What's the difference between this and showTrailingSpace (which should be
> > changed) for my better understanding?
> > 
> 
> Sorry, that was a mistake - we should *not* move showTrailingSpace in this
> bug.  Let's just do code folding here, and handle cleanup / consistency
> issues in other bugs.

OK, I see this working fine in the WebIDE for css, html, js now.

I also see it in the inspector inline editor (F2 on HTML source).

Had to manually add the new pref for now.

Where else should I see the foldGutter now?

I don't see it in the debugger.

> 
> > > 
> > > > Should devtools.scratchpad.enableCodeFolding be renamed to
> > > > devtools.editor.enableCodeFolding
> > > > as it is moved?
> > > 
> > > Yes
> > 
> > Do we need to do something about the scratchpad prefs move, like migration,
> > for user profiles?
> > 
> > Does the move need to be covered in tests?
> 
> Good question, but I doubt hardly anyone has used this pref (searching for
> that string online only shows links to the source code and similar results).
> And it will be visible in the UI now, so if someone has set it to false and
> it now defaults to true they could toggle it in the prefs panel.
(In reply to adrian from comment #13) 
> OK, I see this working fine in the WebIDE for css, html, js now.
> 
> I also see it in the inspector inline editor (F2 on HTML source).
> 
> Had to manually add the new pref for now.
> 
> Where else should I see the foldGutter now?
> 
> I don't see it in the debugger.

I would think Style Editor and Debugger would both show it, but maybe debugger isn't because it's read only?  Or it is maybe overriding the option with false when building a new Editor()?
(In reply to Brian Grinstead [:bgrins] from comment #12)
> (In reply to adrian from comment #11)
> > (In reply to Brian Grinstead [:bgrins] from comment #10)
> > 
> > > > > No, webide shares prefs with the toolbox
> > 
> > So it's just wrong for WebIDE to call it Project Preferences.
> > 
> > And, frankly, without supporting project-specific preferences this will not
> > be of much use than for small private projects where one can enforce one
> > indentation style.
> >
> 
> I don't think it is called Project Preferences anywhere in the UI is it? 
> Just 'Preferences', I think.

Who would guess that under

WebIDE->Project->Preferences
 along with
New App...
 and
Remove Project
 you find **Global**
Preferences for all of Firefox devtools?

> 
> I'd say let's start by making the pref / editor.js changes and add it to the

Sure, I already like what I'm seeing.

> options panel, then let's take a look at possibly sharing the strings
> between the two places next.

Which Bug number covers the change to the options panel?

 Should I include the change from
./browser/app/profile/firefox.js:pref("devtools.scratchpad.enableCodeFolding", true);
 to
./browser/app/profile/firefox.js:pref("devtools.editor.enableCodeFolding", true);
 in my upcoming patch?
(In reply to adrian from comment #15)
> Which Bug number covers the change to the options panel?

I was planning on including the new checkbox in the options panel for this bug.  Filed Bug 1063079 for consolidating the strings into one place.  No need to hold up the work here for that thought (including toolbox options panel changes).

> 
>  Should I include the change from
> ./browser/app/profile/firefox.js:pref("devtools.scratchpad.
> enableCodeFolding", true);
>  to
> ./browser/app/profile/firefox.js:pref("devtools.editor.enableCodeFolding",
> true);
>  in my upcoming patch?

Yes
Mentor: bgrinstead
Here's my first cut. Gave debugger-view.js a try first, but that did not work (available separetly on request).

codeFolding responds dynamically to toggling clicks made in about:config to
devtools.editor.enableCodeFolding

Let me know, I will than run the git to hg stuff.
Attachment #8484499 - Flags: review?(bgrinstead)
Comment on attachment 8484499 [details] [diff] [review]
Bug1058183-20140904T230207+0200.patch

Review of attachment 8484499 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/scratchpad/scratchpad.js
@@ +656,4 @@
>     */
>    prettyPrint: function SP_prettyPrint() {
>      const uglyText = this.getText();
> +    const tabsize = Services.prefs.getIntPref(TAB_SIZE);

Yah, that one was not in the contract.
I am moving this hidden pref down here up into bright daylight and hope that's OK.
Comment on attachment 8484499 [details] [diff] [review]
Bug1058183-20140904T230207+0200.patch

Review of attachment 8484499 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like it's on the right track.  Take a look at adding a case in browser_editor_prefs.js for testing this

::: browser/devtools/scratchpad/scratchpad.js
@@ +34,4 @@
>  const DEVTOOLS_CHROME_ENABLED = "devtools.chrome.enabled";
>  const PREF_RECENT_FILES_MAX = "devtools.scratchpad.recentFilesMax";
>  const SHOW_TRAILING_SPACE = "devtools.scratchpad.showTrailingSpace";
> +const ENABLE_CODE_FOLDING = "devtools.editor.enableCodeFolding";

I'd say let's remove ENABLE_CODE_FOLDING from this file entirely and just let the editor object handle it.

@@ +656,4 @@
>     */
>    prettyPrint: function SP_prettyPrint() {
>      const uglyText = this.getText();
> +    const tabsize = Services.prefs.getIntPref(TAB_SIZE);

Yeah, that's fine

::: browser/devtools/sourceeditor/editor.js
@@ +434,5 @@
>        this.setOption("keyMap", keyMap)
>      else
>        this.setOption("keyMap", "default");
> +    let useCodeFolding = Services.prefs.getBoolPref(ENABLE_CODE_FOLDING);
> +    this.setOption("foldGutter", useCodeFolding ? true : false);

don't need ternary here, just useCodeFolding
Attachment #8484499 - Flags: review?(bgrinstead)
Also, looks like your patch is changing the file mode on the files (you can see at https://bug1058183.bugzilla.mozilla.org/attachment.cgi?id=8484499):

old mode 100644
new mode 100755

Please make sure that the patch just has text changes.
Just a note as discussed on IRC, the reason that this isn't working with debugger is because the fold gutter is only pushed if this.config.gutters is not set, and in the debugger it is already set with breakpoints.  http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/editor.js#195.

This logic should be cleaned up to provide a default value of [] to the gutters option, then separate the line numbers and code folding logic into different conditions.
I find it quite nice to have the Developer Toolbar (Shift+F2) open below the Debugger displaying some editor (Inspector (F2 to Edit as HTML), Debugger, Style Editor)

Then I use these commands in the Developer Toolbar:

pref set devtools.editor.enableCodeFolding false
pref set devtools.editor.enableCodeFolding true

So far I have only noticed tabzilla.css to refuse showing the foldgutter.

With your help from yesterday Debugger code can be folded too now.
This is a proper hg patch without file mode changes.
It should address all your feedback, but you'll tell me.
Adds simple tests for turning code folding first off then on.
This is what other tests already do.

A test log is forthcoming, if only for my own record.
Attachment #8485422 - Flags: review?(bgrinstead)
1023 INFO Browser Chrome Test Summary
1024 INFO Passed:  885
1025 INFO Failed:  0
1026 INFO Todo:    0
1027 INFO *** End BrowserChrome Test Results ***

Up two tests, look for "foldGutter".
Attachment #8484499 - Attachment is obsolete: true
git.mozilla.org/gecko-dev$ log ./mach mochitest-devtools browser/devtools/scratchpad
are unchanged and still pass:

680 INFO TEST-START | Shutdown
681 INFO Browser Chrome Test Summary
682 INFO Passed:  388
683 INFO Failed:  0
684 INFO Todo:    0
685 INFO *** End BrowserChrome Test Results ***
Comment on attachment 8485422 [details] [diff] [review]
0001-Bug-1058183-Expose-code-folding-as-an-option-for-all.patch

Review of attachment 8485422 [details] [diff] [review]:
-----------------------------------------------------------------

Hm, we should either need to disable this for the debugger or make sure that clicking on the little arrow doesn't also set a breakpoint.  I think we should be able to disable this on an editor in general, and the debugger may be the right test case for this.

In order to do this, the pref listener code will have to be updated a bit to not change the state if it was explicitly passed in as true or false.  I think to keep track of this, we could not set enableCodeFolding: useCodeFolding in the constructor, and instead just check config.enableCodeFolding || useCodeFolding there.  Then in the listener, we could just check if config.enableCodeFolding is undefined before changing the value of config.foldGutter.  Let me know if that isn't explained well enough.

::: browser/devtools/sourceeditor/editor.js
@@ +438,5 @@
>        this.setOption("keyMap", keyMap)
>      else
>        this.setOption("keyMap", "default");
> +    let useCodeFolding = Services.prefs.getBoolPref(ENABLE_CODE_FOLDING);
> +    this.setOption("foldGutter", useCodeFolding);

We also need to update the state of gutters in this case if it's changed.  That way we can add the gutter if it started out as false.  I'd make a function for updateGutters() that adds/removes any gutters based on the state of foldGutter / lineNumbers then call this both here and above where gutters are originally added above.
Attachment #8485422 - Flags: review?(bgrinstead)
This should address all of your feedback.
debugger-view.js force this.config.enableCodeFolding false

One sore point is the lack of unfolding code before turning the option off. So one can see folded code in an editor without folding enabled.

One can, however, click each one to unfold.

I was not able to find a solution for this and would like to hear your opinion.

mochitest-devtools still passes on sourceeditor and scratchpad.

For debugger I get following results, but I don't have a baseline for fx-team.
Are fx-team test results available somewhere for me to check?

22363 INFO checking window state
22364 INFO TEST-START | Shutdown
22365 INFO Browser Chrome Test Summary
22366 INFO Passed:  9493
22367 INFO Failed:  11
22368 INFO Todo:    5
22369 INFO *** End BrowserChrome Test Results ***
Attachment #8485422 - Attachment is obsolete: true
Attachment #8485423 - Attachment is obsolete: true
Attachment #8487281 - Flags: review?(bgrinstead)
(In reply to adrian from comment #27)
> Created attachment 8487281 [details] [diff] [review]
> 0001-Bug-1058183-Expose-code-folding-as-an-option-for-all.patch
> 
> This should address all of your feedback.
> debugger-view.js force this.config.enableCodeFolding false
> 
> One sore point is the lack of unfolding code before turning the option off.
> So one can see folded code in an editor without folding enabled.
> 
> One can, however, click each one to unfold.
> 
> I was not able to find a solution for this and would like to hear your
> opinion.

I don't think that's a huge deal, but it looks like there is an unfoldAll command: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/codemirror/fold/foldcode.js#103.  This should be callable with the execCommand function: http://codemirror.net/doc/manual.html#execCommand.

> mochitest-devtools still passes on sourceeditor and scratchpad.
> 
> For debugger I get following results, but I don't have a baseline for
> fx-team.
> Are fx-team test results available somewhere for me to check?

The test results on fx-team are located here: https://tbpl.mozilla.org/?tree=Fx-Team.
(In reply to Brian Grinstead [:bgrins] from comment #28)
> (In reply to adrian from comment #27)
> > Created attachment 8487281 [details] [diff] [review]
> > 0001-Bug-1058183-Expose-code-folding-as-an-option-for-all.patch
> > 
> > This should address all of your feedback.
> > debugger-view.js force this.config.enableCodeFolding false
> > 
> > One sore point is the lack of unfolding code before turning the option off.
> > So one can see folded code in an editor without folding enabled.
> > 
> > One can, however, click each one to unfold.
> > 
> > I was not able to find a solution for this and would like to hear your
> > opinion.
> 
> I don't think that's a huge deal, but it looks like there is an unfoldAll
> command:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/
> codemirror/fold/foldcode.js#103.  This should be callable with the
> execCommand function: http://codemirror.net/doc/manual.html#execCommand.

Thanks! I did not think to use it via execCommand.

It works now.

I see an issue where afer starting a scratchpad with codefolding off, then turning it on via the developer toolbar with
pref set devtools.editor.enableCodeFolding true
I see foldGutter to the left of line numbers and it does not work.

Will investigate with ordering in the gutter.

> 
> > mochitest-devtools still passes on sourceeditor and scratchpad.
> > 
> > For debugger I get following results, but I don't have a baseline for
> > fx-team.
> > Are fx-team test results available somewhere for me to check?
> 
> The test results on fx-team are located here:
> https://tbpl.mozilla.org/?tree=Fx-Team.
Attached image Screen Shot 2014-09-10 at 19.01.12.png (obsolete) — 
folderGutter element clicks don't fold as described in last comment.
Comment on attachment 8487281 [details] [diff] [review]
0001-Bug-1058183-Expose-code-folding-as-an-option-for-all.patch

Review of attachment 8487281 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good when using it, a few notes centered around toggling the pref / config option

::: browser/devtools/sourceeditor/editor.js
@@ +196,5 @@
> +  if (this.config.lineNumbers
> +      && this.config.gutters.indexOf("CodeMirror-linenumbers") === -1) {
> +    this.config.gutters.push("CodeMirror-linenumbers");
> +  }
> +  this.updateCodeFoldingGutter = function (enable) {

Add this function to the prototype instead of setting it on  this.updateCodeFoldingGutter here.

@@ +198,5 @@
> +    this.config.gutters.push("CodeMirror-linenumbers");
> +  }
> +  this.updateCodeFoldingGutter = function (enable) {
> +    // Ignore preference change for explicit editor config option.
> +    if (typeof this.config.enableCodeFolding == "undefined") {

The logic here isn't still quite right for two cases:

1) if the config passed in enableCodeFolding: true it wouldn't add the gutter
2) if the pref started out as true and later got set to false the gutter would never be removed.

I think something like this pseudocode would do the trick (it takes away the param to the function, and instead reads the pref directly from this function if needed).

updateCodeFolding: function() {
  let codeFoldingPref = Services.prefs.getBoolPref(ENABLE_CODE_FOLDING);
  let shouldFoldGutter = this.config.enableCodeFolding;
  if (shouldFoldGutter === undefined) {
    shouldFoldGutter = codeFoldingPref;
  }

  if (shouldFoldGutter) {
    this.config.foldGutter = true;
    if (config.gutters doesn't have the fold gutter) {
      // add the fold gutter
    }
  } else {
    this.config.foldGutter = false;
    if (config.gutters has the fold gutter) {
      // remove the fold gutter
    }
  }
}

Then, updateCodeFolding would be called in constructor, on pref listener, then also on setOption if the option name is "enableCodeFolding"

@@ +201,5 @@
> +    // Ignore preference change for explicit editor config option.
> +    if (typeof this.config.enableCodeFolding == "undefined") {
> +      // let index = this.config.gutters.indexOf("CodeMirror-foldgutter");
> +      if (enable
> +          && this.config.gutters.indexOf("CodeMirror-foldgutter") === -1) {

Nit: move && onto the first line of the condition

::: browser/devtools/sourceeditor/test/browser_editor_prefs.js
@@ +51,5 @@
>      Services.prefs.setBoolPref(AUTOCOMPLETE, true);
>  
>      is(ed.getOption("tabSize"), 4, "tabSize is correct");
>      is(ed.getOption("indentUnit"), 4, "indentUnit is correct");
> +    is(ed.getOption("foldGutter"), true, "foldGutter is correct");

It'd be great if you did a ed.setOption("enableCodeFolding", false) after this to ensure that getOption("foldGutter") is false when that is explicitly specified
Attachment #8487281 - Flags: review?(bgrinstead)
(In reply to adrian from comment #29)
> I see an issue where afer starting a scratchpad with codefolding off, then
> turning it on via the developer toolbar with
> pref set devtools.editor.enableCodeFolding true
> I see foldGutter to the left of line numbers and it does not work.
> 
> Will investigate with ordering in the gutter.

This case may be solved by my suggestion in the latest review, see if that helps
(In reply to Brian Grinstead [:bgrins] from comment #32)
> (In reply to adrian from comment #29)
> > I see an issue where afer starting a scratchpad with codefolding off, then
> > turning it on via the developer toolbar with
> > pref set devtools.editor.enableCodeFolding true
> > I see foldGutter to the left of line numbers and it does not work.
> > 
> > Will investigate with ordering in the gutter.
> 
> This case may be solved by my suggestion in the latest review, see if that
> helps

Which part was that? Nothing sticks out right away.
(In reply to adrian from comment #33)
> (In reply to Brian Grinstead [:bgrins] from comment #32)
> > (In reply to adrian from comment #29)
> > > I see an issue where afer starting a scratchpad with codefolding off, then
> > > turning it on via the developer toolbar with
> > > pref set devtools.editor.enableCodeFolding true
> > > I see foldGutter to the left of line numbers and it does not work.
> > > 
> > > Will investigate with ordering in the gutter.
> > 
> > This case may be solved by my suggestion in the latest review, see if that
> > helps
> 
> Which part was that? Nothing sticks out right away.

The comments in editor.js, specifically the big one about handling a few more cases with the updateCodeFoldingGutter function
(In reply to Brian Grinstead [:bgrins] from comment #26)
> Comment on attachment 8485422 [details] [diff] [review]
> 0001-Bug-1058183-Expose-code-folding-as-an-option-for-all.patch
> 
> Review of attachment 8485422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm, we should either need to disable this for the debugger or make sure that
> clicking on the little arrow doesn't also set a breakpoint.  I think we
> should be able to disable this on an editor in general, and the debugger may
> be the right test case for this.

So editor.js has this little easter egg:
We can use SHIFT+CLICK on the CodeMirror-foldgutter to avoid setting a breakpoint in the debugger.
Not a very explorable feature but good enough for our testing to get a feel for it.

      cm.on("gutterClick", (cm, line, gutter, ev) => {
        let head = { line: line, ch: 0 };
        let tail = { line: line, ch: this.getText(line).length };

        // Shift-click on a gutter selects the whole line.
        if (ev.shiftKey) {
          cm.setSelection(head, tail);
          return;
        }
(In reply to adrian from comment #35)
> (In reply to Brian Grinstead [:bgrins] from comment #26)
> > Comment on attachment 8485422 [details] [diff] [review]
> > 0001-Bug-1058183-Expose-code-folding-as-an-option-for-all.patch
> > 
> > Review of attachment 8485422 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hm, we should either need to disable this for the debugger or make sure that
> > clicking on the little arrow doesn't also set a breakpoint.  I think we
> > should be able to disable this on an editor in general, and the debugger may
> > be the right test case for this.
> 
> So editor.js has this little easter egg:
> We can use SHIFT+CLICK on the CodeMirror-foldgutter to avoid setting a
> breakpoint in the debugger.
> Not a very explorable feature but good enough for our testing to get a feel
> for it.
> 
>       cm.on("gutterClick", (cm, line, gutter, ev) => {
>         let head = { line: line, ch: 0 };
>         let tail = { line: line, ch: this.getText(line).length };
> 
>         // Shift-click on a gutter selects the whole line.
>         if (ev.shiftKey) {
>           cm.setSelection(head, tail);
>           return;
>         }

If a user would have to shift+click on the arrow that would be too hard to discover.  We also have the weird issue of what happens when there is a breakpoint on line 10 then lines 8-12 are collapsed with a fold arrow.  Does the breakpoint disappear, move to be next to the arrow, or what?  I'm perfectly happy to punt on the debugger folding for now since it opens up so many questions.
debugger: punted.

I have found that pushing and splicing the gutters array does not dynamically update the folding.
Adding all necessary gutters and turning them off/on works fine.

I have addressed all your requested and exceeded the testing request a bit, making sure one option reflects the other and vice versa.

Here are the relevant new test results:

489 INFO Turning foldGutter off using enableCodeFolding
490 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_editor_prefs.js | foldGutter is correct
491 INFO Turning enableCodeFolding on using foldGutter
492 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_editor_prefs.js | enableCodeFolding is correct
Attachment #8487281 - Attachment is obsolete: true
Attachment #8487310 - Attachment is obsolete: true
Attachment #8487589 - Flags: review?(bgrinstead)
Comment on attachment 8487589 [details] [diff] [review]
0001-Bug-1058183-Expose-code-folding-as-an-option-for-all.patch

Review of attachment 8487589 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/sourceeditor/editor.js
@@ +142,5 @@
>  function Editor(config) {
>    const tabSize = Services.prefs.getIntPref(TAB_SIZE);
>    const useTabs = !Services.prefs.getBoolPref(EXPAND_TAB);
>    const useAutoClose = Services.prefs.getBoolPref(AUTO_CLOSE);
> +  const useCodeFolding = Services.prefs.getBoolPref(ENABLE_CODE_FOLDING);

Don't need this line

@@ +200,5 @@
> +  // config options must already contain user-provided overwrites.
> +  // onLoad already calls reloadPreferences, but that is too late.
> +  // An editor needs to be created with all necessary gutters, then they can be
> +  // toggled.
> +  this.config.gutters.push("CodeMirror-foldgutter");

As we discussed on IRC you can remove this line (and the comment above it).  See the note below about updateCodeFoldingGutter

@@ +949,5 @@
>     */
>    setOption: function(o, v) {
>      let cm = editors.get(this);
>  
> +    if (o === "enableCodeFolding") {

I actually don't think we want to special case this to set foldGutter directly here.  Instead, lets have this function run it's normal course (with a call to cm.setOption), then at the bottom of it add a condition:

if (o === "enableCodeFolding") {
  this.updateCodeFoldingGutter();
}

This will handle the foldGutter option plus take care of gutter removal / adding.

@@ +975,5 @@
>     * instance.
>     */
>    getOption: function(o) {
>      let cm = editors.get(this);
> +    if (o === "enableCodeFolding") {

Don't need this condition at all.  If the user requests that option it should be set to whatever they last set it to (not to the current value of foldGutter which could have been set by the pref)

@@ +1058,5 @@
>      this.emit("destroy");
> +  },
> +
> +  updateCodeFoldingGutter: function () {
> +    let codeFoldingPref = Services.prefs.getBoolPref(ENABLE_CODE_FOLDING),

As we discussed on IRC, we actually need to do this.setOption("gutters", []); in order to get codemirror to recognize the change (that's why you had to push it to the array in the constructor.  Instead, doing something like this should work (haven't fully tested this code with setting / resetting, but I think it's close):


  updateCodeFoldingGutter: function () {
    let shouldFoldGutter = this.config.enableCodeFolding,
        foldGutterIndex = this.config.gutters.indexOf("CodeMirror-foldgutter"),
        cm = editors.get(this);

    if (shouldFoldGutter === undefined) {
      shouldFoldGutter = Services.prefs.getBoolPref(ENABLE_CODE_FOLDING);
    }

    if (shouldFoldGutter) {
      // Add the gutter before enabling foldGutter
      if (foldGutterIndex === -1) {
        let gutters = this.config.gutters.slice();
        gutters.push("CodeMirror-foldgutter");
        this.setOption("gutters", gutters);
      }

      this.setOption("foldGutter", true);
    } else {
      // No code should remain folded when folding is off.
      if (cm) {
        cm.execCommand("unfoldAll");
      }

      // Remove the gutter so it doesn't take up space
      if (foldGutterIndex !== -1) {
        let gutters = this.config.gutters.slice();
        gutters.splice(foldGutterIndex, 1);
        this.setOption("gutters", gutters);
      }

      this.setOption("foldGutter", false);
    }
  }

::: browser/devtools/sourceeditor/test/browser_editor_prefs.js
@@ +53,5 @@
>  
>      is(ed.getOption("tabSize"), 4, "tabSize is correct");
>      is(ed.getOption("indentUnit"), 4, "indentUnit is correct");
> +    is(ed.getOption("foldGutter"), true, "foldGutter is correct");
> +    is(ed.getOption("enableCodeFolding"), true, "enableCodeFolding is correct");

as I said in editor.js, I don't think "enableCodeFolding" should be true in this case - it should return what the user has set.  If they are interested in whether it has been enabled by the pref they could check "foldGutter".

@@ +68,5 @@
> +
> +    info ("Turning enableCodeFolding on using foldGutter");
> +
> +    ed.setOption("foldGutter", true);
> +    is(ed.getOption("enableCodeFolding"), true, "enableCodeFolding is correct");

Same note about "enableCodeFolding" above
Attachment #8487589 - Flags: review?(bgrinstead)
The patch filename is a bit long, but uploading under the same name probably defeats the bugzilla diffing of attachments (have not tried).

Thanks for your rewrite of updateCodeFoldingGutter, which I took as is.

I had obviously gotten confused about enableCodeFolding and foldGutter, half way down the line.

I added some comment to setOption, which should help.

I also added tests for the gutters option and was surprised to see "breakpoints" in there.

My manual testing of scratchpad, inspector, stylesheet editor, debugger, and webide checked out fine.

viewSourceUtils does not seem to use the editor but OpenDialog on its own xul file.

debugger and webide don't have specific editor prefs testing and I was not able to easily add them. I plan to stay away from these for this contribution.
Attachment #8487589 - Attachment is obsolete: true
Attachment #8488375 - Flags: review?(bgrinstead)
Comment on attachment 8488375 [details] [diff] [review]
0001-Bug-1058183-Expose-code-folding-as-an-option-for-all-20140912T024149+0200.patch

Review of attachment 8488375 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  I made a small note for a change in the test

::: browser/devtools/sourceeditor/test/browser_editor_prefs.js
@@ +18,5 @@
>    waitForExplicitFinish();
>    setup((ed, win) => {
>  
> +    function isSameJson(a, b, comment) {
> +      ok(JSON.stringify(a) == JSON.stringify(b), comment);

Would using Assert.deepEqual work instead?  https://developer.mozilla.org/en-US/docs/Mochitest#Test_functions
Attachment #8488375 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #40)
> Comment on attachment 8488375 [details] [diff] [review]
> 0001-Bug-1058183-Expose-code-folding-as-an-option-for-all-
> 20140912T024149+0200.patch
> 
> Review of attachment 8488375 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!  I made a small note for a change in the test
> 
> ::: browser/devtools/sourceeditor/test/browser_editor_prefs.js
> @@ +18,5 @@
> >    waitForExplicitFinish();
> >    setup((ed, win) => {
> >  
> > +    function isSameJson(a, b, comment) {
> > +      ok(JSON.stringify(a) == JSON.stringify(b), comment);
> 
> Would using Assert.deepEqual work instead? 
> https://developer.mozilla.org/en-US/docs/Mochitest#Test_functions

Ah, some much better!

I'm glad you didn't call me MacGyver for my "solution".

And, of course, the breakpoints are thrown in the gutters in the editor created in waitForFocus in head.js

Uploading a new patch now.
Tests are still passing and log the gutters value on pass as well:

E.g.
468 INFO must wait for focus
469 INFO TEST-PASS | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_editor_prefs.js | gutters is correct - ["CodeMirror-linenumbers","breakpoints","CodeMirror-foldgutter"] deepEqual ["CodeMirror-linenumbers","breakpoints","CodeMirror-foldgutter"]
470 INFO Turning prefs off
Attachment #8488375 - Attachment is obsolete: true
Attachment #8488614 - Flags: review?(bgrinstead)
Comment on attachment 8488614 [details] [diff] [review]
0001-Bug-1058183-Expose-code-folding-as-an-option-for-all-20140912T153758+0200.patch

Review of attachment 8488614 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like there is an error on: browser/devtools/styleeditor/test/browser_styleeditor_highlight-selector.js (I've confirmed locally).  Test results: https://tbpl.mozilla.org/?tree=Try&rev=6539e9dfd3b2
Attachment #8488614 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #43)
> Comment on attachment 8488614 [details] [diff] [review]
> 0001-Bug-1058183-Expose-code-folding-as-an-option-for-all-
> 20140912T153758+0200.patch
> 
> Review of attachment 8488614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like there is an error on:
> browser/devtools/styleeditor/test/browser_styleeditor_highlight-selector.js
> (I've confirmed locally).  Test results:
> https://tbpl.mozilla.org/?tree=Try&rev=6539e9dfd3b2

Yep, I can reproduce this too.

Haven't found anything gutter-specific in that code and tests yet.
Look what I found!
The foldgutter happens to be 16px wide, on my system at least.

If your feedback is positive I can roll an updated complete HG patch.
Attachment #8488810 - Flags: feedback?(bgrinstead)
Comment on attachment 8488810 [details] [diff] [review]
adjust_mouse_move_for_foldgutter_width.patch

Seems fine
Attachment #8488810 - Flags: feedback?(bgrinstead) → feedback+
(In reply to Brian Grinstead [:bgrins] from comment #46)
> Comment on attachment 8488810 [details] [diff] [review]
> adjust_mouse_move_for_foldgutter_width.patch
> 
> Seems fine

(In reply to adrian from comment #45)
> Created attachment 8488810 [details] [diff] [review]
> adjust_mouse_move_for_foldgutter_width.patch
> 
> Look what I found!
> The foldgutter happens to be 16px wide, on my system at least.
> 
> If your feedback is positive I can roll an updated complete HG patch.

Great, patch is forthcoming...

Needless to say, this makes
$ log ./mach mochitest-devtools browser/devtools/styleeditor/
pass for me.

log is my bash function to capture a timestamped, log file with runtime information

----
alias dtz='date +%Y%m%dT%H%M%S%z'
log ()
{
    local logfile="$@";
    logfile=${logfile//[*:?;.]/};
    logfile=${logfile//[\/\\[:blank:]]/_};
    logfile=${logfile//_-/-};
    logfile=${logfile:0:51}-`dtz`.txt;
    local logcmd='(time';
    for a in "$@";
    do
        if [[ "$a" =~ [\|\&\;\(\)\<\>[:blank:]] ]]; then
            logcmd+=" \"$a\"";
        else
            logcmd+=" $a";
        fi;
    done;
    logcmd+=") >> $logfile 2>&1 &";
    echo $logcmd > $logfile;
    echo "wd: `pwd`" >> $logfile;
    echo >> $logfile;
    eval $logcmd
}
This is the complete patch, ready for a "try"
Attachment #8488614 - Attachment is obsolete: true
Attachment #8488810 - Attachment is obsolete: true
Attachment #8488826 - Flags: review?(bgrinstead)
Comment on attachment 8488826 [details] [diff] [review]
0001-Bug-1058183-Expose-code-folding-as-an-option-for-all-20140912T223314+0200.patch

Review of attachment 8488826 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me as long as the rest of the tests are green: https://tbpl.mozilla.org/?tree=Try&rev=9f3548f1f947
Attachment #8488826 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/02648d3811fe
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: