badoo / MVIKotlin

Extendable MVI framework for Kotlin Multiplatform with powerful debugging tools (logging and time travel), inspired by Badoo MVICore library
https://arkivanov.github.io/MVIKotlin
Apache License 2.0
828 stars 64 forks source link

Implement sample todo details screen for iOS #18

Closed arkivanov closed 4 years ago

arkivanov commented 4 years ago

Details screen is already there for Android but is missing for iOS. Would be good to add one, same way as it is done for todo list screen: the business logic is already there and is shared, only UI part is missing (Swift UI).

Sample todo details screen for Android: https://github.com/arkivanov/MVIKotlin/tree/master/sample/todo-app-android/src/main/java/com/arkivanov/mvikotlin/sample/todo/android/details

Sample todo list screen for iOS: https://github.com/arkivanov/MVIKotlin/blob/master/sample/todo-app-ios/todo-app-ios/SceneDelegate.swift Should be moved to a separate class (file) with proper navigation between the screens.

sgallese commented 4 years ago

I'll take a look at this @arkivanov this week. If no update from me this week, fair game for anybody, but I'll try to report back my progress (or lack thereof).

sgallese commented 4 years ago

Explored how View and Bindings work for MVIKotlin. I need to update myself on the SwiftUI basics tomorrow, but should be able to get started.

Here was my experience reading the documentation and sample code: https://gitduck.com/watch/5e82ea9db7d7b8072195fc7b

sgallese commented 4 years ago

New to SwiftUI, but it looks like NavigationView and NavigationLink would be helpful.

I see ContentView uses NavigationView, so I think we can place a TodoDetail view within the ContentView.

I need to set up the TodoRow to distinguish between a click that triggers the navigation vs deleting the item. We will want TodoRow a similar flow to the Android app and handle onItemClicked.

Slow going, but learning. https://gitduck.com/watch/5e84226cb7d7b826c895fc90

final_5e843840247c5700151c7c35_572888

sgallese commented 4 years ago

ItemSelectionHandled Question @arkivanov is this the correct way to think about Event.ItemClicked vs Event.ItemSelectionHandled?

These are separate events because we want to:

  1. click on an item, then receive an updated view showing the item as clicked
  2. then when the view is updated, we want to fire ItemSelectionHandled, which navigates us to the details screen ?

I can see where the mapping between Event.ItemSelectionHandled -> Intent.UnselectItem makes sense but it was confusing at first because the user didn't actually physically unselect the item on a list.

NavigationLink Overall, I think I'm in a better place in tackling this task. After thinking this through, I don't think I want to use a NavigationLink. NavigationLink is great for a simple SwiftUI app, but for MVI, I think we want to use SceneDelegate to programmatically trigger navigation between the ContentViews.

I implemented the basic logic for ItemDoneClicked in the iOS app.

Thinking about iOS vs Android I'm mentally mapping the sample project for iOS -> Android as: TodoAdd/TodoAddViewImpl -> TodoAddViewImpl TodoRow -> TodoListAdapter.ViewHolder SceneDelegate -> TodoListFragment TodoList/TodoListViewImpl -> TodoListViewImpl

As you mentioned in the first comment, we may want two SceneDelegates. I think they seem similar to Android's Fragment.

Design oddities I noticed when wrapping the Delete Button within a TodoRow Button, the trash icon turned blue 🤔

Also, by not using NavigationLink, the TodoRow no longer has a chevron indicating the user can tap into the list. Not an iOS design expert, but later we may want to add the chevron manually to TodoRow.

Using Kotlin in Xcode Overall, I wasn't impressed when using trying to command click on code like this in Xcode: "TodoListViewEvent.ItemDoneClicked(id: item.id))" Xcode does not allow automatic definition lookup, I have to manually search for the code. I don't have the Xcode Kotlin plugin installed, if that makes a difference.

Next steps Next steps would be setting up the programmatic navigation to a TodoDetail in the SceneDelegate. After that, I would need to wire up the actual TodoDetail.

Here was the coding session: https://gitduck.com/watch/5e8620c5b7d7b83c3f95fcb9

Current design:

Screen Shot 2020-04-02 at 12 50 29 PM
arkivanov commented 4 years ago

ItemSelectionHandled Question

First of all this can be simplified now. We can just call onItemSelected callback directly from click listener:

override fun onItemClick(id: String) {
    onItemSelected(id)
}

Why is it like this? Previously onItemClick listener was receiving index instead of id. And in order to get the id we had to pass it through the TodoListStore.

There are two ways of doing navigation from a Store:

  1. We can define a property in the State and set it when needed. Then this information will reach the View which will execute the navigation. When navigation is executed we need to notify the Store so it will clear/reset the State property.
  2. We can define a Label and publish it from the Store when needed. This Label can be transformed into navigation calls.

The first option has more boilerplate code because you need to notify the Store when navigation is executed by the View. But in the second option you can loose the Label if the screen is in background and its View is currently destroyed.

NavigationLink

I'm not really familiar with iOS development. From what I understood the NavigationLink is a button that automatically navigates to a dstination. But we need to pass additional parameters to the Details screen: a selected item id. If this is possible with NavigationLink, then why not? If not possible then maybe SceneDelegate is waht we need. Anyway up to you :-) Blue buttons look fine to me.

Thinking about iOS vs Android

The mappings look correct.

Design oddities

Again maybe we can actually use the NavigationLink. Up to you. Perhaps chevron icon can be added via normal Image(systemName: "...").

Using Kotlin in Xcode

I don't think Xcode Kotlin plugin will make any difference here. I also noticed such a problem but not only for Kotlin classes. Can't help here.

sgallese commented 4 years ago

Sorry for all of the questions. Might be faster/easier to discuss on a video call.

Binding the list In the iOS sample app, looks like we're binding the ViewModel directly to the SwiftUI list https://github.com/arkivanov/MVIKotlin/blob/master/sample/todo-app-ios/todo-app-ios/TodoList.swift#L16

In Android, the ViewModel is passed to the renderer, and then the adapter is updated https://github.com/arkivanov/MVIKotlin/blob/master/sample/todo-app-android/src/main/java/com/arkivanov/mvikotlin/sample/todo/android/list/TodoListViewImpl.kt#L37

I assume it's OK to leave the current iOS binding behavior as-is, but let me know if if needs to be changed.

ItemSelectionHandled Based on https://github.com/arkivanov/MVIKotlin/issues/18#issuecomment-608096088, it sounds like you plan on making changes to the Android sample app then for "onItemSelected".

To reduce the number of moving parts involved, I'll simply try to mimic the Android implementation for iOS. If changes are made to the Android sample app, they can be applied to the iOS app later.

To be honest, I'm still confused as to why ItemSelectionHandled is needed.

In this case, I'm calling ItemSelectionHandled from TodoistViewImpl render: https://github.com/sgallese/MVIKotlin/blob/add-todo-details-screen-ios/sample/todo-app-ios/todo-app-ios/list/TodoListViewImpl.swift#L34

Time Travel I'm a bit confused about the expected functionality for Time Travel in the sample apps.

I played around with using Time Travel on the Android app. In this case, I recorded events while moving back and forth between the List and Details screen. Then when playing back events, it looked like I was able to transition between the screens sometimes.

However, based on https://github.com/arkivanov/MVIKotlin/issues/18#issuecomment-608096088, you mention something regarding driving the UI state from Store. It sounds like the UI state isn't driven by the Store currently.

My main question is if you're using Time Travel for iOS, do you expect it to automatically transition between the List and Details screen?

I raise this because the ContentView contains a NavigationView. I can wrap the ItemRow in a NavigationLink, but the NavigationView's BarItems disappear (see below video). I'm wondering if the TimeTravel window needs to be visible at all times when playing back history. If it does, I might need to think about how this would work. Maybe not possible with NavigationLink.

DiffKt and DiffBuilder Using DiffKt and DiffBuilder in Swift was a tough experience for me. For example, a KotlinBoolean is required for the compare return value. Would expect a normal Boolean. Doesn't look like default value were translated for Objective-C for the compare parameter in DiffBuilder. And also hard for me because I don't know Objective-C, reading the TodoLib headers is difficult for me.

Next steps I need to think about how I can wire up the TodoDetails. I know we want to pass in the id as a parameter. But need to play with some approaches to rendering the ViewModel. Ideally I'd like to mimic the Android example if possible... but when I took an initial stab at this, ran into trouble. Probably need to tackle it again with a fresh mind.

Video of my experience: https://gitduck.com/watch/5e88ca3d74b8d656ae9c8ff0

final_5e88fa09e4fa6200151cfbc0_766808

arkivanov commented 4 years ago

Hey, here are the answers. If you need a video call please let me know.

Binding the list This is done just for consistency with other diffing, but can be is not necessary at all. Anyway DiffUtil is used. It can actually be simplified like this:

diff(get = Model::items, compare = { a, b -> a === b }, bind = adapter::setItems)

So we can compare lists first by reference. Since they are immutable, if the references are same then there is no need to bother the adapter. But if they are different then we should pass it through DiffUtil. Current implementation compares lists by equals which is a bit overhead. Feel free to change it.

For iOS I assume it also does some diffing under the hood, but I'm no sure.

ItemSelectionHandled I will update the sample so it will directly call the callback without acrobatics with the Store.

FYI: There is such a thing as one time event. E.g. error or a navigation event. There are two ways of handling one time events: using Label or using State:

  1. Store can publish a special Label (e.g. Label.ItemSelected(id)) and you can handle this Label for navigation. This way has one downside, you can miss Labels if the screen is in background and its View is destroyed.
  2. You can add a field in State. In this case a View will receive a View Model and will handle this field. But there is also a downside, the View must notify the Store when the event is handled, so it will be cleared. This is why we need ItemSelectionHandled.

Bug again now we can call the navigation callback directly from click listener.

Time Travel Current sample implementation does not have any global state, so time travel won't work properly with navigation. It would work if current screen were part of a global state. But I really don't like having any global state, I like to keep concerns separate.

DiffKt and DiffBuilder I don't think we need diffing for SwiftUI. Same as it will not be needed for Compose. I suppose SwiftUI should handle it.

Next steps Sounds great! Let me know if you have any questions!

sgallese commented 4 years ago

Thank you for the response, very helpful. I think I'm in a bit over my head on the SwiftUI side, but it's been fun (and slow :) ) learning.

On the details screen, will will want these components: TextField Image (checkmark) NavigationbarButton (trash)

Right now, I'm trying to figure out how the TodoDetails controller will fit into the picture. Using NavigationLink, I can pass an item id in TodoDetails initializer NavigationLink( destination: TodoDetails(id: item.id))

But SceneDelegate currently only has the TodoListController set up. I need to set up TodoDetailsController. When navigating via NavigationLink, I don't see a callback to UIWindowSceneDelegate.

In this case, maybe we want one scene per controller? I'm not familiar if it's common for iOS apps to use a multi-scene approach. I assume most would use one scene with nested views. But I need to think about where to set up the DetailsController.

EnvironmentObject looks interesting: https://www.hackingwithswift.com/quick-start/swiftui/how-to-use-environmentobject-to-share-data-between-views The idea of a StoreProvider here is also interesting: https://github.com/Dimillian/SwiftUIFlux

Video of my experience: https://gitduck.com/watch/5e8bf9cf74b8d698789c9018

arkivanov commented 4 years ago

I would try separate SceneDelegates. There definitely should be tutorials for navigation or master-detail. 😀 Thanks for your contribution!

arkivanov commented 4 years ago

Btw the last video does not work

arkivanov commented 4 years ago

Please pay attention that #22 has merged. Please use Lifecycle for Details screen.

sgallese commented 4 years ago

Not sure if I've made progress, but it's been fun learning.

I don't see any examples using separate SceneDelegates. I do see "multi-scene" apps using UIKit UIViewControllers, but not SwiftUI.

From the examples I've seen, it looks like developers are setting up their controller's in the view itself.

For example, playing with this project: https://github.com/nalexn/clean-architecture-swiftui https://github.com/V8tr/ModernMVVM

You'll see for CountryDetails, they initialize the struct with country, but read keys from an environment object: https://github.com/nalexn/clean-architecture-swiftui/blob/master/CountriesSwiftUI/UI/Screens/CountryDetails.swift#L14

I wonder if SceneDelegate is the correct place to hook up the lifecycle callback vs. in View.onAppear/View.onDisappear. Note: I would need to test if onDisappear is actually called, some hint that it wasn't in SwiftUI beta.

Just brainstorming an idea, I still need to play with code to see how this would work: SceneDelegate would set up the Environment(keys or object) with common dependencies for LoggingStoreFactory and TodoDatabase. This would allow any of the views to access them. SceneDelegate would set ContentView as root. ContentView would contain the view TodoListContent TodoListContent would set up the TodoList controller and TodoList/TodoAdd views. TodoRow would contain a NavigationLink the TodoDetailsContent(id) view TodoDetailsContent(id) would set up the TodoDetails controller and TodoDetails view. (Note: since TodoDetails controller only uses one view, we may be able to skip TodoDetailsContent and simply use TodoDetails view to set up the controller).

Sorry, familiar with Android lifecycle, but the SwiftUI stuff is still new to me.

Unrelated note: Interesting that Android code is not using TodoListControllerDeps. I assume that at some point it will.

Developer experience: https://gitduck.com/watch/5e8ea75574b8d601869c904e

arkivanov commented 4 years ago

Thanks for the update! Let's try your idea as well. If I will find something from my side I will let you know.

sgallese commented 4 years ago

@arkivanov, using the MVIDroid framework in your production apps, was it very common for a controller to connect many views?

I ask this because another approach could be to have a separate controller for each view.

Pro: Allows us not to have these parent "Controller Views" that glue together child views. Cons: end up with a lot of boilerplate for each view, especially when views truly work together.

For example, the TodoListController has the TodoListView and TodoAddView: https://github.com/arkivanov/MVIKotlin/blob/master/sample/todo-common/src/commonMain/kotlin/com/arkivanov/mvikotlin/sample/todo/common/controller/TodoListController.kt#L12

arkivanov commented 4 years ago

@sgallese It depends. In this particular example I wanted to stress that there are no any limitations on this.

Also in this particular example the TodoListView and the TodoAddView are completely separate. What do you mean by "views truly work together"? I would rather split them for two different controllers each with its own Store and View.

sgallese commented 4 years ago

I guess work together in the sense of the user experience. Yes, you're right, they are completely separate from a code perspective.

Ok, I'll try to finish this up today using the existing sample code, don't want to make any large change to the common code. Was just something I was thinking about.

Thank you for your patience and help- this has been fun :)

arkivanov commented 4 years ago

Yeah, I just stopped thinking in screens. You can read more about this approach in this nice lightning talk: https://badootech.badoo.com/the-immense-benefits-of-not-thinking-in-screens-6c311e3344a0

There is no rush, take you time! Thanks for your contributions!

sgallese commented 4 years ago

We have a working implementation! Still some cleanup and refactoring todo.

I was able to wire up the Details lifecycle and controller within the Details SwiftUI View. If a controller only needs one view, this is doable.

In the case of the List and Add view, I left the wiring in the scene delegate. If this were a more complex application, we might move that logic into a ParentView to set up the controller and lifecycle for the child views.

I pass in the required dependencies (Database and Store) via an EnvironmentObject.

Feel free to make suggestions/questions or even edit the PR directly (open to any improvements/ideas): https://github.com/arkivanov/MVIKotlin/pull/33

Developer experience: https://gitduck.com/watch/5e94a0bc3c82312f288722eb

final_5e94c1afa0ea6e0015c1efaa_752792

arkivanov commented 4 years ago

Looks awesome! Thanks!

arkivanov commented 4 years ago

We have a working implementation! Still some cleanup and refactoring todo.

I was able to wire up the Details lifecycle and controller within the Details SwiftUI View. If a controller only needs one view, this is doable.

In the case of the List and Add view, I left the wiring in the scene delegate. If this were a more complex application, we might move that logic into a ParentView to set up the controller and lifecycle for the child views.

I pass in the required dependencies (Database and Store) via an EnvironmentObject.

Feel free to make suggestions/questions or even edit the PR directly (open to any improvements/ideas):

33

Developer experience: https://gitduck.com/watch/5e94a0bc3c82312f288722eb

final_5e94c1afa0ea6e0015c1efaa_752792

Hey, looks very cool! But is this a final UI? I would rearrange elements, e.g. input field could allocate the entire screen, the check box could be at the bottom, and the "delete" button could be in nav bar or maybe at the bottom as well.

sgallese commented 4 years ago

@arkivanov man, swiftui spacing is... interesting. 16fe998 moved delete to action item and the text field to top of screen and toggle to bottom.

I am having a hard time trying to center the toggle text/switch. Also, looks like multi line text support in SwiftUi isn't easy (adding line limit with 5 and nil values and multiline alignment didn't help).

I could keep fiddling with this but scared to introduce UI hacks. Maybe a SwiftUI expert knows of a clean way to style this properly.

Made some other minor changes to the PR.

If anything else I should tackle, let me know!

This is what I ended up with

Screen Shot 2020-04-14 at 9 23 36 PM

Developer experience: https://gitduck.com/watch/5e967ddf3c82312a3d8722fc

arkivanov commented 4 years ago

Closed via #33. Thanks @sgallese for your contribution!