Skip to content

Move bookmark database operations off the UI thread #1665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
venkyqz wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move bookmark database operations off the UI thread #1665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 45 additions & 10 deletions app/src/main/java/com/gh4a/activities/RepositoryActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -101,6 +102,7 @@ public static Intent makeIntent(Context context, String repoOwner, String repoNa
private List<Branch> mBranches;
private List<Branch> mTags;
private String mSelectedRef;
private Boolean mIsBookmarked;

private RepositoryFragment mRepositoryFragment;
private ContentListContainerFragment mContentListFragment;
Expand Down Expand Up @@ -157,6 +159,7 @@ private void updateTitle() {

mActionBar.setSubtitle(getCurrentRef());
invalidateFragments();
refreshBookmarkState();
}

private String getCurrentRef() {
Expand Down Expand Up @@ -239,6 +242,7 @@ public void onRefresh() {
mContentListFragment = null;
mActivityFragment = null;
mRepository = null;
mIsBookmarked = null;
mBranches = null;
mTags = null;
clearRefDependentFragments();
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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()
Expand All @@ -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)));
Comment on lines +363 to +369
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to UserActivity, refreshBookmarkState() and the toggle fromCallable can be in-flight at the same time, and out-of-order completion can overwrite mIsBookmarked with stale data. Consider cancelling any previous bookmark-state request before starting a new one, or otherwise ensuring only the most recent query result updates mIsBookmarked.

Copilot uses AI. Check for mistakes.
}

private void showRefSelectionDialog() {
BranchSelectionDialogFragment.newInstance(mBranches, mTags, mSelectedRef, mRepository.defaultBranch())
.show(getSupportFragmentManager(), "branchselection");
Expand Down
59 changes: 47 additions & 12 deletions app/src/main/java/com/gh4a/activities/UserActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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;

Expand All @@ -57,6 +63,7 @@ protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);

setContentShown(false);
refreshBookmarkState();
loadUser(false);
}

Expand All @@ -83,6 +90,7 @@ protected int[] getTabTitleResIds() {
@Override
public void onRefresh() {
mUser = null;
mIsBookmarked = null;
setContentShown(false);
invalidateTabs();
invalidateOptionsMenu();
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
}

Comment on lines +196 to +200
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refreshBookmarkState() builds the bookmark URL manually ("https://github.com/" + mUserLogin) while other code in this class uses IntentUtils.createBaseUriForUser(mUserLogin). Using a single URL construction path avoids subtle mismatches if the base URL logic changes (scheme/encoding/trailing slash) and makes the bookmark key consistent everywhere.

Suggested change
String url = mUserLogin != null ? "https://github.com/" + mUserLogin : null;
if (url == null) {
return;
}
if (mUserLogin == null) {
return;
}
String url = IntentUtils.createBaseUriForUser(mUserLogin).build().toString();

Copilot uses AI. Check for mistakes.
registerTemporarySubscription(Single.fromCallable(() ->
BookmarksProvider.hasBookmarked(this, url))
.compose(RxUtils::doInBackground)
.subscribe(bookmarked -> {
mIsBookmarked = bookmarked;
invalidateOptionsMenu();
}, e -> handleActionFailure("Loading bookmark state failed", e)));
Comment on lines +201 to +207
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refreshBookmarkState() can run concurrently with the toggle operation (and with other refresh calls), and whichever query finishes last will overwrite mIsBookmarked. This can leave the menu title/icon in the wrong state (e.g., initial refresh returns "not bookmarked" after a successful save). Consider keeping a dedicated Disposable for bookmark-state loads and disposing any in-flight request before starting a new one, or otherwise sequencing requests so only the latest result updates mIsBookmarked (e.g., via a request counter / switchMapSingle).

Copilot uses AI. Check for mistakes.
}

private void loadUser(boolean force) {
UserService service = ServiceFactory.get(UserService.class, force);
service.getUser(mUserLogin)
Expand All @@ -182,7 +216,8 @@ private void loadUser(boolean force) {
mUser = result;
invalidateTabs();
setContentShown(true);
refreshBookmarkState();
invalidateOptionsMenu();
}, this::handleLoadFailure);
}
}
}
22 changes: 16 additions & 6 deletions app/src/main/java/com/gh4a/db/BookmarksProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down