Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add API to control logs for animators #474

Merged
merged 2 commits into from
Jul 5, 2021
Merged

Conversation

kiryldz
Copy link
Contributor

@kiryldz kiryldz commented Jul 2, 2021

PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: #243

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Add example if relevant.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Add API to control logging for animation plugin and disable debug logs by default.</changelog>.

Summary of changes

Add API to disable not critical logs for camera plugin. debugMode is set to false by default. Logs could be enabled with:

mapView.camera.debugMode = true

Additionally even if logs are enabled non-critical logs are downgraded to use Logger.d instead of Logger.i.

User impact (optional)

@kiryldz kiryldz requested a review from a team July 2, 2021 16:02
@kiryldz kiryldz self-assigned this Jul 2, 2021
@tobrun
Copy link
Member

tobrun commented Jul 5, 2021

Upstream we just merged the ability to filter out categories from common SDK. I rather see us moving to that solution instead. We will however need to add the log configuration by default to disable log messages for users and remove the filter if we want to enable it again.

@tobrun
Copy link
Member

tobrun commented Jul 5, 2021

Upstream we just merged the ability to filter out categories from common SDK. I rather see us moving to that solution instead. We will however need to add the log configuration by default to disable log messages for users and remove the filter if we want to enable it again.

After internal convo, le'ts move forward on exposing debugMode. This can use the internal API under the hood.

@kiryldz kiryldz requested review from tobrun and Chaoba July 5, 2021 08:36
@kiryldz kiryldz force-pushed the kdz-plugin-debug-mode-api branch from bfc4edc to 5e4b83a Compare July 5, 2021 09:13
Copy link
Contributor

@Chaoba Chaoba left a comment

Choose a reason for hiding this comment

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

LGTM

@kiryldz kiryldz merged commit dbfd7f3 into main Jul 5, 2021
@kiryldz kiryldz deleted the kdz-plugin-debug-mode-api branch July 5, 2021 10:56
* If debug mode is enabled extra logs will be written about animation lifecycle and
* some other events that may be useful for debugging.
*/
var debugMode: Boolean
Copy link
Member

Choose a reason for hiding this comment

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

shall we generate this config option for all the plugins? cc @kiryldz @tobrun @Chaoba

Copy link
Contributor Author

@kiryldz kiryldz Jul 5, 2021

Choose a reason for hiding this comment

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

No, I've taken a look at other plugins, there's no sense in adding it everywhere. We don't have that massive logs anywhere as well as some true debug mode. That's why I closed internal tickets related to similar API for all the plugins and decided to implement it in camera only due to a lot of requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to turn off CameraAnimationPlugin logs
4 participants