Kj recordable view refactor

[KJoslyn]

Recording, deleting, and selecting takes works on both the scripture and resource side.

Requires common #115.

To test, checkout branch kj-all-recordings-testapp. First, you will need to run Main.kt to import ulb and tn, if you haven't already. Then create a project and add an audio plugin.

Then, run

• main in RecordScriptureTestApp.kt to work with takes on the scripture side
• main in RecordResourceTestApp.kt to work with takes on the resource side

If you use these entry points, the app will load the first project, first chapter in that project, and first verse in that chapter. (Navigating strictly down through the app rather than using these separate entry points should work, but I created these entry points to make it faster to test the takes pages alone.)

Try performing record, delete and select (by dragging). Editing takes does not work yet due to an unknown issue with ocenaudio being unable to save edited files.

The most important part of this PR is the creation of a base class called RecordableFragment that handles all of the commonalities between the derived classes RecordScriptureFragment and RecordResourceFragment, namely dragging. Other view and viewmodels have also been renamed for clarity.

Also, I elected to keep the FlowPane as a FlowPane in RecordScriptureFragment. In lieu of finding any helpful examples of lists (particularly grids) with different data/node types, (most people were just talking about using different data types in a TableView, which doesn't solve our problem) I did some investigation:

1. Datagrid is parameterized by a model type- we would have to create a new umbrella type to encompass the three types: "Record New Take" card, the usual take cards, and blank cards. FlowPane is simpler because you can simply add and remove nodes that can be unrelated. This avoids adding unnecessary bloat code to create a new model type for the datagrid.
2. Datagrids (and similar lists) require an ObservableList of some kind of model. Besides point 1, this observable list would be different than but dependent on the alternateTakes list used in RecordableViewModel, which is already an ObservableList. Responding to a change in an observable list that is responds to changes in another observable list is slow (I tried it.)
3. I investigated FlowPane.java to see how I could add or remove blank cards to always empty space in the bottom row with blank cards. This seems possible if I eventually create a copy of FlowPane with some modifications to a private method. It seemed much more complicated to do the same thing using other lists.

[aunger]

With the receiver function + apply + let, this little guy is a doozy for me. Please rework for clarity. Something like this works better at least for me:

        fun dragTargetBottom(runOnNode: (VBox.() -> Unit)? = null): Node = dragTargetBottom.also {
            runOnNode?.let { dragTargetBottom.runOnNode() }
        }

[jsarabia]

Single expression formats are meant to be a single expression; this is too complex for that style of formatting.

If it was fine, this would apply:
https://kotlinlang.org/docs/reference/coding-conventions.html#expression-body-formatting

I believe it was referenced in clean code though I couldn't find it, but basically, how you organize your space is important for readability. We read code top to bottom, and each line left to right. We don't read each line though, often just scan top down across the left hand side, so it's helpful when the leftmost space gives us a lot of context to what the rest of the line does, and we follow further down the line (left to right) if we need more details.

This makes a critical assumption though; that every major step begins on a new line. So in this code example, scanning top down, I see dragTargetBottom.... then the first instruction is "runonNode?..." which is actually incorrect. The fact that the closing } appears to close the function block further adds to misleading a reader just glancing at the code.

Your existing code should look like:

fun dragTargetBottom(runOnNode: (VBox.() -> Unit)? = null): Node { 
    return dragTargetBottom.apply {
        runOnNode?.let { it() }
    }
}

I know this is done elsewhere in the code; I would appreciate if we could all be on the lookout and clean them up as we come across it.

[jsarabia]

Also, @aunger is right, this is really difficult to read; especially with overloading the variable name with the method name.

it might help to prefix the private variable with _; so _dragTargetBottom?

But the name is also really unhelpful to begin with; what is it exactly? is it the takeGridDragTarget?

[KJoslyn]

I'm going to rework all of this, probably into a separate component, something like DragTakeTarget

[jsarabia]

code formatting on all of these add methods

[KJoslyn]

All of these add methods are going away since I'm making a new drag target component anyway

[jsarabia]

don't use single expression function syntax here

[jsarabia]

why is this a lambda?

[KJoslyn]

I don't know- that was silly :)

[KJoslyn]

I remember now- TakesListView constructs TakeCards for each take in the list, and it needed to instantiate an AudioPlayer for each of the TakeCards. In reality this parameter should have been named something like createAudioPlayer. I just pushed a commit that removes this parameter as well as lastPlayOrPauseEvent, in favor of accepting a lambda function called createTakeNode that just constructs the card given a take object.

Here's another thought- I don't know if this entire class is really needed or not. I could just create a vanilla ListView instead in RecordResourceFragment, but perhaps this class still encapsulates the takes list view and reduces the code length of RecordResourceFragment... especially with the lengthy comment explaining the lack of caching the cells.

[jsarabia]

could you use di scopes here instead of the get?

[KJoslyn]

I looked into this, but I haven't figured out how to make it work for this situation

[jsarabia]

formatting on all the adds

[KJoslyn]

These adds are gone now that I created a DragTarget control

[jsarabia]

we often aren't going out of our way to abbreviate things; event is already short, can we just use the full word?

[KJoslyn]

sure! done

[jsarabia]

})

[KJoslyn]

All of these have been fixed. I also just reread parts of the Kotlin Coding Conventions to help with this, since I was unclear about proper formatting for chained call wrapping.

[jsarabia]

})

[jsarabia]

})

[jsarabia]

})

[jsarabia]

})

[jsarabia]

})

[jsarabia]

why nullable?

[jsarabia]

audioPlayerProvider or audioPlayerBuilder... idk a lambda isn't an audio player and the name should reflect what this is

[KJoslyn]

Good point, although I'm working on this file now and I plan to replace that parameter and the next one with a lambda function called createTakeCard that just knows how to create the take card and doesn't care about the take card's details

[jsarabia]

why does this need text?

[KJoslyn]

It doesn't, this was just for testing. I'm going to remove that too as I'm working on TakesFlowPane now to implement the dynamic blank cards

[aunger]

FYI, if you specify the type of result explicitly as non-null, this branch is unneeded. I have no problem with this as-is though.

[KJoslyn]

Thanks! Great point.

[jsarabia]

I thought we had an issue putting nullables in observables?

[aunger]

Oh, yeah, this is easy to forget (and you don't find out until a null is observed at runtime).