975477 - Scratchpad [ Pretty Print ] should not move end of line comments to next line

Status: VERIFIED FIXED

(DevTools Graveyard :: Scratchpad, defect, P4)

Description

[Adrian Aichner [:anaran]]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140220030202

Steps to reproduce:

Please use attached test case and see how end of line comments are moved onto next line.

Not only does it put the comments out of context, it even breaks tools using NON-NLS comments markers for i18n.




Expected results:

End of line comment should just be left there by [ Pretty Print ].

Comment 1

[Adrian Aichner [:anaran]]

Attachment: sample code demonstrating problem in scratchpad.

Just click [ Pretty Print ] and see.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See https://github.com/mozilla/pretty-fast#non-goals

This doesn't break the semantics of the code and in the 99.99% case makes it more readable.

Comment 3

[Adrian Aichner [:anaran]]

I hope I can convince you to really fix this.

js-beautify had this issue and they fixed it:
https://github.com/einars/js-beautify/issues/242

I've never seen a comment documenting the previous line.
Once the confusion would wane that would still be a pretty irritating novelty.
(I just checked firefox sources in the browser toolbox and comments always come befor the line or block they comment.)

Admittedly firefox does not seem to use EOL comments, but shouldn't scratchpad play nice with existing, reasonable source code as well?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Alright, I'll review a patch if you supply one.

Comment 5

[Adrian Aichner [:anaran]]

Attachment: 0002-Bug-975477-Scratchpad-Pretty-Print-moves-end-of-line.patch

OK, I'll bite.

This path contains changes to
git.mozilla.org/gecko-dev/toolkit/devtools/pretty-fast/
which I realize lives upstream at
https://github.com/mozilla/pretty-fast

I can separate this out when my overall approach seems sound.

I have also sneaked in a change to fix switch statement indentation to agree with what codemirror does and the mozilla coding standard at
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
says.

As a consequence I broke the xpcshell tests, but in a good way.

Just let me know how to proceed.

 0:10.19 TEST-UNEXPECTED-FAIL | c:/tmp/git.mozilla.org/gecko-dev/firefox-static/_tests/xpcshell/toolkit/devtools/pretty-fast/tests/unit/test.js | Error: Expected:
switch (x) {
case a:
  foo();
  break;
default:
  bar()
}

Got:
switch (x) {
  case a:
    foo();
    break;
  default:
    bar()
}
 at c:/tmp/git.mozilla.org/gecko-dev/firefox-static/_tests/xpcshell/toolkit/devtools/pretty-fast/tests/unit/test.js:499 ...

Comment 6

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-975477-Scratchpad-Pretty-Print-should-not-move-e.patch

This patch now does what the bug says.
I have moved the switch statement indentation changes to Bug 1018586 Comment 1.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8432126 [details] [diff] [review]
0001-Bug-975477-Scratchpad-Pretty-Print-should-not-move-e.patch

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

Needs a test and see also the comment here about patch formatting and just submitting PRs to upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=1018586#c3

Comment 8

[Adrian Aichner [:anaran]]

Attachment: Upstream pull request, with new test coveralge

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8432770 [details] [review]
Upstream pull request, with new test coveralge

Created bug 1020587 to update pretty-fast in the tree which will fix this.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Resolved as part of https://hg.mozilla.org/mozilla-central/rev/e157171a4df1

Comment 11

[Teodora Vermesan (:TeoVermesan)]

Reproduced the issue on 2014-02-21 Nightly build, verified as fixed on Firefox 32 Beta 2 
(buildID: 20140728123914) under Windows 7 64bit, Mac OS X 10.9 and Ubuntu 64bit.