-
-
Notifications
You must be signed in to change notification settings - Fork 361
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 in-app log viewer #5335
Add in-app log viewer #5335
Conversation
Wow, awesome! We could clone you a few times and we'll have a full iOS version in less than a year! 😆 So, it is really cool that you implemented this logging wrapper and it is even cooler that you even implemented this whole log viewer thing! @Helium314 , could it be that there is also a log viewer in SCEE? If so, would you say that if this is merged to SC it would be sufficient for SCEE too, or would you not replace it because something is missing from that which you deem important? Before I review it, the main use case for the log viewer at all is to help find bugs. So in this respect, it would be very handy to have something in the UI that facilitates making use of this. For example
I haven't looked at the PR yet and what you did or didn't implement, I just wanted to first explain for what the feature would (from my point of view) be used and thus what would be the primary use case for this. The use case dictates how the UI should work. |
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.
Here's my first review. I only reviewed the data part of the PR so far, i.e. nothing from the UI. Should give you enough to look into. I don't want to steal your thunder by letting you wait for long for a review.
First of all, very well done, I love how well you made this PR to be consistent with the code style and overall architecture of the app. The things I noted in this review are all quite minor, but anyway, here it is:
app/src/main/java/de/westnordost/streetcomplete/ApplicationConstants.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/data/logs/LogsTable.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/data/logs/LogsTable.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/util/CrashReportExceptionHandler.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/de/westnordost/streetcomplete/data/logs/LogsDaoTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/util/logs/DatabaseLogger.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/util/logs/DatabaseLogger.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/util/CrashReportExceptionHandler.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/screens/about/LogsController.kt
Outdated
Show resolved
Hide resolved
There is a simpler logger and viewer in SCEE. The logger just stores the most recent (up to 12k) lines in a list. I plan to remove the SCEE logger, or maybe have an option to use the old list-logging instead of writing to database. |
Reverse, you mean reverse the order of the log lines displayed in the list? (Why do you find it handy?) |
Because one does not have to scroll 12000 lines to the bottom of the list in order to see what was the error that just happened? Instead (in reversed order) the latest error is first line shown, which is far more convenient. |
Ah of course, but then you don't need a reverse button. The view should just by default show the latest log messages, just like an email app would. |
If you're only interested in the very last message, sure, that's what But if you want to follow several (dozen?) messages (to find causality), regular chronological order (i.e. oldest at top, newest at bottom, just as it would be if you were writing a journal) is much more useful. This is from general Latin European perspective; I don't know how it would work in languages which are regularly written left-to-right but bottom-to-top (or if any exist, or that matter?)
I'm not sure what you mean here... (click to expand)I guess the intended meaning there depends on what email app you use 😄 and I don't even know what popular solutions look today (either because I try hard to avoid google and its stuff like gmail; and I don't use microsoft platforms at all so have no idea how outlook express (or whatever is used there those days) looks like by default. When I do see/wait on people using them, it looks and feels oh-so-horribly-slow-and-inefficient so I don't pay much attention to details of their design). In my preferred mutt(1), I constantly switch between different sort modes (e.g. when reading e-mail threads from mailing lists and similar I highly prefer oldest message at top, with rest of them forking down each in their own sub-thread; each subthread sorted chronologically from oldest message to newest. Trying to read e-mail threads in plain newest-message-at-top order would be horrible experience, and I'd completely lose the thread who is replying to what - especially combined with fact that many people don't properly trim"e paragraphs to which they are replying to) |
Mhm. Well, towards @neonowy , as I wrote, I didn't review the UI yet, but I would expect that the messages are either ordered by time ascending with by default showing the latest messages (unless you scroll up manually) or alternatively by time descending. In any case, unless you interact with the UI, the latest messages should be in view. |
app/src/main/java/de/westnordost/streetcomplete/screens/about/LogsFragment.kt
Outdated
Show resolved
Hide resolved
This is a good point. I think in that case a reverse button like in SCEE can be useful. We could show the latest message on the top by default (as it does now) and allow the user to reverse the order with the reverse button in the menu. |
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.
The UI code is also very well done!
I commented on some smaller things. I also noticed that some code on the logger interface could be reduced.
How does the UI behave when something is being logged while in the screen but you are scrolled down? When you are not scrolled to top, it shouldn't automatically reset to the top position because if the user is looking through the log, it shouldn't be interrupted by new logs appearing.
Actually, looking at some other logging views, the most usual behavior and layout seems to be:
- new log lines appear at the bottom (logs sorted by date ascending)
- log view is always scrolled to the bottom, where the new log lines appear, by default
- when the log view is not scrolled to the bottom, the scroll position stays where it is when new log lines appear
- filters only filter, i.e. do not change any of the above behavior
- no reverse button
app/src/main/java/de/westnordost/streetcomplete/util/logs/DatabaseLogger.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/util/logs/AndroidLogger.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/util/logs/Log.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/util/logs/Logger.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/screens/about/LogsFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/screens/about/LogsFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/screens/about/LogsFiltersDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/screens/about/LogsFiltersDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/util/logs/Logger.kt
Outdated
Show resolved
Hide resolved
Thanks for all the feedback! ❤️
|
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.
The image in your last post doesn't display, so I made one:
It looks really pretty now and everything works like a charm!
I wonder, are you following any material design guidelines with these chips? My gut feeling is, that any checked chips should have the log level color as background color because buttons in Material 2 have the accent color as background, while any unchecked chips should have grey as their background color.
(Anyway, no need to put any more effort in the UI, it looks already as polished as it can be)
This is the "share" prompt on an Android 5.1.1 emulator. Disappointed that the share prompt didn't have the "copy to clipboard" option back then. I hope it was introduced in an Android version soon after.
app/src/main/java/de/westnordost/streetcomplete/screens/about/LogsFragment.kt
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/screens/about/LogsFragment.kt
Show resolved
Hide resolved
After merge I tested this on an API 25 emulator and it shows this weird behavior (can't make a video due to some error): All checkmarks have the same color, then if I disable and enable another log level, all checkmarks get the color of the last checked one. Not reproducible on an Android API 34 emulator and also wasn't able to fix it playing around with how the filter chip is initialized, so I guess it must be a bug in some older version of Android. (So unless someone has an idea what we do wrong here and/or if there is a workaround, it shall be a will not fix. I didn't test it, but if this issue occurs on relatively recent versions of android too, we should consider to just use the standard, less fancy, style for chips then https://m2.material.io/components/chips#filter-chips ) |
Without checking for the actually involved code: possibly calling |
It works, thank you @Helium314 ! |
Closes #3597.
Feel free to point out how I can improve the UI here, as I don't use Android day-to-day and I'm not familiar with its UI guidelines etc.
Summary of changes:
Logger
interface:AndroidLogger
for writing messages to Android logs (visible in Logcat)DatabaseLogger
for writing message to the databaseLog
object that replaces all the existingandroid.util.Log
callsCleaner
now removes logs older than 3 days from the database (set viaDELETE_OLD_LOG_AFTER
constant)CrashReportExceptionHandler
attaches the last 100 lines of logs to the email crash report (set viaMAX_LOG_LINES_TO_ATTACH_TO_CRASH_REPORT
constant)Screenshots:
Demo:
Screen_recording_20231026_211851-converted.mp4
Sample log copied via Share button: