From 120a409c3854148843ee8eb0ccb2ca1552903416 Mon Sep 17 00:00:00 2001 From: venkyqz Date: Wed, 22 Apr 2026 21:13:16 +0800 Subject: [PATCH] Prevent bookmark toggles from blocking menu callbacks Bookmark add/remove and bookmark-state refresh were doing synchronous ContentResolver work directly from activity menu paths. Move those calls onto the existing Rx background path, cache bookmark state for menu rendering, and split provider helpers so storage work can run without UI side effects. Constraint: Keep the fix localized to bookmark handling without refactoring surrounding activity architecture Rejected: Introduce a broader executor abstraction for local database calls | unnecessary for this narrow fix Confidence: high Scope-risk: narrow Directive: Keep bookmark provider I/O off menu callbacks; UI feedback belongs on the main thread and storage helpers must remain toast-free Tested: ./gradlew testDebugUnitTest Not-tested: Manual add/remove bookmark verification on device for user and repository screens --- .../gh4a/activities/RepositoryActivity.java | 55 +++++++++++++---- .../com/gh4a/activities/UserActivity.java | 59 +++++++++++++++---- .../java/com/gh4a/db/BookmarksProvider.java | 22 +++++-- 3 files changed, 108 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/com/gh4a/activities/RepositoryActivity.java b/app/src/main/java/com/gh4a/activities/RepositoryActivity.java index 7cee018af0..a7daac89f7 100644 --- a/app/src/main/java/com/gh4a/activities/RepositoryActivity.java +++ b/app/src/main/java/com/gh4a/activities/RepositoryActivity.java @@ -21,6 +21,7 @@ import android.widget.BaseAdapter; import android.widget.ImageView; import android.widget.TextView; +import android.widget.Toast; import com.gh4a.BaseFragmentPagerActivity; import com.gh4a.R; @@ -101,6 +102,7 @@ public static Intent makeIntent(Context context, String repoOwner, String repoNa private List mBranches; private List mTags; private String mSelectedRef; + private Boolean mIsBookmarked; private RepositoryFragment mRepositoryFragment; private ContentListContainerFragment mContentListFragment; @@ -157,6 +159,7 @@ private void updateTitle() { mActionBar.setSubtitle(getCurrentRef()); invalidateFragments(); + refreshBookmarkState(); } private String getCurrentRef() { @@ -239,6 +242,7 @@ public void onRefresh() { mContentListFragment = null; mActivityFragment = null; mRepository = null; + mIsBookmarked = null; mBranches = null; mTags = null; clearRefDependentFragments(); @@ -274,9 +278,8 @@ public boolean onPrepareOptionsMenu(Menu menu) { } else { MenuItem bookmarkAction = menu.findItem(R.id.bookmark); if (bookmarkAction != null) { - bookmarkAction.setTitle(BookmarksProvider.hasBookmarked(this, getBookmarkUrl()) - ? R.string.remove_bookmark - : R.string.bookmark); + bookmarkAction.setTitle(mIsBookmarked != null && mIsBookmarked + ? R.string.remove_bookmark : R.string.bookmark); } } @@ -311,15 +314,32 @@ public boolean onOptionsItemSelected(MenuItem item) { startActivity(SearchActivity.makeIntent(this, initialSearch, SearchActivity.SEARCH_TYPE_CODE, false)); return true; - case R.id.bookmark: + case R.id.bookmark: { String bookmarkUrl = getBookmarkUrl(); - if (BookmarksProvider.hasBookmarked(this, bookmarkUrl)) { - BookmarksProvider.removeBookmark(this, bookmarkUrl); - } else { - BookmarksProvider.saveBookmark(this, mActionBar.getTitle().toString(), - BookmarksProvider.Columns.TYPE_REPO, bookmarkUrl, getCurrentRef(), true); - } + String bookmarkName = mActionBar != null + ? mActionBar.getTitle().toString() : mRepoOwner + "/" + mRepoName; + String currentRef = getCurrentRef(); + registerTemporarySubscription(Single.fromCallable(() -> { + boolean hasBookmarked = BookmarksProvider.hasBookmarked(this, bookmarkUrl); + boolean success = hasBookmarked + ? BookmarksProvider.removeBookmarkInternal(this, bookmarkUrl) + : BookmarksProvider.saveBookmarkInternal(this, bookmarkName, + BookmarksProvider.Columns.TYPE_REPO, bookmarkUrl, currentRef); + return new Pair<>(success, !hasBookmarked); + }).compose(RxUtils::doInBackground).subscribe(result -> { + if (result.first) { + Toast.makeText(this, + result.second ? R.string.bookmark_saved : R.string.bookmark_removed, + result.second ? Toast.LENGTH_LONG : Toast.LENGTH_SHORT) + .show(); + mIsBookmarked = result.second; + supportInvalidateOptionsMenu(); + } else { + refreshBookmarkState(); + } + }, e -> handleActionFailure("Toggling bookmark failed", e))); return true; + } case R.id.zip_download: { final String zipUrl = Uri.parse(mRepository.url()) .buildUpon() @@ -334,6 +354,21 @@ public boolean onOptionsItemSelected(MenuItem item) { return super.onOptionsItemSelected(item); } + private void refreshBookmarkState() { + if (mRepository == null) { + return; + } + + String bookmarkUrl = getBookmarkUrl(); + registerTemporarySubscription(Single.fromCallable(() -> + BookmarksProvider.hasBookmarked(this, bookmarkUrl)) + .compose(RxUtils::doInBackground) + .subscribe(bookmarked -> { + mIsBookmarked = bookmarked; + supportInvalidateOptionsMenu(); + }, e -> handleActionFailure("Loading bookmark state failed", e))); + } + private void showRefSelectionDialog() { BranchSelectionDialogFragment.newInstance(mBranches, mTags, mSelectedRef, mRepository.defaultBranch()) .show(getSupportFragmentManager(), "branchselection"); diff --git a/app/src/main/java/com/gh4a/activities/UserActivity.java b/app/src/main/java/com/gh4a/activities/UserActivity.java index ebdb58e88d..319c2e1b6f 100644 --- a/app/src/main/java/com/gh4a/activities/UserActivity.java +++ b/app/src/main/java/com/gh4a/activities/UserActivity.java @@ -5,10 +5,12 @@ import android.net.Uri; import android.os.Bundle; import androidx.annotation.Nullable; +import androidx.core.util.Pair; import androidx.fragment.app.Fragment; import android.view.Menu; import android.view.MenuInflater; import android.view.MenuItem; +import android.widget.Toast; import com.gh4a.BaseFragmentPagerActivity; import com.gh4a.R; @@ -18,11 +20,14 @@ import com.gh4a.fragment.UserFragment; import com.gh4a.utils.ApiHelpers; import com.gh4a.utils.IntentUtils; +import com.gh4a.utils.RxUtils; import com.gh4a.utils.StringUtils; import com.meisolsson.githubsdk.model.User; import com.meisolsson.githubsdk.model.UserType; import com.meisolsson.githubsdk.service.users.UserService; +import io.reactivex.Single; + public class UserActivity extends BaseFragmentPagerActivity { public static Intent makeIntent(Context context, User user) { if (user != null && user.type() == UserType.Mannequin) { @@ -45,6 +50,7 @@ public static Intent makeIntent(Context context, String login) { private String mUserLogin; private User mUser; private UserFragment mUserFragment; + private Boolean mIsBookmarked; private static final int ID_LOADER_USER = 0; @@ -57,6 +63,7 @@ protected void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentShown(false); + refreshBookmarkState(); loadUser(false); } @@ -83,6 +90,7 @@ protected int[] getTabTitleResIds() { @Override public void onRefresh() { mUser = null; + mIsBookmarked = null; setContentShown(false); invalidateTabs(); invalidateOptionsMenu(); @@ -129,10 +137,8 @@ public boolean onCreateOptionsMenu(Menu menu) { public boolean onPrepareOptionsMenu(Menu menu) { MenuItem bookmarkAction = menu.findItem(R.id.bookmark); if (bookmarkAction != null) { - String url = "https://github.com/" + mUserLogin; - bookmarkAction.setTitle(BookmarksProvider.hasBookmarked(this, url) - ? R.string.remove_bookmark - : R.string.bookmark); + bookmarkAction.setTitle(mIsBookmarked != null && mIsBookmarked + ? R.string.remove_bookmark : R.string.bookmark); bookmarkAction.setVisible(mUser != null); } @@ -160,19 +166,47 @@ public boolean onOptionsItemSelected(MenuItem item) { return true; case R.id.bookmark: { String urlString = url.toString(); - if (BookmarksProvider.hasBookmarked(this, urlString)) { - BookmarksProvider.removeBookmark(this, urlString); - } else { - BookmarksProvider.saveBookmark(this, mUserLogin, - BookmarksProvider.Columns.TYPE_USER, - urlString, mUser.name(), true); - } + String userName = mUser != null ? mUser.name() : null; + registerTemporarySubscription(Single.fromCallable(() -> { + boolean hasBookmarked = BookmarksProvider.hasBookmarked(this, urlString); + boolean success = hasBookmarked + ? BookmarksProvider.removeBookmarkInternal(this, urlString) + : BookmarksProvider.saveBookmarkInternal(this, mUserLogin, + BookmarksProvider.Columns.TYPE_USER, urlString, userName); + return new Pair<>(success, !hasBookmarked); + }).compose(RxUtils::doInBackground).subscribe(result -> { + if (result.first) { + Toast.makeText(this, + result.second ? R.string.bookmark_saved : R.string.bookmark_removed, + result.second ? Toast.LENGTH_LONG : Toast.LENGTH_SHORT) + .show(); + mIsBookmarked = result.second; + invalidateOptionsMenu(); + } else { + refreshBookmarkState(); + } + }, e -> handleActionFailure("Toggling bookmark failed", e))); return true; } } return super.onOptionsItemSelected(item); } + private void refreshBookmarkState() { + String url = mUserLogin != null ? "https://github.com/" + mUserLogin : null; + if (url == null) { + return; + } + + registerTemporarySubscription(Single.fromCallable(() -> + BookmarksProvider.hasBookmarked(this, url)) + .compose(RxUtils::doInBackground) + .subscribe(bookmarked -> { + mIsBookmarked = bookmarked; + invalidateOptionsMenu(); + }, e -> handleActionFailure("Loading bookmark state failed", e))); + } + private void loadUser(boolean force) { UserService service = ServiceFactory.get(UserService.class, force); service.getUser(mUserLogin) @@ -182,7 +216,8 @@ private void loadUser(boolean force) { mUser = result; invalidateTabs(); setContentShown(true); + refreshBookmarkState(); invalidateOptionsMenu(); }, this::handleLoadFailure); } -} \ No newline at end of file +} diff --git a/app/src/main/java/com/gh4a/db/BookmarksProvider.java b/app/src/main/java/com/gh4a/db/BookmarksProvider.java index 808d10041a..c92645ddbb 100644 --- a/app/src/main/java/com/gh4a/db/BookmarksProvider.java +++ b/app/src/main/java/com/gh4a/db/BookmarksProvider.java @@ -53,6 +53,14 @@ public interface Columns extends BaseColumns { // url must be resolvable by BrowseFilter! public static void saveBookmark(Context context, String name, int type, String url, String extraData, boolean showToast) { + boolean saved = saveBookmarkInternal(context, name, type, url, extraData); + if (saved && showToast) { + Toast.makeText(context, R.string.bookmark_saved, Toast.LENGTH_LONG).show(); + } + } + + public static boolean saveBookmarkInternal(Context context, String name, int type, String url, + String extraData) { ContentResolver cr = context.getContentResolver(); ContentValues cv = new ContentValues(); @@ -62,9 +70,7 @@ public static void saveBookmark(Context context, String name, int type, String u cv.put(BookmarksProvider.Columns.EXTRA, extraData); cv.put(BookmarksProvider.Columns.ORDER_ID, getNextOrderId(cr)); - if (cr.insert(BookmarksProvider.Columns.CONTENT_URI, cv) != null && showToast) { - Toast.makeText(context, R.string.bookmark_saved, Toast.LENGTH_LONG).show(); - } + return cr.insert(BookmarksProvider.Columns.CONTENT_URI, cv) != null; } private static int getNextOrderId(ContentResolver cr) { @@ -83,12 +89,16 @@ private static int getNextOrderId(ContentResolver cr) { } public static void removeBookmark(Context context, String url) { + if (removeBookmarkInternal(context, url)) { + Toast.makeText(context, R.string.bookmark_removed, Toast.LENGTH_SHORT).show(); + } + } + + public static boolean removeBookmarkInternal(Context context, String url) { int removedRows = context.getContentResolver().delete(Columns.CONTENT_URI, Columns.URI + " = ?", new String[] { url }); - if (removedRows > 0) { - Toast.makeText(context, R.string.bookmark_removed, Toast.LENGTH_SHORT).show(); - } + return removedRows > 0; } public static boolean hasBookmarked(Context context, String url) {