Skip to content
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.

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

Loading resources from workbook #453

Merged
merged 18 commits into from
Jun 3, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ class ResourceListFragment : Fragment() {

add(
workbookheader {
labelText = viewModel.chapter.title + " Resources"
filterText = "Hide Completed"
labelText = "${viewModel.chapter.title} ${messages["resources"]}"
aunger marked this conversation as resolved.
Show resolved Hide resolved
filterText = messages["hideCompleted"]
}
)
add(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
package org.wycliffeassociates.otter.jvm.app.ui.resources.viewmodel

import io.reactivex.Observable
import javafx.application.Platform
import javafx.beans.property.SimpleObjectProperty
import javafx.beans.property.SimpleStringProperty
import javafx.collections.FXCollections
import javafx.collections.ObservableList
import org.wycliffeassociates.otter.common.data.workbook.*
import org.wycliffeassociates.otter.jvm.app.widgets.resourcecard.model.ResourceCardItem
import org.wycliffeassociates.otter.jvm.app.widgets.resourcecard.model.ResourceGroupCardItem
import org.wycliffeassociates.otter.jvm.app.widgets.resourcecard.model.ResourceGroupCardItemList
import org.wycliffeassociates.otter.jvm.app.widgets.resourcecard.model.resourceGroupCardItem
import tornadofx.*

Expand All @@ -24,18 +22,22 @@ class ResourcesViewModel : ViewModel() {
val resourceSlug: String
get() = activeResourceSlugProperty.value

var resourceGroups: ObservableList<ResourceGroupCardItem> = FXCollections.observableArrayList()
var resourceGroups: ResourceGroupCardItemList = ResourceGroupCardItemList(mutableListOf())

fun <T, R:Any> Observable<T>.mapNotNull(f: (T) -> R?): Observable<R> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should probably just be moved out to its own file in a utils package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where would you like to see this go? Should a utils package be made in common or does it need to be under another package such as observable > utils?

Copy link
Collaborator

Choose a reason for hiding this comment

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

put it in common.utils

maybe like RxUtils.kt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

@KJoslyn If it's a util, I guess it should have kdoc:

/**
 * Apply the given function as with `map()`, but remove any resulting nulls from the
 * observable stream.
 * 
 * This is how `observable.map(f).filter { it != null }` should behave, but it keeps
 * nulls from ever appearing in an Observable, which would cause a crash in that
 * two-step map/filter example.
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks! Just added that.

concatMapIterable { listOfNotNull(f(it)) }

fun loadResourceGroups() {
chapter.chunks.map { chunk ->
resourceGroupCardItem(chunk, resourceSlug) { navigateToTakesPage(it) }
}.startWith(
resourceGroupCardItem(chapter, resourceSlug) { navigateToTakesPage(it) }
).buffer(2).subscribe { // Buffering by 2 prevents the list UI from jumping while groups are loading
Platform.runLater {
resourceGroups.addAll(it)
chapter
.children
.startWith(chapter)
.mapNotNull { resourceGroupCardItem(it, resourceSlug, onSelect = this::navigateToTakesPage) }
.buffer(2) // Buffering by 2 prevents the list UI from jumping while groups are loading
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if there's an odd number of resource groups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the source observable completes, the observable will emit whatever is in the current buffer, so it won't matter if there's an odd number of resource groups.

.subscribe {
Platform.runLater {
resourceGroups.addAll(it)
}
}
}
}

private fun navigateToTakesPage(resource: Resource) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.wycliffeassociates.otter.jvm.app.widgets.resourcecard.model

import io.reactivex.disposables.CompositeDisposable
import javafx.beans.property.DoubleProperty
import javafx.beans.property.SimpleDoubleProperty
import org.wycliffeassociates.otter.common.data.workbook.AssociatedAudio
Expand All @@ -8,16 +9,22 @@ import org.commonmark.parser.Parser
import org.commonmark.renderer.text.TextContentRenderer

data class ResourceCardItem(val resource: Resource, val onSelect: () -> Unit) {
val title: String = getTitleTextContent()
val title: String = renderTitleAsPlainText()
private val disposables = CompositeDisposable()
val titleProgressProperty: DoubleProperty = resource.titleAudio.progressProperty()
val bodyProgressProperty: DoubleProperty? = resource.bodyAudio?.progressProperty()
val hasBodyAudio: Boolean = resource.bodyAudio != null

fun clearDisposables() {
disposables.clear()
}

private fun AssociatedAudio.progressProperty(): DoubleProperty {
val progressProperty = SimpleDoubleProperty(0.0)
this.selected.subscribe {
val sub = this.selected.subscribe {
progressProperty.set( if (it.value != null) 1.0 else 0.0)
}
disposables.add(sub)
return progressProperty
aunger marked this conversation as resolved.
Show resolved Hide resolved
}

Expand All @@ -26,7 +33,7 @@ data class ResourceCardItem(val resource: Resource, val onSelect: () -> Unit) {
val renderer: TextContentRenderer = TextContentRenderer.builder().build()
}

private fun getTitleTextContent(): String {
private fun renderTitleAsPlainText(): String {
val document = parser.parse(resource.title.text)
return renderer.render(document)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really a getter, so I'd rename this to something like "renderTitleAsHtml".

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not needed yet, but the second time you need to transform MD to HTML, you should probably pull this commonmark stuff out into an dagger-injectable MarkdownToHtml singleton.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. I'm actually just rendering the title as plain text though and saving html rendering for the body when it appears on the takes page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SarabiaJ I would like to talk to you about dagger-injectable components like this, as opposed to an object in common, like ParseMd.kt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to keep dagger to injection in the framework classes (Android Activities/Fragments, TornadoFX Views/ Fragments, maybe viewmodels)

for a component like this, I'd rather you just pass the dependency into the constructor rather than make dagger do it for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. In this case, it seems like this can be solved without passing a dependency into the constructor. I think I can just create a MarkdownToHtml object like ParseMd that basically just holds static functions to do what I need.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,19 @@ package org.wycliffeassociates.otter.jvm.app.widgets.resourcecard.model

import io.reactivex.Observable
import org.wycliffeassociates.otter.common.data.workbook.*
import tornadofx.*
import tornadofx.FX.Companion.messages

data class ResourceGroupCardItem(
val title: String,
val resources: Observable<ResourceCardItem>
)
) {
fun onRemove() {
resources.forEach {
it.clearDisposables()
}
}
}

fun resourceGroupCardItem(element: BookElement, slug: String, onSelect: (Resource) -> Unit): ResourceGroupCardItem? {
return findResourceGroup(element, slug)?.let { rg ->
Expand All @@ -25,8 +33,8 @@ private fun findResourceGroup(element: BookElement, slug: String): ResourceGroup

private fun getGroupTitle(element: BookElement): String {
return when (element) {
is Chapter -> "Chapter " + element.title
is Chunk -> "Chunk " + element.title
is Chapter -> "${messages["chapter"]} ${element.title}"
is Chunk -> "${messages["chunk"]} ${element.title}"
else -> element.title
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package org.wycliffeassociates.otter.jvm.app.widgets.resourcecard.model

import com.sun.javafx.collections.ObservableListWrapper
import javafx.collections.ListChangeListener

class ResourceGroupCardItemList(list: List<ResourceGroupCardItem>) :
ObservableListWrapper<ResourceGroupCardItem>(list) {

init {
addListener(
ListChangeListener<ResourceGroupCardItem> {
while(it.next()) {
if (it.wasRemoved()) {
it.removed.forEach { item ->
item.onRemove()
}
}
}
}
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ import org.wycliffeassociates.otter.jvm.app.widgets.resourcecard.model.ResourceG
import tornadofx.*

class ResourceGroupCard(group: ResourceGroupCardItem) : VBox() {
private val RENDER_BATCH_SIZE = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Can make this a const if it's at the file level or in the companion object, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I like it in a companion object since it is specific to the resource group card.

init {
importStylesheet<ResourceGroupCardStyles>()

addClass(ResourceGroupCardStyles.resourceGroupCard)
label(group.title)

group.resources.buffer(10).subscribe { items ->
group.resources.buffer(RENDER_BATCH_SIZE).subscribe { items ->
Platform.runLater {
items.forEach {
add(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,58 +89,40 @@ class ResourceRepository(private val database: AppDatabase) : IResourceRepositor
.map(this::buildResourceInfo)
}

/**
* Returns all resources for which the resource content's COLLECTION_FK field references this collection.
* This will return resources about the chapter as well as all resources about the chapter's chunks.
*/
override fun getResourcesForCollectionAndChildren(
collection: Collection,
resourceInfo: ResourceInfo
): Observable<Content> {
return getResources({ table -> table.COLLECTION_FK.eq(collection.id) }, resourceInfo)
}

override fun getResources(content: Content, resourceInfo: ResourceInfo): Observable<Content> {
return getResources({ table -> table.ID.eq(content.id) }, resourceInfo)
}

/**
* Returns collection-specific resources
*/
override fun getResources(collection: Collection, resourceInfo: ResourceInfo): Observable<Content> {
val metadata = mapToResourceMetadataEntity[resourceInfo]
?: return Observable.empty()

val main = CONTENT_ENTITY.`as`("main")
val help = CONTENT_ENTITY.`as`("help")

val selectStatement = database.dsl
.selectDistinct(help.asterisk())
.from(RESOURCE_LINK)
.join(COLLECTION_ENTITY).on(COLLECTION_ENTITY.ID.eq(RESOURCE_LINK.COLLECTION_FK))
.join(help).on(RESOURCE_LINK.RESOURCE_CONTENT_FK.eq(help.ID))
.join(main).on(main.ID.eq(RESOURCE_LINK.CONTENT_FK))
.join(help).on(help.ID.eq(RESOURCE_LINK.RESOURCE_CONTENT_FK))
.where(RESOURCE_LINK.DUBLIN_CORE_FK.eq(metadata.id))
.and(COLLECTION_ENTITY.ID.eq(collection.id))
.and(main.ID.eq(content.id))

return getResources(help, selectStatement)
}

aunger marked this conversation as resolved.
Show resolved Hide resolved
private fun getResources(
condition: (jooq.tables.ContentEntity) -> Condition,
resourceInfo: ResourceInfo
): Observable<Content> {
/**
* Returns collection-specific resources (does not return resources about the collection's children.)
*/
override fun getResources(collection: Collection, resourceInfo: ResourceInfo): Observable<Content> {
val metadata = mapToResourceMetadataEntity[resourceInfo]
?: return Observable.empty()

val main = CONTENT_ENTITY.`as`("main")
val help = CONTENT_ENTITY.`as`("help")

val selectStatement = database.dsl
.selectDistinct(help.asterisk())
.from(RESOURCE_LINK)
.join(main).on(main.ID.eq(RESOURCE_LINK.CONTENT_FK))
.join(help).on(help.ID.eq(RESOURCE_LINK.RESOURCE_CONTENT_FK))
.join(COLLECTION_ENTITY).on(COLLECTION_ENTITY.ID.eq(RESOURCE_LINK.COLLECTION_FK))
.join(help).on(RESOURCE_LINK.RESOURCE_CONTENT_FK.eq(help.ID))
.where(RESOURCE_LINK.DUBLIN_CORE_FK.eq(metadata.id))
.and(condition(main))
.and(COLLECTION_ENTITY.ID.eq(collection.id))

return getResources(help, selectStatement)
}
Expand Down
3 changes: 3 additions & 0 deletions src/main/resources/Messages_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ viewTakes =VIEW TAKES
viewRecordings = VIEW RECORDINGS
edit = EDIT
chapter = Chapter
chunk = Chunk
verse=Verse
take=Take
select = Select
Expand Down Expand Up @@ -74,3 +75,5 @@ selectBook = Select a Book
home = Home
profile = Profile
settings = Settings
resources = Resources
hideCompleted = Hide Completed