Logging screen implementation#163
Conversation
deobald
left a comment
deobald
left a comment
There was a problem hiding this comment.
Hey @soujanyadhungel there are some necessary changes to make before this can be merged.
Also, if you can please pull from pariyatti:master to incorporate the latest changes to master into your branch before we merge it, that would be good.
| "nothing_bookmarked": "您還沒有書籤", | ||
| "only_english": "目前僅提供英文作為替代語言", | ||
| "pali_word": "每日一个巴利语单词", | ||
| "pali_word": "每日一个巴利語單詞", |
There was a problem hiding this comment.
Why has this line changed? This PR shouldn't be changing translations for Pali Word, since that is unrelated.
| "try_again_later": "發生錯誤，請稍後再試。", | ||
| "was_empty": "沒有內容", | ||
| "words_of_the_buddha": "佛陀每日法语" | ||
| "words_of_the_buddha": "佛陀每日法語", |
There was a problem hiding this comment.
Same question as with Pali Word: why has this Chinese translation changed in this PR? This PR should only make changes related to logging.
| /// Adds a new log entry to the queue, respecting size and memory constraints | ||
| void addLog(String message, String level) { | ||
| final entry = LogEntry(message, level); | ||
| final entrySize = entry.estimatedSize; |
There was a problem hiding this comment.
Let's not do this. The following while loop is kind of convoluted and I'm not sure the constraint on memory usage is required. I think that having a max count of 1000 entries will be fine. If that size goes over 10MB by a small amount it's not a big deal. I think trying to constrain the memory usage by the estimated size of the entries makes the code too hard to read.
Feel free to push back if you think this is essential for low-power devices.
| /// - File-based persistent logging with rotation | ||
| /// - Console logging for debugging | ||
| /// - Multiple severity levels (debug, info, warning, error) | ||
| class AppLogger { |
There was a problem hiding this comment.
Let's please name the file and class the same: app_logger.dart
|
|
||
| // In-memory circular buffer | ||
| static final List<String> _memoryLogs = []; | ||
| static const int _maxMemoryLogs = 1000; |
There was a problem hiding this comment.
If it's important that this 1000 log entry limit is the same as the other 1000 from LogManager, that number should be defined in a single place so it's not repeated.
| ), | ||
| body: _isLoading | ||
| ? Center(child: CircularProgressIndicator()) | ||
| : _logs.isEmpty |
There was a problem hiding this comment.
Starting from the end of the first ternary operator (x ? y : z), you should extract a function/method to return the rest of this data. Don't nest code this deeply, even in Flutter UI stuff.
In general, we don't ever want to nest ternary operators. 9/10 times, a ternary operator is the wrong choice if it doesn't fit on a single line:
a ? b : c is good
a
? b
: c
...is much less good and probably deserves an if/else statement instead.
| body: _isLoading | ||
| ? Center(child: CircularProgressIndicator()) | ||
| : _logs.isEmpty | ||
| ? Center( |
There was a problem hiding this comment.
Once extracted into its own function, then the check on _logs.isEmpty can be an if/else which returns the 'logs_empty' Text widget below and in the else clause returns an entirely new function (appropriately named logsWithContent or something) with the RefreshIndicator at the top level.
| color: isError | ||
| ? Theme.of(context).colorScheme.errorContainer | ||
| : isWarning | ||
| ? Theme.of(context).colorScheme.secondaryContainer |
There was a problem hiding this comment.
Even shallowly-nested ternary operators are extremely hard to read. This is less of a problem than the top-level ternary operators but I would say all of the right-hand-side of color: could be extracted into its own function and it would make it clearer, whether you switch away from nested ternary operators or not.
| } | ||
|
|
||
| // Write to persistent storage | ||
| _writeToFile(logMessage); |
There was a problem hiding this comment.
Since the in-memory log is the only one used in the UI (as far as I can tell), why do we bother writing the logs to disk at all?
Preferably, we would only use the disk-based log for all our purposes, since I don't understand the value of the in-memory logs, and the on-disk logs could be shared in the event of a crash. At the moment, if the app crashes and then the user tries to share the logs, they are sharing the empty in-memory log, not the on-disk log which would contain the events before the crash.
Let me know if this question makes sense?
2 participants
Creating pull request for Issue #8.