Use URL parameters for filter states

[whee]

This fixes #8510 by storing Clippy Lints page filter configuration in the URL parameters.

This includes:

• Lint levels
• Lint groups
• Version filters

"Filter" was already present in the URL and its behavior is retained. There is existing support for passing a sel query parameter; this is also retained, but I am not sure if it used in the wild.

The URL parameters only get included if they are modified after loading the page.

I have these changes available here in case people want to play with it: https://whee.github.io/rust-clippy/master/

An example with levels, groups, and versions set (oddly):

https://whee.github.io/rust-clippy/master/#/?groups=pedantic,perf&levels=allow,warn&versions=gte:53,lte:57,eq:54

Adding a filter:

https://whee.github.io/rust-clippy/master/#/manual_str_repeat?groups=pedantic,perf&levels=allow,warn&versions=gte:53,lte:57,eq:54

---

changelog: Docs: [Clippy's lint list] now stores filter parameters in the URL, to allow easy sharing
#10834

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[xFrednet]

Hey @whee, thank you for the PR, I'm excited to see this idea becoming a reality!!

I haven't looked at the code yet, but played with the linked version. (Thank you for hosting it :D). It already works wonderfully. I have a few suggestions which would make it feel better for me:

1. Currently, every typed letter in the search box updates the domain. I think it would be better, to add a debounce or something similar, so the domain is only updated if there are no changes for a second or so. The search already uses this. Another option, which would probably be easier and work as expected, would be to only update the URL if the search box is unfocussed. There should be a listener for that in JS

2. Selecting the default filters, keeps the levels and groups in the URL. This creates a long URL which is equivalent to not having any filter:

   • https://whee.github.io/rust-clippy/master
   • https://whee.github.io/rust-clippy/master/#/?levels=allow,warn,deny,none&groups=cargo,complexity,correctness,nursery,pedantic,perf,restriction,style,suspicious

   It would be nice, if the filters are dropped again, when they're equivalent to the default

3. Clicking the ¶ button next to the link adds the lint name after a /#/<lint> and not a #<lint> like it used to.

As said, I haven't looked at the code yet, so if any of these changes are intentionally or explained there, I'll see it later. Besides these functionally nits, I do love this functionality!

---

Next week I have exams coming up, so it'll most likely take until the end of the week, until I can review the code. You are welcome to push any changes, if you agree with my suggestions above :)

[whee]

I appreciate the feedback @xFrednet! I'll try to address each of your points:

1. I agree that the path updating is a bit noisy in the search case. I think updating when unfocused makes sense. I know I tend to go from editing to copying the URL pretty quickly via shortcuts, so a debounce might add too much of a delay.

2. This is partially intentional, but I think your use case makes sense as a "reset to not including the URL parameters" situation.

The scenario I wanted to address was:

• I've configured a set of filters I like, which coincidentally match the defaults in some way
• I bookmark the page
• I come back later, and the view is different because the defaults changed

Clicking "Default" explicitly is a good indicator that the user wants defaults and not a specific set, though, when compared to clicking the checkboxes and coincidentally matching the defaults.

3. I think this is a side effect of using $location service to consistently set the URL. Adding that slash is "standard" for AngularJS in this case. I'll check into whether or not I can change that behavior.

I'll try to get these changes in soon. Good luck with exams!

[xFrednet]

Hey, I finally have time to look at this properly, sorry that it took so long. I should be done with my semester now 🎉

2. This is partially intentional, but I think your use case makes sense as a "reset to not including the URL parameters" situation.

The scenario I wanted to address was:

• I've configured a set of filters I like, which coincidentally match the defaults in some way
• I bookmark the page
• I come back later, and the view is different because the defaults changed

Clicking "Default" explicitly is a good indicator that the user wants defaults and not a specific set, though, when compared to clicking the checkboxes and coincidentally matching the defaults.

I don't think many people will bookmark specific filters. And even then, I think it's unlikely that the filters will change much. We had two changes to the default in the last two years and one of them was the addition of a new lint group. I see arguments for both sides here :)

[xFrednet]

The changes look good to me, I'm not an JS expert, but the code makes sense and is even a bit cleaner in parts. Thank you!

I would still be in favor of removing the filter from the URL if the default has been manually selected. As said, we rarely change the filter and even in those cases I would guess that the user would like the defaults over their custom filter, if the filter is equivalent to the current default anyways. For example, if a new lint group would be added, they would probably one that one to be selected.

My main argument for this, is that it would make the URL a lot shorter in most cases.

Another change I would like to see, is that the All button for lint levels also removes the filter from the URL, as it's equivalent to the Default button for lint groups.

Besides those nits, I think this looks really promising. Thank you for the time you put into this!

[whee]

Thanks for taking a look!

I would still be in favor of removing the filter from the URL if the default has been manually selected. As said, we rarely change the filter and even in those cases I would guess that the user would like the defaults over their custom filter, if the filter is equivalent to the current default anyways. For example, if a new lint group would be added, they would probably one that one to be selected.

My main argument for this, is that it would make the URL a lot shorter in most cases.

Another change I would like to see, is that the All button for lint levels also removes the filter from the URL, as it's equivalent to the Default button for lint groups.

Makes sense to me. I'll let you know when I have the rest of the tweaks in and think it's ready for another peek.

[whee]

Simplifying the handling of default values was a good call; the code got a bit simpler as well.

Hopefully this behavior makes a bit more sense:
https://whee.github.io/rust-clippy/master/

(This PR matches that deployed version.)

The only change I haven't been able to make is getting that / out of the path. AngularJS's $location service seems to expect it when using path. If I use hash instead, then somehow it's doubling up the #. Enabling html5Mode on the $locationProvider removes the # entirely and breaks existing links, and fixing up the href on the window doesn't stick -- Angular ends up overwriting it again anyway. I haven't found a good solution to this yet, other than maybe rolling back to the custom path handling, which might necessitate custom handling of the rest of the parameters.

[xFrednet]

The only change I haven't been able to make is getting that / out of the path. AngularJS's $location service seems to expect it when using path. If I use hash instead, then somehow it's doubling up the #. Enabling html5Mode on the $locationProvider removes the # entirely and breaks existing links, and fixing up the href on the window doesn't stick -- Angular ends up overwriting it again anyway. I haven't found a good solution to this yet, other than maybe rolling back to the custom path handling, which might necessitate custom handling of the rest of the parameters.

It looks a bit odd, but the old links still work and the new links also work, so I think it's fine either way. That was a tiny nit, but I don't think we should block the PR based on this :)

---

With that, I can say that this version looks good to me. Thank you for this update, I think it turned out pretty well!

@bors r+

[bors]

📌 Commit ac279ef has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit ac279ef with merge a97a94a...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing a97a94a to master...