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

[iOS] Create a cache system for images #573

Merged

Conversation

mltokky
Copy link
Contributor

@mltokky mltokky commented Aug 17, 2024

Issue

Overview (Required)

  • create view for user icon
    • The view use in in-memory cache if speaker icon data exists in it, else fetched from server.
    • URLSession may returns data from cache after second time, so speaker icon is displayed faster than before.

Movie (Optional)

launch at first time

Before After
430_before.mp4
430_after.mp4

launch at second time

Before After
---
430_2_after.mp4

Copy link
Contributor

@shin-usu shin-usu left a comment

Choose a reason for hiding this comment

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

@mltokky
Thank you for your commit! Great!!
SpeakerIcon component could be applied to other screens.
If you have time, could you please apply it to the following screens as well?

  • TimetableDetailView
  • ContributorListItemView
  • StaffView

Of course, you can refuse.
This commitment is enough to resolve the issue.

@mltokky
Copy link
Contributor Author

mltokky commented Aug 18, 2024

@shin-usu
Sorry for the late reply.

Yes, I'll apply it!
May I continue on this PR?

@shin-usu
Copy link
Contributor

@shin-usu
Yes! Thank you very much🙇‍♂️

@mltokky
Copy link
Contributor Author

mltokky commented Aug 20, 2024

@shin-usu
Applying to other screens is done!
Please review changed code.

Notes

  • I renamed component to CircularUserIcon from SpeakerIcon because it is no longer just about displaying speaker icon.
  • I found app crash in Staff screen, Contributor -> KMP Presenter, and KMP Compose view. So I could not confirm these screens.

@shin-usu
Copy link
Contributor

Thank you very much😊
I'll check the pull request later.

About crashes, this bug is known issue.
#527

It is sufficient to check only SwiftUI view on ContributorView👍

import Foundation
import SwiftUI

private actor SpeakerIconInMemoryCache {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be appropriate to rename this actor as well as circular icon view.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry...
I forgot rename this actor.

I fixed it.
Please check again 🙏🏻

…ache`

Because related view is renamed, so I changed it accordingly.
Copy link
Contributor

@shin-usu shin-usu left a comment

Choose a reason for hiding this comment

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

LGTM🎉 Thank you for your commitment to my additional request!

@shin-usu shin-usu merged commit 867c924 into DroidKaigi:main Aug 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS] Create a cache system for images
2 participants