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

Conversation

KJoslyn
Copy link
Contributor

@KJoslyn KJoslyn commented May 20, 2019

To test, please check kj-resources-list-testapp and run main in ResourceCardApp.kt. This branch is a separate entry point to load a single chapter using the workbook repository.

**UPDATE 6/3/19: This PR once again depends on #100 in common.

@@ -129,6 +129,9 @@ dependencies {
implementation 'org.wycliffeassociates:8woc2018-common'
implementation 'org.wycliffeassociates:kotlin-resource-container'
implementation 'com.github.WycliffeAssociates:jdenticon-kotlin:-SNAPSHOT'

//Atlassian commonmark (for rendering markdown)
implementation 'com.atlassian.commonmark:commonmark:0.12.1'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uses the BSD 2-clause license. Should be fine, but I'm not sure where/how we're keeping track of licenses.

@KJoslyn KJoslyn requested review from aunger and natibekele May 20, 2019 16:22
private fun getTitleTextContent(): 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.


fun loadResourceGroups() {
chapter.chunks.map { chunk ->
resourceGroupCardItem(chunk, resourceSlug) { navigateToTakesPage(it) }
Copy link
Contributor

Choose a reason for hiding this comment

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

RxJava2 Observables will crash on null elements. resourceGroupCardItem can return nulls, so guard against it. Maybe you can think of a better way, but I took a crack at it since it seemed fun:

fun <T, R> Observable<T>.mapNotNull(f: (T) -> R?): Observable<R> =
    concatMapMaybe { Maybe.fromCallable { f(it) } }

fun loadResourceGroups() {
    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
        .subscribe {
            Platform.runLater {
                resourceGroups.addAll(it)
            }
        }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better than Maybe :

fun <T, R> Observable<T>.mapNotNull(f: (T) -> R?): Observable<R> =
    concatMapIterable { listOfNotNull(f(it)) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second one requires that R extends Any for the compiler to accept, but I might still prefer that one.

@@ -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.


val 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.


@Suppress("ProtectedInFinal", "Unused")
protected fun finalize() {
clearDisposables()
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you try putting a print statement here and seeing if this actually executes? I'm kinda concerned about the subscribe reference not allowing this to be marked for GC?

Copy link
Contributor Author

@KJoslyn KJoslyn May 29, 2019

Choose a reason for hiding this comment

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

This does in fact execute when the ResourceGroupCardItems are removed from the observable list, even when the ResourceCardItems have subscriptions to AssociatedAudio.selected. Since this is the case (and I should've realized this sooner), I suppose I don't need any of the additional logic that I added to manually clear the subscriptions. However, this was only tested when the ResourceGroupCardItem list is cleared programmatically- I don't know how to test it if the list is not cleared, i.e. how do I just drop the reference to the list or the ViewModel?

Edit: Although there may be a way to test this by undocking the component that is dependent on the ViewModel (maybe), I feel fairly confident that finalize() would run, since clearing the list of ResourceGroupCardItems (and effectively dropping the references to the ResourceGroupCardItems) caused finalize() to be called on the associated ResourceCardItems when they were GC'd. I can still attempt to test if this is desired.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just read through the resolved discussion and realized that what makes me uncomfortable here is that clearDisposables() needs to be tied to a lifecycle API, and as you both discussed, onDock/onUndock are good lifecycle places to attach this to. In the absence of one, you're attaching to the GC lifecycle whether consciously or unconsciously.

The CardItem here isn't exactly a framework object, and as such has no APIs to make use of- so this functionality would have to be exposed as a public method here. I'm going to look and see if ListView has some kind of lifecycle APIs to clean cells that may be more appropriate?

.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.

Copy link
Collaborator

@jsarabia jsarabia left a comment

Choose a reason for hiding this comment

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

Please move the util function out and take a look at the style comment and you should be good to go after that

@jsarabia jsarabia merged commit a61913f into rba-move-workbook-to-common Jun 3, 2019
@jsarabia jsarabia deleted the kj-resources-list branch June 3, 2019 20:40
@jsarabia jsarabia restored the kj-resources-list branch June 4, 2019 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants