Adds multiple accounts support #90

mvietri · 2019-10-19T13:06:03Z

References #83

Updated to master branch and I re-wrote some parts of the manager. Still not 100% sure about this -I'm still learning-

Same mechanics as app shortcuts.

Add multiple accounts following the login flow
Switch or delete account
Logout to userless mode
Re order accounts for custom order

Some images:

app/src/main/java/me/saket/dank/data/DankSqliteOpenHelper.kt

mvietri · 2019-10-19T13:09:12Z

Well I'm leaving these here and wait for some reviews. I've been using it for a couple of days now with no major issues but I'm sure it can be done better.

Tunous · 2019-10-30T18:01:47Z

Hello, sorry for the late reply. I forgot about this pull request and only now found some time to review it. Here are some of my suggestion about UX of the account switcher. Keep in mind that these are highly opinionated so if you disagree with anything let me know and we'll select the better option :)

The next most probable action performed by the user will be viewing subreddits after they add a new account. So based on that I would expect the account selector dialog to hide after adding new account. Similarly same should be done after logging out.
Swiping to switch account doesn't feel natural and is hard to discover (ignoring hint at the top of dialog). I would expect to be able to simply click on an account to switch to it.
Is there any usefulness in being able to reorder accounts? That seems more like an unnecessary feature and a static order (alphabetical?) would be better.
Button to add new account should always be last. At least to me this feels more natural.
If you are logged out the logout button is still visible. I would expect it to hide in that case.
When you click on add new account button but then close the login dialog the account switcher disappears. Shouldn't we keep switcher visible after canceling login?
What happens after you delete currently active account. After closing the dialog I can see that it switched me to another account but it's not clear to the user at first. We should either display an indicator showing which account is now active in the switcher or logout the user once active account is deleted.
This seems like a bug but after I added 2 accounts and then deleted both of them then the app kept me logged in to the latest one. We should logout the user instead. (Perhaps this causes point 5 to happen?)

It's a bit of a long list but I tried to find as much imperfections as possible. Overall I really like how it looks and behaves so I'm definitely into merging this after some improvements 👍

I'll follow with code-focused review in the coming days. For now I wanted to focus mostly on UI and UX.

Tunous

I've went over the code and left some comments. Generally there weren't many issues that I found but I can see that you had some problems with rx inside of AccountManagerActivity. This part needs some rethinking. Mostly the confirmAction which could be cleaned up a bit and split into smaller parts.

Tunous · 2019-10-30T17:41:03Z

app/src/main/java/me/saket/dank/data/InboxRepository.java

+  public Completable clearMessages() {
+    return Completable.fromAction(() -> {
+      try (BriteDatabase.Transaction transaction = briteDatabase.newTransaction()) {
+        briteDatabase.delete(CachedMessage.TABLE_NAME, "");


Based on documentation passing null as whereClause parameter will delete all rows. I would suggest to do that instead of passing empty string.

Suggested change

briteDatabase.delete(CachedMessage.TABLE_NAME, "");

briteDatabase.delete(CachedMessage.TABLE_NAME, null);

Ah OK. Anyways I wasn't 100% sure about deleting the cache table but switching to other account would end in merging PM from all your accounts. It's seems to be more a hack than a fix to me.

Tunous · 2019-11-02T11:44:54Z

app/src/main/java/me/saket/dank/di/RootModule.java

@@ -184,8 +185,8 @@ DraftStore provideReplyDraftStore(ReplyRepository replyRepository) {
  @Provides
  OnLoginRequireListener provideOnLoginRequireListener(Application appContext) {
    return () -> {
-      Intent loginIntent = LoginActivity.intent(appContext);
-      loginIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
+      Intent loginIntent = new Intent(appContext, AccountManagerActivity.class);


AccountManagerActivity has intent function which you could use.

Tunous · 2019-11-02T11:48:53Z

app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java

+  protected void onPostCreate(@Nullable Bundle savedState) {
+    super.onPostCreate(savedState);
+
+    setupUserList();


Why did you decide to place this inside of onPostCreate? Is there anything wrong with placing it inside of onCreate?

Documentation suggests that it isn't intended to be overridden:

Applications will generally not implement this method; it is intended for system classes to do final initialization after application code has run.

Hmm I have to check where I copy that from. Maybe it's wrong on both side.

Tunous · 2019-11-02T11:49:11Z

app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java

+  }
+
+  @Override
+  protected void onSaveInstanceState(Bundle outState) {


Unnecessary override.

Tunous · 2019-11-02T11:52:22Z

app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerAdapter.java

+      return holder;
+
+    } else {
+      throw new AssertionError();


Could add a message for easier debugging in case this ever happens.

Tunous · 2019-11-02T12:42:51Z

app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java

+            .subscribe(
+                () -> {
+                  //this.userSessionRepository.get().switchAccount(null, getApplicationContext());
+                  thread.start();


Mixing threads and rx, that asks for too much trouble :)

If I'm not mistaken you used it to delay the execution by 2 seconds. Did you try using .delay(2, TimeUnit.SECONDS) instead?

Yeah. Rx and me... we don't get along very well haha. When the PR is updated I'll let someone take care of this issue.

Tunous · 2019-11-02T12:44:19Z

app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java

+
+  private Completable confirmAction(int action) {
+    if (confirmTimer.isDisposed()) {
+      ACTION = action;


Is this necessary? I can see that you are using ACTION only inside of this method. You could use the value passed to parameter directly and remove the field.

Will check.

Tunous · 2019-11-02T12:44:33Z

app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java

+  }
+
+  private Completable queueToDelete(AccountManager account){
+    this.selectedAccount = account;


Unnecessary this. prefix.

Tunous · 2019-11-02T12:47:34Z

app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java

+      confirmTimer = Observable.timer(5, TimeUnit.SECONDS)
+          .compose(applySchedulers())
+          .subscribe(o -> {
+            runOnUiThread(() -> {


The call to applySchedulres above sets observation to happen on the main thread which makes this call redundant.

Tunous · 2019-11-02T12:51:22Z

app/src/main/java/me/saket/dank/ui/accountmanager/AccountManagerActivity.java

+    accountsRecyclerView.setLayoutManager(new LinearLayoutManager(this));
+    accountsRecyclerView.setAdapter(accountManagerAdapter.get());
+
+    //fullscreenProgressView.setVisibility(View.VISIBLE);


If there is any commented-out code it should be removed.

mvietri · 2019-11-11T11:35:48Z

Hey @Tunous Thanks for your feedback and review. I'll update the PR (I didn't have the time yet sadly).

Tunous · 2021-01-10T13:03:59Z

Superseded by #329 which continued on work from this pull request. @mvietri Thank you for the original work.

Adds multiple accounts support

955fcf9

mvietri commented Oct 19, 2019

View reviewed changes

app/src/main/java/me/saket/dank/data/DankSqliteOpenHelper.kt Show resolved Hide resolved

Tunous added the feature New feature label Oct 20, 2019

Tunous self-requested a review October 30, 2019 17:33

Tunous suggested changes Nov 2, 2019

View reviewed changes

Tunous added the waiting Pull requests that require changes or issues that await response label Jun 13, 2020

msfjarvis mentioned this pull request Sep 11, 2020

Add support for multiple accounts #329

Merged

Tunous closed this Jan 10, 2021

Tunous added duplicate This issue or pull request already exists and removed waiting Pull requests that require changes or issues that await response labels Jan 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds multiple accounts support #90

Adds multiple accounts support #90

mvietri commented Oct 19, 2019

mvietri commented Oct 19, 2019

Tunous commented Oct 30, 2019

Tunous left a comment

Tunous Oct 30, 2019

mvietri Nov 11, 2019

Tunous Nov 2, 2019

Tunous Nov 2, 2019

mvietri Nov 11, 2019

Tunous Nov 2, 2019

Tunous Nov 2, 2019

Tunous Nov 2, 2019

mvietri Nov 11, 2019

Tunous Nov 2, 2019

mvietri Nov 11, 2019

Tunous Nov 2, 2019

Tunous Nov 2, 2019

Tunous Nov 2, 2019

mvietri commented Nov 11, 2019

Tunous commented Jan 10, 2021

	briteDatabase.delete(CachedMessage.TABLE_NAME, "");
	briteDatabase.delete(CachedMessage.TABLE_NAME, null);

Adds multiple accounts support #90

Adds multiple accounts support #90

Conversation

mvietri commented Oct 19, 2019

mvietri commented Oct 19, 2019

Tunous commented Oct 30, 2019

Tunous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvietri commented Nov 11, 2019

Tunous commented Jan 10, 2021