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

Refactor 'About makers' to make it more translation-friendly #2913

Merged
merged 19 commits into from Sep 16, 2019

Conversation

@Kebap
Copy link
Contributor

commented Aug 3, 2019

Brief overview of PR changes/additions

Create maker information dynamically. Only translate descriptions!

Motivation for adding to Mudlet

Make it easier to review, update and expand upon makers mentioned.
Nobody was going to translate that massive slob of HTML from before.

Other info (issues closed, discussion etc)

Enhance #2912
I just spent upwards of 2-3 hours for (hopefully) no large visible effects.
Honing my craft, learning about Qt data structures and C++ controls.
Probably not the best decision to use this data structure, but hey...

SlySven and others added 2 commits Aug 2, 2019
Enhance: add Discord Ids to some Mudlet Makers in About Dialogue
I suggested this in the Discord "Mudlet-Development" channel and it was
deemed worth trying.  Significantly it uses the same colour for the
Discord link (which seems to have been offered by "Maiyannah" and NOT
Vadim!) as Discord uses (`#7289DA`) as well as for the Discord Ids - this
is so that they can be distinguished from the Github/Gitter Ids and/or
EMail addresses.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Kebap
@add-deployment-links

This comment has been minimized.

Copy link

commented Aug 3, 2019

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@Kebap Kebap marked this pull request as ready for review Aug 3, 2019

@Kebap Kebap requested review from Mudlet/core-cpp as code owners Aug 3, 2019

@vadi2 vadi2 changed the title Refactor about makers Refactor 'About makers' to make it more translation-friendly Aug 3, 2019

@vadi2

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Very nice! Thanks for doing this. I certainly did not want to translate that giant mess of escaped HTML that we got in Crowdin.

I see a few things you can do better, but did you want to try https://codereview.stackexchange.com to see what improvements could be done?

@Kebap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Don't know that URL. How would I go about that?

@vadi2

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Oh you can just post the code there and ask for a review! If you want to, don't have to.

I agree we need a better data structure here - remember you can create your own in C++.

Kebap added 3 commits Aug 3, 2019
@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

All those QString::appends(const QString&) are rather inefficient - and done at run-time, it is a better bet to amalgamate them where possible, for instance:

QString createMakerHTML (QStringList aboutMaker, bool big) {
    QString makerHTML;
    auto realname = aboutMaker.at(0);
    auto discord = aboutMaker.at(1);
    auto github = aboutMaker.at(2);
    auto email = aboutMaker.at(3);
    auto description = aboutMaker.at(4);
    // comments omitted ...!
    makerHTML.append(QStringLiteral("<p><span style=\"color:#bc8942;\">"));
    if (big) {makerHTML.append(QStringLiteral("<big>"));}
    makerHTML.append(QStringLiteral("<b>"));
    makerHTML.append(realname);
    makerHTML.append(QStringLiteral("</b>"));
    if (big) {makerHTML.append(QStringLiteral("</big>"));}
    makerHTML.append(QStringLiteral("</span> ("));
    if (!discord.isEmpty()) {
        makerHTML.append(QStringLiteral("<span style=\"color:#7289DA;\">"));
        makerHTML.append(discord);
        makerHTML.append(QStringLiteral("</span> "));
    }
    if (!github.isEmpty()) {
        makerHTML.append(QStringLiteral("<span style=\"color:#40b040;\">"));
        makerHTML.append(github);
        makerHTML.append(QStringLiteral("</span> "));
    }
    if (!email.isEmpty()) {
        makerHTML.append(QStringLiteral("<span style=\"color:#0000ff;\">"));
        makerHTML.append(email);
        makerHTML.append(QStringLiteral("</span> "));
    }
    makerHTML.append(QStringLiteral(") "));
    makerHTML.append(description);
    makerHTML.append(QStringLiteral("</p>\n"));
    return makerHTML;
}

could be simplified(!?) I think, to:

QString createMakerHTML (QStringList aboutMaker, bool big)
{
    auto realname = aboutMaker.at(0);
    auto discord = aboutMaker.at(1);
    auto github = aboutMaker.at(2);
    auto email = aboutMaker.at(3);
    auto description = aboutMaker.at(4);
    // comments omitted ...!
    return QStringLiteral("<p><span style=\"color:#bc8942;\">%1<b>%2</b>%3</span> (%4%5%6) %7</p>").arg(
        (big ? QStringLiteral("<big>") : QString()), // %1
        realname, // %2
        (big ? QStringLiteral("</big>") : QString()), // %3
        (discord.isEmpty() ? QString() : QStringLiteral("<span style=\"color:#7289DA;\">%1</span> ").arg(discord)), // %4
        (github.isEmpty() ? QString() : QStringLiteral("<span style=\"color:#40b040;\">%1</span> ").arg(github)), // %5
        (email.isEmpty() ? QString() : QStringLiteral("<span style=\"color:#0000ff;\">%1</span> ").arg(email)), // %6
        description); // %7
}

You could even dispose of the local copies of the QVector QStringList elements but you might lose some readability there...

@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Have closed that other PR on the basis that it is incorporated into this one - we just need clarification about whether it is okay to include the Discord IDs of the people so identified.

I am happy to include my Discord ID and I guess that Vadim will be, how about:
@demonnic, @keneanung, @Oneymus and @dicene as well as yourself @Kebap (a.k.a. Leris)?

@vadi2

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

What is the idea behind including people's Discord ID's? I don't see its use.

@Kebap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

I don't understand the current compilation error.

Thanks for the suggestion SlySven. That seems interesting.

@vadi2

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Ohh... That's what you asked about in Discord! I'm sorry, I thought you asked something else 😕

@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

There might be issues with createMakerHTML(...) as it is not a member of the dlgAboutDialog class...?

Also there is no need to mess around with a (std::unique_ptr<QTextDocument>) supportersDocument - a standard QTextDocument pointer is fine when it is instantiated in the class initialiser list with a new QTextDocument(this) as it will automagically get deleted when the this concerned (the dlgAboutDialog class) is destroyed... {indeed that is why many QObject derived classes take a parent pointer in their constructor - to ensure they get destroyed when the parent gets zapped!}

@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Sod it - I can't compile my fixes for this as I am on my laptop away from home and it only has GCC 6.x installed which is breaking on the fancy new C++17 stuff in int TLuaInterpreter::getLineNumber(lua_State* L) ... going to have to get home to test them. :man_shrug:

Kebap added 4 commits Aug 4, 2019
Kebap
Kebap
Kebap
Kebap
@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

@ahmedcharles is still a contributor even if no longer active - so why exterminate him?

@SlySven SlySven closed this Aug 4, 2019

@SlySven SlySven reopened this Aug 4, 2019

@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Ooops, wrong button hit there.

@Kebap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Look at the code, not the commit comment only. ;)

@vadi2

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

I was going to suggest to use a struct with named fields instead of the nameless list... 😊

@Kebap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

What is the idea behind including people's Discord ID's? I don't see its use.

Did you reach a conclusion?

@vadi2

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Nobody answered but I don't mind. Just don't see what use.

@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Well it helps to point out that some of the developers are on Discord and that is the handles they use (far enough once the user gets to Discord they may spot them from the Discord role colour) - why else do we include Github (and thus previously Gitter) and Email details?

Oh and I think there is a missing ' at the beginning of "Oneymus" as it is a Discord nickname and it was simpler to use single quotes rather than double ones for it...

@vadi2
vadi2 approved these changes Aug 5, 2019
Copy link
Member

left a comment

This works fine!

In the future I'd recommend using named fields. Comparing it to Lua, use a named key-value table instead of an indexed one - code makes more sense then and the data structure is properly semantically tagged.

@Kebap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

I plan updating QStringList to struct before merging. Thanks for the suggestion. This is all new to me, so I focus on SlySven's suggestion first to reduce confusion.

Another issue: Maker list on website lists launchpad details instead. I assume those are obsolete?

@vadi2

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

I agree, we can remove those.

@Kebap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Website adjustments come with #1856

Kebap and others added 4 commits Aug 5, 2019

@Kebap Kebap added this to the 4.1.0 milestone Aug 5, 2019

@Kebap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Codereview was a bit underwhelming.
This is ready for review or to merge now. Not seeing any objections left?

@vadi2
vadi2 approved these changes Aug 6, 2019
Copy link
Member

left a comment

👌 very nice!

{
QString coloredText("<span style=\"color:#%1;\">%2</span>");
QStringList contactDetails;
if (!maker.discord.isEmpty()) {contactDetails.append(coloredText.arg("7289DA", maker.discord));}

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 7, 2019

Member

Remember we like even single lines of the ifed code to be indented on another line - though I suspect clang-format will sort that out...

This comment has been minimized.

Copy link
@Kebap

Kebap Sep 16, 2019

Author Contributor

Is that so? I don't use clang-format, and I didn't find anything about that in commit guidelines. Adjusted in this case but will probably forget in next commit.

@@ -183,6 +254,22 @@ void dlgAboutDialog::setAboutTab(const QString& htmlHead) const
// clang-format on
}

QString dlgAboutDialog::createMakerHTML(const aboutMaker maker) const
{
QString coloredText("<span style=\"color:#%1;\">%2</span>");

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 7, 2019

Member

I am not sure this helps much - you will need to mark it up as a QStringLiteral - to avoid QT_NO_CAST_FROM_ASCII issues in the future (if we ever get around to defining it project wide) and from a somewhat biased viewpoint I am not sure that it is too harmful to have similar HTML Qt Rich Text repeated three times...

This comment has been minimized.

Copy link
@Kebap

Kebap Sep 16, 2019

Author Contributor

I like the additional layer of indirection for improved readability. Mixing HTML code and C++ code too much seems too confusing. Marked as QStringLiteral nonetheless.

QString coloredText("<span style=\"color:#%1;\">%2</span>");
QStringList contactDetails;
if (!maker.discord.isEmpty()) {contactDetails.append(coloredText.arg("7289DA", maker.discord));}
if (!maker.github.isEmpty()) {contactDetails.append(coloredText.arg("40b040", maker.github));}

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 7, 2019

Member

Um, the color string here and in three other place (1 above, 2 below) also should have QStringLiteral(....) wrappings... 😉

This comment has been minimized.

Copy link
@Kebap

Kebap Sep 16, 2019

Author Contributor

Committed

@@ -42,6 +50,7 @@ class dlgAboutDialog : public QDialog, public Ui::about_dialog
void setLicenseTab(const QString& htmlHead) const;
void setThirdPartyTab(const QString& htmlHead) const;
void setSupportersTab(const QString &htmlHead);
QString createMakerHTML(const aboutMaker) const;

This comment has been minimized.

Copy link
@SlySven

SlySven Aug 7, 2019

Member

How about using a reference instead - to save the argument copy? I.e. change to:

    QString createMakerHTML(const aboutMaker&) const;

This comment has been minimized.

Copy link
@Kebap

Kebap Sep 16, 2019

Author Contributor

Committed

@Kebap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Here is finally some interesting feedback from Code Review, including:

Avoid explicitly constructing QString and QStringLiteral objects

In many cases, it's totally unnecessary to explicitly write QString("...") or QStringLiteral("..."). Constant string literals will be implicitly converted to QString in many cases. Also, while QStringLiteral might avoid a copy in some cases, it looks like premature optimization to me.

This runs a bit contrary to my recent doing and the perceived standard in Mudlet project.
Using string literals would certainly increase readability. What are our main drawbacks?

@vadi2

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

It's a Qt best practice to use it, so I'd say that commenter isn't quite up to date on it. See https://woboq.com/blog/qstringliteral.html and Qt Creator's code.

The rest of the comments they have are good, you already got a struct going :)

Because this is the only place where we such heavy templating, we don't need to use a separate library for it - extra dependencies aren't worth it.

@SlySven

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Avoid explicitly constructing QString and QStringLiteral objects

In many cases, it's totally unnecessary to explicitly write QString("...") or QStringLiteral("..."). Constant string literals will be implicitly converted to QString in many cases. Also, while QStringLiteral might avoid a copy in some cases, it looks like premature optimization to me.

This runs a bit contrary to my recent doing and the perceived standard in Mudlet project.
Using string literals would certainly increase readability. What are our main drawbacks?

I am guessing that the commenter is an American not used to I18n!

There is another reason for using the QStringLiteral (and QLatin1String) constructs - they explicitly mark a QString as one which is not required to be translated - in the absence of one of them - or the QObject::tr construct. Indeed, try adding:

DEFINES += QT_NO_CAST_FROM_ASCII

to mudlet.pro and you will see how many raw strings are being converted to QStrings without such indications (though modern compilers do tend to bail out of finishing a compilation unit {.cpp} file after 20 errors - so the error numbers are likely to be very conservative)... 💡

@vadi2 vadi2 modified the milestones: 4.1.0, 4.2.0 Sep 8, 2019

Kebap added 3 commits Sep 16, 2019
@vadi2
vadi2 approved these changes Sep 16, 2019
Kebap added 2 commits Sep 16, 2019

@Kebap Kebap merged commit ed922b8 into development Sep 16, 2019

8 of 12 checks passed

Run only on translation PRs Run only on translation PRs
Details
label PR label PR
Details
Filters for GitHub Actions Filters for GitHub Actions
Details
Run only on translation PRs Run only on translation PRs
Details
Create PR Create PR
Details
automerge automerge
Details
automerge automerge
Details
CodeFactor 2 issues fixed.
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.