Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ALTAPPS-542: [Shared, iOS] topics completion #326

Merged
merged 29 commits into from
Feb 2, 2023

Conversation

vladkash
Copy link
Contributor

@vladkash vladkash commented Jan 20, 2023

YouTrack Issues:
#ALTAPPS-542

Checklist

Before Code Review:

  • Fields "Assignees, Labels, Milestone" are filled in the pull request;
  • Sentry Performance Monitoring of screen loading is added for new screens;
  • View analytics events are added for new screens;
  • New analytics events are documented;
  • All checks have been passed;
  • Changes have been checked locally.

Description

Start practicing button flow

StepCompletionFeature.Message.StartPracticingClicked (via StepFeature.Message.StepCompletionMessage) -> StepCompletionFeature.Action.FetchNextStepQuiz -> StepCompletionFeature.Message.NextStepQuizFetchedStatus.(Success|Error) -> StepCompletionFeature.Action.ViewAction.(ReloadStep|ShowPracticingErrorStatus) (via StepFeature.Action.ViewAction.StepCompletionViewAction)

Continue button flow

StepCompletionFeature.Message.ContinuePracticingClicked (via StepFeature.Message.StepCompletionMessage) -> (StepCompletionFeature.Action.FetchNextStepQuiz | StepCompletionFeature.Action.ViewAction.NavigateTo.Back | StepCompletionFeature.Action.ViewAction.NavigateTo.HomeScreen) -> StepCompletionFeature.Message.NextStepQuizFetchedStatus.(Success|Error) -> StepCompletionFeature.Action.ViewAction.(ReloadStep|ShowPracticingErrorStatus) (via StepFeature.Action.ViewAction.StepCompletionViewAction)

Checking topic completion

notificationInteractor.solvedStepsSharedFlow -> StepCompletionFeature.Message.StepSolved -> StepCompletionFeature.Action.CheckTopicCompletion -> StepCompletionFeature.Message.CurrentTopicStatus.(Completed | Uncompleted) -> StepCompletionFeature.Action.ViewAction.ShowTopicCompletedModal (via StepFeature.Action.ViewAction.StepCompletionViewAction)

Minor changes

  • Added BlockName.supportedBlocksNames extension to manage different supported step types on both platforms
  • Added new methods and renamed SwiftUIPushRouter -> SwiftUIStackRouter to implement necessary navigation on iOS

@vladkash vladkash self-assigned this Jan 20, 2023
…pletion

# Conflicts:
#	androidHyperskillApp/src/main/java/org/hyperskill/app/android/track/view/fragment/TrackFragment.kt
#	iosHyperskillApp/iosHyperskillApp/Sources/Modules/Track/Views/TrackView.swift
#	shared/src/commonMain/kotlin/org/hyperskill/app/home/injection/HomeComponentImpl.kt
#	shared/src/commonMain/kotlin/org/hyperskill/app/topics/domain/model/TopicProgress.kt
#	shared/src/commonMain/kotlin/org/hyperskill/app/track/presentation/TrackFeature.kt
@github-actions github-actions bot added android Android module task ios iOS module task shared Shared module task labels Jan 20, 2023
@vladkash vladkash added this to the 1.8 milestone Jan 20, 2023
@ivan-magda

This comment was marked as resolved.

@vladkash vladkash added the awaiting review Pull Request is awaiting code reviews label Jan 20, 2023
@vladkash vladkash marked this pull request as ready for review January 20, 2023 14:53
@vladkash

This comment was marked as resolved.

@XanderZhu

This comment was marked as resolved.

@XanderZhu

This comment was marked as resolved.

@ivan-magda ivan-magda removed the awaiting review Pull Request is awaiting code reviews label Jan 24, 2023
@ivan-magda ivan-magda force-pushed the feature/ALTAPPS_542/shared_iOS_topics_completion branch from 842d3b9 to 569d278 Compare January 26, 2023 10:18
@ivan-magda ivan-magda added awaiting changes Changes requested, waiting on author to update and removed awaiting review Pull Request is awaiting code reviews labels Jan 27, 2023
@vladkash

This comment was marked as resolved.

@vladkash vladkash added awaiting review Pull Request is awaiting code reviews and removed awaiting changes Changes requested, waiting on author to update labels Jan 31, 2023
vladkash and others added 7 commits February 1, 2023 11:52
…pletion

# Conflicts:
#	androidHyperskillApp/src/main/java/org/hyperskill/app/android/core/injection/AndroidAppComponentImpl.kt
#	iosHyperskillApp/iosHyperskillApp/Sources/Modules/Home/Views/HomeView.swift
#	iosHyperskillApp/iosHyperskillApp/Sources/Modules/Track/Views/TrackView.swift
#	shared/src/commonMain/kotlin/org/hyperskill/app/core/injection/AppGraph.kt
#	shared/src/commonMain/kotlin/org/hyperskill/app/home/injection/HomeComponentImpl.kt
#	shared/src/iosMain/kotlin/org/hyperskill/app/core/injection/AppGraphImpl.kt
…opics_completion' into feature/ALTAPPS_542/shared_iOS_topics_completion
@ivan-magda
Copy link
Member

@ivan-magda there is one problem left with @Published var isPracticingLoading in StepQuizViewModel we discussed

@vladkash the problem was that when the "Start Practicing" or "Continue" buttons were tapped it updated StepFeature.State which produces an update of the StepViewModel and StepQuizViewModel and its recreation (new instances are created), so StepViewModel.stepQuizModuleInput has incorrect reference. I updated the callback that provides stepQuizModuleInput, which resolves the issue. I don't know why SwiftUI recreates StepViewModel and StepQuizViewModel on the view model state update, we are using the @StateObject property wrapper to avoid this.

You can print the pointer address of the view model using this code:

let address = Unmanaged<AnyObject>.passUnretained(viewModel).toOpaque()
let _ = print("address of ViewModel = \(address)")

Also, when I debugged this issue I found that we have retained cycles, when SwiftUI View disappears its view model not deallocates. For example, suppose something like:

class MyObject {
    var function = { () }
}

struct MyStruct {
    let obj = MyObject()

    func structMethod() { ... }
    init() {
        obj.function = { structMethod() }
    }
}

The closure will capture self, which retains obj, which retains the closure, so this forms a retain cycle. We have the same situation with our handling of the view actions in the SwiftUI views, to fix this we need directly clear onViewAction callback to break the retain cycle.

I'll take one last look at the PR and we'll be ready to merge it.

@ivan-magda ivan-magda added ready to pull Pull Request is ready to merge and removed awaiting review Pull Request is awaiting code reviews labels Feb 2, 2023
@ivan-magda ivan-magda merged commit 2c0f3e1 into develop Feb 2, 2023
@ivan-magda ivan-magda deleted the feature/ALTAPPS_542/shared_iOS_topics_completion branch February 2, 2023 11:24
@ivan-magda ivan-magda removed the ready to pull Pull Request is ready to merge label Feb 10, 2023
@ivan-magda ivan-magda mentioned this pull request Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Android module task ios iOS module task shared Shared module task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants