Overlapping negative line numbers do not display as a stack

RESOLVED FIXED in Firefox 57

Status

DevTools
Inspector
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: micah, Assigned: micah)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Created attachment 8891146 [details]
Screenshot from 2017-07-27 18-43-21.png

A secondary box is shown beneath a grid line number box to indicate multiple line numbers exist on that same line (Please see attachment for visual aid). This is only working for positive line numbers, but it should also do the same for negatives as well. 

In the case of the attached image, the negative line rows should be as follows:

-1, -2...-3, -4, -5, -6 where the -3 should be shown over -2.
(Assignee)

Updated

a year ago
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8891700 [details]
Bug 1385144 - Overlapping negative line numbers do not display as a stack.

https://reviewboard.mozilla.org/r/162762/#review168330

::: devtools/server/actors/highlighters/css-grid.js:1178
(Diff revision 1)
> -      const negativeLineNumber = i - lines.length;
> +      let negativeLineNumber = i - lines.length;
> +
> +      // Check for overlapping lines. We render a second box beneath the last overlapping
> +      // line number to indicate there are lines beneath it.
> +      const gridLine = gridDimension.tracks[line.number - 1];
> +      if (gridLine) {

Add a new line before the if statement.

::: devtools/server/actors/highlighters/css-grid.js:1180
(Diff revision 1)
> +      // Check for overlapping lines. We render a second box beneath the last overlapping
> +      // line number to indicate there are lines beneath it.
> +      const gridLine = gridDimension.tracks[line.number - 1];
> +      if (gridLine) {
> +        const { breadth }  = gridLine;
> +        if (breadth === 0) {

Add a new line before the if statement

::: devtools/server/actors/highlighters/css-grid.js:1182
(Diff revision 1)
> +      const gridLine = gridDimension.tracks[line.number - 1];
> +      if (gridLine) {
> +        const { breadth }  = gridLine;
> +        if (breadth === 0) {
> +          stackedLines.push(negativeLineNumber);
> +          if (stackedLines.length > 0) {

Add a new line before the if statement

::: devtools/server/actors/highlighters/css-grid.js:1186
(Diff revision 1)
> +          stackedLines.push(negativeLineNumber);
> +          if (stackedLines.length > 0) {
> +            this.renderGridLineNumber(negativeLineNumber, linePos, lineStartPos,
> +              line.breadth, dimensionType, 1);
> +          }
> +          continue;

Add a new line after the if statement.

Comment 3

a year ago
mozreview-review
Comment on attachment 8891700 [details]
Bug 1385144 - Overlapping negative line numbers do not display as a stack.

https://reviewboard.mozilla.org/r/162762/#review168342

Looks great, Thanks!

::: devtools/server/actors/highlighters/css-grid.js:1170
(Diff revision 1)
> +    // Keep track of the number of collapsed lines per line position
> +    let stackedLines = [];
> +
>      const { lines } = gridDimension;
>  
>      for (let i = 0, line = lines[i]; i < lines.length; line = lines[++i]) {

I think this is my fault, I wrote a code like that in a previous review without actually explain it, sorry! Basically you can simplify this line as follow:

```js
for (let i = 0, line; line = lines[i++];) {
 ...
}
```

You don't need to initialize `line`, just declare it. And you don't need the _final expression_, just the _condition_. At each loop in _condition_ you both assign `line`, increment `i` and check if `line` is something that is not evaluated as "falsy". Since it's an array of objects, in sequence, it will be "falsy" only when `lines[i++]` will be `undefined` and that will happen when exceed the values of `lines.length`.

Unless, of course, we've a sparse array but I don't think it's our case.

Plase simplify that here and in the positive number part too (where I belive is still there).
Attachment #8891700 - Flags: review+
Attachment #8891700 - Flags: review?(gl)
Comment hidden (mozreview-request)

Comment 5

a year ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd270d4fe4bc
Overlapping negative line numbers do not display as a stack. r=zer0
Comment hidden (mozreview-request)
(In reply to Phil Ringnalda (:philor) from comment #6)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe87ddf6ce4 for
> eslint bustage,
> https://treeherder.mozilla.org/logviewer.html#?job_id=120249996&repo=mozilla-
> inbound

It's my bad. I didn't notice that my eslint didn't work anymore as expected, so I didn't realize the for…loop wasn't accepted by our eslint rules.

Micah, you basically have to wrap the assignment, so that it would looks like more as expression – and therefore intended:


```js
for (let i = 0, line; (line = lines[i++]);) {
 ...
}
```

My apologies!
Flags: needinfo?(tigleym)

Comment 9

a year ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57ed39ea453a
Overlapping negative line numbers do not display as a stack. r=zer0
Flags: needinfo?(tigleym)

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/57ed39ea453a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.