Toast message is displayed multiple times in during login and signup

[raj10071997]

Actual Behaviour
When you try to login using wrong email or password, the error is showed only once if you click the button only once. Now if you click the login button again the error message is shown more than once. Similar issue can be seen in for sign up too, the error in sign up toast is shown multiple times.

Expected Behaviour

The message should be displayed only once for each button click.

Would you like to work on the issue?

Yes.

[dr0pdb]

@raj10071997 Oh!, I understood the cause here. Have you understood the root cause of it ? If yes, then how do you propose to solve it ?

PS: It is always recommended to attach screenshot/GIF whenever possible.

[raj10071997]

@srv-twry Sorry for not attaching a screenshot. Actually if you see the toast message is called only once for the first time when we click the login button but not afterwards. It is because MutableLivedata object(loginResponse in LoginActivityViewModel.java) is still getting changed even after INVALID response and every time we press the login button, the object loginResponse is changed due to which the observer of loginActivityViewModel(in loginActivity.java) counts it as a change and toasts the message multiple times. So I just replace the line

        loginResponse.setValue(new LoginResponse(INVALID, ""));

with:
try{
if(loginResponse.getValue().response!=INVALID)
loginResponse.setValue(new LoginResponse(INVALID, ""));
}catch (NullPointerException e){
loginResponse.setValue(new LoginResponse(INVALID, ""));
}

preventing the loginResponse from calling the method setValue if the loginResponse is not null. And now the observer observe no change in the object and executes the toast message only once. This is solution that I can think of now. Can you please tell if there is anything wrong with this solution?

[dr0pdb]

I think you're kind of in the right direction for the problem but not quite. IMO the problem is not because it is being changed multiple times(It isn't IMO). It is because of the asynchronous nature of this operation. Here is the case which causes the issue:

1. You enter bad credentials for login. The MutableLiveData is set to "INVALID" and toast is displayed and everything is good till now.

2. Now you enter the credentials again. This time the correct one.

compositeDisposable.add(APIClient.getOpenEventAPI().login(new Login(email, password))
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(response -> {
                            Timber.d("Saved token and logged in successfully");
                            loginResponse.setValue(new LoginResponse(VALID, response.getAccessToken()));
                        }, throwable -> {
                            Timber.e(throwable.toString());
                            loginResponse.setValue(new LoginResponse(INVALID, ""));
                        }));
        return loginResponse;

The above process takes time and works on the io thread and hence the main thread continues to work and returns the MutableLiveData loginResponse(Even before the login process is done) with the previously saved value i.e. INVALID due to which the Activity shows the toast without realising that it is the value from the previous attempt and this current attempt is still in progress. After the login process is done, it sets the value to VALID and you're logged in.

Understood ?
I have a solution for it but i think you should try and find a solution for it first 😄 . In case you can't then i will tell you 👍

[raj10071997]

@srv-twry Actually if you enter the wrong credential again then you will see that the toast message is displayed multiples times. I understand that I need to find a way to synchronize the io thread and the main thread so that the loginResponse object contains the updated value. But if you enter the wrong credential again the toast message is displayed sometimes for three times or four times. Also if you enter the wrong credential twice or thrice and now enter the right credential, the toast message "successfully logged in" message is displayed for thrice or quadraple times despite the function returns only once with the previous value and as soon as the work of the io thread is over, it updates the loginResponse object which is observed by the observer of loginActivityViewModel. So if we consider only the asynchronous nature of the operation, the toast message should be displayed twice (when you enter the wrong credential twice), once with the previous value and then when the io thread updates the loginResponse object. But this doesn't happen, the message is displayed more than twice. What can be the reason for this? 😄

[dr0pdb]

I understand that I need to find a way to synchronize the io thread and the main thread

No, how and why would you even do that ?

if you enter the wrong credential again the toast message is displayed sometimes for three times or four times

Can you attach a gif ? i am only getting it once.

[raj10071997]

Have you tried printing the log messages in INVALID case, you will find that the toast message line is being called 3 to 4 times. Below is the Gif of the issue when you enter the wrong credentials twice :

1234 is wrong password. When the button is pressed again the toast message is displayed thrice instead of twice.

Actually I said it wrong, I meant that if we want to do the work in background and then update the loginResponse and after that the function should return, so there is no need of notifying it in the main thread. Actually if the function returns only when the loginResponse is updated, then this issue might not even occur. So I meant that it will be better that the function return only when loginResponse is updated, and that's what I meant by synchronizing the background thread.

[dr0pdb]

Yes, it is very very weird. Infact it is increasing by one everytime the login button is clicked.
Here is the log

02-08 00:51:17.417 7549-7549/org.fossasia.openevent V/SAURAV: Login button pressed
02-08 00:51:19.167 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:22.187 7549-7549/org.fossasia.openevent V/SAURAV: Login button pressed
02-08 00:51:22.507 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:22.507 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:50.687 7549-7549/org.fossasia.openevent V/SAURAV: Login button pressed
02-08 00:51:51.017 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:51.027 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:51.027 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:54.117 7549-7549/org.fossasia.openevent V/SAURAV: Login button pressed
02-08 00:51:54.417 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:54.417 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:54.427 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:54.437 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:51:59.947 7549-7549/org.fossasia.openevent V/SAURAV: Login button pressed
02-08 00:52:00.237 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:52:00.237 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:52:00.247 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:52:00.257 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown
02-08 00:52:00.267 7549-7549/org.fossasia.openevent V/SAURAV: Toast shown

Everytime the Login button is pressed the number of toast shown increases by one. 🤔 .

[raj10071997]

@srv-twry Yes, it is really weird. And if you make the changes as I have mentioned in the above comments, the toast message is displayed only once.

[dr0pdb]

uh okay! In your proposed solution, why are you catching the NPE ?

[raj10071997]

@srv-twry The best solution till now I could find is to initialize the MutableLiveData loginResponse every time a call to loginUser(String email, String password) method is made in LoginActivityViewModel.java. And also in my view it is good solution because since we are always keeping a memory reference after initializing loginResponse, there is no harm in initializing it again as the previous memory reference will be garbage collected. Though this solution has a little disadvantage of time lagging. Plus I have also tried using a third flag IN_PROGRESS with VALID and INVALID. Since the function returns first before the execution of the background thread, I thought having a third condition which does nothing will be good, but I have found once the loginReponse object is returned by the function, the ViewModel observes the data at every stage and keeps on executing the switch case, same case as you have mentioned with the logcat. But having returned a new loginResponse object which contains no data, so there can be no change in data and the switch case will be executed only once.

[raj10071997]

@iamareebjamal this is the best i could find for this issue. Can you suggest any other solution if you have or should I make a PR with this solution?

[raj10071997]

@iamareebjamal Please reply it's been few days.

[iamareebjamal]

The problem is about a new task/observable is created each time the button is clicked. Simply dispose the previous observable when button is clicked.

[raj10071997]

@iamareebjamal I am removing the obsevers when the button is clicked but the behavior is still the same. I made these changes -

private void loginUser(String password) {
 loginActivityViewModel.loginUser(email, password).removeObservers(this);
 loginActivityViewModel.loginUser(email, password).observe(this, loginResponse -> {

loginUser() method is called every time the user press the login button. So I am removing every observer first and then again creating a new observer, but still the behavior is same. Am I doing anything wrong here?

[iamareebjamal]

Problem is in Observable and not the LiveData

[raj10071997]

@iamareebjamal I have made the following changes to dispose the observables every time the button is clicked.

        if (compositeDisposable.size()!=0)
             compositeDisposable.dispose();

        compositeDisposable.add(APIClient.getOpenEventAPI().login(new Login(email, password))
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(response -> {
                            Timber.d("Saved token and logged in successfully");
                            loginResponse.setValue(new LoginResponse(VALID, response.getAccessToken()));
                        }, throwable -> {
                            Timber.e(throwable.toString());
                            loginResponse.setValue(new LoginResponse(INVALID, ""));
                        }));

But the problem with this is that although it solves the problem of the toast message getting displayed multiple times. But it doesn't solve the problem which is if the user try to login with wrong credential first and then try to login with right credential, the user can't login. It always shows "Authentication failed" toast message. But if the user try to login with correct credential the user is logged in at the first try. How can I solve this problem.

[iamareebjamal]

You don't have to dispose the composite disposable, that means you are ending the process of storing any observable in it. You have to store a disposable just for logging in and disposing it when logging in

[raj10071997]

@iamareebjamal I am disposing any observable which is already stored so that I can stop listening to previous observables and start with a new one. Because initially the size of composite disposable will be zero and then one observable will be subscribed by the composite disposable. So we when click the button again the size won't be zero this time now and I will dispose the previous observable and subscribe to a new one. What if I dispose the observable inside, after setting the loginResponse.setValue(). In that way when the value has been set, we are simply disposing the previous observable as we are not interest in previous data because when we press the button again we will be creating a new disposable.

[raj10071997]

@iamareebjamal Sorry you are right I am ending the process of storing any observable.

[raj10071997]

@iamareebjamal I used clear() instead of dispose() and changed the code to this.

 if(compositeDisposable.size()!=0)
            compositeDisposable.clear();

        compositeDisposable.add(APIClient.getOpenEventAPI().login(new Login(email, password))
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(response -> {
                            Timber.d("Saved token and logged in successfully");
                            loginResponse.setValue(new LoginResponse(VALID, response.getAccessToken()));
                        }, throwable -> {
                            Timber.e(throwable.toString());
                            loginResponse.setValue(new LoginResponse(INVALID, ""));
                        }));

        Log.d("dhanraj", String.valueOf(compositeDisposable.size()));

I am always getting the size = 1 and even changed the code to this

 compositeDisposable.add(APIClient.getOpenEventAPI().login(new Login(email, password))
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(response -> {
                            Timber.d("Saved token and logged in successfully");
                            loginResponse.setValue(new LoginResponse(VALID, response.getAccessToken()));
                            compositeDisposable.clear();
                        }, throwable -> {
                            Timber.e(throwable.toString());
                            loginResponse.setValue(new LoginResponse(INVALID, ""));
                           compositeDisposable.clear();
                        }));

Which means I am clearing all the disposable from compositeDisposable as soon as the loginResponse.setValue() method completes and the observer in the loginActivity will be notified of the changes. But still the behavior is same. Am I thinking right as I thought this is equivalent to storing only one disposable and when the response from the observable source comes we can do our required task and dispose the disposable? IMO this code should make the toast message appear twice because of the asynchronous behavior but the toast message appears in the behavior as if we are adding a new disposable every time.

[iamareebjamal]

you don't have to clear or touch composite disposable at all.

You have to store a disposable just for logging in and disposing it when logging in

if (this.loginDisposable != null)
    this.loginDisposable.dispose();

this.loginDisposable = APIClient.getOpenEventAPI().login(...);
compositeDisposable.add(this.loginDisposable);

[raj10071997]

@iamareebjamal Sorry for the late reply.

I have tried this code using your suggested changes but still the behavior is same.

 private MutableLiveData<LoginResponse> loginResponse;
    private final CompositeDisposable compositeDisposable = new CompositeDisposable();
    private Disposable loginDisposable;

    public LiveData<LoginResponse> loginUser(String email, String password) {
        if (loginResponse == null) {
            loginResponse = new MutableLiveData<>();
        }

        if(loginDisposable!=null)
            this.loginDisposable.dispose();

        this.loginDisposable = APIClient.getOpenEventAPI().login(new Login(email, password))
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(response -> {
                    Timber.d("Saved token and logged in successfully");
                    loginResponse.setValue(new LoginResponse(VALID, response.getAccessToken()));
                }, throwable -> {
                    Timber.e(throwable.toString());
                    loginResponse.setValue(new LoginResponse(INVALID, ""));
                });

        compositeDisposable.add(loginDisposable);

        Log.d("dhanraj", String.valueOf(compositeDisposable.size()));

        return loginResponse;
    }

[iamareebjamal]

I'll see to what's the issue