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

Remove mouse event listeners on browser close #448

Merged
merged 3 commits into from
Feb 28, 2022

Conversation

jonathanKingston
Copy link
Collaborator

Task/Issue URL: https://app.asana.com/0/1199178362774117/1201889534538164/f
Tech Design URL:
CC:

Description:

Fixes browser restart click handler issue.

Steps to test this PR:
Please describe the issue in as much detail as possible.:

  1. Open the browser.
  2. Close the browser window.
  3. Open a new window.
  4. Try to click on the menu button or fire button.
  5. Navigate to a website.
  6. Try to click on something in the web content.

At steps 4 and 6, clicking doesn't should now work.

Testing checklist:

  • Test with Release configuration

Internal references:

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

@tomasstrba tomasstrba changed the base branch from develop to release/0.19.0 February 28, 2022 13:21
@tomasstrba
Copy link
Contributor

tomasstrba commented Feb 28, 2022

@jonathanKingston I changed the base so this is part of RC1 today

@tomasstrba tomasstrba self-assigned this Feb 28, 2022
@tomasstrba tomasstrba requested review from tomasstrba and removed request for bwaresiak February 28, 2022 13:31
Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

Thanks for this fix! 👍 Could you please refactor just one thing that could kick us in the future?

@@ -85,6 +85,11 @@ final class BrowserTabViewController: NSViewController {
addMouseMonitors()
}

override func viewWillDisappear() {
Copy link
Contributor

@tomasstrba tomasstrba Feb 28, 2022

Choose a reason for hiding this comment

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

I know it was previously like this, but safer is to call addMouseMonitors() in viewWillAppear().
The reason is viewWillAppear and viewWillDisappear are called multiple times during the lifecycle of the view controller. viewDidLoad() just once

@tomasstrba
Copy link
Contributor

I would like to compile another build for testing so I speed up this PR a bit by doing the requested change myself. I will ask someone else to review

@tomasstrba tomasstrba assigned brindy and unassigned tomasstrba Feb 28, 2022
@tomasstrba tomasstrba requested a review from brindy February 28, 2022 13:56
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

LGTM! Was able to open / close windows, navigate to sites, click on menu / fire button, repeat.

@tomasstrba tomasstrba force-pushed the jkt/remove-mouse-listeners-browsertab branch from 30ff977 to ffc23e0 Compare February 28, 2022 14:28
@tomasstrba tomasstrba merged commit 904b9c4 into release/0.19.0 Feb 28, 2022
@tomasstrba tomasstrba deleted the jkt/remove-mouse-listeners-browsertab branch February 28, 2022 14:28
samsymons added a commit that referenced this pull request Mar 2, 2022
# By Tomas Strba (6) and others
# Via GitHub (1) and Tomas Strba (1)
* develop:
  Logins+ authentication (#438)
  Update privacy dashboard to main (#451)
  Version 0.19.1
  Remove mouse event listeners on browser close (#448)
  Find in Page Hotfix + refactoring of forced unwrapping (#450)
  Memory leak fixed (#449)
  Version 0.19.0
  Top autofill (#432)
  Option to add new notes or edit existing is disabled (#446)
  Use our own autoconsent fork (#444)
  New Feedback Form (#424)
  Update privacy dashboard (#440)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/SecureVault/View/PasswordManager.storyboard
samsymons added a commit that referenced this pull request Mar 4, 2022
* develop:
  Logins+ authentication (#438)
  Update privacy dashboard to main (#451)
  Version 0.19.1
  Remove mouse event listeners on browser close (#448)
  Find in Page Hotfix + refactoring of forced unwrapping (#450)
  Memory leak fixed (#449)
  Version 0.19.0
  Top autofill (#432)
  Option to add new notes or edit existing is disabled (#446)
  Use our own autoconsent fork (#444)
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