1284855 - Reps: match spacing and brace placement in nested objects/arrays

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Lin Clark [:linclark]]

For example, the console currently outputs this:

> Object { foo: Object }

Reps would output this:

> Object{foo: Object{}}

Comment 1

[Steve Chung [:steveck]]

Sorry I didn't get this part:
(In reply to Lin Clark [:linclark] from comment #0)
> For example, the console currently outputs this:
> 
> > Object { foo: Object }
> 
> Reps would output this:
> 
> > Object{foo: Object{}}

So it means that all the nested object reps should be displayed like Object{foo: Object{}} instead of Object { foo: Object }, and the test[1] should be revised to:

> const defaultOutput = `Object{objProp: Object{}, strProp: "test string"}`;
 
And we should start from object.js. Hope that I didn't misunderstand the requirement.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/test/mochitest/test_reps_grip.html#150

Comment 2

[Lin Clark [:linclark]]

Sorry, my wording was not clear.

We want the reps to output as the current console does. If you did a console.log({foo: "bar"}) in the current console, it would show up as:

> Object { foo: "bar" }

That's what we want.

There is a separate issue for making sure that the space between "Object" and "{" is there. This issue is just about the spacing inside the nested object/array/etc.

Comment 3

[Steve Chung [:steveck]]

Ok, does it only happen in object/array reps, not in grip reps with object/array type? I'm still confused about where to find the nested object/array case in the panel, because it seems that the general case of nested object display would be "Object { foo: Object }".

Comment 4

[Lin Clark [:linclark]]

It also happens in grips and grip-arrays.

Sorry, my wording was confusing. I meant the properties nested inside the object. So the spacing around the properties.

Comment 5

[Steve Chung [:steveck]]

Thanks for the explanation, it looks like the current display between brace/bracket and properties uses css margin for the spacing. Could I assume that we should replace all the margin styling for objectBrace/objectBracket with real space in js?

Comment 6

[Lin Clark [:linclark]]

We should get Honza's opinion on this, but I would say that yes, we should replace margin styling for real spaces.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #6)
> We should get Honza's opinion on this, but I would say that yes, we should
> replace margin styling for real spaces.
Agree, using real spaces is better when coping rendered text into the clipboard.

Honza

Comment 8

[Steve Chung [:steveck]]

Thanks, hope it's the last question about the spacing: For the grip in tiny mode, it would also have a object with class "objectLeftBrace" and empty text content at the end[1]. I'm not sure if this object is necessary for objectBox. Should we simply remove it, or rename the class instead?

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/reps/grip.js#162-168

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Steve Chung [:steveck] from comment #8)
> Thanks, hope it's the last question about the spacing: For the grip in tiny
> mode, it would also have a object with class "objectLeftBrace" and empty
> text content at the end[1]. I'm not sure if this object is necessary for
> objectBox. Should we simply remove it, or rename the class instead?

Are talking about a space between `Object` and `,` ?

E.g. Object { foo: Object , bar: Object }

As far as I can tell this space is again CSS margin and as soon as we remove it from reps.css it'll disappear. Does this answer the question?

Honza

Comment 10

[Steve Chung [:steveck]]

Attachment: bug-1284855.patch

Hi Honza,

It's patch for the replacing the styling with real space. Not sure if it totally fit the requirement. I'll also reply the question in the review.

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8772350 [details] [diff] [review]
bug-1284855.patch

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

::: devtools/client/shared/components/test/mochitest/test_reps_array.html
@@ +47,5 @@
>      const renderedRep = shallowRenderComponent(Rep, { object: stub });
>      is(renderedRep.type, ArrayRep.rep, `Rep correctly selects ${ArrayRep.rep.displayName}`);
>  
>      // Test rendering
> +    const defaultOutput = `[  ]`;

It will cause the empty array displayed with 2 spaces inside the brackets, maybe we should remove all the spaces(or only left one space)?

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8772350 [details] [diff] [review]
bug-1284855.patch

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

(In reply to Steve Chung [:steveck] from comment #11)
> Comment on attachment 8772350 [details] [diff] [review]
> bug-1284855.patch
> 
> Review of attachment 8772350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/components/test/mochitest/test_reps_array.html
> @@ +47,5 @@
> >      const renderedRep = shallowRenderComponent(Rep, { object: stub });
> >      is(renderedRep.type, ArrayRep.rep, `Rep correctly selects ${ArrayRep.rep.displayName}`);
> >  
> >      // Test rendering
> > +    const defaultOutput = `[  ]`;
> 
> It will cause the empty array displayed with 2 spaces inside the brackets,
> maybe we should remove all the spaces(or only left one space)?

Yes, this is the part I don't like in the patch.

Can we do it as follows?

1) No space if the array is empty, example:  `[]`
2) No space if the array is displayed in tiny mode with number of items inside, example:  `[8]`
3) Spaces in short and long modes: [ 1, 2, 3 ]
4) the same for objects: `{}`, `{ p1: "value", p2: value }`

Honza

Comment 13

[Steve Chung [:steveck]]

Attachment: Bug 1284855 - Reps: match spacing and brace placement in nested objects/arrays.

Review commit: https://reviewboard.mozilla.org/r/66008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66008/

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8773626 [details]
Bug 1284855 - Reps: match spacing and brace placement in nested objects/arrays.

https://reviewboard.mozilla.org/r/66008/#review63128

R+, but please resolve the one small thing I commented.

Honza

::: devtools/client/shared/components/reps/object.js:162
(Diff revision 1)
>            this.getTitle(object),
>            objectLink({
>              className: "objectLeftBrace",
>              role: "presentation",
>              object: object
> -          }, "{"),
> +          }, "{ "),

Why there is no space before `{`?
It should be consistent with Grip rep.

Honza

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This will also need try push

Honza

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8773626 [details]
Bug 1284855 - Reps: match spacing and brace placement in nested objects/arrays.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66008/diff/1-2/

Comment 17

[Steve Chung [:steveck]]

Thanks for the review! I applied the suggestion and try build has 2 unrelated  failed.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8773626 [details]
Bug 1284855 - Reps: match spacing and brace placement in nested objects/arrays.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66008/diff/2-3/

Comment 20

[Steve Chung [:steveck]]

Patch rebased and try build passed.

Comment 21

[Carsten Book [:Tomcat]]

has problems to apply:

1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/grip-array.js.rej
patching file devtools/client/shared/components/test/mochitest/test_reps_grip-array.html
Hunk #1 FAILED at 70
Hunk #2 FAILED at 98
Hunk #3 FAILED at 158
3 out of 3 hunks FAILED -- saving rejects to file devtools/client/shared/components/test/mochitest/test_reps_grip-array.html.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8773626 [details]
Bug 1284855 - Reps: match spacing and brace placement in nested objects/arrays.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66008/diff/3-4/

Comment 23

[Steve Chung [:steveck]]

Rebased to central again.

Comment 24

[Steve Chung [:steveck]]

Request checkin-needed again. It is not possible to bust OSX build with this patch... :/

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing :(

applying db587c44ba37
patching file devtools/client/shared/components/test/mochitest/test_reps_array.html
Hunk #2 FAILED at 101
Hunk #4 FAILED at 188
Hunk #5 FAILED at 218
3 out of 5 hunks FAILED -- saving rejects to file devtools/client/shared/components/test/mochitest/test_reps_array.html.rej
patching file devtools/client/shared/components/test/mochitest/test_reps_grip-array.html
Hunk #2 FAILED at 100
1 out of 4 hunks FAILED -- saving rejects to file devtools/client/shared/components/test/mochitest/test_reps_grip-array.html.rej
patching file devtools/client/shared/components/test/mochitest/test_reps_grip.html
Hunk #2 FAILED at 97
Hunk #3 FAILED at 134
2 out of 4 hunks FAILED -- saving rejects to file devtools/client/shared/components/test/mochitest/test_reps_grip.html.rej
patching file devtools/client/shared/components/test/mochitest/test_reps_object.html
Hunk #2 FAILED at 95
Hunk #3 FAILED at 121
2 out of 4 hunks FAILED -- saving rejects to file devtools/client/shared/components/test/mochitest/test_reps_object.html.rej
patch failed to apply

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8773626 [details]
Bug 1284855 - Reps: match spacing and brace placement in nested objects/arrays.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66008/diff/4-5/

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Still doesn't apply.

patching file devtools/client/shared/components/reps/array.js
Hunk #1 FAILED at 117
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/array.js.rej
patching file devtools/client/shared/components/reps/grip-array.js
Hunk #1 FAILED at 100
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/grip-array.js.rej
patching file devtools/client/shared/components/reps/grip.js
Hunk #1 FAILED at 183
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/grip.js.rej
patching file devtools/client/shared/components/reps/object.js
Hunk #1 FAILED at 145
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/object.js.rej
patch failed to apply

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7e914abf3d
Baseline fixes for handling custom NaN payloads; r=lth

Comment 29

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Pulsebot from comment #28)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7e914abf3d
> Baseline fixes for handling custom NaN payloads; r=lth

Oops sorry, landed with the wrong bug number; backed out:

o  changeset:   307856:2dda5f625bc0
|  summary:     Backed out changeset 3e7e914abf3d for landing with the wrong bug number;

Comment 30

[Steve Chung [:steveck]]

Comment on attachment 8773626 [details]
Bug 1284855 - Reps: match spacing and brace placement in nested objects/arrays.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66008/diff/5-6/

Comment 31

[Steve Chung [:steveck]]

Rebase the patch again and triggered a try build...(keep my fingers crossed that no more conflicts)

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a63498564d7d
Reps: match spacing and brace placement in nested objects/arrays. r=Honza

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a63498564d7d

Comment 34

[Steve Chung [:steveck]]

Could we close this bug since it's already in central(and the reason why it left as open seems not the patch's problem)?

Comment 35

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Steve Chung [:steveck] from comment #34)
> Could we close this bug since it's already in central(and the reason why it
> left as open seems not the patch's problem)?

Yes, sorry for the trouble.

Comment 36

[Steve Chung [:steveck]]

Hi Wes, could we close this bug, since this patch is already merged in central?