Last Comment Bug 660560 - Scratchpad lacks indentation, pressing TAB key should indent code
: Scratchpad lacks indentation, pressing TAB key should indent code
Status: VERIFIED FIXED
[scratchpad][fixed-in-devtools]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Firefox 7
Assigned To: Mihai Sucan [:msucan]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-29 17:09 PDT by Gabriel Koritzky
Modified: 2011-06-28 06:41 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
proposed fix and test (4.54 KB, patch)
2011-06-02 11:12 PDT, Mihai Sucan [:msucan]
rcampbell: feedback+
Details | Diff | Splinter Review
updated patch (5.13 KB, patch)
2011-06-03 08:13 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
patch update 2 (7.67 KB, patch)
2011-06-11 08:28 PDT, Mihai Sucan [:msucan]
dietrich: review-
Details | Diff | Splinter Review
[in-devtools] patch update 3 (9.12 KB, patch)
2011-06-14 03:48 PDT, Mihai Sucan [:msucan]
dietrich: review+
Details | Diff | Splinter Review

Description Gabriel Koritzky 2011-05-29 17:09:57 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110529 Firefox/6.0a2
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110529 Firefox/6.0a2

Pretty simple, in fact. The Scratchpad javascript editor lacks simple indentation features of any regular coding interface.

This resumes to two basic issues:
1) If user presses the tab key, a "	" (\t) should be inserted.
2) If user presses ENTER and the current line was indented (by either normal spaces or \t characters), the new line should be created carrying the same indentation.

Reproducible: Always
Comment 1 Rob Campbell [:rc] (:robcee) 2011-05-30 08:17:52 PDT
You're right. We have work underway to add a more feature-complete coding editor to the Scratchpad.

We could probably add a keylistener to watch for tabs and automatically insert some spaces in the meantime.
Comment 2 Kevin Dangoor 2011-06-02 07:17:33 PDT
Rob tells me that the patch for this should be small and safe (and it's effect is limited to this new tool). I think this would improve the usability of Scratchpad a good bit and would like to get this in Aurora assuming the final patch is indeed simple.
Comment 3 Kevin Dangoor 2011-06-02 07:24:06 PDT
argh. the tabs-vs-spaces argument is likely to get into this bug.

To be clear, I think the minimal feature here is that pressing tab adds a few (let's say 4, again that's a point of contention) spaces and doesn't worry about what was on the previous line. Even that would be an improvement over no quick way to indent and having a better code editor (the one from bug 660784) is the real solution that we're hoping to get in fx7.
Comment 4 Rob Campbell [:rc] (:robcee) 2011-06-02 07:47:28 PDT
(In reply to comment #3)
> argh. the tabs-vs-spaces argument is likely to get into this bug.

yeah, let's not tempt fate here. ;)

> To be clear, I think the minimal feature here is that pressing tab adds a
> few (let's say 4, again that's a point of contention) spaces and doesn't
> worry about what was on the previous line. Even that would be an improvement
> over no quick way to indent and having a better code editor (the one from
> bug 660784) is the real solution that we're hoping to get in fx7.

Agreed. Soft tabs, probably 4 spaces by default, maybe even settable via a pref.
Comment 5 Kevin Dangoor 2011-06-02 07:51:43 PDT
(In reply to comment #4)
> Agreed. Soft tabs, probably 4 spaces by default, maybe even settable via a
> pref.

Possible, but keeping this patch as simple and safe as possible increases the odds that it lands in Aurora.
Comment 6 Mihai Sucan [:msucan] 2011-06-02 11:12:29 PDT
Created attachment 536937 [details] [diff] [review]
proposed fix and test

Proposed fix and test.

I also added textbox.focus() because it seemed trivial and worthy of doing so.

Feedback is welcome!
Comment 7 Mihai Sucan [:msucan] 2011-06-02 11:16:33 PDT
I wanted to add that I would kind of prefer the Tab character to be used instead of spaces. Due to the fact it's a textarea, when you press Backspace you delete each space individually. A code editor knows of indentation and deletes the four spaces at once. Here we do not need to expand the patch into such behavior, but using the tab character might make sense - it would also be an improvement for when the user presses the Backspace/Delete keys.

(just a thought!)
Comment 8 Rob Campbell [:rc] (:robcee) 2011-06-02 11:42:51 PDT
Comment on attachment 536937 [details] [diff] [review]
proposed fix and test

+
+    this.textbox.focus();

Thank you. :)

+    let firstPiece = this.textbox.value.substring(0, this.textbox.selectionStart);
+    let lastPiece = this.textbox.value.substring(this.textbox.selectionEnd);
+    this.textbox.value = firstPiece + "    " + lastPiece;
+
+    this.selectRange(firstPiece.length + 4, firstPiece.length + 4);

This code feels like it should be a method in itself. insertTextAtCaret or similar. Good enough for here though.
Comment 9 Rob Campbell [:rc] (:robcee) 2011-06-02 13:45:03 PDT
(In reply to comment #7)
> I wanted to add that I would kind of prefer the Tab character to be used
> instead of spaces. Due to the fact it's a textarea, when you press Backspace
> you delete each space individually. A code editor knows of indentation and
> deletes the four spaces at once. Here we do not need to expand the patch
> into such behavior, but using the tab character might make sense - it would
> also be an improvement for when the user presses the Backspace/Delete keys.
> 
> (just a thought!)

no! I don't want to have to scrub the tabs out of my text editor every time I copy and paste text. If people really want and need tabs in their source file, they can convert spaces to tabs there.

(curses, Kevin, you totally did this)
Comment 10 Johnathan Nightingale [:johnath] 2011-06-02 14:45:37 PDT
tracking-firefox6 is for following issues which are necessarily FF6-specific - this isn't, it's an improvement to a FF6 feature, but can land on trunk and then potentially request approval if necessary.
Comment 11 Shawn Wilsher :sdwilsh 2011-06-02 18:09:25 PDT
> +    this.selectRange(firstPiece.length + 4, firstPiece.length + 4);
magic numbers :(
Comment 12 Mihai Sucan [:msucan] 2011-06-03 08:13:40 PDT
Created attachment 537147 [details] [diff] [review]
updated patch

Updated the patch to address Rob's feedback in comment 8 and your comment 11.

Thank you! and thanks to Rob for his f+!
Comment 13 Mihai Sucan [:msucan] 2011-06-09 08:19:14 PDT
Comment on attachment 537147 [details] [diff] [review]
updated patch

Asking for review from Dietrich.
Comment 14 David Dahl :ddahl 2011-06-09 08:42:36 PDT
Comment on attachment 537147 [details] [diff] [review]
updated patch

Sorry to drive-by, but I was thinking this patch needs a pref for the number of spaces to insert each time tab is hit, as that may be the next bug filed due to differing opinions on spaces.
Comment 15 Kevin Dangoor 2011-06-09 08:54:02 PDT
(In reply to comment #14)
> Sorry to drive-by, but I was thinking this patch needs a pref for the number
> of spaces to insert each time tab is hit, as that may be the next bug filed
> due to differing opinions on spaces.

That's a good idea, and I think we'd be able to use the same pref even once we have a "real editor".
Comment 16 Dietrich Ayala (:dietrich) 2011-06-09 12:05:03 PDT
Comment on attachment 537147 [details] [diff] [review]
updated patch

Agree w/ D & K, this is an appropriate use of a preference, given that this particular issue makes or breaks a feature in such a contentious manner. Canceling review until that bit is addressed. Points for figuring out a way to get proper \t support in there, maybe a -1 value or somesuch?
Comment 17 Mihai Sucan [:msucan] 2011-06-11 07:47:42 PDT
(In reply to comment #16)
> Comment on attachment 537147 [details] [diff] [review] [review]
> updated patch
> 
> Agree w/ D & K, this is an appropriate use of a preference, given that this
> particular issue makes or breaks a feature in such a contentious manner.
> Canceling review until that bit is addressed. Points for figuring out a way
> to get proper \t support in there, maybe a -1 value or somesuch?

Well, we need two prefs for that. One is tab size, which is valid even if we use a proper \t. The tab size option would change how many visual spaces are shown when \t is used. The second option would be "expand tabs" to use spaces instead of \t.
Comment 18 Mihai Sucan [:msucan] 2011-06-11 08:28:46 PDT
Created attachment 538699 [details] [diff] [review]
patch update 2

Updated the patch as requested. Added two options: expandtab and tabsize as explained in the previous comment.

Please note that I am not asking for feedback again, because the change to accommodate the new prefs is trivial/minimal. Added four lines of code to scratchpad.js, out of which two lines read the two prefs.

I also updated the mochitest to make use of the two new prefs.

Looking forward to your review, thanks!
Comment 19 Dietrich Ayala (:dietrich) 2011-06-13 09:29:21 PDT
Comment on attachment 538699 [details] [diff] [review]
patch update 2

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

::: browser/base/content/scratchpad.js
@@ +631,5 @@
> +
> +    let tabsize = Services.prefs.getIntPref("devtools.editor.tabsize");
> +    let expandtab = Services.prefs.getBoolPref("devtools.editor.expandtab");
> +    this._tabCharacter = expandtab ? (new Array(tabsize + 1)).join(" ") : "\t";
> +    this.textbox.style.MozTabSize = tabsize;

can you add validation and suitable defaults for when users put garbage there, and add tests this scenario?

should be ready to r+ with that change, thanks!
Comment 20 Mihai Sucan [:msucan] 2011-06-14 02:49:10 PDT
(In reply to comment #19)
> Comment on attachment 538699 [details] [diff] [review] [review]
> patch update 2
> 
> Review of attachment 538699 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/scratchpad.js
> @@ +631,5 @@
> > +
> > +    let tabsize = Services.prefs.getIntPref("devtools.editor.tabsize");
> > +    let expandtab = Services.prefs.getBoolPref("devtools.editor.expandtab");
> > +    this._tabCharacter = expandtab ? (new Array(tabsize + 1)).join(" ") : "\t";
> > +    this.textbox.style.MozTabSize = tabsize;
> 
> can you add validation and suitable defaults for when users put garbage
> there, and add tests this scenario?

As far as I know getBoolPref will not give back anything other than a boolean. Similarly, getIntPref() will not give back anything but an integer. Please correct me if I am wrong.

For tabsize it makes sense to check if the given integer is lower than 1, and use a default of 4 spaces.

Thank you!
Comment 21 Mihai Sucan [:msucan] 2011-06-14 03:48:28 PDT
Created attachment 539167 [details] [diff] [review]
[in-devtools] patch update 3

Updated to address the last review comment.

Thank you!
Comment 22 Dietrich Ayala (:dietrich) 2011-06-15 09:56:45 PDT
Comment on attachment 539167 [details] [diff] [review]
[in-devtools] patch update 3

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

r=me, thanks!
Comment 23 Mihai Sucan [:msucan] 2011-06-16 01:03:01 PDT
Dietrich: thank you for the r+!

Try server results are green:
http://tbpl.mozilla.org/?tree=Try&rev=e726cf5b4687
Comment 24 Dave Camp (:dcamp) 2011-06-16 11:55:59 PDT
Comment on attachment 539167 [details] [diff] [review]
[in-devtools] patch update 3

http://hg.mozilla.org/projects/devtools/rev/6f5e8757e0cd
Comment 25 Rob Campbell [:rc] (:robcee) 2011-06-24 12:08:39 PDT
http://hg.mozilla.org/mozilla-central/rev/6f5e8757e0cd
Comment 26 George Carstoiu 2011-06-28 06:41:47 PDT
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110627 Firefox/7.0a1

Verified issue on Ubuntu 11.04, Mac OS X 10.6, WinXP, Win 7 using steps from Comment 0.

Setting status to Verified Fixed.

Note You need to log in before you can comment on or make changes to this bug.