Build a preference screen for WebIDE

RESOLVED FIXED in Firefox 33

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 33
x86
All
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

I want to be able to configure codemirror. Toggle numbers, tabs or spaces, autocomplete brackets, ...

This will be useful for the devtools in general, but it is necessary for the new App Manager where Itchpad (so codemirror) will be a central piece.
Note: this needs to be available outside of a toolbox (the app manager doesn't always have a toolbox open).
Basically, a simple UI for most of these options: http://codemirror.net/doc/manual.html#config
Blocks: 999420
Blocks: enable-webide
No longer blocks: 999420
Pretty sure this is a dupe.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: expose-editor-prefs
We want something at the WebIDE level. This will most certainly overlaps part of what we want to do in bug 964356, but the overlap will be small.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Build a preference screen for sourceeditor/codemirror → Build a preference screen for WebIDE
Posted patch v0.1 (obsolete) — Splinter Review
Is there a clean way to reload a projectEditor instance?
Assignee: nobody → paul
Status: REOPENED → ASSIGNED
Attachment #8445120 - Flags: feedback?(bgrinstead)
Comment on attachment 8445120 [details] [diff] [review]
v0.1

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

Not really right now short of destroying then rebuilding all the editors (which could cause loss of unsaved work).  I've got a patch that mostly adds this functionality, but there is going to have to be some workarounds for the autocomplete pref, which is handled differently from the others.  I'll file a blocker for it
Attachment #8445120 - Flags: feedback?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #6)
> Not really right now short of destroying then rebuilding all the editors
> (which could cause loss of unsaved work).  I've got a patch that mostly adds
> this functionality, but there is going to have to be some workarounds for
> the autocomplete pref, which is handled differently from the others.  I'll
> file a blocker for it

Can you check my patch? I added a autocomplete pref.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > Not really right now short of destroying then rebuilding all the editors
> > (which could cause loss of unsaved work).  I've got a patch that mostly adds
> > this functionality, but there is going to have to be some workarounds for
> > the autocomplete pref, which is handled differently from the others.  I'll
> > file a blocker for it
> 
> Can you check my patch? I added a autocomplete pref.

Yeah, I saw the pref but this is a limitation in the Editor component right now.  You can only ever enable the autocompletion, not disable.  And if you have already enabled it once it will reinitialize it.  I have a patch in Bug 1029511 that will provide this ability, along with a projecteditor.reloadEditorPrefs function that you can call.
You can apply https://bug1029511.bugzilla.mozilla.org/attachment.cgi?id=8445220 in front of your patch if you want to get it working for now
Posted patch reloadEditorPrefs.patch (obsolete) — Splinter Review
**To be applied in series with the patch from Bug 1029511**.  This will provide a function called projecteditor.reloadEditorPrefs() that should do what you are wanting.
Brian, it appears to work well, but, autoclosebracket pref doesn't appear to be taken into account, and with I also get this error (not sure if related):

TypeError: popup is null: destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/sourceeditor/autocomplete.js:153:5
Should I keep the autocomplete pref?
The autoclose brackets is working with the patch in 964356 - I remember there was some discussion about reusing the options js or panel after that bug is resolved.  In fact, if we did it that way then the editors would reload prefs immediately as the inputs change instead of needing to be triggered by a function call.  What is the reasoning for rolling the prefs functionality separately here instead of reusing some part of what we have in the toolbox?
>I also get this error (not sure if related):
> 
> TypeError: popup is null:
> destroy@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource:///modules/devtools/sourceeditor/autocomplete.js:153:5

What steps do you do to see this error?  I'm not able to reproduce
(In reply to Brian Grinstead [:bgrins] from comment #13)
> I remember
> there was some discussion about reusing the options js or panel after that
> bug is resolved.  In fact, if we did it that way then the editors would
> reload prefs immediately as the inputs change instead of needing to be
> triggered by a function call.  What is the reasoning for rolling the prefs
> functionality separately here instead of reusing some part of what we have
> in the toolbox?

Only the UI would get duplicated. It's not a lot of code.
Posted patch v0.2 (obsolete) — Splinter Review
Attachment #8445120 - Attachment is obsolete: true
(In reply to Brian Grinstead [:bgrins] from comment #13)
> The autoclose brackets is working with the patch in 964356

I can't make it work.

STR:
- apply v0.2
- open a project
- edit a CSS file
- see that autoclose works
- open preferences
- uncheck autoclose
- click "close"
- in the same CSS file, autoclose is still active
For autocomplete brackets to work, you will have to do (or base this patch on top of ) something like what is done in attachment 8428318 [details] [diff] [review] for bug 964356
(In reply to Girish Sharma [:Optimizer] from comment #18)
> For autocomplete brackets to work, you will have to do (or base this patch
> on top of ) something like what is done in attachment 8428318 [details] [diff] [review]
> [diff] [review] for bug 964356

I see. Thanks.
Posted patch v0.9 (obsolete) — Splinter Review
This patch assumes bug 1029511 comment 4 is addressed.
Attachment #8446476 - Attachment is obsolete: true
Comment on attachment 8445389 [details] [diff] [review]
reloadEditorPrefs.patch

Do not need this anymore after Bug 1031472.  Now any opened editors will automatically reload after a pref changes
Attachment #8445389 - Attachment is obsolete: true
Posted patch v1 (obsolete) — Splinter Review
Attachment #8446523 - Attachment is obsolete: true
Attachment #8456005 - Flags: review?(bgrinstead)
Comment on attachment 8456005 [details] [diff] [review]
v1

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

This is nice.  The UI could use a bit of polish but it could mostly be tackled later, especially as more options get added.

::: browser/devtools/webide/content/prefs.js
@@ +18,5 @@
> +
> +  // Buttons
> +  document.querySelector("#close").onclick = CloseUI;
> +  document.querySelector("#restoreButton").onclick = RestoreDefaults;
> +  document.querySelector("#manageSimulators").onclick = () => window.parent.Cmds.showAddons();

Nit: I'd probably make a new function ManageSimulators or ShowAddons that moves the call to window.parent.* out of this function and to be consistent with CloseUI

::: browser/devtools/webide/content/prefs.xhtml
@@ +81,5 @@
> +      </li>
> +      <li>
> +        <label><span>&prefs_options_tabsize;</span>
> +          <select data-pref="devtools.editor.tabsize">
> +            <option value="2">2</option> 

Trailing whitespace

@@ +82,5 @@
> +      <li>
> +        <label><span>&prefs_options_tabsize;</span>
> +          <select data-pref="devtools.editor.tabsize">
> +            <option value="2">2</option> 
> +            <option value="4" selected="true">4</option>

Doesn't really matter one way or another, but this doesn't need to be selected since FillForm will set it before anything is seen

@@ +89,5 @@
> +        </label>
> +      </li>
> +    </ul>
> +
> +    <button id="manageSimulators">&prefs_simulators;</button>

I would suggest to move this to a link at the top next to the close link, similar to how the addons page has a 'open addons manager' nav link.  But I'm fine either way

::: browser/devtools/webide/locales/en-US/webide.dtd
@@ +109,5 @@
> +<!ENTITY prefs_options_detectindentation "Detect indentation">
> +<!ENTITY prefs_options_detectindentation_tooltip "Guess indentation based on source content">
> +<!ENTITY prefs_options_autoclosebrackets "Autoclose brackets">
> +<!ENTITY prefs_options_autoclosebrackets_tooltip "Automatically insert closing brackets">
> +<!ENTITY prefs_options_expandtab "Expand tabs">

"Expand Tabs" is not very clear - maybe "Indent Using Spaces"?

::: browser/devtools/webide/themes/prefs.css
@@ +15,5 @@
> +body {
> +  padding: 20px;
> +}
> +
> +h1 {

I'd suggest moving the common html/font/color/padding styles for this type of panel into a shared file - not a fan of maintaining duplicate CSS
Attachment #8456005 - Flags: review?(bgrinstead) → review+
Posted patch v1.1Splinter Review
Attachment #8456005 - Attachment is obsolete: true
Attachment #8456151 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/76df92e390b5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.