From b65b5680f9253d9204b38e98846f277fa7a2b699 Mon Sep 17 00:00:00 2001 From: Bradlee Speice Date: Tue, 11 Nov 2014 23:46:51 -0500 Subject: [PATCH] Remove the InstalledManager Largely duplicated work, and makes testing easier! --- ...dManagerTest.java => BookManagerTest.java} | 56 ++++-- .../manager/InstalledManagerTest.java | 162 ------------------ .../activity/downloader/BookItemHolder.java | 19 +- .../downloader/DownloadActivityModules.java | 12 +- .../downloader/manager/InstalledManager.java | 77 --------- ...{BookDownloadManager.kt => BookManager.kt} | 59 +++++-- .../org/crosswire/jsword/book/BookUtil.kt | 4 +- 7 files changed, 106 insertions(+), 283 deletions(-) rename app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/{BookDownloadManagerTest.java => BookManagerTest.java} (73%) delete mode 100644 app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/InstalledManagerTest.java delete mode 100644 app/src/main/java/org/bspeice/minimalbible/activity/downloader/manager/InstalledManager.java rename app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/{BookDownloadManager.kt => BookManager.kt} (67%) diff --git a/app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/BookDownloadManagerTest.java b/app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/BookManagerTest.java similarity index 73% rename from app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/BookDownloadManagerTest.java rename to app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/BookManagerTest.java index bb6d5a0..b1346c3 100644 --- a/app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/BookDownloadManagerTest.java +++ b/app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/BookManagerTest.java @@ -7,14 +7,17 @@ import android.util.Log; import org.bspeice.minimalbible.Injector; import org.bspeice.minimalbible.MBTestCase; import org.bspeice.minimalbible.activity.downloader.DownloadPrefs; -import org.bspeice.minimalbible.activity.downloader.manager.BookDownloadManager; +import org.bspeice.minimalbible.activity.downloader.manager.BookManager; import org.bspeice.minimalbible.activity.downloader.manager.DLProgressEvent; import org.bspeice.minimalbible.activity.downloader.manager.RefreshManager; import org.crosswire.common.progress.JobManager; import org.crosswire.common.progress.WorkEvent; import org.crosswire.common.progress.WorkListener; import org.crosswire.jsword.book.Book; +import org.crosswire.jsword.book.BookDriver; +import org.crosswire.jsword.book.BookException; import org.crosswire.jsword.book.Books; +import org.crosswire.jsword.book.BooksEvent; import org.crosswire.jsword.book.install.InstallManager; import org.crosswire.jsword.book.install.Installer; import org.mockito.Mockito; @@ -35,13 +38,15 @@ import rx.functions.Func1; import static com.jayway.awaitility.Awaitility.await; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.internal.verification.VerificationModeFactory.times; -public class BookDownloadManagerTest extends MBTestCase implements Injector { +public class BookManagerTest extends MBTestCase implements Injector { ObjectGraph mObjectGraph; @Inject - BookDownloadManager bookDownloadManager; + BookManager bookManager; @Inject RefreshManager refreshManager; @Inject @@ -71,10 +76,10 @@ public class BookDownloadManagerTest extends MBTestCase implements Injector { public void testInstallBook() throws Exception { final Book toInstall = installableBooks().toBlocking().first(); - bookDownloadManager.installBook(toInstall); + bookManager.installBook(toInstall); final AtomicBoolean signal = new AtomicBoolean(false); - bookDownloadManager.getDownloadEvents() + bookManager.getDownloadEvents() .subscribe(new Action1() { @Override public void call(DLProgressEvent dlProgressEvent) { @@ -91,7 +96,7 @@ public class BookDownloadManagerTest extends MBTestCase implements Injector { public void testJobIdMatch() { final Book toInstall = installableBooks().toBlocking().first(); - final String jobName = bookDownloadManager.getJobId(toInstall); + final String jobName = bookManager.getJobId(toInstall); final AtomicBoolean jobNameMatch = new AtomicBoolean(false); JobManager.addWorkListener(new WorkListener() { @@ -108,17 +113,46 @@ public class BookDownloadManagerTest extends MBTestCase implements Injector { } }); - bookDownloadManager.installBook(toInstall); + bookManager.installBook(toInstall); await().atMost(1, TimeUnit.SECONDS) .untilTrue(jobNameMatch); } + public void testLocalListUpdatedAfterAdd() { + Book mockBook = mock(Book.class); + BooksEvent event = mock(BooksEvent.class); + when(event.getBook()).thenReturn(mockBook); + + bookManager.bookAdded(event); + assertTrue(bookManager.getInstalledBooksList().contains(mockBook)); + } + + /** + * This test requires deep knowledge of how to remove a book in order to test, + * but the Kotlin interface is nice! + */ + public void testLocalListUpdatedAfterRemove() throws BookException { + BookDriver driver = mock(BookDriver.class); + + Book mockBook = mock(Book.class); + when(mockBook.getDriver()).thenReturn(driver); + + BooksEvent event = mock(BooksEvent.class); + when(event.getBook()).thenReturn(mockBook); + + bookManager.getInstalledBooksList().add(mockBook); + assertTrue(bookManager.getInstalledBooksList().contains(mockBook)); + bookManager.removeBook(mockBook); + assertFalse(bookManager.getInstalledBooksList().contains(mockBook)); + verify(driver, times(1)).delete(mockBook); + } + /** * Modules needed for this test case */ - @Module(injects = {BookDownloadManager.class, + @Module(injects = {BookManager.class, RefreshManager.class, - BookDownloadManagerTest.class}) + BookManagerTest.class}) @SuppressWarnings("unused") public static class BookDownloadManagerTestModules { Injector i; @@ -167,8 +201,8 @@ public class BookDownloadManagerTest extends MBTestCase implements Injector { @Provides @Singleton - BookDownloadManager bookDownloadManager(Books installed, RefreshManager rm) { - return new BookDownloadManager(installed, rm); + BookManager bookDownloadManager(Books installed, RefreshManager rm) { + return new BookManager(installed, rm); } } } \ No newline at end of file diff --git a/app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/InstalledManagerTest.java b/app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/InstalledManagerTest.java deleted file mode 100644 index e693e57..0000000 --- a/app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/InstalledManagerTest.java +++ /dev/null @@ -1,162 +0,0 @@ -package org.bspeice.minimalbible.test.activity.downloader.manager; - -import android.net.ConnectivityManager; -import android.net.NetworkInfo; -import android.util.Log; - -import org.bspeice.minimalbible.Injector; -import org.bspeice.minimalbible.MBTestCase; -import org.bspeice.minimalbible.activity.downloader.DownloadPrefs; -import org.bspeice.minimalbible.activity.downloader.manager.BookDownloadManager; -import org.bspeice.minimalbible.activity.downloader.manager.InstalledManager; -import org.bspeice.minimalbible.activity.downloader.manager.RefreshManager; -import org.crosswire.jsword.book.Book; -import org.crosswire.jsword.book.Books; -import org.crosswire.jsword.book.install.InstallManager; -import org.crosswire.jsword.book.install.Installer; -import org.mockito.Mockito; - -import java.util.Collection; -import java.util.List; -import java.util.concurrent.atomic.AtomicBoolean; - -import javax.inject.Inject; -import javax.inject.Singleton; - -import dagger.Module; -import dagger.ObjectGraph; -import dagger.Provides; -import rx.Observable; -import rx.functions.Action1; -import rx.functions.Func1; - -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -/** - * Test the InstalledManager - * Currently due to limitations with JSword (which I'm currently investigating) you can't delete - * books without restarting the application. That is, if you install it, there must be a restart - * in between it being deleted. Unfortunately, that means that this TestCase really can't guarantee - * much, since I can't install a book at runtime to be removed. - */ -public class InstalledManagerTest extends MBTestCase implements Injector { - ObjectGraph mObjectGraph; - @Inject - InstalledManager iM; - @Inject - Books installedBooks; - - @Override - public void inject(Object o) { - mObjectGraph.inject(o); - } - - @Override - public void setUp() { - super.setUp(); - mObjectGraph = ObjectGraph.create(new IMTestModules(this)); - mObjectGraph.inject(this); - - // Unfortunately, unless something is already installed, we can't actually remove anything - int count = getInstalledBooks().count().toBlocking().first(); - - if (count <= 0) { - Log.w("InstalledManagerTest", "No books available, test can not guarantee anything."); - } - } - - public Observable getInstalledBooks() { - /* The golden copy for testing of what's installed. - NOTE: Currently, I have yet to find a guaranteed way to immediately delete - a book that is freshly installed. While the tests are semantically correct, unfortunately, - this test case specifically doesn't guarantee much of anything. - */ - return Observable.from(installedBooks.getBooks()) - .filter(new Func1() { - @Override - public Boolean call(Book book) { - // Not sure why, but this book can't be deleted... - return book.getDriver().isDeletable(book); - } - }); - } - - public void testIsInstalled() throws Exception { - final AtomicBoolean foundMismatch = new AtomicBoolean(false); - getInstalledBooks() - .subscribe(new Action1() { - @Override - public void call(Book book) { - // Skip if we've already found a mismatch - if (!foundMismatch.get()) { - // We've already filtered to what we know is installed, - // so set to true if iM doesn't think it's installed. - foundMismatch.set(!iM.isInstalled(book)); - } - } - }); - assertFalse(foundMismatch.get()); - } - - @Module(injects = {InstalledManager.class, - InstalledManagerTest.class, - RefreshManager.class, - BookDownloadManager.class}) - @SuppressWarnings("unused") - static class IMTestModules { - Injector i; - ConnectivityManager manager; - DownloadPrefs prefs; - public IMTestModules(Injector i) { - this.i = i; - - // Set reasonable defaults for the manager and preferences, can over-ride if need-be - manager = mock(ConnectivityManager.class); - NetworkInfo mockNetworkInfo = Mockito.mock(NetworkInfo.class); - - when(manager.getActiveNetworkInfo()).thenReturn(mockNetworkInfo); - when(mockNetworkInfo.getType()).thenReturn(ConnectivityManager.TYPE_WIFI); - - prefs = mock(DownloadPrefs.class); - } - - @Provides - @Singleton - Injector provideInjector() { - return this.i; - } - - @Provides - @Singleton - Books provideInstalledBooks() { - return Books.installed(); - } - - @Provides - List provideInstalledBooksList(Books b) { - return b.getBooks(); - } - - @Provides - @Singleton - Collection provideInstallers() { - return new InstallManager().getInstallers().values(); - } - - void setConnectivityManager(ConnectivityManager manager) { - this.manager = manager; - } - - void setPrefs(DownloadPrefs prefs) { - this.prefs = prefs; - } - - @Provides - @Singleton - RefreshManager refreshManager(Collection installers) { - return new RefreshManager(installers, - prefs, manager); - } - } -} \ No newline at end of file diff --git a/app/src/main/java/org/bspeice/minimalbible/activity/downloader/BookItemHolder.java b/app/src/main/java/org/bspeice/minimalbible/activity/downloader/BookItemHolder.java index 49167b3..8bbaf25 100644 --- a/app/src/main/java/org/bspeice/minimalbible/activity/downloader/BookItemHolder.java +++ b/app/src/main/java/org/bspeice/minimalbible/activity/downloader/BookItemHolder.java @@ -11,9 +11,8 @@ import com.todddavies.components.progressbar.ProgressWheel; import org.bspeice.minimalbible.Injector; import org.bspeice.minimalbible.R; -import org.bspeice.minimalbible.activity.downloader.manager.BookDownloadManager; +import org.bspeice.minimalbible.activity.downloader.manager.BookManager; import org.bspeice.minimalbible.activity.downloader.manager.DLProgressEvent; -import org.bspeice.minimalbible.activity.downloader.manager.InstalledManager; import org.crosswire.jsword.book.Book; import javax.inject.Inject; @@ -45,9 +44,7 @@ public class BookItemHolder { @InjectView(R.id.download_prg_download) ProgressWheel downloadProgress; @Inject - BookDownloadManager bookDownloadManager; - @Inject - InstalledManager installedManager; + BookManager bookManager; @Inject @Named("DownloadActivityContext") Context ctx; private Subscription subscription; @@ -62,14 +59,14 @@ public class BookItemHolder { public void bindHolder() { acronym.setText(b.getInitials()); itemName.setText(b.getName()); - DLProgressEvent dlProgressEvent = bookDownloadManager.getDownloadProgress(b); + DLProgressEvent dlProgressEvent = bookManager.getDownloadProgress(b); if (dlProgressEvent != null) { displayProgress((int) dlProgressEvent.toCircular()); - } else if (installedManager.isInstalled(b)) { + } else if (bookManager.isInstalled(b)) { displayInstalled(); } //TODO: Refactor - subscription = bookDownloadManager.getDownloadEvents() + subscription = bookManager.getDownloadEvents() .observeOn(AndroidSchedulers.mainThread()) .filter(new Func1() { @Override @@ -91,9 +88,9 @@ public class BookItemHolder { @OnClick(R.id.download_ibtn_download) public void onDownloadItem(View v) { - if (installedManager.isInstalled(b)) { + if (bookManager.isInstalled(b)) { // Remove the book - boolean didRemove = installedManager.removeBook(b); + boolean didRemove = bookManager.removeBook(b); if (didRemove) { isDownloaded.setImageResource(R.drawable.ic_action_download); } else { @@ -101,7 +98,7 @@ public class BookItemHolder { , Toast.LENGTH_SHORT).show(); } } else { - bookDownloadManager.installBook(this.b); + bookManager.installBook(this.b); } } diff --git a/app/src/main/java/org/bspeice/minimalbible/activity/downloader/DownloadActivityModules.java b/app/src/main/java/org/bspeice/minimalbible/activity/downloader/DownloadActivityModules.java index 2c6775d..92c5ecf 100644 --- a/app/src/main/java/org/bspeice/minimalbible/activity/downloader/DownloadActivityModules.java +++ b/app/src/main/java/org/bspeice/minimalbible/activity/downloader/DownloadActivityModules.java @@ -5,8 +5,7 @@ import android.net.ConnectivityManager; import org.bspeice.minimalbible.Injector; import org.bspeice.minimalbible.MinimalBibleModules; -import org.bspeice.minimalbible.activity.downloader.manager.BookDownloadManager; -import org.bspeice.minimalbible.activity.downloader.manager.InstalledManager; +import org.bspeice.minimalbible.activity.downloader.manager.BookManager; import org.bspeice.minimalbible.activity.downloader.manager.LocaleManager; import org.bspeice.minimalbible.activity.downloader.manager.RefreshManager; import org.crosswire.common.util.Language; @@ -34,11 +33,10 @@ import de.devland.esperandro.Esperandro; injects = { BookListFragment.class, BookItemHolder.class, - BookDownloadManager.class, + BookManager.class, RefreshManager.class, DownloadNavDrawerFragment.class, - DownloadActivity.class, - InstalledManager.class + DownloadActivity.class }, addsTo = MinimalBibleModules.class, library = true @@ -77,8 +75,8 @@ public class DownloadActivityModules { } @Provides @Singleton - BookDownloadManager provideBookDownloadManager(Books installedBooks, RefreshManager rm) { - return new BookDownloadManager(installedBooks, rm); + BookManager provideBookDownloadManager(Books installedBooks, RefreshManager rm) { + return new BookManager(installedBooks, rm); } @Provides @Singleton diff --git a/app/src/main/java/org/bspeice/minimalbible/activity/downloader/manager/InstalledManager.java b/app/src/main/java/org/bspeice/minimalbible/activity/downloader/manager/InstalledManager.java deleted file mode 100644 index 1baa8b4..0000000 --- a/app/src/main/java/org/bspeice/minimalbible/activity/downloader/manager/InstalledManager.java +++ /dev/null @@ -1,77 +0,0 @@ -package org.bspeice.minimalbible.activity.downloader.manager; - -import android.util.Log; - -import org.bspeice.minimalbible.Injector; -import org.crosswire.jsword.book.Book; -import org.crosswire.jsword.book.BookException; -import org.crosswire.jsword.book.Books; -import org.crosswire.jsword.book.BooksEvent; -import org.crosswire.jsword.book.BooksListener; - -import java.util.List; - -import javax.inject.Inject; -import javax.inject.Singleton; - -/** - * Manager to keep track of which books have been installed - */ -@Singleton -public class InstalledManager implements BooksListener { - - @Inject Books installedBooks; - // TODO: Why is this injected if we initialize in the constructor? - @Inject List installedBooksList; - - private String TAG = "InstalledManager"; - - @Inject - InstalledManager(Injector injector) { - injector.inject(this); - installedBooksList.addAll(installedBooks.getBooks()); - installedBooks.addBooksListener(this); - } - - public boolean isInstalled(Book b) { - return installedBooksList.contains(b); - } - - @Override - public void bookAdded(BooksEvent booksEvent) { - Log.d(TAG, "Book added: " + booksEvent.getBook().toString()); - Book b = booksEvent.getBook(); - if (!installedBooksList.contains(b)) { - installedBooksList.add(b); - } - } - - @Override - public void bookRemoved(BooksEvent booksEvent) { - Log.d(TAG, "Book removed: " + booksEvent.getBook().toString()); - Book b = booksEvent.getBook(); - if (installedBooksList.contains(b)) { - installedBooksList.remove(b); - } - } - - /** - * Remove a book from being installed. - * Currently only supports books that have been installed outside the current application run. - * Not quite sure why this is, but And-Bible exhibits the same behavior. - * @param b The book to remove - * @return Whether the book was removed. - */ - public boolean removeBook(Book b) { - try { - // This worked in the past, but isn't now... - // installedBooks.remove(b); - Book realBook = installedBooks.getBook(b.getInitials()); - b.getDriver().delete(realBook); - return true; - } catch (BookException e) { - Log.e("InstalledManager", "Unable to remove book (already uninstalled?): " + e.getLocalizedMessage()); - return false; - } - } -} diff --git a/app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/BookDownloadManager.kt b/app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/BookManager.kt similarity index 67% rename from app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/BookDownloadManager.kt rename to app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/BookManager.kt index bcbda3b..8ab85fa 100644 --- a/app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/BookDownloadManager.kt +++ b/app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/BookManager.kt @@ -13,20 +13,29 @@ import org.crosswire.jsword.book.BooksListener; import rx.Observable; import rx.schedulers.Schedulers; import rx.subjects.PublishSubject; +import org.crosswire.jsword.book.BookException +import org.crosswire.jsword.book.remove /** * Single point of authority for what is being downloaded and its progress + * Please note that you should never be modifying installedBooks, + * only operate on installedBooksList */ //TODO: Install indexes for Bibles -class BookDownloadManager(val installedBooks: Books, val rM: RefreshManager) : +class BookManager(private val installedBooks: Books, val rM: RefreshManager) : WorkListener, BooksListener { /** * Cached copy of downloads in progress so views displaying this info can get it quickly. */ // TODO: Combine to one map - var bookMappings: MutableMap = hashMapOf() - var inProgressDownloads: MutableMap = hashMapOf() + val bookMappings: MutableMap = hashMapOf() + val inProgressDownloads: MutableMap = hashMapOf() + + /** + * A list of books that is locally maintained - installedBooks isn't always up-to-date + */ + val installedBooksList: MutableList = installedBooks.getBooks() ?: linkedListOf() val downloadEvents: PublishSubject = PublishSubject.create(); { @@ -41,7 +50,7 @@ class BookDownloadManager(val installedBooks: Books, val rM: RefreshManager) : * @param b The book to predict the download job name of * @return The name of the job that will/is download/ing this book */ - fun getJobId(b: Book) = "INSTALL_BOOK-" + b.getInitials() + fun getJobId(b: Book) = "INSTALL_BOOK-${b.getInitials()}" fun installBook(b: Book) { downloadBook(b) @@ -57,11 +66,37 @@ class BookDownloadManager(val installedBooks: Books, val rM: RefreshManager) : // First, look up where the Book came from Observable.just(rM installerFromBook b) .subscribeOn(Schedulers.io()) - .subscribe { it.toBlocking().first().install(b) } + .subscribe { it.toBlocking().first() install b } downloadEvents onNext DLProgressEvent(DLProgressEvent.PROGRESS_BEGINNING, b) } + /** + * Remove a book from being installed. + * Currently only supports books that have been installed outside the current application run. + * Not quite sure why this is, but And-Bible exhibits the same behavior. + * @param b The book to remove + * @return Whether the book was removed. + */ + fun removeBook(b: Book): Boolean { + try { + b.remove() + return installedBooksList remove b + } catch (e: BookException) { + Log.e("InstalledManager", + "Unable to remove book (already uninstalled?): ${e.getDetailedMessage()}"); + return false; + } + } + /** + * Check the status of a book download in progress. + * @param b The book to get the current progress of + * @return The most recent DownloadProgressEvent for the book, or null if not downloading + */ + fun getDownloadProgress(b: Book) = inProgressDownloads get b + + fun isInstalled(b: Book) = installedBooksList contains b + // TODO: I have a strange feeling I can simplify this further... override fun workProgressed(ev: WorkEvent) { val job = ev.getJob() @@ -79,13 +114,6 @@ class BookDownloadManager(val installedBooks: Books, val rM: RefreshManager) : } } - /** - * Check the status of a book download in progress. - * @param b The book to get the current progress of - * @return The most recent DownloadProgressEvent for the book, or null if not downloading - */ - fun getDownloadProgress(b: Book) = inProgressDownloads.get(b) - override fun workStateChanged(ev: WorkEvent) { Log.d("BookDownloadManager", ev.toString()) } @@ -94,15 +122,18 @@ class BookDownloadManager(val installedBooks: Books, val rM: RefreshManager) : // It's possible the install finished before we received a progress event for it, // we handle that case here. val b = booksEvent.getBook() - Log.d("BookDownloadManager", "Book added: " + b.getName()) + Log.d("BookDownloadManager", "Book added: ${b.getName()}") inProgressDownloads remove b // Not sure why, but the inProgressDownloads might not have our book, // so we always trigger the PROGRESS_COMPLETE event. downloadEvents onNext DLProgressEvent(DLProgressEvent.PROGRESS_COMPLETE, b) + + // And update the locally available list + installedBooksList add b } override fun bookRemoved(booksEvent: BooksEvent) { + installedBooksList remove booksEvent.getBook() } } - diff --git a/app/src/main/kotlin/org/crosswire/jsword/book/BookUtil.kt b/app/src/main/kotlin/org/crosswire/jsword/book/BookUtil.kt index b2db842..2eb64a2 100644 --- a/app/src/main/kotlin/org/crosswire/jsword/book/BookUtil.kt +++ b/app/src/main/kotlin/org/crosswire/jsword/book/BookUtil.kt @@ -25,4 +25,6 @@ fun Book.getVersification(): Versification { fun Book.bookNames(): List = this.getVersification().getBookNames() fun Book.bookName(bBook: BibleBook): String = - this.getVersification().getBookName(bBook).getLongName() \ No newline at end of file + this.getVersification().getBookName(bBook).getLongName() + +fun Book.remove() = this.getDriver().delete(this) \ No newline at end of file