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

Don't persist following #1136

Merged
merged 24 commits into from
Nov 9, 2023
Merged

Don't persist following #1136

merged 24 commits into from
Nov 9, 2023

Conversation

zeitschlag
Copy link
Contributor

@zeitschlag zeitschlag commented Oct 19, 2023

tl;dr: Don't use MastodonUser on "Following"-screen, but Mastodon.Entity.Account

This PR does several things:

  • Add MBProgressHUD (cause I'm lazy) to show loading-states
  • It fetches the accounts that an account follows and the relationships
  • It improves the Loading-state of the FollowButton in UserView
  • Speaking of which: The FollowButton uses configuration now (which makes showing an ActivityIndicator very easy). Oh, the FollowButton is gone and got replaced by a basic UIButton.
  • It migrates Kanna from Pods to SPM (We could think about removing Cocoapods, we now only use it for tooling aka SwiftGen and Sourcery and those are available via homebrew etc., too)

also: Refactor Button-Background-Stuff
automatic conformance FTW!
This is preparation, but as you know: Proper Preperation and Planning Prevent Piss Poor Performance
This is a first step, for now we show the name to see if it works (and it does!), the other properties and functionality will follow.

Again, this includes some refactoring, like getting rid of Configuration
cause I'm lazy
Bridge account to user as long as Profile-screen doesn't work with Mastodon.Entity.Account, but MastodonUser
For whatever reason, fetchUser and accountInfo returned different results for me (something something ID), that's why I replaced accountInfo which came from #1053 with fetchUser, so the displayed profile is consistent
@zeitschlag zeitschlag requested a review from kimar October 25, 2023 13:22
@zeitschlag zeitschlag changed the title WIP: Don't persist follower-list WIP: Don't persist following Oct 25, 2023
Copy link
Contributor

@kimar kimar left a comment

Choose a reason for hiding this comment

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

When trying to view the followers of any account in my timeline, I'm only seeing an infinite spinner but no results. Looking at my own followers (Account Tab) works.

This might be related to the fact that, we're not showing following people which are on another server than the currently logged in instance. (In the current prod version I'm seeing the message in the screenshot instead).

Also I'm seeing lots of JSON decoding errors when going back to my Timeline (Mastodon Server v4.2.1):

Simulator Screenshot - iPhone 12 mini - 2023-10-30 at 14 53 17

Swift.DecodingError.keyNotFound(CodingKeys(stringValue: "id", intValue: nil), Swift.DecodingError.Context(codingPath: [], debugDescription: "No value associated with key CodingKeys(stringValue: \"id\", intValue: nil) (\"id\").", underlyingError: nil))

and

Swift.DecodingError.typeMismatch(Swift.Array<Any>, Swift.DecodingError.Context(codingPath: [], debugDescription: "Expected to decode Array<Any> but found a dictionary instead.", underlyingError: nil))

@kimar
Copy link
Contributor

kimar commented Oct 30, 2023

I've used MBProgressHUD in the past as well but I'm not a huge fan of pulling in a dependency which hasn't been updated in years, but also I totally feel the pain of not wanting to write an indicator from scratch.

@zeitschlag what do you think of creating a light-weight wrapper around UIAlertController to server as a modal loading indicator?

@zeitschlag
Copy link
Contributor Author

@kimar Thank you for your review :) Regarding MBProgressHUD: I'm not a huge fan of pulling in dependencies (regardless of their age) either, but in this case, I'd consider it a pragmatic way:

  • It does basic stuff, nothing critical. So in case, it might stop working (to me, MBProgressHUD has proven to be a reliable library for years), we know how to easily replace it (create a light-weith wrapper that does the same thing and handle presentation).
  • It hasn't been updated in three years, yes, but I'd consider it more due to a "It does its thing good enough" than "Noone wants to take care of it"
  • I wanted to focus on the task at hand (migrating the following-list to entities and building endless loops :D) than adding a new, fancy feature.

We can still replace it with something better/nicer/... at any time, I'd have nothing against it and I'd encourage you (or everybody else reading this) to suggest/build something better/nicer/... :)

…implementation

No more recursion due to no timer and better state handling
@zeitschlag
Copy link
Contributor Author

zeitschlag commented Nov 1, 2023

Regarding the DecodingErrors: It's probably connected to 02207d1. I'll have a look at them. Also I'll also check the slighty higher memory usage. I didn't notice that before, could be connected to the images?

The reason for the DecodingErrors were issues with domains. On my machine, it works now™️

@zeitschlag zeitschlag requested a review from kimar November 4, 2023 14:25
@zeitschlag zeitschlag marked this pull request as ready for review November 5, 2023 20:27
@zeitschlag zeitschlag changed the title WIP: Don't persist following Don't persist following Nov 5, 2023
Copy link
Contributor

@kimar kimar left a comment

Choose a reason for hiding this comment

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

Tested and LGTM 🚀 Looks solid to me now.

@zeitschlag zeitschlag merged commit 7004a22 into develop Nov 9, 2023
1 check passed
@zeitschlag zeitschlag deleted the remove_coredata/following branch November 9, 2023 10:01
@zeitschlag zeitschlag added this to the 2023.14 milestone Nov 9, 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