Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

[Bug] History: Consecutive Duplicates should be filtered out #1331

Closed
cadeyrn opened this issue Apr 2, 2019 · 15 comments · Fixed by #4122
Closed

[Bug] History: Consecutive Duplicates should be filtered out #1331

cadeyrn opened this issue Apr 2, 2019 · 15 comments · Fixed by #4122
Assignees
Labels
🐞 bug Crashes, Something isn't working, .. Feature:History needs:ac Needs Android Component Work P2 Upcoming release 🙅 waiting Issues that are blocked or has dependencies that are not ready
Milestone

Comments

@cadeyrn
Copy link
Contributor

cadeyrn commented Apr 2, 2019

Steps to reproduce

  1. open history

Expected behavior

no duplicated entries, because the duplicates don't add much value

Actual behavior

duplicated entries

ScreenshotUNITO-UNDERSCORE!20190402-223933!

Device information

  • Android device: HTC U11 / Android 8.0
  • Fenix version: Fenix revision bd81e72

┆Issue is synchronized with this Jira Task

@cadeyrn cadeyrn added the 🐞 bug Crashes, Something isn't working, .. label Apr 2, 2019
@vesta0 vesta0 added P1 Current sprint needs:ac Needs Android Component Work Feature:History labels Apr 4, 2019
@vesta0 vesta0 added this to the Bugs milestone Apr 4, 2019
@vesta0 vesta0 added 🙅 waiting Issues that are blocked or has dependencies that are not ready beta labels Apr 4, 2019
@vesta0 vesta0 removed the beta label Apr 30, 2019
@vesta0 vesta0 added eng:qa:needed QA Needed and removed 🙅 waiting Issues that are blocked or has dependencies that are not ready labels May 24, 2019
@ekager
Copy link
Contributor

ekager commented May 24, 2019

Those 2 AC issues are merged in and being used. We should have higher quality history now.

@lobontiumira
Copy link

Hi @cadeyrn,
I'm not completely sure if I understood correctly the expected behavior.
For example, I searched for YouTube and in History, I get http://youtube.com, and https://m.youtube.com/, although I typed "youtube" inside the tab, and I didn't select any video. Is this expected or not?
Thank you!

Please see the attached screenshot:

Screenshot_20190529-165112

@ekager
Copy link
Contributor

ekager commented May 29, 2019

@grigoryk could you possibly weigh in on if the above screenshot is the "best" we can do with history filtering right now?

@lobontiumira lobontiumira added in progress and removed eng:qa:needed QA Needed labels May 30, 2019
@bifleming bifleming added Release Blocker Blocks a Release and removed P1 Current sprint labels May 30, 2019
@sblatz sblatz removed the in progress label Jun 6, 2019
@sblatz
Copy link
Contributor

sblatz commented Jun 7, 2019

Still need @grigoryk's input on this.

@sblatz sblatz added the 🙅 waiting Issues that are blocked or has dependencies that are not ready label Jun 7, 2019
@vesta0 vesta0 removed this from the Bugs milestone Jun 13, 2019
@thomcc
Copy link

thomcc commented Jul 9, 2019

We intentionally aren't filtering duplicates, but could easily add a parameter to do so.

@thomcc
Copy link

thomcc commented Jul 9, 2019

More broadly, there are a couple things you might want here:

  1. Only show the (globally) most recent visit for a given URL. This is consistent with desktop's handling, and is easy for us to do, however it would mean that deeper into your history pages, the data might be noticably less accurate.
  2. Within a given page, only show the most recent visit of a specific url. This is also easy for us to add, but the difference between adding it in rust and kotlin wouldn't be huge -- we'd just be doing the filtering after the query.
  3. Other variants... I could imagine also a situation where only consecutive duplication is filtered...

Basically, I think if you want number 1, we should do it in the Rust. (It would be an optional flag though, since I can imagine cases where all pages are desired)

Otherwise, it probably is worth you doing it in Kotlin, so that you retain that flexibility to change it later without needing to go through us.

EDIT: OTOH it's worth noting that while passing data over the FFI isn't that expensive, it might be expensive enough that it's still worth it to do the filtering in rust first. (I'm not sure how many items would be excluded in practice)

@jdragojevic
Copy link

@vesta0 - Can you please consider the options that @thomcc outlined above for behaviours and let us know how you like it to be resolved?

@yoasif
Copy link
Contributor

yoasif commented Jul 11, 2019

Other variants... I could imagine also a situation where only consecutive duplication is filtered...

I think consecutive duplication is the only one that is desirable for users. People on desktop already complain about behavior #1, and I don't see any reason why it is desirable to be consistent with this disliked behavior.

@vesta0
Copy link
Collaborator

vesta0 commented Jul 15, 2019

@thomcc thanks for listing the options.

I don't think we should only show a most recent visit for a URL. Comparing how this works in Fennec vs Fenix, I think the problem is that Fenix is not pulling a correct/distinct page name all the time. For example, if you tap on different ideos on YouTube in Fenix they will all have the same name (m.youtube.com) even if the URL is different, whereas Fennec correctly displays the name of the video. Can we match this behaviour in Fenix?

Also what does the effort for option 3 (filtering out only consecutive dups) look like?

@jhugman jhugman self-assigned this Jul 16, 2019
@jhugman
Copy link
Contributor

jhugman commented Jul 16, 2019

So, threading the needle through all the above:

  • we should filter out duplicate consecutive entries.
  • I think we should do this in Kotlin, to give us some ongoing flexibility.

Other issues alluded to in this bug:

1. title of the page is not being saved

Fenix is not pulling a correct/distinct page name all the time. For example, if you tap on different ideos on YouTube in Fenix they will all have the same name (m.youtube.com) even if the URL is different, whereas Fennec correctly displays the name of the video.

1a. intermediate pages in a redirect should not be stored in history.

This has been filed before at ac#3027

2. filtering duplicates in history should be using title, or PSL/common prefixes trimmed URL.

Solving 2. by filtering by title would be relatively cheap, however, we cannot trust that the title for consecutive duplicates are actually the same, because 1. exists. Until then, we have garbage in, garbage out.

@st3fan
Copy link
Contributor

st3fan commented Jul 16, 2019

This may be an unrelated problem but it is worth investigating:

Fenix is not pulling a correct/distinct page name all the time. For example, if you tap on different ideos on YouTube in Fenix they will all have the same name (m.youtube.com) even if the URL is different, whereas Fennec correctly displays the name of the video.

Is this unique to YouTube because it reloads the page not with actual reloads but by doing ajax requests and modifying the history stack? We've had this that problem in Firefox iOS where we did not get the proper notifications that the page changed.

This is not unique to YouTube - there are plenty of SPAs that use these techniques.

@jhugman
Copy link
Contributor

jhugman commented Jul 16, 2019

@st3fan pushState may well be relevant here.

I'll file an ac issue to track that.

@bifleming bifleming removed the a-s Application Services work needed label Jul 16, 2019
@jhugman jhugman changed the title [Bug] History: Duplicates should be filtered out [Bug] History: Consecutive Duplicates should be filtered out Jul 17, 2019
@vesta0 vesta0 added the P2 Upcoming release label Jul 24, 2019
@vesta0 vesta0 removed this from the 1.1 Backlog milestone Jul 24, 2019
@boek boek added this to the v1.2 milestone Aug 1, 2019
@data-sync-user data-sync-user changed the title [Bug] History: Consecutive Duplicates should be filtered out FNX2-17041 ⁃ [Bug] History: Consecutive Duplicates should be filtered out Aug 3, 2020
@data-sync-user data-sync-user changed the title FNX2-17041 ⁃ [Bug] History: Consecutive Duplicates should be filtered out FNX3-15489 ⁃ [Bug] History: Consecutive Duplicates should be filtered out Aug 11, 2020
@data-sync-user data-sync-user changed the title FNX3-15489 ⁃ [Bug] History: Consecutive Duplicates should be filtered out FNX-4962 ⁃ [Bug] History: Consecutive Duplicates should be filtered out Aug 11, 2020
@data-sync-user data-sync-user changed the title FNX-4962 ⁃ [Bug] History: Consecutive Duplicates should be filtered out [Bug] History: Consecutive Duplicates should be filtered out May 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Crashes, Something isn't working, .. Feature:History needs:ac Needs Android Component Work P2 Upcoming release 🙅 waiting Issues that are blocked or has dependencies that are not ready
Projects
None yet
Development

Successfully merging a pull request may close this issue.