-
Notifications
You must be signed in to change notification settings - Fork 155
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
FileViewer: fix coloration issue for logs files. #2299
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
* ^ use this char to determine the color | ||
* Ex for logs: | ||
* `2024-01-26T10:22:26.947416Z WARN elementx: Restore with non-empty map | MatrixClientsHolder.kt:68` | ||
* ^ use this char to determine the color, see [LogLevel] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the last char since the level name is always 5 chars with end alignment. So we could not make the diff between INFO
and WARN
.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2299 +/- ##
===========================================
+ Coverage 70.04% 70.07% +0.03%
===========================================
Files 1353 1353
Lines 33194 33236 +42
Branches 6870 6877 +7
===========================================
+ Hits 23250 23291 +41
- Misses 6637 6638 +1
Partials 3307 3307 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! I have a couple of nits, nothing important, so feel free to ignore them.
|
||
enum class ColorationMode { | ||
Logcat, | ||
Logs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be TracingLogs
/RustLogs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'D' -> Color(0xFF299999) | ||
'I' -> Color(0xFFABC023) | ||
'W' -> Color(0xFFBBB529) | ||
'E' -> Color(0xFFFF6B68) | ||
'A' -> Color(0xFFFF6B68) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the colors used here could be extracted? Most of them are shared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -51,5 +51,6 @@ fun aViewFileState( | |||
) = ViewFileState( | |||
name = name, | |||
lines = lines, | |||
colorationMode = ColorationMode.Logcat, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have another preview with the other mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, 7efbee8 and this has revealed a bug!
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Type of change
Content
Complement for #2283.
Coloration was not working for logs file filled by the SDK since the format is not the same than on logcat files.
Motivation and context
Better readable log file.
Screenshots / GIFs
Tests
Tested devices
Checklist