Add ability to check configuration changing #6484
Conversation
kmadsen
force-pushed
the
km-add-ability-to-check-configuration-changing
branch
from
52a4d96 to
427154d
Compare
Codecov Report
@@ Coverage Diff @@
## main #6484 +/- ##
============================================
- Coverage 72.50% 70.05% -2.46%
+ Complexity 5550 4839 -711
============================================
Files 779 708 -71
Lines 30069 27875 -2194
Branches 3547 3278 -269
============================================
- Hits 21801 19527 -2274
- Misses 6843 7066 +223
+ Partials 1425 1282 -143
|
...ation-core/src/main/java/com/mapbox/navigation/core/lifecycle/MapboxNavigationAppDelegate.kt
Show resolved
Hide resolved
| */ | ||
| @UiThread | ||
| @JvmStatic | ||
| fun isConfigurationChanging(): Boolean = mapboxNavigationAppDelegate.isConfigurationChanging() |
There was a problem hiding this comment.
How do you envision using these methods? Do you have an example?
I can't say about options changing but regarding configuration changing I'm not sure if that's the way to go.
I'm worried that we might use this method somewhere and when it returns true we'll just ignore some actions, meanwhile the state will become inconsistent because some other components will be recreated.
I think it would be more neat and clear if the components that don't care about configuration changes just had a broader lifecycle. It's basically your first idea from #6766 (comment).
There was a problem hiding this comment.
Yeah I have the same concern. I've been debating whether the MapboxNavigationObserver implementation should ignore the detach->attach when configuration is changing. Or if MapboxNavigaitonApp should not detach observers when configuration is changing.
I think we need to support the case where, a MapboxNavigationObserver is only attached while in landscape mode. So I've been thinking it should be the MapboxNavigationObserver responsibility to create a definition and usage requirements
There was a problem hiding this comment.
There are issues with putting the logic in MapboxNavigationObserver implementations. For example, if we want the observer to be usable without MapboxNavigationApp.
There was a problem hiding this comment.
So another option am considering, is what if you can specify what you want during registration. For example, add a new way to register an observer
MapboxNavigationApp.registerStrongObserver(observer)
the strong (there is probably a better word) observer would not detach during configuration or option changes. another option is to add parameters to the registerObserver function.
There was a problem hiding this comment.
I actually like the strong observer idea. And I think that a new type of observer is more explicit than passing the options as an argument. It's also less flexible but I'm not sure if we're gonna need a lot of flexibility here.
And I guess we should also support the case when we want to do A on onAttached, B on configuration/options change and C on onDetached. We'd probably need two different observers to resolve this. To make it possible we should guarantee the order in which the observers are called. For example, if we register one MapboxNavigationObserver (O1) and MapboxNavigationStrongObserver (O2), then common onAttached/onDetached should be passed first to O1, then to O2 (or whatever order is more convenient). WDYT?
There was a problem hiding this comment.
I actually like the strong observer idea.
👍
And I think that a new type of observer is more explicit than passing the options as an argument
Do you mean, preferring 2 over 1 like this? I can see it, but there are pros and cons 🤔
- MapboxNavigationApp.registerStrongObserver(object : MapboxNavigationObserver ..)
- MapboxNavigationApp.registerObserver(object : MapboxNavigationStrongObserver ..)
- Add MapboxNavigationObserver#isStrong() overridable configuration
- Add MapboxNavigationObserver#onConfigurationChanged() and then make all observers strong
- Add MapboxNavigationApp#isConfigurationChanging() so that onAttached/onDetached can figure it out
I'm not sure what I prefer yet.. it seems all of them technically work so we could list some pros/cons to decide.
And I guess we should also support the case when we want to do A on onAttached, B on configuration/options change and C on onDetached.
We can also consider, adding an overridable function to MapboxNavigationObserver (e.g., onConfigurationChanged..). The pull request as it is now, requires a condition to see if onAttached/onDetached is triggered by a configuration change.
MapboxNavigationObserver (O1) and MapboxNavigationStrongObserver (O2), then common onAttached/onDetached should be passed first to O1, then to O2 (or whatever order is more convenient). WDYT?
I'm not yet sold on MapboxNavigationStrongObserver because I bet if we listed the pros and cons, we may prefer solution 3 to add an overridable configuration to MapboxNavigationObserver, or we may decide on something else. I don't like that we would have to change the type of the observer. I would like to at least start with the flexibility to be able to make any MapboxNavigationObserver a strong observer.
There was a problem hiding this comment.
I don't like that we would have to change the type of the observer.
Why? We are intoducing a new type of observer with behaviour that was not present before. We could do with a new interface here.
Add MapboxNavigationObserver#isStrong() overridable configuration
How are you going to implement it without breaking the API? There were some problems with introducing an interface method with default implementation. Converting it to an abstract class will break java users for sure.
There was a problem hiding this comment.
I don't like that we would have to change the type of the observer.
Why? We are intoducing a new type of observer with behaviour that was not present before. We could do with a new interface here.
There is no difference with the implementation of the interface. Maybe we do this approach, just think we should consider other approaches first.
How are you going to implement it without breaking the API?
We can change the MapboxNavigationObserver implementation if we convert the interface to a java interface (an example here). Default functions are supported in java, default functions will have better support in kotlin after version 1.6.20 https://youtrack.jetbrains.com/issue/KT-54239
kmadsen
force-pushed
the
km-add-ability-to-check-configuration-changing
branch
from
d8f325b to
ee9c56b
Compare
Description
I'm finding more and more reasons for this when I'm trying to change the
NavigationOptionsat runtime.Having a way to know that the options are changing, is useful. Also, exposing the configuration boolean is useful.
Opening as draft because I didn't spend much time testing
Screenshots or Gifs