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

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.

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.

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.

(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.

(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.

Proposed fix and test. I also added textbox.focus() because it seemed trivial and worthy of doing so. Feedback is welcome!

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 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.

(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)

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.

> + this.selectRange(firstPiece.length + 4, firstPiece.length + 4); magic numbers :(

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 on attachment 537147 [details] [diff] [review] updated patch Asking for review from Dietrich.

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.

(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 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?

(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.

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 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!

(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!

Updated to address the last review comment. Thank you!

Comment on attachment 539167 [details] [diff] [review] [in-devtools] patch update 3 Review of attachment 539167 [details] [diff] [review]: ----------------------------------------------------------------- r=me, thanks!

Dietrich: thank you for the r+! Try server results are green: http://tbpl.mozilla.org/?tree=Try&rev=e726cf5b4687

Comment on attachment 539167 [details] [diff] [review] [in-devtools] patch update 3 http://hg.mozilla.org/projects/devtools/rev/6f5e8757e0cd

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.