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 low hanging fruit #491

Merged
merged 10 commits into from
Mar 31, 2022
Merged

new tab page low hanging fruit #491

merged 10 commits into from
Mar 31, 2022

Conversation

brindy
Copy link
Contributor

@brindy brindy commented Mar 30, 2022

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

Description:

New tab page low hanging fruit.

Steps to test this PR:

  1. Check NTP works as expected
  2. Check github.com shows "no trackers blocked" message
  3. Check https://news.ycombinator.com/ shows "no trackers found" message
  4. Check hover state for domains and page links is highlighted and underligned and uses the pointer mouse icon
  5. Check point mouse icon is shown for favorites and the site icon

Testing checklist:

  • Test with Release configuration
  • Test proper deallocation of tabs
  • Make sure committed submodule changes are desired

Internal references:

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


if isHovering {
DispatchQueue.main.async {
NSCursor.pointingHand.push()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm seeing a situation where the push event here is happening after the pop event, leaving the cursor in pointing hand state until I mouse over something else that changes it. I added log breakpoints to each of these push pop calls which narrowed it down to this one:

HyperLink Push
HyperLink Pop
SiteIconAndConnector Pop
SiteIconAndConnector Push
HyperLink Push
HyperLink Pop
HyperLink Push
HyperLink Pop
SiteIconAndConnector Pop
SiteIconAndConnector Push
HyperLink Push
HyperLink Pop
HyperLink Push
HyperLink Pop
SiteIconAndConnector Pop
SiteIconAndConnector Push

Seems like HyperLink consistently pushes and then pops, but SiteIconAndConnector doesn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I must say that my extension is not bulletproof enough. @brindy is using async dispatch for a reason, and I was able to avoid it only because I didn't have to store isHovered in a @State variable. And I forgot to explain it in a comment. I'll try and update my code and with API that handles a scenario like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw - I've just removed the dispatch main calls, they didn't seem to be needed any more for some reason and in fact were the cause of this the problem Sam pointed out.

Let's consolidate this cursor logic as a small PR once they're both merged :)

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.

New changes work great! This looks good 👏

@brindy brindy merged commit 35742e9 into develop Mar 31, 2022
@brindy brindy deleted the brindy/new-tab-page-lhf branch March 31, 2022 13:29
samsymons added a commit that referenced this pull request Apr 4, 2022
# By Tomas Strba (5) and others
# Via GitHub
* develop:
  Refresh the Autofill panel after importing (#496)
  Require authentication for login export (#494)
  Observations database clearing improvements (#495)
  Fix handling external schemes in address bar (#493)
  New autoconsent rules and move tab cleanup out of onCommitted (#489)
  Safari CSV Passwords import +LastPass, 1Password (#483)
  Split preferences into multiple screens and add an option to show full URL (#487)
  Color of hover label fixed (#497)
  Add AMP links protection (#488)
  Enhancements of fire button options (#484)
  Rename Logins+ to Autofill (#492)
  new tab page low hanging fruit (#491)
  Fix threading issues in Autoconsent (#478)
  Navigation bar improvement: CMD + back or CMD + forward opens a new tab (#486)
  TextField refactored to TextView in order to make it scrollable (#481)
  More Xcode cleaning (#485)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
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