Add support for multiple accounts #329
Conversation
msfjarvis
force-pushed
the
multiple_accounts
branch
from
ceaa9f1 to
96e7a0a
Compare
|
Could you tell me what is your plan here? I don't see this being marked as a draft pull request but you said that not all review comments have been addressed. I wonder what is to be done now. Should I go on with new review or are you still planning more improvements? Non-passing CI suggests that there is still some work to do. |
|
I should have marked the PR as a draft, apologies for that. I'm still working on getting tests fixed and addressing the remaining few review comments and will undraft this once done. |
msfjarvis
force-pushed
the
multiple_accounts
branch
from
c4c1d45 to
2f8cccf
Compare
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
msfjarvis
force-pushed
the
multiple_accounts
branch
from
2f8cccf to
8624602
Compare
|
Rebased and fixed tests, should be good to go now. |
app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java
Outdated
Show resolved
Hide resolved
app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java
Outdated
Show resolved
Hide resolved
| List<AccountManager> accounts = Observable.fromIterable(accountManagerAdapter.get().getData()) | ||
| .ofType(AccountManager.class) | ||
| .toList() | ||
| .blockingGet(); |
There was a problem hiding this comment.
Why are we creating an observable and then converting it back? Isn't it possible to avoid switching between these?
There was a problem hiding this comment.
Seems like it's for the ofType filtering functionality, I'll try to filter directly.
There was a problem hiding this comment.
I've filtered it with the stream() extension but I'm not sure it works down till API 23.
app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerAdapter.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
|
Hm, it seems gradle-cache-action's latest version is having intermittent problems. I'll rollback to v1.5. Edit: tracking issue for the bug burrunan/gradle-cache-action#27 |
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
The lighter one caused a nearly-white on white effect when switching accounts. Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
There was a problem hiding this comment.
I'm going to merge this. I have found few things that could be improved but these are not blocking as the feature is working correctly.
My notes:
- It would be more intuitive if account rows were clickable. Clicking on them could switch accounts replacing swipe left to switch gesture.
- I think that it would be a good idea if the dialog automatically closed after user switched account.
- Some sort of indicator marking which account is currently selected
- Logout button is visible even when I'm logged out
- Also there doesn't seem to be anything indicating that we are logged out. But that probably would be solved if we had selected account indicator and hiding logout button, not sure.
|
Thanks for merging and your notes! I'll try to address them sometime over the next couple weeks. |
@mvietri has been unable to continue his work on #90 and I use the feature every day so figure I'd get the ball rolling. The existing code has been rebased and most of the pending review comments have been addressed in a separate commit to make reviewing simpler.