Skip to content

Conversation

@tobrun
Copy link
Member

@tobrun tobrun commented Mar 30, 2018

@tobrun tobrun self-assigned this Mar 30, 2018
@tobrun tobrun requested a review from cammace March 30, 2018 15:51
* @since 0.1.0
*/
public final class LocationLayerPlugin implements LifecycleObserver {
public final class LocationLayerPlugin implements LifecycleObserver, OnCameraMoveInvalidateListener {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nicer to have an anonymous inner class so we don't add unnecessary public methods to the LocationLayerPlugin api

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OnCameraMoveInvalidateListener is package private, I don't see the issue here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cammace When implementing an interface we are unnecessary exposing LocationLayerPlugin#onInvalidateCameraMove. Making it an anonymous inner class hides that method.

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to call onCameraMoveInvalidateListener.onInvalidateCameraMove(); for LocationLayerCamera#setBearing as well.

Additionally, it'd be great to optimize LocationLayerPlugin#onCameraMove not to update when unnecessary, for ex. we don't need to update accuracy radius when zoom has not changed etc. This might help with table overflow issues we've seen recently.

* @since 0.1.0
*/
public final class LocationLayerPlugin implements LifecycleObserver {
public final class LocationLayerPlugin implements LifecycleObserver, OnCameraMoveInvalidateListener {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cammace When implementing an interface we are unnecessary exposing LocationLayerPlugin#onInvalidateCameraMove. Making it an anonymous inner class hides that method.

@cammace
Copy link

cammace commented Apr 4, 2018

I've moved the implementation into a private variable and fixed it not getting invoked inside setBearing. For the optimizations with onCameraMoved, I have ticketed it out and will work on it in a separate PR #413

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@cammace cammace merged commit 682cef7 into master Apr 4, 2018
@cammace cammace deleted the tvn-oncameramove-invalidate branch April 4, 2018 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants