Closed Bug 1402023 Opened 2 years ago Closed 2 years ago

Limit the 'Recent Activity' list to a maximum of 6 items

Categories

(Firefox :: Toolbars and Customization, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + verified
firefox58 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 file)

As per UX' request.

[Tracking Requested - why for this release]: because the current list is 12 items long, which is a bit much.
Flags: qe-verify+
QA Contact: gwimberly
Comment on attachment 8910824 [details]
Bug 1402023 - Limit the 'Recent Activity' list in the Library to a maximum of 6 items.

https://reviewboard.mozilla.org/r/182298/#review187606
Attachment #8910824 - Flags: review?(gijskruitbosch+bugs) → review+
Activity Stream tries to be a bit smarter on its shorter list by picking out one item per domain instead of just showing the most recent items. That logic can't be easily moved into NewTabUtils as Activity Stream also dedupes items, so filtering too early would mean potentially mean removing desired results that wouldn't have been deduped.
Something like this might work although not quite exactly what activity stream does (in particular multiple bookmarks from same host can get removed here):

let highlights = await getHighlights();
const highlightsHosts = new Set();
highlights = highlights.filter(({url}) => {
  try {
    const host = Services.io.newURI(url).host;
    if (highlightsHosts.has(host)) {
      return false;
    }
    highlightsHosts.add(host);
    return true;
  } catch (e) {}
}).slice(0, 6)
I can see how that'd work, but wouldn't you prefer to provide a simple consumer API (as you have now), with a options you can provide so that there's not too much to update when you change something on the Activity Stream end of things?
Flags: needinfo?(edilee)
Maybe.. I don't think we expected someone else to be consuming it yet. ;) As I mentioned briefly in comment 3, this logic isn't easily pushed into activityStreamLinks as the actual Highlights section uses internal state of other sections, cache, prefs, etc.

I suppose the most important one is the top sites preventing a highlight from appearing then picking out one highlight per host. That could be moved into activityStreamLinks to run both queries, but activity stream probably wouldn't want to use it as it already has a copy of top sites. Although this could just be another option on activityStreamLinks.getHighlights({topSites}) that recent activity would pass in nothing resulting in 2 places queries, and activity stream passes in its state.. but then that breaks activity stream's places caching model…
Flags: needinfo?(edilee)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bb338b4b503
Limit the 'Recent Activity' list in the Library to a maximum of 6 items. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/9bb338b4b503
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Please request uplift to beta when you get a chance.  Thanks!
Flags: needinfo?(mdeboer)
Comment on attachment 8910824 [details]
Bug 1402023 - Limit the 'Recent Activity' list in the Library to a maximum of 6 items.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1354536.
[User impact if declined]: List may appear to be too long.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, the list of Highlights in the Library panel should never exceed six (6) items.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The limit feature is part of AS and covered by unit tests.
[String changes made/needed]: n/a.
Flags: needinfo?(mdeboer)
Attachment #8910824 - Flags: approval-mozilla-beta?
Comment on attachment 8910824 [details]
Bug 1402023 - Limit the 'Recent Activity' list in the Library to a maximum of 6 items.

Smaller list, taking it.
Should be in 57b4 (gtb tomorrow Thursday)
Attachment #8910824 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced the issue mentioned in comment 10 using an affected Firefox 58.0a1 build (BuildId:20170921220243).

I have verified that the issue is not reproducible using Firefox 57.0b7 (Build Id:20171009192146) and 58.0a1 (BuildId:20171010220102) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.