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.

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

New Tab Page #433

Merged
merged 135 commits into from
Mar 10, 2022
Merged

New Tab Page #433

merged 135 commits into from
Mar 10, 2022

Conversation

brindy
Copy link
Contributor

@brindy brindy commented Feb 20, 2022

Task/Issue URL: https://app.asana.com/0/392891325557410/1201829216434626
Tech Design URL:
CC:

Description:

All new tab page re-written in Swift UI.

Steps to test this PR:

  1. Default Browser prompt is shown
  2. Favorites show correctly and menu works as expected
  3. Empty state for feed looks correct
  4. Open a bunch of site and check the feed
  5. Ensuring burning sites works
  6. Ensure favoriting works (note that the ordering is determined by the lifecycle of the bookmark which may get created in order to create the favorite)
  7. Check double tap the shield easter egg to toggle permanently showing the feed list

Internal references:

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

@@ -138,6 +138,10 @@ final class Tab: NSObject {
super.init()

setupWebView(shouldLoadInBackground: shouldLoadInBackground)

if content == .homepage {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensures the home page enters the history so that it can be navigated back to without causing back/forward list complexity.


import SwiftUI

extension Color {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from iOS to give consistent, but seemingly random, colours for favorites.

let size: CGFloat

@State var image: NSImage?
@State private var timer = Timer.publish(every: 0.3, tolerance: 0, on: .main, in: .default, options: nil).autoconnect()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the SwiftUI view is shown too quickly then the Favicon cache does not appear to load and give the cached image. This refreshes the image after a short time, but only once as the timer is cancelled after being fired.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is probably fine, though ideally the FaviconManager should publish when the favicon cache loads or changes, after which this view could just subscribe to that (probably debouncing the events within a short period in case of rapid changes). Though if this is just for setting up the view and canceled right after, then I think this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but didn't want to change the FaviconManager if I could avoid it. I did look at making a publisher but the areFaviconsLoaded var is a composite so didn't seem like a quick refactor.

But actually I will make one improvement here and that's to check until the caches are loaded.


import SwiftUI

struct MoreOrLess: View {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A component that shows a "More ^" or "Less v" link that can be used to hide and show other views.

@@ -40,4 +40,17 @@ extension View {
}
}

func link(onHoverChanged: ((Bool) -> Void)? = nil, clicked: @escaping () -> Void) -> some View {
self.onHover { over in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates the cursor to show pointy hand. Unfortunately if the containing view also uses this flag to change itself, then Swift UI rebuilds the view resets the point back to the normal arrow.

Comment on lines +245 to +249
var historyDictionary = self.historyDictionary ?? [:]
historyDictionary[entry.url] = entry
self.historyDictionary = historyDictionary
self._history = self.makeHistory(from: historyDictionary)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed both existing functions called this same code and I was about to add another to do the same just refactored it into here.


struct FavoriteModel {

static let addButtonUUID = UUID()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The add button is a specialised "Favorite" so we need to be able to identify it. This reduces the amount of logic required in the view / model for determining when to show the add button based on how many rows of favorites are already showing.

@samsymons samsymons self-assigned this Mar 7, 2022
@samsymons samsymons self-requested a review March 7, 2022 19:36
Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

Only just started working on this, one question and one bug:

  1. Question: What's the expected order of the tracker feed on the home page? It almost always adds new sites on chronological order, but in some cases I can visit two sites in sequence and they'll be backwards. For instance, if I visit Amazon, and then the NY Times, Amazon appears as the most recent visited website still, with NY Times second.
  2. Bug: When you have text selected in the new tab's address bar, visiting another tab will have the same number of characters selected in the address bar.

DuckDuckGo/Common/Extensions/DateExtension.swift Outdated Show resolved Hide resolved
DuckDuckGo/Common/Localizables/UserText.swift Outdated Show resolved Hide resolved
let size: CGFloat

@State var image: NSImage?
@State private var timer = Timer.publish(every: 0.3, tolerance: 0, on: .main, in: .default, options: nil).autoconnect()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is probably fine, though ideally the FaviconManager should publish when the favicon cache loads or changes, after which this view could just subscribe to that (probably debouncing the events within a short period in case of rapid changes). Though if this is just for setting up the view and canceled right after, then I think this is fine.

@brindy brindy changed the title [draft] New Tab Page - Phase 1 New Tab Page Mar 8, 2022
@brindy
Copy link
Contributor Author

brindy commented Mar 8, 2022

Thanks @samsymons !

  1. It's possibly because we exclude the domain root from the list so even though you visit NY after Amazon, it's not changed the list per se. I think I can fix that.
  2. Yep, am working on this today.

@brindy
Copy link
Contributor Author

brindy commented Mar 9, 2022

Ah 1) is caused because we're truncating the date of the history ... it was being trunacted to the same day, now it's being truncated to the same minute. Actully, I need to run a privacy triage on that.

Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

🥇

@brindy brindy assigned brindy and unassigned samsymons Mar 10, 2022
@brindy brindy merged commit 5a216e1 into develop Mar 10, 2022
@brindy brindy deleted the brindy/new-tab-page branch March 10, 2022 16:47
samsymons added a commit that referenced this pull request Mar 10, 2022
* develop:
  New Tab Page (#433)
  Make website URLs clickable in Logins+ (#462)
  Correction of URL if it is missing one slash '/' (#459)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants