Closed Bug 1374298 Opened 7 years ago Closed 7 years ago

Filter ping by type in about:telemetry

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: flyingrub, Assigned: flyingrub)

References

Details

Attachments

(4 files)

Bug 1374298 - Filter ping by type in about:telemetry
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1374298 - Convert the ping-picker to a popup in about:telemetry
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1374298 - Revisit the ping selection in about:telemetry
59 bytes, text/x-review-board-request
chutten
: review+
Details
bug 1374298 - revert in-place translation change.
59 bytes, text/x-review-board-request
flod
: review+
Details 
      No description provided.
Comment on attachment 8879155 [details]
Bug 1374298 - Filter ping by type in about:telemetry

https://reviewboard.mozilla.org/r/150488/#review155328

::: toolkit/content/aboutTelemetry.js:270
(Diff revision 1)
>  };
>  
>  var PingPicker = {
>    viewCurrentPingData: null,
>    _archivedPings: null,
> +  TYPE_ALL: "all",

I'm on the fence about whether or not this should be localised. What do you think? The other ping types certainly won't be, but this one has special meaning.

::: toolkit/content/aboutTelemetry.js:468
(Diff revision 1)
> +    let first = true;
> +    let pingSelector = document.getElementById("choose-ping-id");
> +    let typeSelector = document.getElementById("choose-ping-type");
> +    let type = typeSelector.options[typeSelector.selectedIndex].value;
> +    if (type == this.TYPE_ALL) {
> +      pingSelector.childNodes.forEach((option) => {

more straightforward as

pingSelector.children.forEach(option => option.hidden = false);
pingSelector.children[0].selected = true;

::: toolkit/content/aboutTelemetry.js:476
(Diff revision 1)
> +          first = false;
> +        }
> +        option.hidden = false;
> +      });
> +    } else {
> +      pingSelector.childNodes.forEach((option) => {

Similarly, use children here. childNodes includes text Nodes, if present.

::: toolkit/content/aboutTelemetry.xhtml:123
(Diff revision 1)
>              <button id="newer-ping" type="button">&aboutTelemetry.showNewerPing;</button>
>              <button id="older-ping" type="button">&aboutTelemetry.showOlderPing;</button><br />
>              <table>
>                <tr>
>                    <th>&aboutTelemetry.archiveWeekHeader;</th>
> +                  <th>Ping type</th>

This requires l10n
Attachment #8879155 - Flags: review?(chutten) → review-
Comment on attachment 8879155 [details]
Bug 1374298 - Filter ping by type in about:telemetry

https://reviewboard.mozilla.org/r/150488/#review158644

::: toolkit/content/aboutTelemetry.css:62
(Diff revision 4)
> +  cursor: pointer;
> +}
> +
> +.controls {
> +  display: flex;
> +  justify-content: space-between; 

EOL whitespace

::: toolkit/content/aboutTelemetry.js:343
(Diff revision 4)
> -    let pingName = "<span class=\"change-ping\">" + this._getSelectedPingName() + "</span>";
> +    let pingName = this._getSelectedPingName();
> +
> +    let pingDate = document.getElementById("ping-date");
> +    pingDate.textContent = pingName;
> +    pingDate.setAttribute('title', pingName);
> +    

EOL whitespace

::: toolkit/content/aboutTelemetry.js:345
(Diff revision 4)
> +    let pingDate = document.getElementById("ping-date");
> +    pingDate.textContent = pingName;
> +    pingDate.setAttribute('title', pingName);
> +    
> +    let pingType = document.getElementById("ping-type");
> +    let older = document.querySelector(".older-ping");

You're using querySelectorAll above, and do indeed have multiple instances of elements with these classes.

You should either querySelector from some parent Element instead of document, or add another class/id that will allow you to uniquely identify these ones.

::: toolkit/content/aboutTelemetry.js:350
(Diff revision 4)
> +    let older = document.querySelector(".older-ping");
> +    let newer = document.querySelector(".newer-ping");
> +    if (pingName !== "current") {
> +      pingType.hidden = false;
> +      older.hidden = false;
> +      newer.hidden = false;

All this suggests to me that these should be grouped under a span/div so that we only need to set hidden on a single element.

::: toolkit/content/aboutTelemetry.js:497
(Diff revision 4)
>        pingSelector.appendChild(option);
>      }
> +    this._renderPingTypes(pingTypes);
> +  },
> +
> +filterDisplayedPings() {

Indentation's off here...

...looks like a lot of things `mach eslint` ought to catch.

::: toolkit/content/aboutTelemetry.js:502
(Diff revision 4)
> +filterDisplayedPings() {
> +    let pingSelector = document.getElementById("choose-ping-id");
> +    let typeSelector = document.getElementById("choose-ping-type");
> +    let type = typeSelector.selectedOptions.item(0).value;
> +    if (type == this.TYPE_ALL) {
> +      pingSelector.childNodes.forEach(option => option.hidden = false);

Use children instead of childNodes throughout. That way you're only iterating over Elements, not Nodes.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd:36
(Diff revision 4)
>  <!ENTITY aboutTelemetry.showNewerPing "
> -&lt;&lt; Newer ping
> +Newer ping &gt;&gt;
>  ">
>  
>  <!ENTITY aboutTelemetry.showOlderPing "
> -Older ping &gt;&gt;
> +&lt;&lt; Older ping 

EOL Whitespace
Attachment #8879155 - Flags: review?(chutten) → review-
Comment on attachment 8882144 [details]
Bug 1374298 - Convert the ping-picker to a popup in about:telemetry

https://reviewboard.mozilla.org/r/153264/#review158664

I'm not convinced that this is a good interaction method for picking pings. The popover blocks other content and doesn't match any other control I can think of in other about: pages.

I think we should ask for UX help with this, and for now move on to something else.

::: toolkit/content/aboutTelemetry.js:374
(Diff revision 2)
>      let pingNameHtml = "<span class=\"change-ping\">" + pingName + "</span>";
>  
>      let explanation = bundle.formatStringFromName("pingExplanation", [pingLink, pingNameHtml], 2);
>      let pingExplanation = document.getElementById("ping-explanation");
>      pingExplanation.innerHTML = explanation;
> +    pingExplanation.querySelector('.change-ping').addEventListener('click', () =>

Should be using double-quotes throughout, if I'm not mistaken.

::: toolkit/content/aboutTelemetry.xhtml:139
(Diff revision 2)
> +      <div class="header">
> +          <div id="sectionTitle" class="header-name">
> +              &aboutTelemetry.pageTitle;
> +          </div>
> +      </div>
> +      

EOL Whitespace
Attachment #8882144 - Flags: review?(chutten) → review-
Comment on attachment 8879155 [details]
Bug 1374298 - Filter ping by type in about:telemetry

https://reviewboard.mozilla.org/r/150488/#review158644

> Use children instead of childNodes throughout. That way you're only iterating over Elements, not Nodes.

children doesn't have the forEach iterator, which is very practical.
But we could use Array.from().forEach() :)
Comment on attachment 8882144 [details]
Bug 1374298 - Convert the ping-picker to a popup in about:telemetry

https://reviewboard.mozilla.org/r/153264/#review158664

So you think I should revert this commit ?
I think this popup is very practical, as you can open it on any section. You can then hide it to show the rest of the page.
Comment on attachment 8879155 [details]
Bug 1374298 - Filter ping by type in about:telemetry

https://reviewboard.mozilla.org/r/150488/#review158644

> All this suggests to me that these should be grouped under a span/div so that we only need to set hidden on a single element.

```
        <span id="ping-type" hidden="true" class="change-ping"></span>
        <div class="controls">
          <span id="older-ping" hidden="true">&lt;&lt;</span>
          <span id="ping-date" class="change-ping"></span>
          <span id="newer-ping" hidden="true">&gt;&gt;</span>
        </div>
```

The problem in grouping all those element is that #ping-date must be between #older-ping and #newer-ping and #ping-type above them...
Comment on attachment 8879155 [details]
Bug 1374298 - Filter ping by type in about:telemetry

https://reviewboard.mozilla.org/r/150488/#review159348

::: toolkit/content/aboutTelemetry.css:53
(Diff revision 5)
> +  margin: 0;
> +  padding-bottom: 12px;
> +}
> +
> +#ping-type {
> +    align-self: center;

two-space indent for consistency, please

::: toolkit/content/aboutTelemetry.css:62
(Diff revision 5)
> +  cursor: pointer;
> +}
> +
> +.controls {
> +  display: flex;
> +  justify-content: space-between; 

end-of-line whitespace should be omitted

::: toolkit/content/aboutTelemetry.js:358
(Diff revision 5)
> +      pingType.hidden = true;
> +      older.hidden = true;
> +      newer.hidden = true;
> +    }
> +
> +    if (pingName !== "current") pingName += ", " + this._getSelectedPingType();

single-line if statements are discouraged in favour of three-line statements with both newlines and braces

::: toolkit/content/aboutTelemetry.js:502
(Diff revision 5)
> +      Array.from(pingSelector.children).forEach(option => option.hidden = false);
> +      pingSelector.children[0].selected = true;
> +    } else {
> +      let first = true;
> +      Array.from(pingSelector.children).forEach((option) => {

Parens are optional for argument lists of length one on arrow functions. If the linter (mach eslint) doesn't tell you which one it requires, chose one and be consistent through the file (see line 502, for instance)

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd:36
(Diff revision 5)
>  <!ENTITY aboutTelemetry.showNewerPing "
> -&lt;&lt; Newer ping
> +Newer ping &gt;&gt;
>  ">
>  
>  <!ENTITY aboutTelemetry.showOlderPing "
> -Older ping &gt;&gt;
> +&lt;&lt; Older ping 

EOL whitespace
Attachment #8879155 - Flags: review?(chutten) → review-
Comment on attachment 8882144 [details]
Bug 1374298 - Convert the ping-picker to a popup in about:telemetry

https://reviewboard.mozilla.org/r/153264/#review159350

::: toolkit/content/aboutTelemetry.xhtml:99
(Diff revision 3)
>            <span class="category-name">&aboutTelemetry.raw;</span>
>        </div>
>      </div>
>  
>      <div class="main-content">
> -      <div class="header">
> +      

EOL whitespace

::: toolkit/content/aboutTelemetry.xhtml:139
(Diff revision 3)
> +      <div class="header">
> +          <div id="sectionTitle" class="header-name">
> +              &aboutTelemetry.pageTitle;
> +          </div>
> +      </div>
> +      

more EOL whitespace
Attachment #8882144 - Flags: review?(chutten) → review-
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review159352

::: toolkit/content/aboutTelemetry.js:186
(Diff revision 1)
>    d.setDate(d.getDate() - 1);
>    return d;
>  }
>  
>  /**
> + * Return tomorrows date with the same time.

s/tomorrows/tomorrow's/

::: toolkit/content/aboutTelemetry.js:428
(Diff revision 1)
>    },
>  
>    _updateArchivedPingData() {
>      let id = this._getSelectedPingId();
> -    return TelemetryArchive.promiseArchivedPingById(id)
> +    if (id) {
> +      TelemetryArchive.promiseArchivedPingById(id)

This used to return the promise from the TelemetryArchive method. We should probably keep that behaviour.

::: toolkit/content/aboutTelemetry.js:429
(Diff revision 1)
>  
>    _updateArchivedPingData() {
>      let id = this._getSelectedPingId();
> -    return TelemetryArchive.promiseArchivedPingById(id)
> +    if (id) {
> +      TelemetryArchive.promiseArchivedPingById(id)
>                             .then((ping) => displayPingData(ping, true));

indentation should have the '.' line up.

::: toolkit/content/aboutTelemetry.js:479
(Diff revision 1)
>      // Update the displayed ping.
>      await this._updateArchivedPingData();
>    },
>  
> -  _renderWeeks() {
> -    let weekSelector = document.getElementById("choose-ping-week");
> +  _getSelectedDate() {
> +    return document.getElementById("choose-date").valueAsDate;

If removing the week chooser temporarily, just comment out the code.

If removing it permanently, remove it completely (including this._weeks, any other storage, and any relevant CSS)

::: toolkit/content/aboutTelemetry.js
(Diff revision 1)
>        let content = document.createTextNode(text);
>        option.appendChild(content);
>        option.setAttribute("value", p.id);
>        option.dataset.type = p.type;
> -      if (id && p.id == id) {
> +      option.dataset.date = datetext;
> -        option.selected = true;

The mechanism for selecting the correct option from the list is unclear.

::: toolkit/content/aboutTelemetry.js:576
(Diff revision 1)
>      let pingSelector = document.getElementById("choose-ping-id");
>      let selected = pingSelector.selectedOptions.item(0);
> -    return selected.getAttribute("value");
> +    return selected ? selected.getAttribute("value") : false;
>    },
>  
>    _movePingIndex(offset) {

It looks like a better solution would be to find the next index that matches the filter properly rather than stepping day by day this way.

So it would be...

let newIndex = ...
let ping = this._ArchivedPings
  .slice(newIndex)
  .find(p => {
    newIndex++;
    return <whatever the filter code is>;
  });
if (!ping) {
  ...uh... ping = this._archivedPings[index]?
}
this._renderPingList(ping.id);
this._updateArchivedPingData();

Then you shouldn't need this.selectPingId.

::: toolkit/content/aboutTelemetry.js:586
(Diff revision 1)
>      const index = this._archivedPings.findIndex((p) => p.id == id);
>      const newIndex = Math.min(Math.max(index + offset, 0), this._archivedPings.length - 1);
>      const ping = this._archivedPings[newIndex];
> +    console.log(newIndex);
>  
> -    const weekIndex = this._weeks.findIndex(
> +    if (!this.selectPingId(ping.id)) {

This looks like fixups for the "filter by ping type" commit rather than to do with the date picker.
Attachment #8883348 - Flags: review?(chutten) → review-
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review159352

> This used to return the promise from the TelemetryArchive method. We should probably keep that behaviour.

This promise was never used as the callback is directly attached in the function.
Should I still let the return ?
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review159352

> The mechanism for selecting the correct option from the list is unclear.

Previously when we selected a ping we entirely recreated the pingList. Now, we just iterate throught the list and try to find the ping to select, if it is not in the list we return false else true.
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review159352

> This promise was never used as the callback is directly attached in the function.
> Should I still let the return ?

async _updateArchivedPingList awaits on this function, so yes.
Comment on attachment 8879155 [details]
Bug 1374298 - Filter ping by type in about:telemetry

https://reviewboard.mozilla.org/r/150488/#review159610

Looks good
Attachment #8879155 - Flags: review?(chutten) → review+
Comment on attachment 8882144 [details]
Bug 1374298 - Convert the ping-picker to a popup in about:telemetry

https://reviewboard.mozilla.org/r/153264/#review159614

I'm still not a fan of popups, but getting this into a build might be the fastest way to get comments :)
Attachment #8882144 - Flags: review?(chutten) → review+
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review159616

::: toolkit/content/aboutTelemetry.css:60
(Diff revision 2)
>    cursor: pointer;
>  }
>  
> -.older-ping, .newer-ping, #ping-date {
> +#older-ping, #newer-ping, #ping-date {
>    pointer-events: all;
> +  -moz-user-select: none;

Changes to this css should be in the earlier commit.

::: toolkit/content/aboutTelemetry.js:516
(Diff revision 2)
> +    let found = false;
> +    // Use some() to break if we find the ping.
> +    Array.from(pingSelector.children).some((group) => {
> +      Array.from(group.children).some((option) => {
> +        if (option.value == ping.id) {
> +          option.selected = true;
> +          found = true;
> +        }
> +        return found;
> +      });
> +      return found;
> +    });

A little more cleanly (since `some` returns the return value) would be to return the value from `some` and ditch `found` altogether.

::: toolkit/content/aboutTelemetry.js:564
(Diff revision 2)
>    },
>  
>    _getSelectedPingId() {
>      let pingSelector = document.getElementById("choose-ping-id");
>      let selected = pingSelector.selectedOptions.item(0);
> -    return selected.getAttribute("value");
> +    return selected ? selected.getAttribute("value") : false;

In what situations is selected empty?

::: toolkit/content/aboutTelemetry.xhtml:113
(Diff revision 2)
>          <div id="current-ping-picker">
>            <input id="show-subsession-data" type="checkbox" checked="checked" />&aboutTelemetry.showSubsessionData;
>          </div>
>          <div id="archived-ping-picker" class="hidden">
>            <h4 class="title">&aboutTelemetry.choosePing;</h4>
> -          <button class="older-ping" type="button">&aboutTelemetry.showOlderPing;</button>
> +          <select id="choose-ping-week" hidden="true"></select>

Shouldn't this be removed?
Attachment #8883348 - Flags: review?(chutten) → review-
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review159616

> In what situations is selected empty?

This was a fix for the previous implementation. It can be removed now.
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review159918

::: toolkit/content/aboutTelemetry.js:450
(Diff revision 3)
> -    return this._weeks[weekSelector.selectedIndex];
> -  },
> -
> -  _renderPingList(id = null) {
>      let pingSelector = document.getElementById("choose-ping-id");
> -    removeAllChildNodes(pingSelector);
> +    Array.from(pingSelector).forEach((child) => removeAllChildNodes(child));

pingSelector is not a nodelist. It's a single element. I think you want pingSelector.children, and to add a manual test to double-check situations where the ping list is re-rendered.

::: toolkit/content/aboutTelemetry.js:467
(Diff revision 3)
>        option.appendChild(content);
>        option.setAttribute("value", p.id);
>        option.dataset.type = p.type;
> -      if (id && p.id == id) {
> -        option.selected = true;
> +      option.dataset.date = datetext;
> +
> +      if (date.toDateString() == (new Date()).toDateString()) {

This new Date() and yesterday(new Date()) are loop invariants. May as well have a todayString and a yesterdayString created outside the loop.

::: toolkit/content/aboutTelemetry.js:498
(Diff revision 3)
> +    let typeSelector = document.getElementById("choose-ping-type");
> +    let type = typeSelector.selectedOptions.item(0).value;
> +
> +    let id = this._getSelectedPingId();
> +    let index = this._archivedPings.findIndex((p) => p.id == id);
> +    let newIndex = Math.min(Math.max(index + offset, 0), this._archivedPings.length - 1);

For readability, these clamp-like calls are generally Math.min(Math.max(low, variable), high). For extra points, put the 0 first in the max call. Then it looks like a math statement:

0 <= index + offset <= this._archivedPings.length - 1
Attachment #8883348 - Flags: review?(chutten) → review-
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review159918

> pingSelector is not a nodelist. It's a single element. I think you want pingSelector.children, and to add a manual test to double-check situations where the ping list is re-rendered.

Good catch ! :)
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review160036

::: toolkit/content/aboutTelemetry.js:450
(Diff revision 4)
> -    return this._weeks[weekSelector.selectedIndex];
> -  },
> -
> -  _renderPingList(id = null) {
>      let pingSelector = document.getElementById("choose-ping-id");
> -    removeAllChildNodes(pingSelector);
> +    Array.from(pingSelector).forEach((child) => removeAllChildNodes(child));

Looks like this still hasn't been fixed?
Attachment #8883348 - Flags: review?(chutten) → review-
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review160036

> Looks like this still hasn't been fixed?

I don't understand why this isn't in the latest revision ...
Comment on attachment 8883348 [details]
Bug 1374298 - Revisit the ping selection in about:telemetry

https://reviewboard.mozilla.org/r/154236/#review160266

Ship it!
Attachment #8883348 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd231109ff44
Filter ping by type in about:telemetry r=chutten
https://hg.mozilla.org/integration/autoland/rev/8d29ef14ca81
Convert the ping-picker to a popup in about:telemetry r=chutten
https://hg.mozilla.org/integration/autoland/rev/358616d03cc0
Revisit the ping selection in about:telemetry r=chutten
Keywords: checkin-needed
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf92c23ed96e
revert in-place translation change. r=flod
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: