Improve the search in about:telemetry

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: flyingrub, Assigned: flyingrub)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
* Display a text message like "Nothing was found" when no data is available for the current filter.

* Clear the search filter when switching section.

* The search term could be included in the url for the ease of sharing.
Oh, and the global search on "Home" shouldn't have a placeholder of "Find in Home" it should have a placeholder more like "Find in entire ping" or just something bland and generic like "Search"

The following two notes are from the feedback etherpad (https://public.etherpad-mozilla.org/p/about_telemetry_feedback):

* search result should have headings for "scalars", "histograms", etc. (currently i can't tell what kind of measurement i found)
* the search in start page is named "find in home", should be "find in all" or similar
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8901202 [details]
Bug 1393478 - Clear the search filter when switching section

https://reviewboard.mozilla.org/r/172666/#review178556

::: toolkit/content/aboutTelemetry.js
(Diff revision 1)
>    let blacklist = [
>      "raw-payload-section"
>    ];
>    let search = document.getElementById("search");
>    search.hidden = blacklist.includes(selectedSection);
> -  // Filter element on section change.

Does this mean it will clear the filter when browsing through pings? Is this intentional?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8901202 [details]
Bug 1393478 - Clear the search filter when switching section

https://reviewboard.mozilla.org/r/172666/#review178556

> Does this mean it will clear the filter when browsing through pings? Is this intentional?

this is fixed in the next commit :)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8902184 [details]
Bug 1393478 - Display when no results are available in about:telemetry

https://reviewboard.mozilla.org/r/173658/#review179012

::: toolkit/content/aboutTelemetry.css:159
(Diff revision 1)
>  }
>  
> +#no-search-results {
> +  position: fixed;
> +  top: 50%;
> +  left: 50%;

Rather than cook your own, why not steal about:preferences' "Sorry! There are no results in <Section> for "<searchTerms>"" .... and its cute bear, too :D

::: tools/lint/hooks.py:18
(Diff revision 1)
>  
>  
>  def run_mozlint(hooktype, args):
>      # --quiet prevents warnings on eslint, it will be ignored by other linters
>      python = find_executable('python2.7') or find_executable('python')
> -    cmd = [python, os.path.join(topsrcdir, 'mach'), 'lint', '--quiet']
> +    cmd = [python, os.path.join(topsrcdir, 'mach'), 'lint', '--quiet', '-wo']

I think this isn't something you meant to include
Attachment #8902184 - Flags: review?(chutten) → review-

Comment 8

2 years ago
mozreview-review
Comment on attachment 8901202 [details]
Bug 1393478 - Clear the search filter when switching section

https://reviewboard.mozilla.org/r/172666/#review179016

r+ pending the fixing of the "lose search filter when switching pings" issue in the next commit.
Attachment #8901202 - Flags: review?(chutten) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8902259 [details]
Bug 1393478 - Keep the search input at the same place in about:telemetry

https://reviewboard.mozilla.org/r/173788/#review179098

Consistency is good.
Attachment #8902259 - Flags: review?(chutten) → review+

Comment 18

2 years ago
mozreview-review
Comment on attachment 8902260 [details]
Bug 1393478 - Fix subsection search in about:telemetry

https://reviewboard.mozilla.org/r/173790/#review179100

::: toolkit/content/aboutTelemetry.js:1438
(Diff revision 2)
>          let datas = key.querySelector("table").rows;
>          keyedElements.push({key, datas});
>        }
>        noSearchResults = this.filterKeyedElements(keyedElements, text);
> +    } else if (section.querySelector(".sub-section")) {
> +      let keyedElements = [];

They're not really keyed, though, are they. They're subsections?

I realize we're using filterKeyedElements to filter the subsections, but naming is important for how readable the code will be.
Attachment #8902260 - Flags: review?(chutten)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8902184 [details]
Bug 1393478 - Display when no results are available in about:telemetry

https://reviewboard.mozilla.org/r/173658/#review179096

::: toolkit/content/aboutTelemetry.js:1417
(Diff revision 3)
>        let sectionId = document.querySelector(".category.selected").getAttribute("value");
>        section = document.getElementById(sectionId);
>      }
> +    let noSearchResults;
>      if (section.id === "home-section") {
> -      this.homeSearch(text);
> +      return this.homeSearch(text);

Why is searching on home a special case for search? Does that mean we don't get a "No Results" prompt if  we search on the home page?

::: toolkit/content/aboutTelemetry.js:2121
(Diff revision 3)
>  
>    // Update ping data when async Telemetry init is finished.
>    Telemetry.asyncFetchTelemetryData(async () => {
> -    await PingPicker.update();
>      urlStateRestore();
> +    await PingPicker.update();

These changes seem unintentionally added
Attachment #8902184 - Flags: review?(chutten) → review-

Comment 20

2 years ago
mozreview-review
Comment on attachment 8902286 [details]
Bug 1393478 - Fix Search on home

https://reviewboard.mozilla.org/r/173812/#review179164

This seems fine but putting fixes to bugs introduced by earlier commits in the same bug is confusing and not kind to the source history. Please rework the commits so that the fixes are squashed in the commits that introduced the problems.
Attachment #8902286 - Flags: review?(chutten) → review-
(Assignee)

Comment 21

2 years ago
mozreview-review
Comment on attachment 8902184 [details]
Bug 1393478 - Display when no results are available in about:telemetry

https://reviewboard.mozilla.org/r/173658/#review179258

::: toolkit/content/aboutTelemetry.js:1417
(Diff revision 3)
>        let sectionId = document.querySelector(".category.selected").getAttribute("value");
>        section = document.getElementById(sectionId);
>      }
> +    let noSearchResults;
>      if (section.id === "home-section") {
> -      this.homeSearch(text);
> +      return this.homeSearch(text);

This is for a lint issue. The search on home is introduced on a next commit.

::: toolkit/content/aboutTelemetry.js:2121
(Diff revision 3)
>  
>    // Update ping data when async Telemetry init is finished.
>    Telemetry.asyncFetchTelemetryData(async () => {
> -    await PingPicker.update();
>      urlStateRestore();
> +    await PingPicker.update();

This is needed to keep the search state when reloading.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8902286 - Attachment is obsolete: true

Comment 26

2 years ago
mozreview-review
Comment on attachment 8902184 [details]
Bug 1393478 - Display when no results are available in about:telemetry

https://reviewboard.mozilla.org/r/173658/#review179498
Attachment #8902184 - Flags: review?(chutten) → review+

Comment 27

2 years ago
mozreview-review
Comment on attachment 8902260 [details]
Bug 1393478 - Fix subsection search in about:telemetry

https://reviewboard.mozilla.org/r/173790/#review179500
Attachment #8902260 - Flags: review?(chutten) → review+

Updated

2 years ago
Duplicate of this bug: 1395090
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bed1aa8b73f5
Clear the search filter when switching section r=chutten
https://hg.mozilla.org/integration/autoland/rev/a841cfcda18f
Display when no results are available in about:telemetry r=chutten
https://hg.mozilla.org/integration/autoland/rev/6f00c4602bc1
Keep the search input at the same place in about:telemetry r=chutten
https://hg.mozilla.org/integration/autoland/rev/56940b743275
Fix subsection search in about:telemetry r=chutten
Keywords: checkin-needed
(In reply to Chris H-C :chutten from comment #1)
> The following two notes are from the feedback etherpad
> (https://public.etherpad-mozilla.org/p/about_telemetry_feedback):
> 
> * search result should have headings for "scalars", "histograms", etc.
> (currently i can't tell what kind of measurement i found)
> * the search in start page is named "find in home", should be "find in all"
> or similar

Headings in search results sounds like bug 1395091, but the second one will need its own bug now, right?
Flags: needinfo?(flyinggrub)
(Assignee)

Comment 32

2 years ago
This as already been fixed in Bug 1392230 :)
Flags: needinfo?(flyinggrub)
I don't see this fixed in the current Nightly build.
I get some sub-level headings (e.g. "settings"), but i don't see any top-level headings (e.g. "scalars", "environment").
Flags: needinfo?(flyinggrub)
(Assignee)

Comment 34

2 years ago
I was talking about the second point about the placeholder text of the search on the home page. :D
The heading bug is not fixed for now.
Flags: needinfo?(flyinggrub)
You need to log in before you can comment on or make changes to this bug.