Remove the InstalledManager

Largely duplicated work, and makes testing easier!

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<DLProgressEvent>() {
                     @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) {
+        BookManager bookDownloadManager(Books installed, RefreshManager rm) {
-            return new BookDownloadManager(installed, rm);
+            return new BookManager(installed, rm);
         }
     }
 }

app/src/androidTest/java/org/bspeice/minimalbible/test/activity/downloader/manager/InstalledManagerTest.java

@@ -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<Book> 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<Book, Boolean>() {
-                    @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<Book>() {
-                    @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<Book> provideInstalledBooksList(Books b) {
-            return b.getBooks();
-        }
-
-        @Provides
-        @Singleton
-        Collection<Installer> 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<Installer> installers) {
-            return new RefreshManager(installers,
-                    prefs, manager);
-        }
-    }
-}

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;
+    BookManager bookManager;
-    @Inject
-    InstalledManager installedManager;
     @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<DLProgressEvent, Boolean>() {
                     @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);
         }
     }
 

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.BookManager;
-import org.bspeice.minimalbible.activity.downloader.manager.InstalledManager;
 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,
+                DownloadActivity.class
-                InstalledManager.class
         },
         addsTo = MinimalBibleModules.class,
         library = true
@@ -77,8 +75,8 @@ public class DownloadActivityModules {
     }
 
     @Provides @Singleton
-    BookDownloadManager provideBookDownloadManager(Books installedBooks, RefreshManager rm) {
+    BookManager provideBookDownloadManager(Books installedBooks, RefreshManager rm) {
-        return new BookDownloadManager(installedBooks, rm);
+        return new BookManager(installedBooks, rm);
     }
 
     @Provides @Singleton

app/src/main/java/org/bspeice/minimalbible/activity/downloader/manager/InstalledManager.java

@@ -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<Book> 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;
-        }
-    }
-}

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<String, Book> = hashMapOf()
+    val bookMappings: MutableMap<String, Book> = hashMapOf()
-    var inProgressDownloads: MutableMap<Book, DLProgressEvent> = hashMapOf()
+    val inProgressDownloads: MutableMap<Book, DLProgressEvent> = hashMapOf()
+
+    /**
+     * A list of books that is locally maintained - installedBooks isn't always up-to-date
+     */
+    val installedBooksList: MutableList<Book> = installedBooks.getBooks() ?: linkedListOf()
     val downloadEvents: PublishSubject<DLProgressEvent> = 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()
     }
 }
-

app/src/main/kotlin/org/crosswire/jsword/book/BookUtil.kt

@@ -26,3 +26,5 @@ fun Book.bookNames(): List<String> = this.getVersification().getBookNames()
 
 fun Book.bookName(bBook: BibleBook): String =
         this.getVersification().getBookName(bBook).getLongName()
+
+fun Book.remove() = this.getDriver().delete(this)