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: use state flow for did in InstantEditor #17957

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

criticalAY
Copy link
Contributor

Purpose / Description

Resolves a TODO and uses state flow for deck id in Instant Editor

Approach

Use stateFlow()

How Has This Been Tested?

POCO API 33

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

launchCatchingTask {
viewModel.deckId?.let { selectDeckById(it, true) }

viewModel.viewModelScope.launch {
Copy link
Member

Choose a reason for hiding this comment

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

This and the other coroutine in this file is incorrect both in how the code behaves and the purpose.

How the code behaves: This creates a coroutine on the ViewModel's own scope with a lambda declared here which will capture the activity and potentially leak it as the viewmodel can outlive the activity(the context shouldn't referenced in a viewmodel).

Purpose of the code:

TL;DR: These changes are not needed and the TODO should just be removed

Inside the viewmodel the deckId is only used when saving the note(no need for a flow).
In the activity we don't need to do "something" in response to the deck id changing(the flow emitting), we only need the id in response to a user action: showing the dialog/the deck was changed. When those two situation happen we can just read the id from the viewmodel property(old code).

Also, I see that there's another todo in line 112: // TODO: Use did here . If it releates to the deck Id, it should probably be addressed here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I am ok removing TODO

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm fine with it as well. Maybe also verify here the other TODO that I mentioned above if it's related.

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants