Fixes #533 - changing to retain the keyword searched instead final url #1215

marchiore · 2018-08-11T18:25:04Z

This PR is a continuation of an old PR (#544) and it fixes #533.

@oliviabrown9 can you check if everything is ok now? if you find something wrong i'll be glad in adjust it.

oliviabrown9

Overall looks good. Two major things to change: the functionality of display vs. truncated URL when editing regresses with this PR, and I think we should erase the search stack when the user presses erase.

oliviabrown9 · 2018-08-13T19:28:33Z

Blockzilla.xcodeproj/project.pbxproj

@@ -53,7 +54,7 @@
 		747ADA181FC38EFC00970132 /* IntroViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 747ADA171FC38EFC00970132 /* IntroViewController.swift */; };
 		74F94FF21FD9CA070047E629 /* Intro.strings in Resources */ = {isa = PBXBuildFile; fileRef = 74F94FF41FD9CA070047E629 /* Intro.strings */; };
 		A57245250F88CCA4993E55B2 /* CoreText.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = F4C4943B406FCA9B74B4E186 /* CoreText.framework */; };
-		A89766DA1F57DCA9008183C5 /* BuildFile in Resources */ = {isa = PBXBuildFile; };
+		A89766DA1F57DCA9008183C5 /* (null) in Resources */ = {isa = PBXBuildFile; };


I'm a little bit confused by why/where this change is coming from.

oliviabrown9 · 2018-08-13T19:29:03Z

Blockzilla/URLBar.swift

@@ -470,6 +470,8 @@ class URLBar: UIView {
    func fillUrlBar(text: String) {
        urlText.text = text
    }
+


Nit: remove unnecessary whitespace changes.

oliviabrown9 · 2018-08-13T19:31:22Z

Blockzilla/URLBar.swift

            truncatedURL = components?.host
-            urlText.text = isEditing ? fullUrl : truncatedURL


This reverts one of the 7.0 PRs that displays a truncated version of the URL except when editing. We now need to have three states (search query if a search, full URL when editing, truncated url otherwise). All URLs should still display and act exactly the same as before, only search queries changing.

I've changed it to check if the value on my stack has a prefix like http:// or https://, so now i can check if it has to show the truncated url or my searched keyword.

One thing i've noticed, is that the isEditing value, is never getting the true value, so i made a clean clone from the project (without my changes) and noticed that this happens in there too. I don't know if it's a bug or not (or if someone is working on this in parallel).

oliviabrown9 · 2018-08-13T19:36:43Z

Blockzilla/SearchHistoryUtils.swift

+        return ""
+    }
+
+    // go back


I don't think this comment adds anything.

Yeah, agree with you. Fixed!

oliviabrown9 · 2018-08-13T19:36:55Z

Blockzilla/SearchHistoryUtils.swift

+        }
+    }
+
+    // go foward


I don't think this comment adds anything.

Yeah, agree with you. Fixed!

oliviabrown9 · 2018-08-13T19:37:19Z

Blockzilla/SearchHistoryUtils.swift

+    }
+
+    // go back
+    static func goFoward() {


Nit: should be "goForward"

oliviabrown9 · 2018-08-13T19:37:41Z

Blockzilla/SearchHistoryUtils.swift

+            }
+        }
+
+        return ""


Should return nil since it's optional.

oliviabrown9 · 2018-08-13T19:38:18Z

Blockzilla/URLBar.swift

        var truncatedURL: String? = nil

        if let url = url {
            // Strip the username/password to prevent domain spoofing.
            var components = URLComponents(url: url, resolvingAgainstBaseURL: false)
            components?.user = nil
            components?.password = nil
-            fullUrl = components?.url?.absoluteString
+
+            if let searchedText = SearchHistoryUtils.pullSearchFromStack(), searchedText != "" {


After you change the function to return nil, you should be able to remove the check for != ""

oliviabrown9 · 2018-08-13T19:41:29Z

Blockzilla/SearchHistoryUtils.swift

+    // go back
+    static func goFoward() {
+        isNavigating = true
+        var currentStack = [textSearched]()


Can you clarify that this will effectively "erase" the stack on each new session? I might be more comfortable having an explicit clear of the stack when the user presses erase -> I think it goes along with the idea that we clear everything we have on the user when they press erase.

I'm cleaning the stack on BrowserViewController.swiftin line 439.

oliviabrown9 · 2018-08-13T19:42:24Z

Blockzilla.xcodeproj/project.pbxproj

@@ -1428,7 +1431,7 @@
 			files = (
 				742C99D41F3A3AD200717D69 /* Assets.xcassets in Resources */,
 				16938C0121010C2D00DCD489 /* InfoPlist.strings in Resources */,
-				A89766DA1F57DCA9008183C5 /* BuildFile in Resources */,


I'm a little bit confused by why/where this change is coming from.

…into v7.0-dev

marchiore · 2018-08-14T03:33:39Z

@oliviabrown9, Thank You for your notes! I've updated my PR and comment on your notes above.

If something wrong, let me know!

:)

oliviabrown9

Good progress! Still looks like the truncated URLs are not displaying correctly.

oliviabrown9 · 2018-08-14T17:46:47Z

Blockzilla/URLBar.swift

+            } else {
+                displayText = truncatedURL
+            }
+            urlText.text = isEditing ? fullUrl : displayText


Looks like this still isn't working as expected. Rather than checking for http/https (which isn't always a given), you might want to check if it can be a URL. You can verify that your solution works by comparing it to the urls currently displayed when you run v7.0-dev.

Example: twitter.com (when using autocomplete)
Expected:

Result:

…into v7.0-dev

marchiore · 2018-08-15T02:53:26Z

@oliviabrown9, i've changed the validation to check if the string it's a URL, as you suggested, and i've updated my PR, can you check this out?

Thanks again for helping me out to solve this issue!

oliviabrown9 · 2018-08-15T21:24:52Z

The URL bar is still displaying the incorrect full/truncated URL, but I realized that's actually a 7.0 issue rather than specific to this PR. I'm going to wait on #1248 to land to see how that affects this one and see if it all comes together.

…into v7.0-dev

oliviabrown9 · 2018-08-20T17:42:04Z

@marchiore Can you merge in the latest v7.0-dev and ping me when ready for review again?

marchiore · 2018-08-20T22:09:52Z

@oliviabrown9 merged!

I've noticed a problem on Autocomplete, where it sometimes put a '/' at the begging of type. Is there someting the team is working on?

P.S.: i've checked on a clean branch and the problem persists.

oliviabrown9 · 2018-08-20T22:15:45Z

@marchiore I haven't heard about or seen this issue before. Would you mind reproducing and uploading a photo/gif for us to investigate separately from this PR since it sounds unrelated.

oliviabrown9

Looks great! Thanks for all your work on this!

…earched instead final url (mozilla-mobile/focus-ios#1215) * Storing the Searched Keyword to show in the URLBar * Adding a validation to URLBar to check what's correct info to display * Adding a validation to check if the value on the stack is a url

…he keyword searched instead final url (mozilla-mobile/focus-ios#1215) * Storing the Searched Keyword to show in the URLBar * Adding a validation to URLBar to check what's correct info to display * Adding a validation to check if the value on the stack is a url

Storing the Searched Keyword to show in the URLBar

f2811a9

marchiore mentioned this pull request Aug 11, 2018

Fixes #533 - changing to retain the keyword searched instead final url #544

Closed

oliviabrown9 suggested changes Aug 13, 2018

View reviewed changes

marchiore added 2 commits August 13, 2018 22:34

Merge branch 'v7.0-dev' of https://github.com/mozilla-mobile/focus-ios …

bdbdafd

…into v7.0-dev

Adding a validation to URLBar to check what's correct info to display

34272be

oliviabrown9 suggested changes Aug 14, 2018

View reviewed changes

marchiore added 2 commits August 14, 2018 23:45

Adding a validation to check if the value on the stack is a url

ae79d76

Merge branch 'v7.0-dev' of https://github.com/mozilla-mobile/focus-ios …

38d5ea0

…into v7.0-dev

marchiore mentioned this pull request Aug 15, 2018

Fixes #1247: URL bar no longer displays full URL when editing #1248

Merged

Merge branch 'v7.0-dev' of https://github.com/mozilla-mobile/focus-ios …

d36e152

…into v7.0-dev

merge

6cace13

oliviabrown9 approved these changes Aug 20, 2018

View reviewed changes

oliviabrown9 merged commit cf1c3ec into mozilla-mobile:v7.0-dev Aug 20, 2018

oliviabrown9 mentioned this pull request Aug 20, 2018

Retain keyword search rather than resolved address after query #533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #533 - changing to retain the keyword searched instead final url #1215

Fixes #533 - changing to retain the keyword searched instead final url #1215

marchiore commented Aug 11, 2018

oliviabrown9 left a comment

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018 •

edited

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

oliviabrown9 Aug 13, 2018

marchiore Aug 14, 2018

marchiore commented Aug 14, 2018 •

edited

oliviabrown9 left a comment

oliviabrown9 Aug 14, 2018

marchiore commented Aug 15, 2018

oliviabrown9 commented Aug 15, 2018

oliviabrown9 commented Aug 20, 2018

marchiore commented Aug 20, 2018

oliviabrown9 commented Aug 20, 2018

oliviabrown9 left a comment

		truncatedURL = components?.host
		urlText.text = isEditing ? fullUrl : truncatedURL

Fixes #533 - changing to retain the keyword searched instead final url #1215

Fixes #533 - changing to retain the keyword searched instead final url #1215

Conversation

marchiore commented Aug 11, 2018

oliviabrown9 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

marchiore Aug 14, 2018 • edited

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

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

marchiore commented Aug 14, 2018 • edited

oliviabrown9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchiore commented Aug 15, 2018

oliviabrown9 commented Aug 15, 2018

oliviabrown9 commented Aug 20, 2018

marchiore commented Aug 20, 2018

oliviabrown9 commented Aug 20, 2018

oliviabrown9 left a comment

Choose a reason for hiding this comment

marchiore Aug 14, 2018 •

edited

marchiore commented Aug 14, 2018 •

edited