seadowg/pain-experienced.md

## pain-experienced.md

      
    Raw
  

              pain-experienced.md
            
          
    Pain experienced during MediaPlayer refactor

I recently spent some time refactoring how Collect interacts with the Android MediaPlayer (PR here). This resulted in my having to touch a fair amount of the stack involed in form entry so I thought it would be good to do a write up of the pain I experienced (within the code) with some suggestions around improvement we could make.
Disclaimer: all the "Next steps" are definitely just my own and first opinion so are very open to discussion.
ODKView having a lifecycle but not being a Fragment

When Collect renders a Form it uses an ODKView to represent each question (FormEntryPrompt) or "group" of questions. This view is hosted inside a FormEntryActivity. When the user swipes from left to right the ODKView is removed from the view hierarchy and a new one is made for the new question.
This is problematic for any situation that requires us to do something when the ODKView is no longer needed. This could be releasing resources or deregistering observers. In Android a View is not aware of its own "lifecycle" in the same way that Activity or Fragment are so this means we have to carry out any clean up outside of the ODKView. This becomes even more problematic if we want to use any "Lifecycle aware" components (like LiveData) within an ODKView (or basically any Widget) as the ODKView doesn't have a LifecycleOwner to reference other than its host Activity - which would outlast it.
Next steps

I think the best way forward would be to look at moving the ODKView towards being a Fragment. This will give it a "true" lifecycle we can use but will also have other advantages in allowing us to use a ViewPager - currently the swipe code is all custom and so has more potential for bugs or performance issues that using an SDK component.
This change over won't be the easiest thing in the world but I think we could look at doing it in a few steps:

Extract and test code that builds the view the user sees on each screen of the form (createView in FormEntryActivity) to a use case object that creates views based on the "state" of a form (expressed using the FormController events). Doing this makes it easier to use the code within a Fragment but also reduces a lot of the code sitting in FormEntryActivity.
Replace the swapping in and out of pages of the form using views (showView in FormEntryActivity) with something that uses a Fragment that essentially renders the current page of the form.
Replace custom swiping code with a ViewPager (it might be easier to do this as part of the above).
Use the Fragment that represents a page of a form as the "lifecycle" for anything that needs it.

No easy way to create FormEntryPrompt for tests

Buiding a FormEntryPrompt for tests (for Widget classes for example) is not easy as it and the objects that make up it's building blocks (FormDef and FormIndex) are pretty complex and require a lot of JavaRosa knowledge. Most tests end up creating fakes using mockito as we don't need a lot of the underlying parts of the object to work - usually we just need certain methods to return values. Ultimately this still means setting a lot up each time and because the interface isn't the "cleanest" it can add a lot of noise to tests.
Next steps

For the work towards autoplay funcionality I've introduced a "Builder" for creating instances of FormEntryPrompt for tests. This still uses mockito under the hood but lets us use an interface that's driven more by the "data" of the FormEntryPrompt and how Collect thinks about it than the individual methods of the interface itself. I think using this object and expanding it is a good way forward.
No consistent patterns around passing dependencies to Widgets

Generally an ODKView will contain one or more Widget objects that represent the Form's prompts. As the Widget classes are quite complex they can have many dependencies but there isn't a clear pattern around getting these into the object themselves. Some widgets use Dagger, some have dependencies come in at the constructor, some end up having them set later and some use singleton style getInstance fetchers. The fact that there is no real consistency around this makes it hard to choose how you should add a new dependency to a Widget and some of these methods make it very hard to write dependenable tests (using static singletons can put you in a bad place for example).
Next steps

Personally I think we should push as much as possible for Widget dependencies to move to constructors rather than relying on any other wiring (DI etc). I think a good way towards this would be to pull depdencies up the hierarcy one at a time. For instance, all widgets need to be able to interact with audio as any question can have audio media attached. Pulling up our interface for interacting with audio to the top level (QuestionWidget) would be a nice step.
If we did this, I think we'd want to remove/deprecate the injectDependencies method on Widget to discourage the use of that pattern in the future.
Lack of documentation/opinions around how "dumb" View classes should be

Should a view encapsulate all the funcionality of a feature? Should it have listeners? Should it rerender automatically? All of this confusion is as much the fault of the SDK as the app itself but I think the fact that there isn't a large amount of consistency within the app and the fact that we don't have any documentation on it makes it even harder.
Next steps

I think a nice step would be to add a guide around the "ideal" View in our style guide. I've written a first pass here:

Views should generally be as dumb and as stateless as possible
View's shouldn't interact with other layers of your application (repositories, network etc). They should be able to take in and render data (via setters) and emit "meaningful" events via listeners.
A view should have a single public Listener sub interface and setListener method. Methods on the interface should correspond to "meaningful" events. If the view is a button that could just be onClick but if it's a volume slider this might be something like onVolumeChanged.
Views would ideally have just one setter for data but more complex views (often that have many subviews) that take in data at different times may have more - less setters is better as it's less changing state in the view.
Views that render more than one kind of data (and that have more than one setter) might benefit from render method that encapsulates all the logic around displaying the state of a view.

No clear place for utils, helpers etc to live

It's hard to feel like you've found a good place for shared interfaces, helpers etc to live in the app. The package structure follows a "what kind of object is this" which works for Android defined patterns but has fallen apart a little and left us a with a "junk drawer" utilities package. This is not only a little frustrating from an organizational perspective but I think makes it hard to see what kinds of patterns are being used in the app. What is a "helper"? What is a "util"?
Next steps

I think we could do a few  things:

Reorganize packages into domain "verticals" rather than organizing them by what kind of object they are. An example could be having a form package that contains all the classes associated with form entry. This still gives us quite a lot of disorganization within each "vertical" but should make it easier to navigate the code by feature area and make it easier to understand the "world" your code exists within.
Create packages for shared "layers" of our app. This could be things that are shared by different features (http for example) or things that don't need to exist within a feature. An example would be the work we've just done to seperate out audio from form entry features. This lets us run all the tests for that "part" of the application really easily (something that's also an advantage of the change before this).
Write guidelines about what we mean by "helper" etc in this app in our style guide. This helps people understand what kind of objects exist and also gives them hints on structuring new code. I also like that it'd give us a shared vocabulary when we're discussing code - especially useful given we're remote. I'd never see something like this as gospel though. If someone wants to introduce a new pattern that works that's awesome.

Stretch goal: move isolated packages to Gradle submodules (like audio). This would prevent us from being able to accidently import code from our other layers/domains of our application (without an explicit import between the submodules) and also lets us bring in depencies that aren't accidently shared by the rest of the app. We could even convert modules to Kotlin without worry of it leaking into other parts of the application...