From 468f0b72747589f82a984703cf187287326d59f6 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Mon, 18 Mar 2019 15:43:36 +0530 Subject: [PATCH] Bug Fix #2585 (#2647) * Bug Fix #2585 * Added null checks on view in SearchImageFragment when updating views from external sources * Disposed the disposables in SearchActivity and SearchImageFragment when no longer in active lifecycle * use FragmentUtils to verify fragment active state --- .../nrw/commons/explore/SearchActivity.java | 73 +++++++++++-------- .../explore/images/SearchImageFragment.java | 49 ++++++++----- .../free/nrw/commons/utils/FragmentUtils.java | 2 +- 3 files changed, 74 insertions(+), 50 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/explore/SearchActivity.java b/app/src/main/java/fr/free/nrw/commons/explore/SearchActivity.java index 9d7ba08ca..3e1fea51c 100644 --- a/app/src/main/java/fr/free/nrw/commons/explore/SearchActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/explore/SearchActivity.java @@ -12,16 +12,10 @@ import android.text.TextUtils; import android.view.View; import android.widget.FrameLayout; import android.widget.SearchView; - -import com.jakewharton.rxbinding2.view.RxView; -import com.jakewharton.rxbinding2.widget.RxSearchView; - -import java.util.ArrayList; -import java.util.List; -import java.util.concurrent.TimeUnit; - import butterknife.BindView; import butterknife.ButterKnife; +import com.jakewharton.rxbinding2.view.RxView; +import com.jakewharton.rxbinding2.widget.RxSearchView; import fr.free.nrw.commons.Media; import fr.free.nrw.commons.R; import fr.free.nrw.commons.explore.categories.SearchCategoryFragment; @@ -29,8 +23,13 @@ import fr.free.nrw.commons.explore.images.SearchImageFragment; import fr.free.nrw.commons.explore.recentsearches.RecentSearchesFragment; import fr.free.nrw.commons.media.MediaDetailPagerFragment; import fr.free.nrw.commons.theme.NavigationBaseActivity; +import fr.free.nrw.commons.utils.FragmentUtils; import fr.free.nrw.commons.utils.ViewUtil; import io.reactivex.android.schedulers.AndroidSchedulers; +import io.reactivex.disposables.Disposable; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.TimeUnit; /** * Represents search screen of this app @@ -52,6 +51,7 @@ public class SearchActivity extends NavigationBaseActivity implements MediaDetai private MediaDetailPagerFragment mediaDetails; ViewPagerAdapter viewPagerAdapter; private String query; + private Disposable searchDisposable; @Override protected void onCreate(Bundle savedInstanceState) { @@ -98,28 +98,33 @@ public class SearchActivity extends NavigationBaseActivity implements MediaDetai viewPagerAdapter.setTabData(fragmentList, titleList); viewPagerAdapter.notifyDataSetChanged(); - RxSearchView.queryTextChanges(searchView) - .takeUntil(RxView.detaches(searchView)) - .debounce(500, TimeUnit.MILLISECONDS) - .observeOn(AndroidSchedulers.mainThread()) - .subscribe( query -> { - this.query = query.toString(); - //update image list - if (!TextUtils.isEmpty(query)) { - viewPager.setVisibility(View.VISIBLE); - tabLayout.setVisibility(View.VISIBLE); - searchHistoryContainer.setVisibility(View.GONE); - searchImageFragment.updateImageList(query.toString()); - searchCategoryFragment.updateCategoryList(query.toString()); - }else { - viewPager.setVisibility(View.GONE); - tabLayout.setVisibility(View.GONE); - searchHistoryContainer.setVisibility(View.VISIBLE); - recentSearchesFragment.updateRecentSearches(); - // open search history fragment - } - } - ); + searchDisposable = RxSearchView.queryTextChanges(searchView) + .takeUntil(RxView.detaches(searchView)) + .debounce(500, TimeUnit.MILLISECONDS) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe(query -> { + this.query = query.toString(); + //update image list + if (!TextUtils.isEmpty(query)) { + viewPager.setVisibility(View.VISIBLE); + tabLayout.setVisibility(View.VISIBLE); + searchHistoryContainer.setVisibility(View.GONE); + if (FragmentUtils.isFragmentUIActive(searchImageFragment)) { + searchImageFragment.updateImageList(query.toString()); + } + + if (FragmentUtils.isFragmentUIActive(searchCategoryFragment)) { + searchCategoryFragment.updateCategoryList(query.toString()); + } + + } else { + viewPager.setVisibility(View.GONE); + tabLayout.setVisibility(View.GONE); + searchHistoryContainer.setVisibility(View.VISIBLE); + recentSearchesFragment.updateRecentSearches(); + // open search history fragment + } + }); } /** @@ -260,4 +265,12 @@ public class SearchActivity extends NavigationBaseActivity implements MediaDetai searchImageFragment.addImagesToList(query); } } + + @Override protected void onDestroy() { + super.onDestroy(); + //Dispose the disposables when the activity is destroyed + if (searchDisposable != null) { + searchDisposable.dispose(); + } + } } diff --git a/app/src/main/java/fr/free/nrw/commons/explore/images/SearchImageFragment.java b/app/src/main/java/fr/free/nrw/commons/explore/images/SearchImageFragment.java index 3e066d979..d1e8e871d 100644 --- a/app/src/main/java/fr/free/nrw/commons/explore/images/SearchImageFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/explore/images/SearchImageFragment.java @@ -1,6 +1,5 @@ package fr.free.nrw.commons.explore.images; - import android.annotation.SuppressLint; import android.content.res.Configuration; import android.os.Bundle; @@ -13,19 +12,9 @@ import android.view.View; import android.view.ViewGroup; import android.widget.ProgressBar; import android.widget.TextView; - -import com.pedrogomez.renderers.RVRendererAdapter; - -import java.util.ArrayList; -import java.util.Date; -import java.util.List; -import java.util.concurrent.TimeUnit; - -import javax.inject.Inject; -import javax.inject.Named; - import butterknife.BindView; import butterknife.ButterKnife; +import com.pedrogomez.renderers.RVRendererAdapter; import fr.free.nrw.commons.Media; import fr.free.nrw.commons.R; import fr.free.nrw.commons.di.CommonsDaggerSupportFragment; @@ -37,7 +26,14 @@ import fr.free.nrw.commons.mwapi.OkHttpJsonApiClient; import fr.free.nrw.commons.utils.NetworkUtils; import fr.free.nrw.commons.utils.ViewUtil; import io.reactivex.android.schedulers.AndroidSchedulers; +import io.reactivex.disposables.Disposable; import io.reactivex.schedulers.Schedulers; +import java.util.ArrayList; +import java.util.Date; +import java.util.List; +import java.util.concurrent.TimeUnit; +import javax.inject.Inject; +import javax.inject.Named; import timber.log.Timber; import static android.view.View.GONE; @@ -75,6 +71,7 @@ public class SearchImageFragment extends CommonsDaggerSupportFragment { ((SearchActivity)getContext()).onSearchImageClicked(index); saveQuery(query); }); + private Disposable searchImagesDisposable; /** * This method saves Search Query in the Recent Searches Database. @@ -140,11 +137,11 @@ public class SearchImageFragment extends CommonsDaggerSupportFragment { bottomProgressBar.setVisibility(GONE); queryList.clear(); imagesAdapter.clear(); - okHttpJsonApiClient.searchImages(query, queryList.size()) - .subscribeOn(Schedulers.io()) - .observeOn(AndroidSchedulers.mainThread()) - .timeout(TIMEOUT_SECONDS, TimeUnit.SECONDS) - .subscribe(this::handleSuccess, this::handleError); + searchImagesDisposable = okHttpJsonApiClient.searchImages(query, queryList.size()) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .timeout(TIMEOUT_SECONDS, TimeUnit.SECONDS) + .subscribe(this::handleSuccess, this::handleError); } @@ -230,8 +227,15 @@ public class SearchImageFragment extends CommonsDaggerSupportFragment { * Handles the UI updates for no internet scenario */ private void handleNoInternet() { - progressBar.setVisibility(GONE); - ViewUtil.showShortSnackbar(imagesRecyclerView, R.string.no_internet); + if (null + != getView()) {//We have exposed public methods to update our ui, we will have to add null checks until we make this lifecycle aware + if (null != progressBar) { + progressBar.setVisibility(GONE); + } + ViewUtil.showShortSnackbar(imagesRecyclerView, R.string.no_internet); + } else { + Timber.d("Attempt to update fragment ui after its view was destroyed"); + } } /** @@ -259,4 +263,11 @@ public class SearchImageFragment extends CommonsDaggerSupportFragment { return imagesAdapter.getItem(i); } } + + @Override public void onDestroyView() { + super.onDestroyView(); + if(searchImagesDisposable!=null) { + searchImagesDisposable.dispose();//Always dispose the disposables when view is no longer there + } + } } diff --git a/app/src/main/java/fr/free/nrw/commons/utils/FragmentUtils.java b/app/src/main/java/fr/free/nrw/commons/utils/FragmentUtils.java index 075e4dfdd..38b332af3 100644 --- a/app/src/main/java/fr/free/nrw/commons/utils/FragmentUtils.java +++ b/app/src/main/java/fr/free/nrw/commons/utils/FragmentUtils.java @@ -15,6 +15,6 @@ public class FragmentUtils { * @return */ public static boolean isFragmentUIActive(Fragment fragment) { - return fragment.getActivity() != null && fragment.isAdded() && !fragment.isDetached() && !fragment.isRemoving(); + return fragment!=null && fragment.getActivity() != null && fragment.isAdded() && !fragment.isDetached() && !fragment.isRemoving(); } }