Hides delimiters in banner when surrounded by loaded shield images #1046

akitchen · 2018-01-19T21:42:48Z

This delivers #1027

Includes some refactoring to avoid use of disk cache under test, and adds some synchronous and asynchronous integrated tests of the desired behavior.

Presentation and domain logic are moved into an InstructionPresenter class, which already knows about too much but I think it's an incremental improvement on doing all this work in the view.

TO DO list before merging:

Update constants / namespace identifiers per @frederoni 's comments
Get feedback on appropriate visibility level for new classes (?)
Implement error handling in line with existing patterns
Remove SDWebImage from Cartfile, delete related extensions, etc.
Discover more test cases and update implementation so that delimiter hiding logic is more robust

Tail work:

Backfill some tests as indicated as TODOs int the code
Add background execution handler so that we can allow our download operations to continue when the containing app goes into the background (some investigation needed; want to test on a device in order to evaluate the need)
Force download / cache busting ?
Test in the wild (again)

frederoni · 2018-01-21T20:15:10Z

MapboxNavigation/InstructionPresenter.swift

+    }
+
+    private func instructionHasDownloadedAllShields() -> Bool {
+        for component in instruction! {


Nit: this seems to be safe considering the guard in attributedTextForLabel(_:) but the linter will complain about unwrapping when we decide to enable it.

frederoni · 2018-01-21T20:15:29Z

MapboxNavigationTests/MapboxNavigationTests.swift

+
+        SDImageCache.shared().clearMemory()
+
+        let clearDiskSemaphore = DispatchSemaphore.init(value: 1)


nit: .init can be dropped.

frederoni · 2018-01-21T20:24:05Z

MapboxNavigation/InstructionPresenter.swift

+import UIKit
+import MapboxDirections
+
+class InstructionPresenter {


👍 Cool. Perhaps an Instructable protocol could be the next step.

I'm into the idea -- let's look out for a unified interface when we write/refactor the next view which might otherwise contain logic for setting up & displaying a complex domain object.

bsudekum

Looks great. Let's hold off on finally merging until we can properly test this with the server. It looks like there is a slight bug that prevents delimiter: true from showing up in some cases.

akitchen · 2018-01-23T16:59:08Z

Cool. I addressed the code nit and rebased on master again.

frederoni · 2018-02-07T12:25:28Z

MapboxNavigation/ImageCache.swift

+    let diskCacheURL: URL = {
+        let fileManager = FileManager.default
+        let basePath = fileManager.urls(for: .cachesDirectory, in: .userDomainMask).first!
+        return basePath.appendingPathComponent("com.mapbox.directions.downloadedImages")


Nit: directions is somewhat misleading, perhaps we could use Bundle.mapboxNavigation.bundleIdentifier+suffix? ×2

This is helpful, you've answered one of my tail work questions.

frederoni · 2018-02-07T12:26:44Z

MapboxNavigation/ImageCache.swift

+
+        if toDisk == true {
+            diskAccessQueue.async {
+                self.createCacheDirIfNeeded()


Might be mistaken but capturing self here should result in a retain cycle?

Yes, there is a transient retain cycle there. While this block is in the queue or executing, self will be retained which ensures it sticks around long enough to complete its work. Once the block exits it is disposed of, breaking the cycle.

Does this reasoning hold water or does the team prefer a weakSelf/strongSelf pattern in these disk access methods?

akitchen · 2018-02-09T00:37:35Z

Looking forward to merging this. There's still a bit of work to do added to the OP

But I think it's looking good. If there are other PRs which collide, maybe we can integrate them together before the final merge into master. /cc @bsudekum @JThramer

bsudekum · 2018-02-09T19:42:17Z

MapboxNavigation/ImageCache.swift

+    }
+
+    private func createCacheDirIfNeeded() {
+        if fileManager!.fileExists(atPath: diskCacheURL.absoluteString) == false {


Can we gently unwrap fileManager here?

Done - also refactored to rely less on data from self in closures, addressing @frederoni 's previous observation about the transient retain cycles on the disk access queue.

bsudekum · 2018-02-09T19:44:58Z

MapboxNavigation/ImageDownloader.swift

+            completionHandler(.cancel)
+            return
+        }
+        operation.urlSession!(session, dataTask: dataTask, didReceive: response, completionHandler: completionHandler)


operation.urlSession! can also be unwrapped above in the guard.

Instead of unwrapping, I've opted to ask before telling in the latest commit ( which will ensure that the force unwrap is safe )

A few TODOs remain, specifically around handling memory warnings. Also some DRY refactoring still needed

ImageCache dumps its in-memory cache if a memory warning is received

Provide a backwards-compatible shim for SDImageCache even though we're no longer using it

This only works for one request per URL at the moment. Next step is to encapsulate work into an operation which will allow for the addition of callbacks for an in-flight request (and avoid duplicate parallel requests for the same resource)

Sad paths still need to be implemented.

Co-Authored-By: Fredrik Karlsson <bjorn.fredrik.karlsson@gmail.com>

Remove redundant success flag from callbacks A few test backfills still needed Co-Authored-By: Fredrik Karlsson <bjorn.fredrik.karlsson@gmail.com>

Allow presenter to be thrown away when re-setting an instruction Log error on image download error Pad shield images with a space

Refactor InstructionPresenter to take its dependencies on init, removing some unnecessary optionality Nil out the instructions in StepTableViewCell's prepareForReuse()

akitchen · 2018-02-20T01:44:07Z

I just merged master into this branch as the semantic disconnects were a bit much. /cc @frederoni

Would love to merge this with a final 👍 and follow on with some tail work as enumerated in the OP

akitchen · 2018-02-20T01:45:52Z

MapboxNavigation/InstructionsBannerView.swift

@@ -71,8 +71,8 @@ open class BaseInstructionsBannerView: UIControl {
        delegate?.didTapInstructionsBanner(self)
    }

-    func set(_ instruction: VisualInstruction) {
-        let secondaryInstruction = instruction.secondaryTextComponents
+    func set(_ instruction: VisualInstruction?) {


This optionality is needed in order to unset the instruction in StepTableViewCell.prepareForReuse()

/cc @frederoni

also, should we use a different semantic here now that we've wrapped the primary and secondary components in the VisualInstruction class? Or does this feel idiomatic enough?

Just set or set(_:) in this case almost feels like a reserved keyword but I don't have a strong opinion about that. Maybe setInstruction(_:) or updateInstruction(_:) would be more idiomatic?

💯 % agree, @frederoni. A update(routeProgress:), update(legProgress:) or update(instruction:) pattern would be pretty straightforward.

Let's do this as tail work.

Related to #1046

akitchen requested review from frederoni and bsudekum January 19, 2018 21:42

akitchen force-pushed the delimiter branch from cf4e676 to 0d0bb24 Compare January 19, 2018 22:01

frederoni reviewed Jan 21, 2018

View reviewed changes

frederoni approved these changes Jan 21, 2018

View reviewed changes

bsudekum approved these changes Jan 22, 2018

View reviewed changes

akitchen force-pushed the delimiter branch from 0d0bb24 to 194e139 Compare January 23, 2018 16:57

akitchen force-pushed the delimiter branch 3 times, most recently from 25c4a07 to a748cd1 Compare February 1, 2018 00:42

akitchen force-pushed the delimiter branch 5 times, most recently from 812935b to 45caabf Compare February 7, 2018 00:49

frederoni reviewed Feb 7, 2018

View reviewed changes

frederoni mentioned this pull request Feb 8, 2018

Refactor instructions banner #1093

Closed

4 tasks

akitchen force-pushed the delimiter branch from 99cf823 to 9546351 Compare February 8, 2018 18:13

akitchen force-pushed the delimiter branch 4 times, most recently from d82aaed to 14de9cb Compare February 9, 2018 18:30

bsudekum reviewed Feb 9, 2018

View reviewed changes

akitchen force-pushed the delimiter branch from dbc4d88 to 7e13603 Compare February 13, 2018 02:44

akitchen and others added 14 commits February 15, 2018 08:42

WIP / checkpoint test-driving new ImageCache implementation

6142bcc

A few TODOs remain, specifically around handling memory warnings. Also some DRY refactoring still needed

Wire up new ImageCache class.

12dbe4d

ImageCache dumps its in-memory cache if a memory warning is received

Remove optionality to be more in line with API intent

8a4ad2c

Provide a backwards-compatible shim for SDImageCache even though we're no longer using it

ImageDownloader supports basic happy-path case, including reentrancy

1ce0114

Sad paths still need to be implemented.

ImageRepository now uses the new ImageDownloader

9be47d6

Updates namespace identifiers to use bundle identifier

7544da9

Remove SDWebImage and fix up remaining test helper

661c210

Co-Authored-By: Fredrik Karlsson <bjorn.fredrik.karlsson@gmail.com>

Introduce Error type for client and server side download errors

fbd9693

Remove redundant success flag from callbacks A few test backfills still needed Co-Authored-By: Fredrik Karlsson <bjorn.fredrik.karlsson@gmail.com>

Extract private method for writing to memory cache with cost

61dc2d1

Improvements to optionality, unwrapping, and the "no image data" error

fe713fe

Delegate method unwrapping didn't work as expected. Alternate approach.

e5a5825

Ensure images are recreated with the appropriate scale factor

363fde3

More use of weak references to better handle cell re-use edge cases

76e8417

Allow presenter to be thrown away when re-setting an instruction Log error on image download error Pad shield images with a space

akitchen force-pushed the delimiter branch from 4fb07e9 to 76e8417 Compare February 15, 2018 16:43

akitchen added 3 commits February 19, 2018 16:51

Implement remaining Operation lifecycle hooks to avoid jamming the queue

25c588f

Refactor InstructionPresenter to take its dependencies on init, removing some unnecessary optionality Nil out the instructions in StepTableViewCell's prepareForReuse()

Update Maps SDK version

6491236

Merge remote-tracking branch 'origin' into delimiter

7edc0cc

akitchen commented Feb 20, 2018

View reviewed changes

bsudekum approved these changes Feb 20, 2018

View reviewed changes

bsudekum merged commit 101d430 into master Feb 20, 2018

bsudekum deleted the delimiter branch February 20, 2018 19:28

1ec5 mentioned this pull request Feb 24, 2018

Hide delimiters in banner when surrounded by loaded shield images #1027

Closed

akitchen mentioned this pull request Feb 26, 2018

ImageDownloader/InstructionPresenter tail work / cleanup / refactor #1157

Merged

akitchen added a commit that referenced this pull request Feb 26, 2018

ImageDownloader/InstructionPresenter tail work / cleanup / refactor

9b838b6

Related to #1046

akitchen added a commit that referenced this pull request Feb 26, 2018

ImageDownloader/InstructionPresenter tail work / cleanup / refactor

76be4e5

Related to #1046

frederoni mentioned this pull request Mar 1, 2018

Load resources from Bundle #1017

Closed

frederoni mentioned this pull request Jun 28, 2019

Add MapboxAccounts #2151

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hides delimiters in banner when surrounded by loaded shield images #1046

Hides delimiters in banner when surrounded by loaded shield images #1046

akitchen commented Jan 19, 2018 •

edited

Loading

frederoni Jan 21, 2018

frederoni Jan 21, 2018

frederoni Jan 21, 2018

akitchen Jan 23, 2018

bsudekum left a comment

akitchen commented Jan 23, 2018

frederoni Feb 7, 2018

akitchen Feb 9, 2018

frederoni Feb 7, 2018 •

edited

Loading

akitchen Feb 9, 2018

akitchen commented Feb 9, 2018 •

edited

Loading

bsudekum Feb 9, 2018

akitchen Feb 9, 2018

bsudekum Feb 9, 2018

akitchen Feb 9, 2018

akitchen commented Feb 20, 2018

akitchen Feb 20, 2018

akitchen Feb 20, 2018

frederoni Feb 20, 2018

JThramer Feb 20, 2018 •

edited

Loading

bsudekum Feb 20, 2018


		SDImageCache.shared().clearMemory()

		let clearDiskSemaphore = DispatchSemaphore.init(value: 1)

Hides delimiters in banner when surrounded by loaded shield images #1046

Hides delimiters in banner when surrounded by loaded shield images #1046

Conversation

akitchen commented Jan 19, 2018 • 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

bsudekum left a comment

Choose a reason for hiding this comment

akitchen commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akitchen commented Feb 9, 2018 • 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

akitchen commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akitchen commented Jan 19, 2018 •

edited

Loading

frederoni Feb 7, 2018 •

edited

Loading

akitchen commented Feb 9, 2018 •

edited

Loading

JThramer Feb 20, 2018 •

edited

Loading