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

introduce AppFlow to support different application flows #587

Merged
merged 2 commits into from
Sep 10, 2023
Merged

Conversation

mormaer
Copy link
Collaborator

@mormaer mormaer commented Sep 9, 2023

Checklist

  • I have read CONTRIBUTING.md
  • I have described what this PR contains
  • This PR addresses one or more open issues that were assigned to me:
    - Related to Discovery / Guest Mode #21
    - Resolves a separate issue where multiple instance of AppState could be in memory 😱
  • If this PR alters the UI, I have attached pictures/videos

Pull Request Information

About this Pull Request

At present the application start-up logic is based around the concept of us either having a default account or not, and so decisions about what to render were based on the presence of a non-optional account. The changes in this PR swap out this logic to instead work on an AppFlow enumeration which currently only has .account(SavedAccount) or .onboarding as its cases.

These two cases continue to represent the previous state of having an account, or not having an account. So our behaviour remains much as it was.

However, there are a few things that these changes now allow:

  1. We can add additional application flow states, eg .guest(URL), which will (soon) link up with the work done in introduce alternative session states #567 so that we can create .unauthenticated sessions when the app enters guest mode.
  2. The AppState is now back to having no arguments passed to its .init and is now stored as an @StateObject. This resolves a separate issue I found on another branch where we were ending up with more than one of our app states in memory... which is less than ideal 😭
  3. Allows the app to work without the presence of an account, as the .currentActiveAccount is now an optional value there are various changes in this PR to handle that optionality. Note - there will need to be more work done here, I've done the bare minimum required in this PR as although the account now supports optionality we have no way to enter the application without an account... that is still to come with the work for Discovery / Guest Mode #21.

Screenshots and Videos

There are no major changes to the UI in this PR, however I noticed that our current App Store build shows an icon next to the add account button which had disappeared on dev, so I've added that back.

One noticeable thing that the new flow does introduce is a bit of jank when you add a new account while already inside the app, the dismiss animation can misbehave at times. There are some existing DispatchQueue tricks being done to reduce this elsewhere so I will see if I can make it pretty as part of the upcoming onboarding/guest mode work.

Additional Context

One other thing I noticed while working on this PR is that the way we determine if a post/comment etc belongs to the active account needs some work. We make a comparison on the .id of the account, however there is a (small?) risk that we incorrectly identify something as belonging to us when it doesn't since .id will only be unique on our home instance - not across the entire Fediverse.

We could/should potentially be using the .actorId or such, but I've not tried to fix this as part of this PR to avoid it growing any larger than it needs to be. I've left a TODO in the code for it.

@mormaer mormaer requested a review from a team as a code owner September 9, 2023 16:41
@mormaer mormaer requested review from WestonHanners and EricBAndrews and removed request for a team September 9, 2023 16:41
@mormaer mormaer force-pushed the app_flow branch 2 times, most recently from caaaa8d to 67b56da Compare September 10, 2023 08:48
Copy link
Contributor

@Sjmarf Sjmarf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Mlem/AppDelegate.swift Outdated Show resolved Hide resolved
Mlem/AppFlow.swift Outdated Show resolved Hide resolved
Mlem/Models/Trackers/Favorite Community Tracker.swift Outdated Show resolved Hide resolved
Mlem/Views/Shared/Accounts/Accounts Page.swift Outdated Show resolved Hide resolved
@mormaer mormaer merged commit 08f6060 into dev Sep 10, 2023
4 checks passed
@mormaer mormaer deleted the app_flow branch September 10, 2023 17:49
boscojwho pushed a commit to boscojwho/mlem that referenced this pull request Sep 16, 2023
boscojwho pushed a commit that referenced this pull request Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants