From 241a512a7fd9532b5d7e95537d3a9f3276b42957 Mon Sep 17 00:00:00 2001 From: dosiecki Date: Fri, 3 Oct 2014 12:28:46 -0700 Subject: [PATCH 1/5] Show story list loading indicator only if stories are being fetched, not just if they are queued. (read: don't spin if offline) (#590) --- .../NewsBlur/src/com/newsblur/fragment/ItemListFragment.java | 5 ++++- .../NewsBlur/src/com/newsblur/service/NBSyncService.java | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/clients/android/NewsBlur/src/com/newsblur/fragment/ItemListFragment.java b/clients/android/NewsBlur/src/com/newsblur/fragment/ItemListFragment.java index 7387600f5..4c41930cb 100644 --- a/clients/android/NewsBlur/src/com/newsblur/fragment/ItemListFragment.java +++ b/clients/android/NewsBlur/src/com/newsblur/fragment/ItemListFragment.java @@ -46,6 +46,7 @@ public abstract class ItemListFragment extends NbFragment implements OnScrollLis protected DefaultFeedView defaultFeedView; protected StateFilter currentState; private boolean isLoading = true; + private boolean cursorSeenYet = false; @Override public void onCreate(Bundle savedInstanceState) { @@ -58,6 +59,7 @@ public abstract class ItemListFragment extends NbFragment implements OnScrollLis * Indicate that the DB was cleared. */ public void resetEmptyState() { + cursorSeenYet = false; setLoading(true); } @@ -76,7 +78,7 @@ public abstract class ItemListFragment extends NbFragment implements OnScrollLis } TextView emptyView = (TextView) itemList.getEmptyView(); - if (isLoading) { + if (isLoading || (!cursorSeenYet)) { emptyView.setText(R.string.empty_list_view_loading); } else { emptyView.setText(R.string.empty_list_view_no_stories); @@ -130,6 +132,7 @@ public abstract class ItemListFragment extends NbFragment implements OnScrollLis @Override public void onLoadFinished(Loader loader, Cursor cursor) { if (cursor != null) { + cursorSeenYet = true; if (cursor.getCount() == 0) { ((ItemsList) getActivity()).triggerRefresh(1); } diff --git a/clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java b/clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java index 9ad776299..a082985e1 100644 --- a/clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java +++ b/clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java @@ -653,7 +653,7 @@ public class NBSyncService extends Service { * Is there a sync for a given FeedSet running? */ public static boolean isFeedSetSyncing(FeedSet fs) { - return (PendingFeeds.containsKey(fs)); + return (PendingFeeds.containsKey(fs) && StorySyncRunning); } public static String getSyncStatusMessage() { From d1f8c23f60728e90517dbd6ca638046e4102a89a Mon Sep 17 00:00:00 2001 From: dosiecki Date: Fri, 3 Oct 2014 13:27:01 -0700 Subject: [PATCH 2/5] Cleaner service shutdown. (#591) --- .../src/com/newsblur/service/NBSyncService.java | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java b/clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java index a082985e1..abdbfeadf 100644 --- a/clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java +++ b/clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java @@ -44,6 +44,7 @@ import java.util.Map.Entry; import java.util.Set; import java.util.concurrent.Executors; import java.util.concurrent.ExecutorService; +import java.util.concurrent.TimeUnit; /** * A background service to handle synchronisation with the NB servers. @@ -752,9 +753,21 @@ public class NBSyncService extends Service { @Override public void onDestroy() { - Log.d(this.getClass().getName(), "onDestroy"); + if (AppConstants.VERBOSE_LOG) Log.d(this.getClass().getName(), "onDestroy - stopping execution"); HaltNow = true; executor.shutdown(); + boolean cleanShutdown = false; + try { + cleanShutdown = executor.awaitTermination(60, TimeUnit.SECONDS); + } catch (InterruptedException e) { + // this value is somewhat arbitrary. ideally we would wait the max network timeout, but + // the system like to force-kill terminating services that take too long, so it is often + // moot to tune. + executor.shutdownNow(); + Thread.currentThread().interrupt(); + } + if (AppConstants.VERBOSE_LOG) Log.d(this.getClass().getName(), "onDestroy - execution halted"); + super.onDestroy(); dbHelper.close(); } From 32cdc1b512e891f2a4c4587e0e5147c6e1a35316 Mon Sep 17 00:00:00 2001 From: dosiecki Date: Fri, 3 Oct 2014 13:59:43 -0700 Subject: [PATCH 3/5] Explicity synchro DB writes. (#591) --- .../newsblur/database/BlurDatabaseHelper.java | 55 ++++++++++--------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabaseHelper.java b/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabaseHelper.java index 4e84057a7..96df2685a 100644 --- a/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabaseHelper.java +++ b/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabaseHelper.java @@ -41,6 +41,9 @@ import java.util.Set; */ public class BlurDatabaseHelper { + // manual synchro isn't needed if you only use one DBHelper, but at present the app uses several + private final static Object RW_MUTEX = new Object(); + private Context context; private final BlurDatabase dbWrapper; private SQLiteDatabase dbRO; @@ -103,7 +106,7 @@ public class BlurDatabaseHelper { " ORDER BY " + DatabaseConstants.STORY_TIMESTAMP + " DESC" + " LIMIT -1 OFFSET " + (keepOldStories ? AppConstants.MAX_READ_STORIES_STORED : 0) + ")"; - dbRW.execSQL(q); + synchronized (RW_MUTEX) {dbRW.execSQL(q);} } } @@ -112,25 +115,27 @@ public class BlurDatabaseHelper { " WHERE " + DatabaseConstants.STORY_TEXT_STORY_HASH + " NOT IN " + "( SELECT " + DatabaseConstants.STORY_HASH + " FROM " + DatabaseConstants.STORY_TABLE + ")"; - dbRW.execSQL(q); + synchronized (RW_MUTEX) {dbRW.execSQL(q);} } public void cleanupFeedsFolders() { - dbRW.delete(DatabaseConstants.FEED_TABLE, null, null); - dbRW.delete(DatabaseConstants.FOLDER_TABLE, null, null); - dbRW.delete(DatabaseConstants.FEED_FOLDER_MAP_TABLE, null, null); + synchronized (RW_MUTEX) {dbRW.delete(DatabaseConstants.FEED_TABLE, null, null);} + synchronized (RW_MUTEX) {dbRW.delete(DatabaseConstants.FOLDER_TABLE, null, null);} + synchronized (RW_MUTEX) {dbRW.delete(DatabaseConstants.FEED_FOLDER_MAP_TABLE, null, null);} } private void bulkInsertValues(String table, List valuesList) { if (valuesList.size() < 1) return; - dbRW.beginTransaction(); - try { - for(ContentValues values: valuesList) { - dbRW.insertWithOnConflict(table, null, values, SQLiteDatabase.CONFLICT_REPLACE); + synchronized (RW_MUTEX) { + dbRW.beginTransaction(); + try { + for(ContentValues values: valuesList) { + dbRW.insertWithOnConflict(table, null, values, SQLiteDatabase.CONFLICT_REPLACE); + } + dbRW.setTransactionSuccessful(); + } finally { + dbRW.endTransaction(); } - dbRW.setTransactionSuccessful(); - } finally { - dbRW.endTransaction(); } } @@ -148,8 +153,8 @@ public class BlurDatabaseHelper { ContentValues values = new ContentValues(); values.put(DatabaseConstants.STARRED_STORY_COUNT_COUNT, count); // this DB just has one row and one column. blow it away and replace it. - dbRW.delete(DatabaseConstants.STARRED_STORY_COUNT_TABLE, null, null); - dbRW.insert(DatabaseConstants.STARRED_STORY_COUNT_TABLE, null, values); + synchronized (RW_MUTEX) {dbRW.delete(DatabaseConstants.STARRED_STORY_COUNT_TABLE, null, null);} + synchronized (RW_MUTEX) {dbRW.insert(DatabaseConstants.STARRED_STORY_COUNT_TABLE, null, values);} } public List getStoryHashesForFeed(String feedId) { @@ -232,7 +237,7 @@ public class BlurDatabaseHelper { for (ContentValues values : classifierValues) { values.put(DatabaseConstants.CLASSIFIER_ID, classifierFeedId); } - dbRW.delete(DatabaseConstants.CLASSIFIER_TABLE, DatabaseConstants.CLASSIFIER_ID + " = ?", new String[] { classifierFeedId }); + synchronized (RW_MUTEX) {dbRW.delete(DatabaseConstants.CLASSIFIER_TABLE, DatabaseConstants.CLASSIFIER_ID + " = ?", new String[] { classifierFeedId });} bulkInsertValues(DatabaseConstants.CLASSIFIER_TABLE, classifierValues); } } @@ -288,7 +293,7 @@ public class BlurDatabaseHelper { ContentValues values = new ContentValues(); values.put(DatabaseConstants.STORY_READ, read); values.put(DatabaseConstants.STORY_READ_THIS_SESSION, read); - dbRW.update(DatabaseConstants.STORY_TABLE, values, DatabaseConstants.STORY_HASH + " = ?", new String[]{hash}); + synchronized (RW_MUTEX) {dbRW.update(DatabaseConstants.STORY_TABLE, values, DatabaseConstants.STORY_HASH + " = ?", new String[]{hash});} } /** @@ -335,7 +340,7 @@ public class BlurDatabaseHelper { } else { throw new IllegalStateException("Asked to mark stories for FeedSet of unknown type."); } - dbRW.update(DatabaseConstants.STORY_TABLE, values, conjoinSelections(feedSelection, rangeSelection), null); + synchronized (RW_MUTEX) {dbRW.update(DatabaseConstants.STORY_TABLE, values, conjoinSelections(feedSelection, rangeSelection), null);} refreshFeedCounts(fs); } @@ -370,7 +375,7 @@ public class BlurDatabaseHelper { values.put(DatabaseConstants.FEED_NEGATIVE_COUNT, getUnreadCount(singleFs, StateFilter.NEG)); values.put(DatabaseConstants.FEED_NEUTRAL_COUNT, getUnreadCount(singleFs, StateFilter.NEUT)); values.put(DatabaseConstants.FEED_POSITIVE_COUNT, getUnreadCount(singleFs, StateFilter.BEST)); - dbRW.update(DatabaseConstants.FEED_TABLE, values, DatabaseConstants.FEED_ID + " = ?", new String[]{feedId}); + synchronized (RW_MUTEX) {dbRW.update(DatabaseConstants.FEED_TABLE, values, DatabaseConstants.FEED_ID + " = ?", new String[]{feedId});} } for (String socialId : socialFeedIds) { @@ -379,7 +384,7 @@ public class BlurDatabaseHelper { values.put(DatabaseConstants.SOCIAL_FEED_NEGATIVE_COUNT, getUnreadCount(singleFs, StateFilter.NEG)); values.put(DatabaseConstants.SOCIAL_FEED_NEUTRAL_COUNT, getUnreadCount(singleFs, StateFilter.NEUT)); values.put(DatabaseConstants.SOCIAL_FEED_POSITIVE_COUNT, getUnreadCount(singleFs, StateFilter.BEST)); - dbRW.update(DatabaseConstants.SOCIALFEED_TABLE, values, DatabaseConstants.SOCIAL_FEED_ID + " = ?", new String[]{socialId}); + synchronized (RW_MUTEX) {dbRW.update(DatabaseConstants.SOCIALFEED_TABLE, values, DatabaseConstants.SOCIAL_FEED_ID + " = ?", new String[]{socialId});} } } @@ -391,7 +396,7 @@ public class BlurDatabaseHelper { } public void enqueueAction(ReadingAction ra) { - dbRW.insertOrThrow(DatabaseConstants.ACTION_TABLE, null, ra.toContentValues()); + synchronized (RW_MUTEX) {dbRW.insertOrThrow(DatabaseConstants.ACTION_TABLE, null, ra.toContentValues());} } public Cursor getActions(boolean includeDone) { @@ -400,7 +405,7 @@ public class BlurDatabaseHelper { } public void clearAction(String actionId) { - dbRW.delete(DatabaseConstants.ACTION_TABLE, DatabaseConstants.ACTION_ID + " = ?", new String[]{actionId}); + synchronized (RW_MUTEX) {dbRW.delete(DatabaseConstants.ACTION_TABLE, DatabaseConstants.ACTION_ID + " = ?", new String[]{actionId});} } public Cursor getStory(String hash) { @@ -412,7 +417,7 @@ public class BlurDatabaseHelper { public void setStoryStarred(String hash, boolean starred) { ContentValues values = new ContentValues(); values.put(DatabaseConstants.STORY_STARRED, starred); - dbRW.update(DatabaseConstants.STORY_TABLE, values, DatabaseConstants.STORY_HASH + " = ?", new String[]{hash}); + synchronized (RW_MUTEX) {dbRW.update(DatabaseConstants.STORY_TABLE, values, DatabaseConstants.STORY_HASH + " = ?", new String[]{hash});} } public String getStoryText(String hash) { @@ -435,7 +440,7 @@ public class BlurDatabaseHelper { ContentValues values = new ContentValues(); values.put(DatabaseConstants.STORY_TEXT_STORY_HASH, hash); values.put(DatabaseConstants.STORY_TEXT_STORY_TEXT, text); - dbRW.insertOrThrow(DatabaseConstants.STORY_TEXT_TABLE, null, values); + synchronized (RW_MUTEX) {dbRW.insertOrThrow(DatabaseConstants.STORY_TEXT_TABLE, null, values);} } /** @@ -444,7 +449,7 @@ public class BlurDatabaseHelper { public void markSavedReadingSession() { ContentValues values = new ContentValues(); values.put(DatabaseConstants.STORY_READ_THIS_SESSION, true); - dbRW.update(DatabaseConstants.STORY_TABLE, values, DatabaseConstants.STORY_STARRED + " = 1", null); + synchronized (RW_MUTEX) {dbRW.update(DatabaseConstants.STORY_TABLE, values, DatabaseConstants.STORY_STARRED + " = 1", null);} } /** @@ -453,7 +458,7 @@ public class BlurDatabaseHelper { public void clearReadingSession() { ContentValues values = new ContentValues(); values.put(DatabaseConstants.STORY_READ_THIS_SESSION, false); - dbRW.update(DatabaseConstants.STORY_TABLE, values, null, null); + synchronized (RW_MUTEX) {dbRW.update(DatabaseConstants.STORY_TABLE, values, null, null);} } public Loader getStoriesLoader(final FeedSet fs, final StateFilter stateFilter) { From 1462d53cb01dfbcb6b74226d0c3fbc5b2f5d6a07 Mon Sep 17 00:00:00 2001 From: dosiecki Date: Fri, 3 Oct 2014 14:39:08 -0700 Subject: [PATCH 4/5] Synchronise DB upgrades with other DB actions. (#591) --- .../NewsBlur/src/com/newsblur/database/BlurDatabase.java | 4 +--- .../src/com/newsblur/database/BlurDatabaseHelper.java | 4 ++++ .../android/NewsBlur/src/com/newsblur/util/FeedUtils.java | 4 ++++ .../android/NewsBlur/src/com/newsblur/util/PrefsUtils.java | 7 ++----- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabase.java b/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabase.java index 4e759540b..8d6a9275a 100644 --- a/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabase.java +++ b/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabase.java @@ -30,7 +30,7 @@ public class BlurDatabase extends SQLiteOpenHelper { db.execSQL(DatabaseConstants.ACTION_SQL); } - public void dropAndRecreateTables() { + void dropAndRecreateTables() { SQLiteDatabase db = getWritableDatabase(); String drop = "DROP TABLE IF EXISTS "; db.execSQL(drop + DatabaseConstants.FEED_TABLE); @@ -48,8 +48,6 @@ public class BlurDatabase extends SQLiteOpenHelper { db.execSQL(drop + DatabaseConstants.ACTION_TABLE); onCreate(db); - - db.close(); } @Override diff --git a/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabaseHelper.java b/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabaseHelper.java index 96df2685a..8373461e1 100644 --- a/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabaseHelper.java +++ b/clients/android/NewsBlur/src/com/newsblur/database/BlurDatabaseHelper.java @@ -72,6 +72,10 @@ public class BlurDatabaseHelper { return dbRW.isOpen(); } + public void dropAndRecreateTables() { + synchronized (RW_MUTEX) {dbWrapper.dropAndRecreateTables();} + } + private List getAllFeeds() { String q1 = "SELECT " + DatabaseConstants.FEED_ID + " FROM " + DatabaseConstants.FEED_TABLE; diff --git a/clients/android/NewsBlur/src/com/newsblur/util/FeedUtils.java b/clients/android/NewsBlur/src/com/newsblur/util/FeedUtils.java index 2b562c345..bff16ad09 100644 --- a/clients/android/NewsBlur/src/com/newsblur/util/FeedUtils.java +++ b/clients/android/NewsBlur/src/com/newsblur/util/FeedUtils.java @@ -50,6 +50,10 @@ public class FeedUtils { c.startService(i); } + public static void dropAndRecreateTables() { + dbHelper.dropAndRecreateTables(); + } + public static void setStorySaved(final Story story, final boolean saved, final Context context) { new AsyncTask() { @Override diff --git a/clients/android/NewsBlur/src/com/newsblur/util/PrefsUtils.java b/clients/android/NewsBlur/src/com/newsblur/util/PrefsUtils.java index 0cd5a2db7..9f54bbb52 100644 --- a/clients/android/NewsBlur/src/com/newsblur/util/PrefsUtils.java +++ b/clients/android/NewsBlur/src/com/newsblur/util/PrefsUtils.java @@ -24,7 +24,6 @@ import android.util.Log; import com.newsblur.R; import com.newsblur.activity.Login; -import com.newsblur.database.BlurDatabase; import com.newsblur.domain.UserDetails; import com.newsblur.service.NBSyncService; @@ -58,8 +57,7 @@ public class PrefsUtils { if ( (oldVersion == null) || (!oldVersion.equals(version)) ) { Log.i(PrefsUtils.class.getName(), "detected new version of app, clearing local data"); // wipe the local DB - BlurDatabase databaseHelper = new BlurDatabase(context.getApplicationContext()); - databaseHelper.dropAndRecreateTables(); + FeedUtils.dropAndRecreateTables(); // in case this is the first time we have run since moving the cache to the new location, // blow away the old version entirely. This line can be removed some time well after // v61+ is widely deployed @@ -99,8 +97,7 @@ public class PrefsUtils { context.getSharedPreferences(PrefConstants.PREFERENCES, 0).edit().clear().commit(); // wipe the local DB - BlurDatabase databaseHelper = new BlurDatabase(context.getApplicationContext()); - databaseHelper.dropAndRecreateTables(); + FeedUtils.dropAndRecreateTables(); // prompt for a new login Intent i = new Intent(context, Login.class); From c29c93c6291ca0bcf11ea0086b38ba9ddc7bfa36 Mon Sep 17 00:00:00 2001 From: dosiecki Date: Fri, 3 Oct 2014 14:49:04 -0700 Subject: [PATCH 5/5] Don't re-fetch already cached images. --- clients/android/NewsBlur/src/com/newsblur/util/ImageCache.java | 1 + 1 file changed, 1 insertion(+) diff --git a/clients/android/NewsBlur/src/com/newsblur/util/ImageCache.java b/clients/android/NewsBlur/src/com/newsblur/util/ImageCache.java index a54f634dd..00a5648b8 100644 --- a/clients/android/NewsBlur/src/com/newsblur/util/ImageCache.java +++ b/clients/android/NewsBlur/src/com/newsblur/util/ImageCache.java @@ -48,6 +48,7 @@ public class ImageCache { } File f = new File(cacheDir, fileName); + if (f.exists()) return; URL u = new URL(url); NetworkUtils.loadURL(u, new FileOutputStream(f)); } catch (IOException e) {