Page MenuHomePhabricator

Don't do unnecessary work when deleting a highlight-only tag
Closed, ResolvedPublic1 Story Points

Description

TLDR: When deleting a highlight-only tag, the results area reloads and somehow createTagItemWidget is called. These things shouldn't happen, when removing a highlight by setting the color picker to "none", they don't.

  • Highlight a filter without selecting it. This creates a muted tag with a highlight
  • Put a breakpoint in mw.rcfilters.ui.FilterTagMultiselectWidget.prototype.createTagItemWidget
  • Remove the highlight tag by clicking its "x" button
  • Be confused: the results area reloads, even though the filters haven't changed
  • Breakpoint fires
  • Be confused: why is createTagItemWidget called when we're not adding any tags, only removing them?

Event Timeline

Catrope created this task.Apr 29 2017, 1:11 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 29 2017, 1:11 AM
Catrope set the point value for this task to 1.Apr 29 2017, 1:11 AM
SBisson added a subscriber: SBisson.May 2 2017, 3:02 PM

I do the steps above and createTagItemWidget doesn't seem to get called. Anything else in the state of the UI that maybe you didn't mention?

Catrope renamed this task from Don't create a TagItemWidget when deleting a highlight-only tag to Don't do unnecessary work when deleting a highlight-only tag.Oct 11 2017, 4:19 PM
Catrope updated the task description. (Show Details)

I have a wip patch to NOT trigger a reload when an action results in changes to highlights only. It doesn't address the createTagItemWidget part but still makes a difference.

I will rebase and publish it when @Mooeypoo's big patch lands.

@Catrope is meant to be writing this to make it broader.

The commit prevents reloading if the removed tag is only highlights.
I couldn't reproduce the createTagItemWidget issue.

Change 383738 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/core@master] RCFilters: Don't reload when removing highlighted item

https://gerrit.wikimedia.org/r/383738

The following operations can also be "highlights" only

  • clearing
  • restore defaults
  • apply saved query

Do we want to address them in this ticket as well?

So I actually started working on something like that, but ended up unsure if it actually will be faster...

The problem with these, is that they're not always "highlights only". We always have to check whether there's a *difference* between the current filter-state and the new filter-state and only then know if the only difference was applying/removing highlights. I have a WIP with an idea for that, but it became a bit convoluted and I wasn't sure it would end up being worth it. I can post it so we can discuss.

Consider this scenario for example -

Case1:

  • Your current filters are [filter1]
  • You apply a saved query that's ONLY highlights
    • This is not actually "highlights only" application, because by applying this saved query, you dismissed/canceled [filter1].

Case2:

  • Your current filters are [filter1] and [filter2], but [filter2] is a subset (so it doesn't actually affect the results
  • You apply a saved query that's [filter1] and some highlight
    • This is basically a "highlights only" operation... but can we tell easily?

My idea was to try and solve the "easiest" cases where we can compare states and see if anything changed in the actual state and then decide if we should reload or not. I have a work in progress, but again, it became a bit weird to compute and I am not sure if it will be worth it -- I am going to take another stab at it (I think if I check *filter* state rather than compare parameters, it will work, but that will mean making a conversion every time, again) and we can see if it makes a real difference.

Change 383893 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/core@master] RCFilters: Don't reload the list if the change was highlights-only

https://gerrit.wikimedia.org/r/383893

Change 383738 merged by jenkins-bot:
[mediawiki/core@master] RCFilters: Don't reload when removing highlighted item

https://gerrit.wikimedia.org/r/383738

Checked in betalabs and wmf.4 - adding/removing highlight to displayed set of of filters does not reload the results.

Regarding the cases that @Mooeypoo listed. they are essentially the different states of the results set and it seems logical that the result set reloads.

Etonkovidova closed this task as Resolved.Oct 18 2017, 9:55 PM

Change 383893 merged by jenkins-bot:
[mediawiki/core@master] RCFilters: Don't reload the list if the change was highlights-only

https://gerrit.wikimedia.org/r/383893