refactor: use std::chrono for formatDurationStr() helper #549

jonatack commented at 12:08 AM on February 15, 2022: contributor

Updates formatDurationStr() to use the chrono standard lib. No change in behavior.

gui, refactor: use std::chrono for formatDurationStr() helper 6f2593dc23

jonatack renamed this:
~~gui, refactor: use std::chrono for formatDurationStr() helper~~
refactor: use std::chrono for formatDurationStr() helper
on Feb 15, 2022

shaavan approved

shaavan commented at 11:50 AM on February 16, 2022: contributor

ACK 6f2593dc23565abaa3d176595cba6e07883f512e

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.

w0xlt approved

w0xlt commented at 3:01 PM on February 18, 2022: contributor

tACK 6f2593d on Ubuntu 21.10 Qt 5.15.2

hebasto added the label Refactoring on Feb 20, 2022

in src/qt/guiutil.cpp:722 in 6f2593dc23

 734 | +    const auto d{std::chrono::duration_cast<days>(dur)};
 735 | +    const auto h{std::chrono::duration_cast<std::chrono::hours>(dur - d)};
 736 | +    const auto m{std::chrono::duration_cast<std::chrono::minutes>(dur - d - h)};
 737 | +    const auto s{std::chrono::duration_cast<std::chrono::seconds>(dur - d - h - m)};
 738 | +    QStringList str_list;
 739 | +    if (auto d2{d.count()}) str_list.append(QObject::tr("%1 d").arg(d2));

hebasto commented at 8:58 PM on February 20, 2022:

Maybe add translator comments, here and after?

jonatack commented at 9:13 PM on February 20, 2022:

Yes, doing for several of these (FormatPeerAge, formatDurationStr, TimeDurationField, etc.) as a follow-up to this and to #543 (see #543 (review)). That way this remains a straight refactor and current review on both pulls isn't invalidated.

in src/qt/guiutil.cpp:726 in 6f2593dc23

 738 | +    QStringList str_list;
 739 | +    if (auto d2{d.count()}) str_list.append(QObject::tr("%1 d").arg(d2));
 740 | +    if (auto h2{h.count()}) str_list.append(QObject::tr("%1 h").arg(h2));
 741 | +    if (auto m2{m.count()}) str_list.append(QObject::tr("%1 m").arg(m2));
 742 | +    const auto s2{s.count()};
 743 | +    if (s2 || str_list.empty()) str_list.append(QObject::tr("%1 s").arg(s2));

promag commented at 10:42 PM on February 22, 2022:

6f2593dc23565abaa3d176595cba6e07883f512e

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(" ");

jonatack commented at 7:59 PM on February 23, 2022:

Sure, will do if have to retouch or will sneak it into the translator comments follow-up.

RandyMcMillan commented at 8:30 PM on March 1, 2022:

Note: When following up - "Connection Age" will bring the naming convention together - in the gui as well as CL

tr("Age")

promag approved

promag commented at 10:44 PM on February 22, 2022: contributor

Code review ACK 6f2593dc23565abaa3d176595cba6e07883f512e.

Maybe leave a comment that <format> can be used once we jump to C++20.

RandyMcMillan commented at 8:31 PM on March 1, 2022: contributor

tACK 6f2593dc23565abaa3d176595cba6e07883f512e

tested on macOS x86_64, Arm64

Screen Shot 2022-03-01 at 3 17 51 PM

Screen Shot 2022-03-01 at 3 19 26 PM

hebasto merged this on Mar 5, 2022

hebasto closed this on Mar 5, 2022

jonatack deleted the branch on Mar 5, 2022

sidhujag referenced this in commit 7d8e3e480d on Mar 6, 2022

bitcoin-core locked this on Mar 5, 2023