This repository was archived by the owner on Feb 22, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[firebase_crashlytics] Fix Printing, README, Changelog & log FlutterErrorDetails.informationCollector #1912
Merged
collinjackson
merged 22 commits into
flutter:master
from
creativecreatorormaybenot:crashlytics-null-stack-trace
Jul 26, 2019
Merged
[firebase_crashlytics] Fix Printing, README, Changelog & log FlutterErrorDetails.informationCollector #1912
collinjackson
merged 22 commits into
flutter:master
from
creativecreatorormaybenot:crashlytics-null-stack-trace
Jul 26, 2019
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart
Outdated
Show resolved
Hide resolved
packages/firebase_crashlytics/lib/src/firebase_crashlytics.dart
Outdated
Show resolved
Hide resolved
...a/io/flutter/plugins/firebase/crashlytics/firebasecrashlytics/FirebaseCrashlyticsPlugin.java
Outdated
Show resolved
Hide resolved
Contributor
|
Thank you so much for the fast turnaround on implementing my suggestions. |
Contributor
Author
|
Thanks for taking the time. |
collinjackson
approved these changes
Jul 26, 2019
mithun-mondal
pushed a commit
to bKash-developer/archived_plugins
that referenced
this pull request
Aug 6, 2019
…rrorDetails.informationCollector (flutter#1912)
creativecreatorormaybenot
referenced
this pull request
in axel-op/feature_discovery
Sep 28, 2019
julianscheel
pushed a commit
to jusst-engineering/plugins
that referenced
this pull request
Mar 11, 2020
…rrorDetails.informationCollector (flutter#1912)
13 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The initial issue I was facing was that
firebase_crashlyticswould throw an unhandled exception on e.g.RenderFlexexceptions without a stack trace (and the stack trace formatting for exceptions with stack traces was unreadable + the reports were missing information):Along the way of fixing this behavior, I noticed that the
README.mdfile was outdated and did not even contain the breaking change from0.1.0. Consequently, I fixed my original issue, added what was missing to the README and also adjusted some inconsistent formatting I found in the README.I also noticed that
Error reported to Crashlytics.was not even being printed and added that and also made theResultsection more clear.Notes
I found myself adding
print(details)to the oldonErrorimplementation because the formatting fromTrace.formatis terrible for reading it in the console and it was also missing what the actual exception is, which I also added in this PR. This is the reason I averted from usingTrace.formatin the debug implementation.I noticed that
contextwas not being used in the native Android implementation, so I thought that I would add it as a string as it is a very short term describing when the exception was thrown.Additionally, I added
informationand logged that usingCrashlytics.logandCS_LOG. For me, this is a very crucial part for working with Flutter exceptions as it gives information about the tree, the specific widget and also some background information on why the exception occurred.Furthermore, I noticed that the tests were printing errors when trying to access
lineParts[2], which was distracting. Hence, I added a check to prevent that from happening.If the PR itself is fine but there are minor issues like comments being abundant, then please just fix them to speed things up.
I hope that my comments also help to understand my changes.
Testing
It seems like driving examples was added for both Android and iOS now. Thus, I would assume that the tests run by CI should confirm whether the changes are implemented correctly or not as I added
informationCollectorandcontextto the test call.The logging part I could only confirm locally by trying it out. I am not sure how I would write a test for that.
Question
After going through the code and reading the README, I am wondering if the error is also supposed to be printed locally in either of the following two scenarios because it was not printed before my change and I did not adjust that (only clarified in the README):
App is in release mode.
App is in debug mode but
enableInDevModeistrue.Changelog
I fixed the changelog typo that was pointed out in #1913 by @lsaudon in here as well.