Flow coordinators #927

stefanceriu · 2023-05-19T12:49:45Z

This PR is the first step towards introducing more flow coordinators, solidifying our navigation and fixing existing bugs, while making a whole class of potential problems easy to handle.

Fixes #918
Relates to #919

We've added a new RoomFlowCoordinator that is responsible for everything happening within a Room's context:

takes over some of the work that used to be done in the UserSessionFlowCoordinator, simplifying both
works in tandem with the above to enable more complex flows like invitations
has a state machine that correctly asserts the state between all the involved screens at all times (and clearly prints out what flows don't work)
also simplifies the old RoomScreenCoordinator removing things like the media uploading and room details from it
converges on a single place from where room details are presented, both from within the room header and from the room list contextual menu
can handle app routes and can easily be used to implement deep linking into member details for example
fixes bugs like
- the userSessionStateMachine current state often being wrong
- not being able to select a room after opening its details from the room list contextual menu
- not being able to a room's details from the room list contextual menu if that room is already opened
- not rewidinging the navigation stack when re-selecting the room
- and most likely others we haven't found yet

github-actions · 2023-05-19T12:50:58Z

	Warnings
⚠️	This pull request seems relatively large. Please consider splitting it into multiple smaller ones.
⚠️	Please add a changelog.
⚠️	Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️	ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift#L205 - SwiftLint rule 'function_body_length' did not trigger a violation in the disabled region. Please remove the disable command. (`superfluous_disable_command`)
⚠️	ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift#L299 - SwiftLint rule 'function_body_length' did not trigger a violation in the disabled region. Please remove the disable command. (`superfluous_disable_command`)

Generated by 🚫 Danger Swift against 4a123ca

codecov · 2023-05-19T13:03:37Z

Codecov Report

Patch coverage has no change and project coverage change: -0.32 ⚠️

Comparison is base (5249974) 48.24% compared to head (f32431b) 47.92%.

❗ Current head f32431b differs from pull request most recent head 4a123ca. Consider uploading reports for the commit 4a123ca to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #927      +/-   ##
===========================================
- Coverage    48.24%   47.92%   -0.32%     
===========================================
  Files          335      333       -2     
  Lines        20679    20556     -123     
  Branches     11326    11354      +28     
===========================================
- Hits          9976     9852     -124     
- Misses       10422    10425       +3     
+ Partials       281      279       -2

Flag	Coverage Δ
unittests	`22.64% <0.00%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...Sources/FlowCoordinators/RoomFlowCoordinator.swift	`0.00% <0.00%> (ø)`
...Coordinators/RoomFlowCoordinatorStateMachine.swift	`0.00% <0.00%> (ø)`
.../FlowCoordinators/UserSessionFlowCoordinator.swift	`0.00% <0.00%> (ø)`
...ators/UserSessionFlowCoordinatorStateMachine.swift	`0.00% <0.00%> (ø)`
...ces/Screens/RoomScreen/RoomScreenCoordinator.swift	`18.57% <0.00%> (+10.57%)`	⬆️
.../Sources/Screens/RoomScreen/RoomScreenModels.swift	`84.21% <ø> (ø)`
...urces/Screens/RoomScreen/RoomScreenViewModel.swift	`40.81% <0.00%> (-0.71%)`	⬇️

... and 65 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

alfogrillo

Nice refactor! 🎉
Left some comments/ideas inline.

Also noticed the navigation to room's settings using the contextual menu changed.

Before: the room's settings were pushed on the Home Screen
Now: the is the room's timeline pushed in between

To me it looks still good, but let's check everyone is happy.

ElementX/Sources/Application/FlowCoordinatorProtocol.swift

stefanceriu · 2023-05-23T08:26:17Z

let's check everyone is happy

Definitely, thanks for the review!

pixlwave

Overall seems like a decent change to me. We discussed offline about a possibility of only having handleAppRoute as the public method to encourage the use of AppRoutes as a single system for navigation throughout the app.

ElementX/Sources/FlowCoordinators/RoomFlowCoordinatorStateMachine.swift

ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift

ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift

ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift

ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift

ElementX/Sources/FlowCoordinators/RoomFlowCoordinatorStateMachine.swift

…d FlowCoordinator protocol Squashed commits: Add unit tests and move the state machine into the FlowCoordinator [bb68686] Replace the RoomFlowCoordinator's public interface with just `handleAppRoute` [0d9a4f8] Remove the navigationStackCoordinator dependency from the roomScreenCoordinator [4b5fbdf] Allow rooms to be selected from any other state [41dbd12] Move all missing coordinators to the RoomFlowCoordinator and state machines [f32431b] The UserSessionFlowCoordinator does not need to conform to the CoordinatorProtocol [0f07e87] Fix leaving a room dismissing the currently selected one when different [138385a] Rewind the navigation stack when re-selecting the same room (iPad) [0727eb9] Fix presenting different room details from the side menu on iPads [faf4cc6] Fix selecting the same room multiple times [fb3391d] Move room details presentation responsibility to the RoomFlowCoordinator. Fixed invitation flows. [fa2a68d] Rename RoomTimelineFlowCoordinator -> RoomFlowCoordinator [0c9c06b] Start moving things away from the RoomScreenCoordinator and into the RoomTimelineFlowCoordinator [86cbbdc] Introduce a RoomTimelineFlowCoordinator to deal with timeline related operations [9b2381b] Introduce the FlowCoordinatorProtocol

…meline provider and other screens

sonarcloud · 2023-05-26T14:35:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

stefanceriu requested a review from a team as a code owner May 19, 2023 12:49

stefanceriu requested review from Velin92 and pixlwave and removed request for a team May 19, 2023 12:49

alfogrillo reviewed May 23, 2023

View reviewed changes

ElementX/Sources/Application/FlowCoordinatorProtocol.swift Show resolved Hide resolved

ElementX/Sources/Application/FlowCoordinatorProtocol.swift Show resolved Hide resolved

pixlwave approved these changes May 23, 2023

View reviewed changes

pixlwave reviewed May 23, 2023

View reviewed changes

ElementX/Sources/FlowCoordinators/RoomFlowCoordinatorStateMachine.swift Outdated Show resolved Hide resolved

stefanceriu force-pushed the stefan/flowCoordinators branch from 31e9bcb to ef29037 Compare May 26, 2023 12:00

stefanceriu force-pushed the stefan/flowCoordinators branch from ef29037 to 9be50cb Compare May 26, 2023 12:10

Prevent race conditions when adding timeline listeners between the ti…

4a123ca

…meline provider and other screens

stefanceriu merged commit 142a8d6 into develop May 26, 2023

stefanceriu deleted the stefan/flowCoordinators branch May 26, 2023 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flow coordinators #927

Flow coordinators #927

stefanceriu commented May 19, 2023 •

edited

Loading

github-actions bot commented May 19, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading

alfogrillo left a comment

stefanceriu commented May 23, 2023

pixlwave left a comment •

edited

Loading

sonarcloud bot commented May 26, 2023

Flow coordinators #927

Flow coordinators #927

Conversation

stefanceriu commented May 19, 2023 • edited Loading

github-actions bot commented May 19, 2023 • edited Loading

codecov bot commented May 19, 2023 • edited Loading

Codecov Report

alfogrillo left a comment

Choose a reason for hiding this comment

stefanceriu commented May 23, 2023

pixlwave left a comment • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented May 26, 2023

stefanceriu commented May 19, 2023 •

edited

Loading

github-actions bot commented May 19, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading

pixlwave left a comment •

edited

Loading