Closed Bug 1353038 Opened 8 years ago Closed 8 years ago

Quoting strings breaks pprint

Categories

(DevTools :: Console, defect, P3)

53 Branch
defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: Oriol, Assigned: tromey)

References

Details

Attachments

(1 file)

Bug 1342526 displays strings in a quoted form in the console. This makes pprint basically useless: > pprint([1,2,3,4,5]) " 0: 1\n 1: 2\n 2: 3\n 3: 4\n 4: 5" Previously it was better: > pprint([1,2,3,4,5]) 0: 1 1: 2 2: 3 3: 4 4: 5 And probably print() shouldn't be quoted neither. The problem is that, unlike console.log(), these functions return a string instead of logging it. So it can't be fixed easily. This could be solved by checking if the code being evaluated is of the form pprint(expr), and in that case display the result unquoted. But this is hacky and will produce a different result for pprint(expr)+"". So I prefer backing out bug 1342526 and marking it as wontfix. The console is supposed to show the result of evaluating your JS code in a meaningful way. It's not supposed to show valid JS code.
Another option is to have the string formatter display newlines and tabs in the console, but not when used in something like the variables view. WONTFIXing 1342526 would be inappropriate as it is already a regression from the previous ("old") console.
Not sure I understand, it seems variablesview already removes newlines and tabs: > inspect({"a\nb\tc" : "a\nb\tc"}) abc: "abc" __proto__: Object The old console had quotes around strings but did not escape characters. This was a bit confusing, e.g. > '"' """ so I liked when the new console removed the quotes. What it might make sense is to escape strings in object previewers. Before bug 1342526, > ({'\n\ta: "b",\n\ta' : "a\nb\tc\n"}) Object { a: "b", a: "a\nb\tc\n" } was a bit weird, maybe the current is better: > ({"a\nb\tc":"a\nb\tc"}) Object { "\n\ta: \"b\",\n\ta": "a\nb\tc\n" } But for simple strings, I think they should be displayed raw.
(In reply to Oriol from comment #2) > Not sure I understand, it seems variablesview already removes newlines and > tabs: Yeah, but object inspector doesn't (now) and that's what we'll be switching to. Escaping in this context is important as otherwise the display doesn't accurately reflect the data. > But for simple strings, I think they should be displayed raw. I don't agree, because this approach means that string results aren't distinguished. For example, a case like `return 23` versus `return "23"`.
A regression from a patch of mine, so taking.
Assignee: nobody → ttromey
(In reply to Tom Tromey :tromey from comment #3) > I don't agree, because this approach means that string results aren't > distinguished. They are distinguished via color: --number-color: var(--theme-highlight-green); --string-color: var(--theme-highlight-orange); However, this might be problematic for people with color perception problems. Alternatively, there is the Chrome approach: add quotes, do not escape, and only highlight the string without the quotes. Basically, what the old console did, but with better syntax highlighting.
Priority: -- → P3
Step 2 will be a fix in the console but I believe that will be blocked until a new reps bundle is imported.
Depends on: 1357341
Comment on attachment 8860127 [details] Bug 1353038 - do not quote whitespace in console evaluation results; https://reviewboard.mozilla.org/r/132156/#review135280 Thanks Tom. Looks good, R+ if TRY is green ::: devtools/client/webconsole/new-console-output/components/message-types/evaluation-result.js:46 (Diff revision 1) > - messageBody = GripMessageBody({grip: parameters, serviceContainer, useQuotes: true}); > + messageBody = GripMessageBody({grip: parameters, serviceContainer, useQuotes: true, > + escapeWhitespace: false}); nit: I like it better like ``` GripMessageBody({ grip: parameters, serviceContainer, useQuotes: true, escapeWhitespace: false }) ``` Feel free to do it or leave it as is
Attachment #8860127 - Flags: review?(nchevobbe) → review+
Also fixing an eslint error.
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/279c3586de53 do not quote whitespace in console evaluation results; r=nchevobbe
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Now it's better, but it's still ugly because quotes are escaped: > pprint(['a','b','c','d']) " 0: \"a\" 1: \"b\" 2: \"c\" 3: \"d\"" In the old console it was better: > pprint(['a','b','c','d']) " 0: "a" 1: "b" 2: "c" 3: "d"" And before bug 1342526 it was perfect: > pprint(['a','b','c','d']) 0: "a" 1: "b" 2: "c" 3: "d" Should I file another bug?
Oriol, yes please :)
I have reproduced this bug with Nightly 55.0a1 (2017-04-03) on Windows 10 (64 bit). This bug's fix is verified on Latest Nightly 56.0a1. Build ID: 20170616030207 User Agent : Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0 [bugday-20170614]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: