Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tweaks of suggestions and autocomplete #403

Merged
merged 9 commits into from
Feb 4, 2022
Merged

Tweaks of suggestions and autocomplete #403

merged 9 commits into from
Feb 4, 2022

Conversation

tomasstrba
Copy link
Contributor

@tomasstrba tomasstrba commented Jan 23, 2022 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1177771139624306/1201690434477352/f

Description:
This PR includes:

  1. Fix of isDownload flag not being cleared.
  2. Filtering various suggestion types from Top Hits section of suggestions
  3. Autogeneration of root URLs removed

Steps to test this PR:
Test clearing of isDownload flag

  1. Click this file to download a file
  2. Visit https://www.w3schools.com/
  3. Open a new tab and type "w3sch"
  4. Make sure https://www.w3schools.com/ is autocompleted

Test new rules of Top Hits section (Suggestions have 3 sections Top Hits, DuckDuckGo Search Suggestions, History and Bookmarks)
Please see my comment below

Test autogeneration removal

  1. Visit https://www.newworldencyclopedia.org/entry/bicycle
  2. Open new tab and type "newworldencyclopedia"
  3. Make sure Top Hits section is empty and https://www.newworldencyclopedia.org/entry/bicycle is part of the third section
Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@tomasstrba tomasstrba mentioned this pull request Jan 23, 2022
Merged
2 tasks
@bstandaert-ddg
Copy link

RE clearing of isDownload, the autocomplete behavior still isn't exactly what I'd expect. If I download a file and then open a new tab, the site doesn't autocomplete, and it also doesn't autocomplete after reloading the page. It only starts autocompleting again after visiting the site again in a new tab:

isdownload.clearing.mov

Also, I'm not sure I understand the difference between favorites and non-favorite bookmarks. If I make a favorite, it doesn't appear in top hits until I visit it 4 times, and then it does:

Screen.Recording.2022-01-24.at.2.18.32.PM.mov

But if I make a non-favorite bookmark, the behavior is the same:
https://user-images.githubusercontent.com/90369497/150859379-58b21f0c-5bcf-4fb3-83a6-93ea96a11d08.mov

The autogeneration change does seem to be working as intended.

@mallexxx mallexxx self-assigned this Jan 25, 2022
@mallexxx
Copy link
Collaborator

Valid points from Ben,
Also I found pretty weird behaviour when you have something autocompleted and you paste another URL over it, it's not navigated but the last autocompletion value is opened:

screencast.2022-01-28.17-16-52.mp4

@mallexxx mallexxx assigned tomasstrba and unassigned mallexxx Jan 28, 2022
@tomasstrba
Copy link
Contributor Author

tomasstrba commented Feb 3, 2022 •
edited
Loading

Thank you both for careful testing!

  1. isDownload flag
    Unfortunately, there was a chicken egg problem. I couldn't reset the flag after a download because we don't know if the URL is also regular or not. We only know it after a next visit.
    I decided to remove the flag from the algorithm completely because:
  • We no longer automatically generate root URLs. (Download subdomains as east-00213.asana.com ended autocompleted very easily, but that won't be issue anymore)
  • We now have a filter in Top Hits for non-root URLs with visit count < 4 so download URLs shouldn't be autocompleted often (unless you download the same file 4 times)
  • In general, currently the flag produces more harm than good
  1. Favorites and non-favorite bookmarks vs Top Hits section
    Ben, you are right. There was a conflict in multiple "rules" that caused a favorite bookmark not being allowed in Top Hits.
    To properly address it, I needed to solve one big inconsistency. In previous implementation, if a URL was in bookmarks and also part of history, we presented the suggestion as history entry (with clock). People reported this issue and prefer to have bookmark(or favourite) icon if the URL is bookmarked.
    Once I solved this, a bookmark(no favorite) could also be part of the Top Hits. It can be prioritized as history entry with high visit count but presented as bookmark because the URL is bookmarked. Here is how it should work:

Favorite without a visit can be part of Top Hits:

  1. Add economist.com into favorites
  2. Visit it and burn the tab
  3. Open a new tab and type "econom"
  4. Make sure economist.com is autocompleted and there is a valid icon

Favorite after the visit can be part of Top Hits

  1. Add economist.com into favorites
  2. Visit it few times
  3. Open a new tab and type "econom"
    1. Make sure economist.com is autocompleted and there is a valid icon (favorite, no clock)

Bookmark without a visit can't be part of Top Hits (The idea is not to suggest too much after user imports 1000 bookmarks)

  1. Add twitter.com into bookmarks (no favorite!)
  2. Visit it and burn the tab
  3. Open a new tab and type "twitte"
  4. Make sure twitter.com is part of the third section and there is the valid bookmark icon

Bookmark after visiting can be part of Top Hits

  1. Add twitter.com into bookmarks (no favorite!)
  2. Visit it
  3. Open a new tab and type "twitte"
  4. Make sure twitter.com is autocompleted and has the valid bookmark icon

@tomasstrba tomasstrba assigned mallexxx and unassigned tomasstrba Feb 3, 2022
@tomasstrba
Copy link
Contributor Author

Alex, issue you found is probably not related to this PR. Do you mind posting it to our feedback project?

@tomasstrba tomasstrba force-pushed the tom/suggestion-tweaks branch from 0cf72c9 to 321d8d1 Compare February 3, 2022 14:44
mallexxx
mallexxx approved these changes Feb 4, 2022
View reviewed changes
Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tomasstrba tomasstrba force-pushed the tom/suggestion-tweaks branch from 9a3c371 to 94586fc Compare February 4, 2022 13:58
@tomasstrba tomasstrba merged commit 55de41f into develop Feb 4, 2022
@tomasstrba tomasstrba deleted the tom/suggestion-tweaks branch February 4, 2022 14:06
samsymons added a commit that referenced this pull request Feb 9, 2022
@samsymons
Merge branch 'develop' into sam/logins-filtering-and-sorting
54882aa 
# By Alexey Martemyanov (2) and others
# Via GitHub
* develop:
  Replace Burn and Fireproof icons (#416)
  don't disable the UI unless onboarding has been marked as finished (#420)
  fix nested RunLoop waiting (#422)
  Version 0.18.6
  Tweaks of suggestions and autocomplete (#403)
  Bump privacy dashboard to latest version (#409)
  Point to the latest BrowserServicesKit branch. (#414)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Feb 18, 2022
@samsymons
Merge branch 'develop' into sam/macos-12-crash-report-handling
c34d3d3 
# By Alexey Martemyanov (20) and others
# Via Tomas Strba (2) and others
* develop: (63 commits)
  Tweaks of suggestions and autocomplete (#403)
  Bump privacy dashboard to latest version (#409)
  Point to the latest BrowserServicesKit branch. (#414)
  Move embedded TDS from BSK to platform repo (#412)
  Image of shield with dot replaced (#410)
  Expand Fireproofing to include Local Storage and IndexedDB (#408)
  Version 0.18.5
  support privacy config for clickToLoad (#407)
  Automatically select available login (#405)
  initial FB Click to Load (WIP) (#329)
  onboarding updates (#398)
  Version 0.18.4
  Configuration of Sparkle - Setting SUAllowsAutomaticUpdates to NO (#404)
  Hide downloads button if the popover is opened/closed manually (#397)
  Textfield of the homepage is empty and unfocused right after switching to the homepage (#400)
  Remove navigatorCredentials (#392)
  Remove GPC header if it exists when not needed (#366)
  Version 0.18.3
  Fireproofing encrypted storage (#332)
  Fix Lock Screen UI issues (#399)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo/Crash Reports/Model/CrashReportSender.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mallexxx mallexxx mallexxx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomasstrba @bstandaert-ddg @mallexxx