Closed
Bug 1353038
Opened 8 years ago
Closed 8 years ago
Quoting strings breaks pprint
Categories
(DevTools :: Console, defect, P3)
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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Reporter | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
(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"`.
Assignee | ||
Comment 4•8 years ago
|
||
A regression from a patch of mine, so taking.
Assignee: nobody → ttromey
Reporter | ||
Comment 5•8 years ago
|
||
(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.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
Step 2 will be a fix in the console but I believe that will be blocked until a new reps bundle is imported.
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
Also fixing an eslint error.
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/279c3586de53
do not quote whitespace in console evaluation results; r=nchevobbe
Comment 15•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Reporter | ||
Comment 16•8 years ago
|
||
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?
Comment 17•8 years ago
|
||
Oriol, yes please :)
Reporter | ||
Updated•8 years ago
|
Comment 18•7 years ago
|
||
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]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•