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

Refactor ReadingListFragment to use ViewModel. #5214

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

terofeev
Copy link
Contributor

@terofeev terofeev commented Jan 9, 2025

What does this do?

  • Refactored ReadingListFragment to use ViewModel.
  • Added ReadingListFragmentViewModel and moved the update list logic into it.

Why is this needed?

Tech-debt.
Reduce complexity by simplifying the UI layer.

Phabricator:
https://phabricator.wikimedia.org/T303935

fun getEmptyTitle(context: Context) = context.getString(R.string.reading_lists_preview_header_title)

fun getEmptyDescription(context: Context) = DateUtil.getTimeAndDateString(context, Date())

suspend fun receiveReadingLists(context: Context, json: String, encoded: Boolean): ReadingList {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The following changes make this function unused, would it be possible to avoid/undo the change below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the unused function but kept getEmptyTitle and getEmptyDescription so that receiveReadingLists can be used in ViewModels without requiring the Android Context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, it makes sense to me. Instead of creating separate single-use functions, can we define those strings directly as
val in the updateReadingListData()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I’ve moved these functions to ReadingListFragment

@terofeev terofeev requested a review from voyagerfan as a code owner February 28, 2025 18:22
@terofeev terofeev requested a review from cooltey February 28, 2025 18:37
}) {
val list = AppDatabase.instance.readingListDao().getListById(readingListId, true)
if (list == null) {
_updateListByIdFlow.emit(Resource.Error(Throwable("No reading list found with id: $readingListId")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

For better localization, let's move this string into strings.xml. And to ensure accurate translations, could you provide a clear description for it in values-qq/strings.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, do we need this string? The activity finishes without showing a message, so it may not be necessary. Here's the error case:

is Resource.Error -> {
    // The list is no longer in the database (likely removed due to sync).
    // Nothing to do, so exit the activity.
    requireActivity().finish()
}

Comment on lines 27 to 29
fun getEmptyTitle(context: Context) = context.getString(R.string.reading_lists_preview_header_title)

fun getEmptyDescription(context: Context) = DateUtil.getTimeAndDateString(context, Date())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see the comment in the previous thread.

@terofeev terofeev requested a review from cooltey March 4, 2025 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants