Swipe improvements #70

theginbin · 2017-02-02T01:19:24Z

What?

Improvements to the swipe-between-projects experience.

Endless swipe
Return to the correct position on closing a project (Discovery, Profile, Search).
Smoother transitioning between projects.
Tracking "next" vs "previous" swipes.

The basic flow is: ProjectNavigatorViewController calls a delegate method when a page transition is complete with the current project index. The delegate view controller then can use that index to determine whether to load new projects. If new projects are loaded and the Project Navigator is being presented, the new projects are passed to it and its datasource is updated. The same delegate method is also triggering the scroll-to-position after the page transitions.

theginbin · 2017-02-02T01:22:35Z

Kickstarter-iOS/DataSources/ProjectNavigatorPagesDataSource.swift

@@ -18,6 +18,10 @@ internal final class ProjectNavigatorPagesDataSource: NSObject, UIPageViewContro
    self.padControllers(toLength: self.playlist.count)
  }

+  internal func updatePlaylist(_ playlist: [Project]) {
+    self.playlist = playlist


This is almost too simple to be real, but I haven't seen any issues. Basically just replacing the UIPageViewController list of projects completely. Is it really this simple?

theginbin · 2017-02-02T01:24:26Z

Kickstarter-iOS/DataSources/ProjectPamphletContentDataSource.swift

-      .comments(project.stats.commentsCount ?? 0, liveStreamSubpages.isEmpty ? .first : .middle),
-      .updates(project.stats.updatesCount ?? 0, .last)
+      .comments(project.stats.commentsCount as Int?, liveStreamSubpages.isEmpty ? .first : .middle),
+      .updates(project.stats.updatesCount as Int?, .last)


Initially had this here for doing animation in the subpage cell, but didn't need the animation after all. Decided to leave the coalescing to the view model because it felt better.

theginbin · 2017-02-02T01:27:20Z

Kickstarter-iOS/Tests/Views/ProjectPamphletContentViewControllerTests.swift

+   VideoViewController (viewDidLoad is the last lifecycle method called on this VC).
+   Option A: Keep the play button off the new screenshots. Option B: Figure out how to call viewDidAppear.
+   Let's chat.
+  */


let's chat.

theginbin · 2017-02-02T01:30:13Z

Kickstarter-iOS/Views/Controllers/SearchViewController.swift

@@ -124,6 +125,13 @@ internal final class SearchViewController: UITableViewController {
      .observeValues { [weak self] in
        self?.changeSearchFieldFocus(focus: $0, animated: $1)
    }
+
+    self.viewModel.outputs.scrollToProjectRow
+      .observeForControllerAction() // NB: this also is required to prevent NSException.


noticed this was crashing with observeForUI. Something special about the Search controller?

Possibly related to #68?

i wasnt able to repro this crash so i started thinking maybe it got fixed somehow.

i just paired on this e547c7d with gina. we can use a feature flag to slowly ramp up how many people get the observeForUI version and if we don't see any crashes we can go 100%

theginbin · 2017-02-02T01:33:07Z

Library/Tests/ViewModels/SearchViewModelTests.swift

+
+        // this works in practice, gotta figure out what i'm missing in the test.
+        self.hasAddedProjects.assertValues([true, true], "More projects are loaded.")
+


Same test in DiscoveryPage and Profile, but it fails here (works in the sim/device though). Just need to step away for a hot min and think about what's missing.

theginbin · 2017-02-02T01:55:34Z

Looks like after merging with master LiveStream updates, the project pamp seems to be loading a project sooner and is messing with how I set things up so gotta investigate...

theginbin · 2017-02-02T02:02:00Z

Library/ViewModels/ProjectPamphletContentViewModel.swift

-        )
-        .take(first: 1)
+      projectAndLiveStreamEvents,
+      self.viewWillAppearAnimatedProperty.signal.take(first: 1)


I don't know the reason we had the merge of these different appearance events, so we should chat about this @mbrandonw. I changed this to just viewWillAppear so that the skeleton of the project could appear while the main project cell loads. This way the subpage cell and title can show some content until the project gets refreshed instead of the blank space. Problem is, after merging with master, this is happening at lightning speed, as if the full project is being loaded much earlier than before. It's interfering with page swiping and locking up the UI. Need to look into it.

i have a solution, will chat you when i'm finished!

theginbin · 2017-02-03T22:54:07Z

ok i think this bad boy is ready to be reviewed! cc @justinswart cuz we refactored ProjectPamphletViewModel where LiveStreamEvents are being fetched.

justinswart

Nice! I like how you've changed the refreshing of project and fetching the live streams.

Made a couple small comments 👍

justinswart · 2017-02-04T10:17:46Z

Library/ViewModels/ProjectPamphletViewModel.swift

@@ -224,3 +192,29 @@ private func cookieFrom(refTag: RefTag, project: Project) -> HTTPCookie? {

  return HTTPCookie(properties: properties)
 }
+
+private func fetchProjectAndLiveStreams(projectOrParam: Either<Project, Param>)


Very cool! It's nice having the fetching bundled like this and I think it makes what we're doing clearer. This would just make project refreshes take slightly longer though right, i.e. max 5 secs if live stream fetch times out?

I like the added benefit of less emissions from that combineLatest 👍

Yes the fetch does take slightly longer, but only when coming from a Push or other Param-based route (since in that case the project does not emit immediately with the prefix value). I noticed the UI was stuttering a bit on swipe after merging Live stuff so wanted to cut down on the number of times the view is rendered.

justinswart · 2017-02-04T10:19:09Z

LiveStream/Service/MockLiveStreamService.swift

@@ -55,7 +55,7 @@ internal struct MockLiveStreamService: LiveStreamServiceProtocol {

  internal func fetchEvents(forProjectId projectId: Int, uid: Int?) ->
    SignalProducer<LiveStreamEventsEnvelope, LiveApiError> {
-    if let error = self.fetchEventResult?.error {
+    if let error = self.fetchEventsForProjectResult?.error {


Whoops! Good catch 👍

justinswart · 2017-02-04T12:22:09Z

Library/ViewModels/ProjectPamphletViewModel.swift

+          .flatMapError { _ in SignalProducer(error: SomeError()) }
+          .timeout(after: 5, raising: SomeError(), on: AppEnvironment.current.scheduler)
+          .materialize()
+          .map { (project, .some($0.value?.liveStreamEvents ?? [])) }


Think this can just be

.map { (project, $0.value?.liveStreamEvents) }

Ah so this is really subtle and will cause some tracking tests to fail. We want to be explicit that this is an optional for the filtering logic to work when tracking the page view (i.e. don't track if LiveStreamEvents array is nil, wait until we have the full project data). Specifically on a timeout or when the load errors, we still want to track everything so we have to coalesce here. Make sense?

Ah gotcha, ok cool thanks for explaining!

super subtle and glad tests helped make my intent clear! gina described it perfectly.

justinswart · 2017-02-04T12:22:44Z

Library/ViewModels/ProjectPamphletViewModel.swift

@@ -129,9 +95,11 @@ ProjectPamphletViewModelOutputs {

  fileprivate let projectOrParamProperty = MutableProperty<Either<Project, Param>?>(nil)
  fileprivate let refTagProperty = MutableProperty<RefTag?>(nil)


projectOrParamProperty and refTagProperty look like they can be removed.

ah yes, thanks

justinswart · 2017-02-04T12:24:54Z

Library/ViewModels/ProjectPamphletViewModel.swift

+    let projectAndLiveStreams = AppEnvironment.current.apiService.fetchProject(param: param)
+      .ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler)
+      .demoteErrors()
+      .flatMap { project -> SignalProducer<(Project, [LiveStreamEvent]?), NoError> in


Do we want this to be a switchMap potentially or should the fact that the calling site is in a switchMap be sufficient for discarding earlier emissions? 🤔

good question. i believe that the earlier switchMap is sufficient, but I am no expert.

I also think the call-site being a switchMap should take care of it but perhaps @mbrandonw can confirm.

justinswart · 2017-02-04T12:30:43Z

Library/ViewModels/ProjectPamphletViewModel.swift

      .take(first: 1)

-    Signal.combineLatest(project, trackLiveStreamEvents, refTag, cookieRefTag)
+    Signal.combineLatest(freshProjectAndLiveStreamsAndRefTag, cookieRefTag)
+      .map { ($0.0, $0.1, $0.2, $1) }


More verbose but perhaps for readability we could do:

.map { projectAndLiveStreamsAndRefTag, cookieRefTag in let (project, liveStreamEvents, refTag) = projectAndLiveStreamsAndRefTag return (project, liveStreamEvents, refTag, cookieRefTag) }

what about this, simpler, but shorter:

.map { (project: $0.0, liveStreamEvents: $0.1, refTag: $0.2, cookieRefTag: $1) }

Ah, yes that is good, reads better!

justinswart · 2017-02-06T18:24:31Z

Approved the couple things I noticed/had questions about but perhaps someone closer to this part of the app should still give it a once-over! :)

Nice work!

mbrandonw

great job gin! can't believe how short and simple this was.

i only got a few small comments.

mbrandonw · 2017-02-08T16:37:55Z

Kickstarter-iOS/Views/Controllers/ProjectNavigatorViewController.swift

@@ -4,6 +4,8 @@ import Prelude
 import UIKit

 internal protocol ProjectNavigatorDelegate: class {
+  /// Call when a page view controller has completed transitioning.


nit-picky, but this comment should be Called when... since we are describing when we invoke this method on the delegate, not when to call it.

that makes sense. i think 90% of our delegate comments are "Call when" so we should consider updating across the board.

mbrandonw · 2017-02-08T16:44:26Z

Kickstarter-iOS/Views/Controllers/ProjectNavigatorViewController.swift

@@ -118,6 +127,10 @@ internal final class ProjectNavigatorViewController: UIPageViewController {
    return self.viewControllers?.first
  }

+  internal func updatePlaylist(_ playlist: [Project]) {


maybe we could add some docs here to describe that it is expected that the delegate call this anytime it gets an updated playlist of projects.

mbrandonw · 2017-02-08T16:47:48Z

Library/ViewModels/DiscoveryPageViewModel.swift

-      .map { row, total in row >= total - 3 && row > 0 }
+    let isCloseToBottom = Signal.merge(
+      self.willDisplayRowProperty.signal.skipNil(),
+      self.transitionedToProjectRowAndTotalProperty.signal.skipNil()


nice that you can just merge these!

mbrandonw · 2017-02-08T16:49:30Z

Kickstarter-iOS/DataSources/DiscoveryProjectsDataSource.swift

@@ -52,6 +52,10 @@ internal final class DiscoveryProjectsDataSource: ValueCellDataSource {
    return self[indexPath] as? Project
  }

+  internal func indexPath(for projectRow: Int) -> IndexPath {


this should be renamed to something like indexPath(forProjectRow:) so that it's clear we are looking in the project section. right now at the call-site it looks like self.dataSource.indexPath(for: row) which isn't super clear what we are asking for.

mbrandonw · 2017-02-08T16:49:46Z

Kickstarter-iOS/DataSources/SearchDataSource.swift

@@ -37,6 +37,10 @@ internal final class SearchDataSource: ValueCellDataSource {
    }
  }

+  internal func indexPath(for projectRow: Int) -> IndexPath {


mbrandonw · 2017-02-08T16:57:57Z

Kickstarter-iOS/Views/Controllers/ProfileViewController.swift

+      .observeValues { [weak self] itemIndex in
+        guard let _self = self else { return }
+        _self.collectionView?.scrollToItem(at: _self.dataSource.indexPath(for: itemIndex), at: .top,
+                                           animated: false)


could we put at: .top on a newline so that all params are grouped together?

mbrandonw · 2017-02-08T17:23:28Z

Kickstarter-iOS/Views/Controllers/SearchViewController.swift

@@ -124,6 +125,13 @@ internal final class SearchViewController: UITableViewController {
      .observeValues { [weak self] in
        self?.changeSearchFieldFocus(focus: $0, animated: $1)
    }
+
+    self.viewModel.outputs.scrollToProjectRow
+      .observeForControllerAction() // NB: this also is required to prevent NSException.


i wasnt able to repro this crash so i started thinking maybe it got fixed somehow.

i just paired on this e547c7d with gina. we can use a feature flag to slowly ramp up how many people get the observeForUI version and if we don't see any crashes we can go 100%

mbrandonw · 2017-02-08T18:33:11Z

Library/ViewModels/ProjectNavigatorViewModel.swift

@@ -4,11 +4,11 @@ import ReactiveSwift
 import Result

 public protocol ProjectNavigatorViewModelInputs {
-  /// Call with the config data give to the view.
+  /// Call with the config data to give to the view.


oh this comment should prob be "Call with the config data given to the view."

mbrandonw · 2017-02-08T19:02:40Z

Library/ViewModels/ProjectNavigatorViewModel.swift

+    let swipedToProjectAtIndexFromIndex = self.willTransitionToProjectAtIndexProperty.signal.skipNil()
+      .takePairWhen(
+        self.pageTransitionCompletedFromIndexProperty.signal.skipNil()
+          .filter { completed, _ in completed }


looks like we do self.pageTransitionCompletedFromIndexProperty.signal.skipNil().filter { completed, _ in completed } twice so it could maybe be pulled out into a more descriptive signal

mbrandonw · 2017-02-08T19:34:14Z

Library/ViewModels/ProjectPamphletViewModel.swift

+          .flatMapError { _ in SignalProducer(error: SomeError()) }
+          .timeout(after: 5, raising: SomeError(), on: AppEnvironment.current.scheduler)
+          .materialize()
+          .map { (project, .some($0.value?.liveStreamEvents ?? [])) }


super subtle and glad tests helped make my intent clear! gina described it perfectly.

…os-oss into swipe-improvements

mbrandonw

just a few comments, lemme know what you think. we can merge after!

mbrandonw · 2017-02-09T17:18:20Z

Library/ViewModels/ProjectPamphletViewModel.swift

@@ -210,7 +213,7 @@ private func fetchProjectAndLiveStreams(projectOrParam: Either<Project, Param>)
          .take(first: 1)
    }

-    if let project = projectOrParam.left {
+    if let project = projectOrParam.left, shouldPrefix {


i really wish swift still had the where syntax. this reads kinda weird

i hear ya. would you prefer something else?

i'm kind of used to the comma at this point so i don't care.

sorry i was just thinking out loud. didnt have anything actionable

mbrandonw · 2017-02-09T17:19:49Z

Library/ViewModels/ProjectPamphletViewModel.swift

-      Signal.merge(self.viewDidLoadProperty.signal,
-                   self.viewDidAppearAnimated.signal.filter(isTrue).ignoreValues()
+      Signal.merge(self.viewDidLoadProperty.signal.mapConst(true),
+                   self.viewDidAppearAnimated.signal.filter(isTrue).mapConst(false)


i still think we should not emit configData multiple times cause the input is only called a single time. we could move the Signal.merge to a takePairWhen in the freshProjectAndLiveStreamsAndRefTag signal. also helps localize the shouldPrefix logic to near where it's needed

I hear what you're saying, but I don't really see these as being different. The only thing we'd be doing is getting rid of the let config and moving the whole thing to freshProjectAndLiveStreamsAndRefTag. In fact, I feel safer with the combineLatest then the takePairWhen to be honest. We very often put config data into a let, not sure what makes this one different?

well after a few more minutes of thinking, i guess it's not a problem to change it

yeah just ignore me

mbrandonw · 2017-02-09T17:22:35Z

Library/ViewModels/ProjectPamphletViewModel.swift

+                         cookieRefTag,
+                         self.viewDidAppearAnimated.signal.ignoreValues()
+      )
+      .map { (project: $0.0, liveStreamEvents: $0.1, refTag: $0.2, cookieRefTag: $1, _: $2) }


i don't think the named tuple params are necessary cause it doesnt seem like we use em. we could just do .map { ($0.0, $0.1, $0.2, $1, $2) }, which is just a fancy unpack

i named them b/c @justinswart thought it was more readable

np leave as is!

mbrandonw

theginbin added 17 commits January 27, 2017 14:42

fix some flickering. track next prev swiping

231af80

return to correct position in discovery

b331100

endless discovery swipe

ecd8c06

smoother trans

c60f165

still playin with numbers

acd5b5f

Merge branch 'master' into swipe-improvements

7310c8e

smoother transitioning

f53d32b

forget subpage transition. fix count bubble

46fac07

add koala swipetype enum

48a7b59

fix lint

5a5e755

clean up transition

b3246cb

sync up the other controllers that use the navigator

bbc174a

Merge branch 'master' into swipe-improvements

d64b320

fix tests

63e317d

more tests. clean up.

7019f74

fix lint

9ae13f2

Merge branch 'master' into swipe-improvements

8dfadf9

theginbin commented Feb 2, 2017

View reviewed changes

attempt to fix some tests after merge

46f43fc

theginbin commented Feb 2, 2017

View reviewed changes

theginbin added 4 commits February 3, 2017 17:28

project pamph refactoring. LiveStream erroring update. Tests fixed.

9ed3957

new screenies

8a67d9d

fix lint

702e34a

fix swiping test with delay

c367dcb

redo the overlapping screenshot

7c41db0

justinswart suggested changes Feb 4, 2017

View reviewed changes

theginbin added 2 commits February 6, 2017 12:03

pr feedback. add a test.

0d340e5

fix lint

832e6d2

justinswart approved these changes Feb 6, 2017

View reviewed changes

mbrandonw added 6 commits February 7, 2017 14:51

Merge remote-tracking branch 'oss/master' into swipe-improvements

40593c8

Merge remote-tracking branch 'oss/master' into swipe-improvements

5ed459d

Merge remote-tracking branch 'oss/master' into swipe-improvements

3e1d92a

Use a feature flag to determine how to observe.

e547c7d

fixed a screenshot test

bb61a2a

swiftlint

62ea992

mbrandonw suggested changes Feb 8, 2017

View reviewed changes

theginbin added 3 commits February 9, 2017 11:40

Merge branch 'master' into swipe-improvements

516568e

Merge branch 'swipe-improvements' of https://github.com/kickstarter/i…

ed53925

…os-oss into swipe-improvements

fix view appearing issue. track only on view appeared.

bedd823

mbrandonw approved these changes Feb 9, 2017

View reviewed changes

theginbin added 2 commits February 9, 2017 13:09

pr feedback

c2cca89

more pr feedback

b64f40f

mbrandonw approved these changes Feb 9, 2017

View reviewed changes

theginbin merged commit 7bb82ef into master Feb 9, 2017

theginbin deleted the swipe-improvements branch February 9, 2017 19:24


		// this works in practice, gotta figure out what i'm missing in the test.
		self.hasAddedProjects.assertValues([true, true], "More projects are loaded.")

		@@ -129,9 +95,11 @@ ProjectPamphletViewModelOutputs {

		fileprivate let projectOrParamProperty = MutableProperty<Either<Project, Param>?>(nil)
		fileprivate let refTagProperty = MutableProperty<RefTag?>(nil)

Swipe improvements #70

Swipe improvements #70

Conversation

theginbin commented Feb 2, 2017

What?

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

theginbin commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theginbin commented Feb 3, 2017

justinswart 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

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

justinswart commented Feb 6, 2017

mbrandonw 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrandonw 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrandonw left a comment

Choose a reason for hiding this comment