-
Notifications
You must be signed in to change notification settings - Fork 46
Added LoginStateRepository to manage login state centrally #1677
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
Conversation
|
|
||
|
|
||
| init { | ||
| var initialState = sharedPrefs.getString("state", null)?.let { serializedState -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could extract those strings since they are re-used to avoid making mistakes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| private val currentPoller: StateFlow<Poller?> = channelFlow { | ||
| prefers.watchLocalNumber() | ||
| loginStateRepository | ||
| .loggedInState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be able to use your new flowWithLoggedInState here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately no, the use case here is too specific. It needs to be able to explicitly cancel the poller, while with flowWithLoggedInState it only emits "nothing" which can't be use to drive an action.
| // we want to preload the data as soon as the user is logged in | ||
| scope.launch { | ||
| textSecurePreferences.watchLocalNumber() | ||
| loginStateRepository.loggedInState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use flowWithLoggedInState?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit awkward to use that in here, as flowWithLoggedInState is used to limit the creation of a flow, but in here it's actually a coroutine so i don't think it's worth doing it here.
ThomasSession
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
This PR adds a
LoginStateRepositoryto manage user credentials: all access to keys will go through here. It also handles migration and persistent.We now have
LoggedInStateto encapsulate all the keys the user will use to do all sort of stuff, including pro related operations.All calls to
prefs.getLocalNumber(),prefs.watchLocalNumber()and so on are replaced to call to the repository instead.Also added a
flowWithLoggedInState()to help with the common case where we need to be logged in before a flow can be created.