about:telemetry - put search terms in the url fragment

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P4
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: chutten, Assigned: vedantc98, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
To make it easier to share what you're looking at, we should include the search terms in the url the same way we include the section name.
(Reporter)

Updated

2 years ago
Blocks: 1384534
No longer depends on: 1384534
I'd like to take up this bug, can I get assigned to this?
(Assignee)

Updated

2 years ago
Flags: needinfo?(chutten)
(Reporter)

Comment 2

2 years ago
Certainly! Let me know if you have any questions.
Assignee: nobody → vedantc98
Status: NEW → ASSIGNED
Flags: needinfo?(chutten)
Should the search query be seperated by a pound sign, like the section name?
(Reporter)

Comment 4

2 years ago
Use whatever you think would be clear to people reading the URLs and easy for developers to maintain in the future.
Posted patch bug_1396855.patch (obsolete) — Splinter Review
I've added a function that does the job.
I'm relatively new here, so please let me know how I could improve. Thanks!
Attachment #8911544 - Flags: review?(chutten)
Attachment #8911544 - Flags: feedback?(chutten)
(Reporter)

Comment 6

2 years ago
Comment on attachment 8911544 [details] [diff] [review]
bug_1396855.patch

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

Looking good so far! You don't need to set both feedback and review to the same person, it's more for asking someone else to take a quick look. Review is a deeper dive.


This is one half of the necessary code. The second half is to place code within urlStateRestore so that loading an about:telemetry url that contains a search term will perform the search on load.

::: toolkit/content/aboutTelemetry.js
@@ +1368,4 @@
>        clearTimeout(this.idleTimeout);
>      }
>      this.idleTimeout = setTimeout(() => Search.search(e.target.value), FILTER_IDLE_TIMEOUT);
> +    changeUrlSearch(e.target.value);

This isn't the only place search is called. It might be better to put this within Search.search itself to catch every case.

Also, that will allow you to properly handle the case when the section that has been switched to doesn't actually have a search field :)
Attachment #8911544 - Flags: review?(chutten)
Attachment #8911544 - Flags: review-
Attachment #8911544 - Flags: feedback?(chutten)
Thanks for the feedback, Chris!
I'll make the necessary changes and submit another patch.
Comment hidden (mozreview-request)
(Reporter)

Comment 9

2 years ago
Comment on attachment 8911544 [details] [diff] [review]
bug_1396855.patch

Marking the old patch as obsolete.
Attachment #8911544 - Attachment is obsolete: true
(Reporter)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8912741 [details]
Bug 1396855 - about:telemetry : Added search terms in the URL fragment.

https://reviewboard.mozilla.org/r/184050/#review189232

Thank you for your patch!

This is a good first draft, I have mostly just small things to say about it.

::: commit-message-bc567:4
(Diff revision 1)
> +Bug 1396855 - about:telemetry : Added search terms in the URL fragment. r=chutten
> +
> +The search from the url hash doesn't work, probably because of Bug 1403585. However, searching in particular sections works, since it ensures the sections load.
> +For example, about:telemetry#environment-data-tab_search=searchTerms works, but about:telemetry#search=searchTerms and about:telemetry#environment-data-tab_partner_search=searchTerms do not.

Before submitting you should fix it so about:telemetry#search=searchTerms works. Subsection you can't do anything about until bug 1403585 is taken care of, but home search is an important new feature of about:telemetry in Firefox 57.

::: toolkit/content/aboutTelemetry.js:1928
(Diff revision 1)
>  /**
> + * Change the url according to the current search text
> + */
> +function changeUrlSearch(searchText){
> +  let currentHash = window.location.hash;
> +  let baseSearchString = "search=";

If this is a constant, declare it with const and give it a NAME_IN_CAPS_AND_UNDERSCORES

::: toolkit/content/aboutTelemetry.js:1931
(Diff revision 1)
> +function changeUrlSearch(searchText){
> +  let currentHash = window.location.hash;
> +  let baseSearchString = "search=";
> +  let hash = "";
> +
> +  if(currentHash.includes(baseSearchString)){

This if/elseif/else block can be rewritten more concisely as

let hashWithoutSearch = currentHash.split(baseSearchString)[0];
hash = hashWithoutSearch + baseSearchString + searchText.replace(/ /g, "+");

::: toolkit/content/aboutTelemetry.js:1932
(Diff revision 1)
> +  let currentHash = window.location.hash;
> +  let baseSearchString = "search=";
> +  let hash = "";
> +
> +  if(currentHash.includes(baseSearchString)){
> +    hash = currentHash.slice(0, currentHash.indexOf(baseSearchString)) + 

there's some end-of-line whitespace here that should be removed.

::: toolkit/content/aboutTelemetry.js:1933
(Diff revision 1)
> +  let baseSearchString = "search=";
> +  let hash = "";
> +
> +  if(currentHash.includes(baseSearchString)){
> +    hash = currentHash.slice(0, currentHash.indexOf(baseSearchString)) + 
> +                             baseSearchString + searchText.replace(/ /g, "_");

We're using underscores as delimiters for subsections. Using them for spaces as well will likely cause more problems than it will solve.

::: toolkit/content/aboutTelemetry.js:1941
(Diff revision 1)
> +      hash = baseSearchString + searchText.replace(/ /g, "_");
> +    }
> +  } else {
> +    hash = currentHash + "_" + baseSearchString + searchText.replace(/ /g, "_");
> +  }
> +  

A little more EOL whitespace

::: toolkit/content/aboutTelemetry.js:1942
(Diff revision 1)
> +    }
> +  } else {
> +    hash = currentHash + "_" + baseSearchString + searchText.replace(/ /g, "_");
> +  }
> +  
> +  if(!searchText){

This can be combined as well with the earlier logic. 

if (!searchText) {
  hash = hashWithoutSearch;
}

::: toolkit/content/aboutTelemetry.js:2114
(Diff revision 1)
> -// Restore sections states
> +// Restore sections states and search terms
>  function urlStateRestore() {
> -  if (window.location.hash) {
> -    let section = window.location.hash.slice(1).replace("-tab", "-section");
> +  let hash = window.location.hash;
> +  let searchQuery = "";
> +  if (hash) {
> +    if(hash.includes("search=")){

If baseSearchString were a constant (say, within the Search object, as Search.HASH_SEARCH or a better name) then you could use it here.

::: toolkit/content/aboutTelemetry.js:2115
(Diff revision 1)
>  function urlStateRestore() {
> -  if (window.location.hash) {
> -    let section = window.location.hash.slice(1).replace("-tab", "-section");
> +  let hash = window.location.hash;
> +  let searchQuery = "";
> +  if (hash) {
> +    if(hash.includes("search=")){
> +      searchQuery = hash.slice(hash.indexOf("search=") + 7).replace(/_/g, " ");

the 7 could be Search.HASH_SEARCH.length

::: toolkit/content/aboutTelemetry.js:2118
(Diff revision 1)
> +  if (hash) {
> +    if(hash.includes("search=")){
> +      searchQuery = hash.slice(hash.indexOf("search=") + 7).replace(/_/g, " ");
> +      hash = hash.slice(0, hash.indexOf("search="));
> +    }
> +    if(hash){

need a space between if and (.

Run `./mach eslint -wo` on your machine to check the style of your submitted JavaScript.

::: toolkit/content/aboutTelemetry.js:2133
(Diff revision 1)
> -        showSubSection(subcategory);
> +          showSubSection(subcategory);
> -      }
> +        }
> -    }
> +      }
> -  }
> +    }
> +    let search = document.getElementById("search");
> +    search.value = searchQuery;

Does this result in a search happening, or the search string just being present in the input field?
Attachment #8912741 - Flags: review?(chutten) → review-
Thank you for the patient feedback, Chris!
I'll make sure the changes are made and update the patch. Also I'll keep in mind these tips for future patches in general.
Comment hidden (mozreview-request)
(In reply to Chris H-C :chutten from comment #10)

> Before submitting you should fix it so about:telemetry#search=searchTerms
> works. Subsection you can't do anything about until bug 1403585 is taken
> care of, but home search is an important new feature of about:telemetry in
> Firefox 57.

about:telemetry#search=searchTerms works with this patch. :)

> This if/elseif/else block can be rewritten more concisely as
> 
> let hashWithoutSearch = currentHash.split(baseSearchString)[0];
> hash = hashWithoutSearch + baseSearchString + searchText.replace(/ /g, "+");
> 

Since the function also had to ensure that the underscores that delimit the search and subsection portions of the hashes, I have tried to make the if/else constructs as concise as possible, while ensuring minimal bugs. Please let me know if there's anything I could do better.

> there's some end-of-line whitespace here that should be removed.

I've run my code through eslint this time. :)

> We're using underscores as delimiters for subsections. Using them for spaces
> as well will likely cause more problems than it will solve.

Changed the delimiters to '+'.

> Does this result in a search happening, or the search string just being
> present in the input field?

Yes, since search is implemented dynamically with an eventListener, it results in the search occuring, rather than just changing the value of the input.

Please let me know if there are any more issues. Thanks. :)
(Reporter)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8912741 [details]
Bug 1396855 - about:telemetry : Added search terms in the URL fragment.

https://reviewboard.mozilla.org/r/184050/#review190190

Just a little whitespace problem here, and then it should be good to go.

::: toolkit/content/aboutTelemetry.js:1942
(Diff revision 2)
> +    hashWithoutSearch += "_";
> +  }
> +  if (searchText) {
> +    hash = hashWithoutSearch + Search.HASH_SEARCH + searchText.replace(/ /g, "+");
> +  } else if (hashWithoutSearch) {
> +      hash = hashWithoutSearch.slice(0, hashWithoutSearch.length - 1);

This line is double-indented.
Attachment #8912741 - Flags: review?(chutten) → review+
Comment hidden (mozreview-request)
Done.

Comment 17

2 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c767e969819
about:telemetry : Added search terms in the URL fragment. r=chutten
(Reporter)

Comment 18

2 years ago
I've told the commit to head for autoland, so it should soon be in the tree. Thank you for your contribution! Do you need help finding your next task, or do you have a feel for where to find bugs now?

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5c767e969819
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Thanks for helping me with this.
I think I do have a good idea of how to find bugs, so that's okay. :)
You need to log in before you can comment on or make changes to this bug.