Introduce Plugin System #4
Conversation
julianharty
left a comment
julianharty
left a comment
There was a problem hiding this comment.
Overall this PR is very encouraging, by using a plug in architecture. I've added various comments in an initial code review - I hope at least some are useful at this early stage of the project.
| //-------------------------------------------------------------------------------- | ||
| init { | ||
|
|
||
| // Some settings affect use-cases |
There was a problem hiding this comment.
...for example? i.e. can you add at least one reason or example to this comment please?
| @@ -0,0 +1,105 @@ | |||
| package co.stonephone.stonecamera.plugins | |||
There was a problem hiding this comment.
❓ I'm a great fan of providing file level documentation that describes the purpose of a file. This file's name left me guessing what the purpose of it is. What do you think in terms of such documentation??
| // If difference in zoom is above threshold, we auto-cancel focus | ||
| // if (kotlin.math.abs(zoomFactor - oldZoom) > ZOOM_CANCEL_THRESHOLD) { | ||
| // cancelFocus("zoom changed by more than $ZOOM_CANCEL_THRESHOLD") | ||
| // } |
There was a problem hiding this comment.
❔ Why keep these 3 lines of commented-out code?
There was a problem hiding this comment.
Because the logic is now missing, and I want to remember the maths :)
There was a problem hiding this comment.
❔ remove once the code's been merged?
julianharty
left a comment
julianharty
left a comment
There was a problem hiding this comment.
LGTM, thank you for the various updates to improve the code documentation.
| // ZoomBar depends on ZoomBase, etc. | ||
| val PLUGINS = listOf( | ||
| ZoomBasePlugin(), | ||
| ZoomBarPlugin(), |
There was a problem hiding this comment.
maybe consider inheriting ZoomBarPlugin from ZoomBasePlugin to get rid of dependency on load?
There was a problem hiding this comment.
I was imagining in this way we'd be able to replace the zoom behaviour by just switching the plugin. (e.g. some OEM wants zoom to be much slower or faster)
|
|
||
| val shootModes = arrayOf("Photo", "Video") | ||
|
|
||
| // Order here is important, they are loaded and initialised in the order they are listed |
There was a problem hiding this comment.
just as an alternative for the future
some kind of priority discriminator (e.g. stage) can help to not care about the order in the list here but will decrease readability
There was a problem hiding this comment.
Yes, that could make sense - we definitely need to do that for the settings items (e.g. we should be able to configure the order and position of the settings controls, separately from the order plugins are initialised)
Only partially transitioned to it, but this PR moves the following functionality into "plugins".
The purpose of this plugin system is to make it much easier and simpler to extend the camera without significant code re-factors or merge conflicts. Even more important nowadays when significant code might be touched/modified/written by LLMs.
It has also allowed us to move individual feature logic out of the ViewModel, and into their own relevant areas of code, making reading through much easier.
The following functionality now lives in plugins:
Other functionality that ought to be moved to plugins in the future:
This system makes it easy to do various things, including:
In future, it'll also make it easy to manipulate imageAnalysis pipelines (e.g. for QR Code Scanning, Portrait Mode, etc.)
I'll write a silly example plugin and add some documentation alongside to explain how to use this soon.