refactor: use std::chrono for formatDurationStr() helper #549
Conversation
jonatack
changed the title
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
ACK 6f2593d
The updated code looks clean and is straightforward to understand. I was able to compile the PR branch successfully and test that updated formatDurationStr is providing correct results for the ui->peerConnTime.
Here is a screenshot of the correct working of the PR.
As a small nit suggestion, I think it would be a good idea to space out the code a little bit into logical sections. But this is a non-critical suggestion, and you may take it up in case you are going to update the PR for some other reasons.
| const auto m{std::chrono::duration_cast<std::chrono::minutes>(dur - d - h)}; | ||
| const auto s{std::chrono::duration_cast<std::chrono::seconds>(dur - d - h - m)}; | ||
| QStringList str_list; | ||
| if (auto d2{d.count()}) str_list.append(QObject::tr("%1 d").arg(d2)); |
There was a problem hiding this comment.
Maybe add translator comments, here and after?
There was a problem hiding this comment.
Yes, doing for several of these (FormatPeerAge, formatDurationStr, TimeDurationField, etc.) as a follow-up to this and to #543 (see #543 (comment)). That way this remains a straight refactor and current review on both pulls isn't invalidated.
| if (auto h2{h.count()}) str_list.append(QObject::tr("%1 h").arg(h2)); | ||
| if (auto m2{m.count()}) str_list.append(QObject::tr("%1 m").arg(m2)); | ||
| const auto s2{s.count()}; | ||
| if (s2 || str_list.empty()) str_list.append(QObject::tr("%1 s").arg(s2)); |
There was a problem hiding this comment.
nit, suggestion:
if (auto s2{s.count()}) str_list.append(QObject::tr("%1 s").arg(s2));
return str_list.empty() ? "0 s" : str_list.join(" ");There was a problem hiding this comment.
Sure, will do if have to retouch or will sneak it into the translator comments follow-up.
There was a problem hiding this comment.
Note: When following up - "Connection Age" will bring the naming convention together - in the gui as well as CL
|
tACK 6f2593d tested on macOS x86_64, Arm64
|
…ionStr() helper 6f2593d gui, refactor: use std::chrono for formatDurationStr() helper (Jon Atack) Pull request description: Updates `formatDurationStr()` to use the `chrono` standard lib. No change in behavior. ACKs for top commit: RandyMcMillan: tACK 6f2593d shaavan: ACK 6f2593d w0xlt: tACK 6f2593d on Ubuntu 21.10 Qt 5.15.2 promag: Code review ACK 6f2593d. Tree-SHA512: 61e9afdb1db779150df338e6af08727c34f69639add465c2f7003ff775d97dce3e78e78d325bc6dea5bc13f0fce9ef1c3506d13f1661a5e083e52bba8a32ba44
6 participants
Updates
formatDurationStr()to use thechronostandard lib. No change in behavior.