map implementation #175
map implementation #175
Conversation
…sts, more tests to come
ghost
requested review from
ghost
added
the
| import javax.inject.Inject | ||
|
|
||
|
|
||
| class MapFragment : BindginBaseMapViewFragment<MapFragmentComponent>() { |
There was a problem hiding this comment.
Fix typo and newline
| @Inject | ||
| internal lateinit var simpleMapViewViewModel: MapViewModel | ||
|
|
||
| private val dataBinder = object : DataBinder { |
There was a problem hiding this comment.
Use SimpleDataBinder
| } | ||
|
|
||
| @Test | ||
| fun hasGeotag() { |
There was a problem hiding this comment.
With the test method name, you should include the scenario that you are testing in the name. The general form is like: <assert condition>_<input/state condition>
| ``` | ||
| freesound.api.clientId=yourapiclientidvaluegoeshere | ||
| freesound.api.clientSecret=yourapiclientsecretvaluegoeshere | ||
| google.maps.api.key=yourgooglemapsapikeygoeshere |
There was a problem hiding this comment.
I'll look up how to include this secret so that Travis builds.
| d += tabController.tabRequestStream | ||
| .observeOn(schedulerProvider.ui()) | ||
| .subscribe( | ||
| { soundItem -> |
There was a problem hiding this comment.
Make this action a named method.
| } | ||
|
|
||
| private fun switchTab(soundInfo: SoundInfo) { | ||
| container.currentItem = if (soundInfo.tabType == TabType.RESULTS) 0 else 1 |
There was a problem hiding this comment.
There ought to be a better way to couple/define the TabType and its corresponding position.
| import com.google.android.gms.maps.model.LatLng | ||
| import com.google.android.gms.maps.model.MarkerOptions | ||
|
|
||
| internal abstract class SimpleMapViewViewModel : BaseViewModel(), OnMapReadyCallback { |
There was a problem hiding this comment.
This class should most likely be removed when refactoring to treat GoogleMap as a View. Perhaps parts of it could be absorbed into the BaseBindingMapFragment?
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent"> | ||
|
|
||
| </com.google.android.gms.maps.MapView> No newline at end of file |
There was a problem hiding this comment.
Could just terminate XML tag block with /> because the body is empty.
|
|
||
| <ImageButton | ||
| android:id="@+id/mapButton" | ||
| android:layout_width="48dp" |
There was a problem hiding this comment.
We have some constants for size, but they could probably use some revision. Consider also, using styles.
| class MapFragment : BindingBaseMapViewFragment<MapFragmentComponent>() { | ||
|
|
||
| @Inject | ||
| internal lateinit var simpleMapViewViewModel: MapViewModel |
There was a problem hiding this comment.
simpleMapViewViewModel name will need to be changed.
| import polanski.option.Option | ||
| import timber.log.Timber | ||
|
|
||
| internal class MapViewModel(private val tabController: TabController, |
There was a problem hiding this comment.
Expose Observables with map properties in this ViewModel.
Also move Map related classes such as this to a subpackage: com.futurice.freesound.feature.search.map.
| when (position) { | ||
| 0 -> getString(R.string.search_tab_results) | ||
| 1 -> getString(R.string.search_tab_map) | ||
| else -> "" |
There was a problem hiding this comment.
A position other than 0 or 1 is a error condition, correct? If so then throw an IllegalArgumentException with an explanation from here.
| super.onLowMemory() | ||
| map?.onLowMemory() | ||
| } | ||
| } No newline at end of file |
| } | ||
|
|
||
| override fun onPause() { | ||
| super.onPause() |
There was a problem hiding this comment.
Does Google give an advice about whether map.onPause() should be called before after super.onPause()? (similarly for onStop, onDestroy and any other "teardown" lifecycle callbacks).
| @@ -0,0 +1 @@ | |||
| mock-maker-inline No newline at end of file | |||
There was a problem hiding this comment.
newline at eof. Is there some sort of content-agnostic Linter we can use to check this?
| @@ -0,0 +1,36 @@ | |||
| /* | |||
| * Copyright 2016 Futurice GmbH | |||
There was a problem hiding this comment.
Also update your template for the copyright - 2018.
1 participant
results->map, not done yet: map->results, some tests, more tests to come
Before I write more tests and handle the way from maps-tab back to results-tab I would like to ask for some feedback :)
I would also like to do nice Markers as a separate issue, as it's already quite a lot to review.