Skip to content
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

Complete TODO : Show more user friendly labels #1002

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nishanthkarthik
Copy link
Contributor

This PR completes the task at

// TODO: Show more user friendly labels, like "5 hours ago"

@trollixx
Copy link
Member

trollixx commented Oct 3, 2018

Linux build failed because QDateTime::toSecsSinceEpoch() was added in Qt 5.8. I'll bump the requirement for 0.7.x.

@nishanthkarthik
Copy link
Contributor Author

I'll try replacing it with a backwards compatible method later today

@trollixx
Copy link
Member

trollixx commented Oct 3, 2018

No need, the plan is to switch to newer Qt in 0.7 anyway.


const qint8 MAX_FIELDS_DISPLAYED = 3;

QString ZERO_INTERVAL_STRING = "now";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these should just be constant strings in an anonymous namespace in the source file.

ReadableInterval(QDateTime timestamp, QDateTime reference);
ReadableInterval(QDateTime timestamp);

QString toReadableString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just make the whole thing a static method?

@nishanthkarthik
Copy link
Contributor Author

@trollixx Done (I also fixed some clang tidy warnings). Please let me know if there's anything else :)

@trollixx
Copy link
Member

This pull request introduces 3 alerts when merging 7d4e43c into d9bb3c5 - view on LGTM.com

new alerts:

  • 3 for Comparison result is always the same

Comment posted by LGTM.com

@nishanthkarthik
Copy link
Contributor Author

Hey, on the three LGTM warnings of comparing with known value at compile time, how do I transform this snippet to a compile time branch?

const int X = 3;

int a = 0;
if (m && ++a < X) 
    f();
if (n && ++a<X)
    g();

Here, the increment occurs only if m or n is true. If not, this has to go to the next branch until a = X. I am not sure how to optimize this out in compile time as we have no idea if first condition is true or not until runtime.

I think this is a false alert since we depend on the boolean expression short-circuiting to increment the counter.


const qint8 MAX_FIELDS_DISPLAYED = 3;

const QString ZERO_INTERVAL_STRING = "now";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dangerous, see static initialization order fiasco and bullet point 3 here.


delta = reference.toSecsSinceEpoch() - timestamp.toSecsSinceEpoch();

if (delta) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always try to return ASAP, and avoid unnecessary conditional blocks of code.

if (delta == 0) {
    return ZERO_INTERVAL_STRING;
}

QStingList list;
...


const qint8 MAX_FIELDS_DISPLAYED = 3;

const QString ZERO_INTERVAL_STRING = "now";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it's better to avoid making translatable strings constant. Just put them inside tr() inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving plural forms of units with a (s). Once we apply translation to this project, qt should automatically detect the quantity and append s.

http://doc.qt.io/qt-5/i18n-source-translation.html#handling-plurals

@trollixx
Copy link
Member

I see rightfully LGTM complains about always true conditions. Maybe rewrite the whole thing as a loop?

@trollixx
Copy link
Member

trollixx commented Oct 20, 2018

I am still thinking on how to implement this in a cleaner way. The current proposal is a bit too heavy for the task it accomplishes. Perhaps, worth checking if we can use std::chrono for the duration manipulations...

@nishanthkarthik
Copy link
Contributor Author

nishanthkarthik commented Oct 24, 2018

Hey @trollixx

std::chrono::duration looks like a viable alternative on the computation part where we split seconds to multiple duration units. It doesn't look like there is a way in this lib to print it in human readable format.

If you tell me which part of my snippet seems heavy, I can try replacing the same with chrono equivalent.

@trollixx
Copy link
Member

Right, we still need to print generate the text ourselves, but I think std::chrono should be used for manipulating the duration. That should reduce the amount of code required as well.

@nishanthkarthik
Copy link
Contributor Author

Sure, let me update this PR with chrono

gcoelho added a commit to gcoelho/zeal that referenced this pull request Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants