From bcebe86926e1fe78ae3f531e07287631c9ddf2aa Mon Sep 17 00:00:00 2001 From: Bradlee Speice Date: Mon, 15 Dec 2014 00:15:42 -0500 Subject: [PATCH] Only show languages for selected book category Previously displayed all languages, period. Also includes some testing updates to make sure everything is still covered. --- app-test/build.gradle | 33 +++++----- .../downloader/manager/LocaleManagerTest.java | 46 ------------- .../manager/DLProgressEventSpek.kt} | 2 +- .../downloader/manager/LocaleManagerSpek.kt | 66 +++++++++++++++++++ app/src/debug/res/values/strings.xml | 2 - .../activity/downloader/BookListFragment.java | 20 ++++-- .../downloader/DownloadActivityModules.java | 35 +++++----- .../downloader/manager/LocaleManager.kt | 61 +++++++++-------- 8 files changed, 155 insertions(+), 110 deletions(-) delete mode 100644 app-test/src/test/java/org/bspeice/minimalbible/test/activity/downloader/manager/LocaleManagerTest.java rename app-test/src/test/kotlin/{DLProgressEventTest.kt => org/bspeice/minimalbible/activity/downloader/manager/DLProgressEventSpek.kt} (94%) create mode 100644 app-test/src/test/kotlin/org/bspeice/minimalbible/activity/downloader/manager/LocaleManagerSpek.kt diff --git a/app-test/build.gradle b/app-test/build.gradle index a0f3170..cf8696d 100644 --- a/app-test/build.gradle +++ b/app-test/build.gradle @@ -28,15 +28,16 @@ def firstVariant = androidModule.android.applicationVariants.toList().first() // TODO: Not yet including Spek tests, fix that. def testIncludes = [ - '**/*Test.class' + '**/*Test.class', + '**/*Spek.class' ] def jacocoExcludes = [ 'android/**', 'com/todddavies/**', - 'com/cmwmobile/**', - 'org/bspeice/minimalbible/R*', + 'com/cmwmobile/**', + 'org/bspeice/minimalbible/R*', '**/BookAdapter$ChapterInfo*', - '**/*$$*' // Dagger/Butterknife + '**/*$$*' // Dagger/Butterknife ] dependencies { @@ -54,24 +55,24 @@ dependencies { } def buildExcludeTree(path, excludes) { - def tree = fileTree(path).exclude(excludes) - tree + def tree = fileTree(path).exclude(excludes) + tree } jacocoTestReport { doFirst { - // First we build a list of our base directories - def fileList = new ArrayList() + // First we build a list of our base directories + def fileList = new ArrayList() def outputsList = firstVariant.javaCompile.outputs.files - outputsList.each { fileList.add(it.absolutePath.toString()) } + outputsList.each { fileList.add(it.absolutePath.toString()) } - // And build a fileTree from those - def outputTree = fileList.inject { tree1, tree2 -> - buildExcludeTree(tree1, jacocoExcludes) + - buildExcludeTree(tree2, jacocoExcludes) - } - - // And finally tell Jacoco to only include said files in the report + // And build a fileTree from those + def outputTree = fileList.inject { tree1, tree2 -> + buildExcludeTree(tree1, jacocoExcludes) + + buildExcludeTree(tree2, jacocoExcludes) + } + + // And finally tell Jacoco to only include said files in the report // For whatever reason, firstVariant.javaCompile.exclude(jacocoExcludes) doesn't work... classDirectories = outputTree sourceDirectories = files(androidModule.android.sourceSets.main.java.srcDirs) diff --git a/app-test/src/test/java/org/bspeice/minimalbible/test/activity/downloader/manager/LocaleManagerTest.java b/app-test/src/test/java/org/bspeice/minimalbible/test/activity/downloader/manager/LocaleManagerTest.java deleted file mode 100644 index a95ccab..0000000 --- a/app-test/src/test/java/org/bspeice/minimalbible/test/activity/downloader/manager/LocaleManagerTest.java +++ /dev/null @@ -1,46 +0,0 @@ -package org.bspeice.minimalbible.test.activity.downloader.manager; - -import org.bspeice.minimalbible.activity.downloader.manager.LocaleManager; -import org.crosswire.common.util.Language; -import org.junit.Test; - -import java.util.List; - -import rx.Observable; - -import static org.junit.Assert.assertTrue; - -/** - * Test cases for the Locale Manager - */ -public class LocaleManagerTest { - - @Test - public void testSortedLanguagesList() { - Language english = new Language("en"); - Language russian = new Language("ru"); - Language french = new Language("fr"); - Language german = new Language("de"); - Language hebrew = new Language("he"); - Language afrikaans = new Language("af"); - - Observable languages = Observable.just(english, russian, french, - german, hebrew, afrikaans); - - LocaleManager.Core core = LocaleManager.Core.INSTANCE$; - - //noinspection ConstantConditions - List sortedLanguages = core.sortedLanguagesList(languages, english) - .toBlocking().first(); - - // First language should be the 'current' (note this is an identity compare) - assertTrue(sortedLanguages.get(0) == english); - // Second language should be 'less than' third - assertTrue(sortedLanguages.toString(), - sortedLanguages.get(1).toString().compareTo( - sortedLanguages.get(2).toString()) < 0); - // Fifth language should be greater than the fourth - assertTrue(sortedLanguages.toString(), sortedLanguages.get(4).toString().compareTo( - sortedLanguages.get(3).toString()) > 0); - } -} diff --git a/app-test/src/test/kotlin/DLProgressEventTest.kt b/app-test/src/test/kotlin/org/bspeice/minimalbible/activity/downloader/manager/DLProgressEventSpek.kt similarity index 94% rename from app-test/src/test/kotlin/DLProgressEventTest.kt rename to app-test/src/test/kotlin/org/bspeice/minimalbible/activity/downloader/manager/DLProgressEventSpek.kt index dbad40e..8cfc34c 100644 --- a/app-test/src/test/kotlin/DLProgressEventTest.kt +++ b/app-test/src/test/kotlin/org/bspeice/minimalbible/activity/downloader/manager/DLProgressEventSpek.kt @@ -8,7 +8,7 @@ import org.mockito.Mockito.mock import org.jetbrains.spek.api.Spek import kotlin.test.assertEquals -class DLProgressEventSpecs : Spek() {{ +class DLProgressEventSpek : Spek() {{ given("a DLProgressEvent created with 50% progress and a mock book") { val mockBook = mock(javaClass()) diff --git a/app-test/src/test/kotlin/org/bspeice/minimalbible/activity/downloader/manager/LocaleManagerSpek.kt b/app-test/src/test/kotlin/org/bspeice/minimalbible/activity/downloader/manager/LocaleManagerSpek.kt new file mode 100644 index 0000000..21007d5 --- /dev/null +++ b/app-test/src/test/kotlin/org/bspeice/minimalbible/activity/downloader/manager/LocaleManagerSpek.kt @@ -0,0 +1,66 @@ +package org.bspeice.minimalbible.activity.downloader.manager + +import org.jetbrains.spek.api.Spek +import org.crosswire.common.util.Language + +/** + * Created by bspeice on 12/14/14. + */ + +class LocaleManagerSpek() : Spek() {{ + + given("some example language objects") { + val english = Language("en") + val russian = Language("ru") + val french = Language("fr"); + + on("sorting between english and russian with current as english") { + val result = LocaleManager.compareLanguages(english, russian, english) + + it("should prioritize english") { + assert(result < 0) + } + } + + on("sorting between russian and english with current as english") { + val result = LocaleManager.compareLanguages(russian, english, english) + + it("should prioritize english") { + assert(result > 0) + } + } + + on("sorting between russian and english with current as french") { + val result = LocaleManager.compareLanguages(russian, english, french) + + it("should inform us that russian is greater") { + assert(result > 0) + } + } + + on("sorting between english and russian with current as french") { + val result = LocaleManager.compareLanguages(english, russian, french) + + it("should inform us that english is lesser") { + assert(result < 0) + } + } + + on("comparing the same languages with current language as the language being compared") { + val result = LocaleManager.compareLanguages(english, english, english) + + it("should report that the languages are duplicate") { + assert(result == 0) + } + } + + on("comparing the same languages with current language as something different") { + val result = LocaleManager.compareLanguages(english, english, russian) + + it("should report that the languages are duplicate") { + assert(result == 0) + } + } + } +} +} \ No newline at end of file diff --git a/app/src/debug/res/values/strings.xml b/app/src/debug/res/values/strings.xml index e19613e..19f16b4 100644 --- a/app/src/debug/res/values/strings.xml +++ b/app/src/debug/res/values/strings.xml @@ -1,5 +1,3 @@ FragmentTestActivity - Hello world! - Settings diff --git a/app/src/main/java/org/bspeice/minimalbible/activity/downloader/BookListFragment.java b/app/src/main/java/org/bspeice/minimalbible/activity/downloader/BookListFragment.java index 63634cf..34a3c9f 100644 --- a/app/src/main/java/org/bspeice/minimalbible/activity/downloader/BookListFragment.java +++ b/app/src/main/java/org/bspeice/minimalbible/activity/downloader/BookListFragment.java @@ -15,6 +15,7 @@ import android.widget.Toast; import org.bspeice.minimalbible.Injector; import org.bspeice.minimalbible.R; import org.bspeice.minimalbible.activity.BaseFragment; +import org.bspeice.minimalbible.activity.downloader.manager.LocaleManager; import org.bspeice.minimalbible.activity.downloader.manager.RefreshManager; import org.crosswire.common.util.Language; import org.crosswire.jsword.book.Book; @@ -36,18 +37,21 @@ import rx.functions.Func1; import rx.functions.Func2; /** - * A placeholder fragment containing a simple view. + * A fragment to list out the books available for downloading. + * Each fragment will be responsible for a single category, + * another fragment will be created if a second category is needed. */ public class BookListFragment extends BaseFragment { protected static final String ARG_BOOK_CATEGORY = "book_category"; + protected BookCategory bookCategory; @Inject DownloadPrefs downloadPrefs; @Inject RefreshManager refreshManager; @Inject - List availableLanguages; + LocaleManager localeManager; @InjectView(R.id.lst_download_available) ListView downloadsAvailable; @@ -56,6 +60,9 @@ public class BookListFragment extends BaseFragment { LayoutInflater inflater; + // A cache of the languages currently available for this category + List availableLanguages; + /** * Returns a new instance of this fragment for the given section number. * TODO: Switch to AutoFactory/@Provides rather than inline creation. @@ -72,6 +79,9 @@ public class BookListFragment extends BaseFragment { public void onCreate(Bundle state) { super.onCreate(state); ((Injector)getActivity()).inject(this); + + bookCategory = BookCategory.fromString(getArguments().getString(ARG_BOOK_CATEGORY)); + availableLanguages = localeManager.sortedLanguagesForCategory(bookCategory); } @Override @@ -122,7 +132,8 @@ public class BookListFragment extends BaseFragment { void displayLanguageSpinner() { ArrayAdapter adapter = new ArrayAdapter<>(this.getActivity(), android.R.layout.simple_spinner_item, - availableLanguages.toArray()); + availableLanguages.toArray() + ); adapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item); languagesSpinner.setAdapter(adapter); @@ -138,7 +149,7 @@ public class BookListFragment extends BaseFragment { public void onClick(final int position) { booksByLanguage(refreshManager.getFlatModules(), availableLanguages.get(position), - BookCategory.fromString(getArguments().getString(ARG_BOOK_CATEGORY))) + bookCategory) // Repack all the books .toSortedList(new Func2() { @Override @@ -157,6 +168,7 @@ public class BookListFragment extends BaseFragment { }); } + // TODO: Refactor out, this information should come from LocaleManager protected Observable booksByLanguage(Observable books, final Language language, final BookCategory category) { return books 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 92c5ecf..466469f 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 @@ -8,7 +8,6 @@ import org.bspeice.minimalbible.MinimalBibleModules; 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; import org.crosswire.jsword.book.Book; import org.crosswire.jsword.book.BookCategory; import org.crosswire.jsword.book.Books; @@ -49,17 +48,20 @@ public class DownloadActivityModules { this.activity = activity; } - @Provides @Singleton + @Provides + @Singleton DownloadPrefs provideDownloadPrefs() { return Esperandro.getPreferences(DownloadPrefs.class, activity); } - @Provides @Singleton + @Provides + @Singleton DownloadActivity provideDownloadActivity() { return activity; } - @Provides @Singleton + @Provides + @Singleton Injector provideActivityInjector() { return activity; } @@ -67,19 +69,24 @@ public class DownloadActivityModules { /** * Provide the context for the DownloadActivity. We name it so that we don't have to * \@Provides a specific class, but can keep track of what exactly we mean by "Context" + * * @return The DownloadActivity Context */ - @Provides @Singleton @Named("DownloadActivityContext") + @Provides + @Singleton + @Named("DownloadActivityContext") Context provideActivityContext() { return activity; } - @Provides @Singleton + @Provides + @Singleton BookManager provideBookDownloadManager(Books installedBooks, RefreshManager rm) { return new BookManager(installedBooks, rm); } - @Provides @Singleton + @Provides + @Singleton @Named("ValidCategories") List provideValidCategories() { return new ArrayList() {{ @@ -91,7 +98,8 @@ public class DownloadActivityModules { } //TODO: Move this to a true async - @Provides @Singleton + @Provides + @Singleton Books provideInstalledBooks() { return Books.installed(); } @@ -101,12 +109,14 @@ public class DownloadActivityModules { return b.getBooks(); } - @Provides @Singleton + @Provides + @Singleton Collection provideInstallers() { return new InstallManager().getInstallers().values(); } - @Provides @Singleton + @Provides + @Singleton RefreshManager provideRefreshManager(Collection installers, DownloadPrefs prefs, @Named("DownloadActivityContext") Context context) { return new RefreshManager(installers, prefs, @@ -117,9 +127,4 @@ public class DownloadActivityModules { LocaleManager provideLocaleManager(RefreshManager refreshManager) { return new LocaleManager(refreshManager); } - - @Provides - List availableLanguages(LocaleManager localeManager) { - return localeManager.getSortedLanguagesList(); - } } diff --git a/app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/LocaleManager.kt b/app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/LocaleManager.kt index dd51bfd..edd2a71 100644 --- a/app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/LocaleManager.kt +++ b/app/src/main/kotlin/org/bspeice/minimalbible/activity/downloader/manager/LocaleManager.kt @@ -2,43 +2,52 @@ package org.bspeice.minimalbible.activity.downloader.manager import org.crosswire.common.util.Language import rx.Observable -import rx.observables.GroupedObservable +import org.crosswire.jsword.book.BookCategory +import kotlin.platform.platformStatic +/** + * Took me a significant amount of time, but this is an implementation I can live with. + * An ideal solution would be able to group by the category first, then language, with all + * modules underneath, so something like Map>>. + * That said, trying to build said map is a bit ridiculous. The way I wrote it requires + * using functions instead of cached values, but I'll get over it. + */ class LocaleManager(val rM: RefreshManager) { val currentLanguage = Language.DEFAULT_LANG - private val languageModuleMap = rM.flatModules - // Language doesn't have hashCode(), so we actually group by its String - .groupBy { FixedLanguage(it.getLanguage()) } + // Get all modules grouped by language first + val modulesByCategory = rM.flatModules.groupBy { it.getBookCategory() } - // I would suppress the warning here if I could figure out how... - val modulesByLanguage = languageModuleMap - .map { GroupedObservable.from(it.getKey(): Language, it) } + fun languagesForCategory(cat: BookCategory): Observable = modulesByCategory + // Then filter according to the requested language + .filter { it.getKey() == cat } + // Then map the GroupedObservable Book element to its actual language + .flatMap { it.map { it.getLanguage() } } + // Making sure to discard anything with a null language + .filter { it != null } + // And remove duplicates. The flatMap above means that we will have one entry + // for each book, so we need to remove duplicate entries of + // languages with more than one book to them + .distinct() - // Cast back to the original Language implementation - val availableLanguages: Observable = languageModuleMap.map { it.getKey() } - val sortedLanguagesList = - Core.sortedLanguagesList(availableLanguages, currentLanguage).toBlocking().first() + fun sortedLanguagesForCategory(cat: BookCategory): List = + languagesForCategory(cat) + // Finally, sort all languages, prioritizing the current + .toSortedList { left, right -> compareLanguages(left, right, currentLanguage) } + // And flatten this into the actual List needed + .toBlocking().first() - object Core { - fun sortedLanguagesList(availableLanguages: Observable, - currentLanguage: Language) = - availableLanguages.toSortedList {(left, right) -> - // Prioritize our current language first - if (left.getName() == currentLanguage.getName()) + class object { + platformStatic + fun compareLanguages(left: Language, right: Language, current: Language) = + if (left == right) + 0 + else if (left.getName() == current.getName()) -1 - else if (right.getName() == currentLanguage.getName()) + else if (right.getName() == current.getName()) 1 else left.getName() compareTo right.getName() - } - } - - // TODO: Fix the actual Language implementation - Pull Request? - // Can't use a data class because we need to get the name of the language - private class FixedLanguage(language: Language?) : - Language(language?.getCode() ?: Language.UNKNOWN_LANG_CODE) { - override fun hashCode() = this.getName().hashCode() } } \ No newline at end of file