find in page #24

brindy · 2021-01-18T13:12:52Z

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

Description:

Adds Find in Page support

Steps to test this PR:

Try open find in page on new tab - shouldn't be able to
Navigate to a page and find something that exists, should show current selection and total matches and allow navigation
Navigate to a page and "find" something that doesn't exist - should show 0 of 0 and not allow navigation

Internal references:

Software Engineering Expectations
Technical Design Template

# Conflicts: # DuckDuckGo/Main/View/Base.lproj/Main.storyboard # DuckDuckGo/NavigationBar/View/AddressBarViewController.swift

tomasstrba

@brindy looks good! I've only asked a few questions, but these are just details. The architecture is great 👍

tomasstrba · 2021-01-18T17:33:07Z

DuckDuckGo.xcodeproj/project.pbxproj

 				AA585DB02490E6FA00E9A3E2 /* Main */,
 				AA97BF4425135CB60014931A /* Menus */,
 				AA86491524D83384001BABEE /* NavigationBar */,
 				4B677422255DBEB800025BD8 /* Smarter Encryption */,
+				4B677447255DBF1400025BD8 /* Submodules */,


Agree on ordering the full list. What about having standalone files as Bridging.h, Info.plist and DuckDuckGo.entitlements near the bottom of the list also in alphabetical order? It's too distracting to have them between groups. What do you think?

Thanks @brindy 👍

tomasstrba · 2021-01-18T17:52:45Z

DuckDuckGo/BrowserTab/ViewModel/TabViewModel.swift

+        tab.findInPage = findInPage
+    }
+
+    func closeFindInPage() {


Just checking: This is called from tabDidStartNavigation. Does it makes sense to check whether the find in page is displayed to avoid unnecessary execution of findDone()?

tomasstrba · 2021-01-18T17:57:48Z

DuckDuckGo/Menus/MainMenuActions.swift

+
+    @IBAction func findInPage(_ sender: Any?) {
+        tabCollectionViewModel.selectedTabViewModel?.startFindInPage()
+        findInPageContainerView.isHidden = false


Is it possible to bind hiding/showing of the view to model/viewmodel instead of setting it manually?

Yeah, it already was - had a bit of duplication going on there. Good spot.

…nd retain search term per tab)

brindy · 2021-01-20T07:09:14Z

Made some updates based on demo / PFR and your comments @tomasstrba - good for a final review, I think.

tomasstrba · 2021-01-20T13:50:53Z

@brindy thanks, I'll take a look at it this evening 👍

tomasstrba

Very nice work @brindy! I only found a few very minor details and one bug, but other than that it's perfect. Also thank you for fixing address bar and removing the VerticallyCenteredTextFieldCell class 👍 🥇

tomasstrba · 2021-01-21T11:15:53Z

DuckDuckGo.xcodeproj/project.pbxproj

 				AA585DB02490E6FA00E9A3E2 /* Main */,
 				AA97BF4425135CB60014931A /* Menus */,
 				AA86491524D83384001BABEE /* NavigationBar */,
 				4B677422255DBEB800025BD8 /* Smarter Encryption */,
+				4B677447255DBF1400025BD8 /* Submodules */,


Thanks @brindy 👍

tomasstrba · 2021-01-21T11:16:49Z

DuckDuckGo.xcodeproj/project.pbxproj

@@ -466,6 +475,25 @@
 			path = Model;
 			sourceTree = "<group>";
 		};
+		85A0115D25AF1C4700FA6A0C /* FindInPage */ = {


I love how nicely organized things are so far :)

tomasstrba · 2021-01-21T11:17:38Z

DuckDuckGo.xcodeproj/project.pbxproj

@@ -1486,7 +1518,7 @@
 					"$(inherited)",
 					"@executable_path/../Frameworks",
 				);
-				MARKETING_VERSION = 0.6.5;
+				MARKETING_VERSION = 0.6.5.findinpage;


Should be the .findinpage removed?

Can do, I forgot to remove it before committing. 😨 Figured it would get removed by someone else's commit or during a release.

tomasstrba · 2021-01-21T11:20:01Z

DuckDuckGo/Common/Extensions/NSViewExtension.swift

@@ -40,4 +40,11 @@ extension NSView {
        window.makeFirstResponder(self)
    }

+    func applyDropShadow() {


tomasstrba · 2021-01-21T11:22:36Z

DuckDuckGo/Common/View/VerticallyCenteredTextFieldCell.swift

-
-import Cocoa
-
-class VerticallyCenteredTextFieldCell: NSTextFieldCell {


Happy to see this going! 🥇

tomasstrba · 2021-01-21T11:28:44Z

DuckDuckGo/FindInPage/FindInPageViewController.swift

+    }
+
+    private func listenForTextFieldResponderNotifications() {
+        NotificationCenter.default.addObserver(self,


I would rather see the calling of removeObserver(:)_ too, but I know, we don't need to :)

tomasstrba · 2021-01-21T11:31:29Z

DuckDuckGo/FindInPage/FindInPageWindowController.swift

+import Cocoa
+import os.log
+
+class FindInPageWindowController: NSWindowController {


I think this class is not used anywhere and can be removed but I may be wrong.

Yep, from my first attempt to do this as a window rather than view - will remove.

Xcode hadn't deleted it from disk. I definitely selected "move to trash" - need to keep an eye on that!

Happend to me to :) I just can't believe source code editor can have this issue for almost a year

tomasstrba · 2021-01-21T11:43:49Z

DuckDuckGo/Main/MainViewController.swift

@@ -112,6 +134,21 @@ class MainViewController: NSViewController {
        }
    }

+    private func updateFindInPage() {


This method is probably responsible for a few problems with the first responder. It's not happening all the time, but sometimes opening a new tab doesn't make the text field first responder. Opening a new window helps to reproduce the issue.

tomasstrba · 2021-01-21T11:46:10Z

DuckDuckGo/NavigationBar/View/AddressBarViewController.swift

+    }
+
+    override func mouseEntered(with event: NSEvent) {
+        NSCursor.iBeam.set()


Much nicer and cleaner 👍

tomasstrba · 2021-01-21T13:30:18Z

DuckDuckGo/Main/MainViewController.swift

@@ -135,18 +135,23 @@ class MainViewController: NSViewController {
    }

    private func updateFindInPage() {
+        print("***", #function)


tomasstrba

LGTM! :)

Task/Issue URL: https://app.asana.com/0/1203512625915051/1203969517643013/f and https://app.asana.com/0/1203512625915051/1203971625280790/f Tech Design URL: BSK PR: more-duckduckgo-org/browserserviceskit-network-protection#17

brindy added 11 commits January 13, 2021 15:57

handle cmd-f and show window

0d68503

initial ui for find in page

ce87f5b

refresh the accent color when set by the system

4a0f9e9

rename address bar view to focus ring view

e8825a2

remove the vertical alignment logic for the address bar

bb9320a

handle escape button

a302ef4

make address bar more responsive

597af65

disable next / previous menu items when there's no matches

96c1f05

basically works now, just needs to be tidied up

a2a5abd

Merge branch 'develop' into feature/brindy/find-in-page

8f57b91

# Conflicts: # DuckDuckGo/Main/View/Base.lproj/Main.storyboard # DuckDuckGo/NavigationBar/View/AddressBarViewController.swift

tidy up

1539937

tomasstrba reviewed Jan 18, 2021

View reviewed changes

brindy added 5 commits January 19, 2021 18:28

fix issues from demo / feedback (buttons not working unless focused a…

21a25a9

…nd retain search term per tab)

Merge branch 'develop' into feature/brindy/find-in-page

33d5347

update to use latest styles

248f0b2

move standalone files to the bottom

80f8b05

update from feedback

66f81ca

brindy changed the title ~~[WIP] find in page~~ find in page Jan 20, 2021

minor change to show status when there are no hits as 0 of 0

bc07740

tomasstrba suggested changes Jan 21, 2021

View reviewed changes

remove unused file and fix giving address bar focus on new tab

45e30e3

brindy requested a review from tomasstrba January 21, 2021 13:14

tomasstrba reviewed Jan 21, 2021

View reviewed changes

remove debug

0c6f3a7

tomasstrba approved these changes Jan 21, 2021

View reviewed changes

tomasstrba merged commit 3ea4418 into develop Jan 21, 2021

tomasstrba deleted the feature/brindy/find-in-page branch January 21, 2021 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

find in page #24

find in page #24

brindy commented Jan 18, 2021 •

edited

Loading

tomasstrba left a comment

tomasstrba Jan 18, 2021

tomasstrba Jan 21, 2021

tomasstrba Jan 18, 2021

tomasstrba Jan 18, 2021

brindy Jan 20, 2021

brindy commented Jan 20, 2021

tomasstrba commented Jan 20, 2021

tomasstrba left a comment

tomasstrba Jan 21, 2021

tomasstrba Jan 21, 2021

tomasstrba Jan 21, 2021

brindy Jan 21, 2021

tomasstrba Jan 21, 2021

tomasstrba Jan 21, 2021

tomasstrba Jan 21, 2021

tomasstrba Jan 21, 2021

brindy Jan 21, 2021

brindy Jan 21, 2021 •

edited

Loading

tomasstrba Jan 21, 2021

tomasstrba Jan 21, 2021

tomasstrba Jan 21, 2021

tomasstrba Jan 21, 2021

tomasstrba left a comment


		import Cocoa

		class VerticallyCenteredTextFieldCell: NSTextFieldCell {

find in page #24

find in page #24

Conversation

brindy commented Jan 18, 2021 • edited Loading

Internal references:

tomasstrba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brindy commented Jan 20, 2021

tomasstrba commented Jan 20, 2021

tomasstrba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brindy Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasstrba left a comment

Choose a reason for hiding this comment

brindy commented Jan 18, 2021 •

edited

Loading

brindy Jan 21, 2021 •

edited

Loading