pack of small cleanup fixes / optimizations#2334
Conversation
This commit removes 2 loops and a vector which I don't believe are necessary in CMasternode::FlagGovernanceItemsAsDirty. I could be missing something, but can't think of any good reason why this was implemented this way.
This is the entire purpose of the Get<X>String methods on MNP class.
| NetMsgType::MNBUDGETPROPOSAL, // deprecated since 12.1 | ||
| NetMsgType::MNBUDGETFINAL, // deprecated since 12.1 | ||
| NetMsgType::MNBUDGETFINALVOTE, // deprecated since 12.1 | ||
| NetMsgType::MNQUORUM, // not implemented |
There was a problem hiding this comment.
Can't do this, read the NOTE (line 83) ;)
NOTE: include non-implmented here, we must keep this list in sync with enum in protocol.h
There was a problem hiding this comment.
Yep, I see your point (and commented more below), but the fact that we must keep this list in sync with enum in protocol.h seems like a code smell to me. I'm also not convinced that an enum is actually the right data type, since we're just directly assigning values and they're part of the protocol. Really I think that's a poor case for an enum, since the values have to be maintained.
src/qt/masternodelist.cpp
Outdated
| strHTML += "<b>" + tr("Version") + ": </b>" + (mn.lastPing.nDaemonVersion > DEFAULT_DAEMON_VERSION ? GUIUtil::HtmlEscape(FormatVersion(mn.lastPing.nDaemonVersion)) : tr("Unknown")) + "<br>"; | ||
| strHTML += "<b>" + tr("Sentinel") + ": </b>" + (mn.lastPing.nSentinelVersion > DEFAULT_SENTINEL_VERSION ? GUIUtil::HtmlEscape(SafeIntVersionToString(mn.lastPing.nSentinelVersion)) : tr("Unknown")) + "<br>"; | ||
| strHTML += "<b>" + tr("Version") + ": </b>" + GUIUtil::HtmlEscape(mn.lastPing.GetDaemonString()) + "<br>"; | ||
| strHTML += "<b>" + tr("Sentinel") + ": </b>" + GUIUtil::HtmlEscape(mn.lastPing.GetSentinelString()) + "<br>"; |
There was a problem hiding this comment.
New code won't produce the same results - unlike RPC, GUI uses tr (i.e. translation) for the "Unknown" string.
There was a problem hiding this comment.
Yep, I see your point. But I'd really like to try and find a solution that doesn't duplicate the logic both here and in the GetString methods. Is there a way of providing translation for "Unknown" in the code for (e.g.) GetDaemonString instead?
There was a problem hiding this comment.
The problem is that it's common to see translated strings in GUI but not in RPC, that's why it was implemented this way. I don't see how to reuse it cleanly tbh, maybe only smth like
diff --git a/src/qt/masternodelist.cpp b/src/qt/masternodelist.cpp
index 266a14335..e2a42df4a 100644
--- a/src/qt/masternodelist.cpp
+++ b/src/qt/masternodelist.cpp
@@ -492,6 +492,9 @@ void MasternodeList::ShowQRCode(std::string strAlias) {
// Title above QR-Code
QString strQRCodeTitle = tr("Masternode Private Key");
+ std::string strDaemon = mn.lastPing.GetDaemonString();
+ std::string strSentinel = mn.lastPing.GetSentinelString();
+
// Create dialog text as HTML
QString strHTML = "<html><font face='verdana, arial, helvetica, sans-serif'>";
strHTML += "<b>" + tr("Alias") + ": </b>" + GUIUtil::HtmlEscape(strAlias) + "<br>";
@@ -500,8 +503,8 @@ void MasternodeList::ShowQRCode(std::string strAlias) {
strHTML += "<b>" + tr("IP") + ": </b>" + GUIUtil::HtmlEscape(strIP) + "<br>";
if (fFound) {
strHTML += "<b>" + tr("Protocol") + ": </b>" + QString::number(mn.nProtocolVersion) + "<br>";
- strHTML += "<b>" + tr("Version") + ": </b>" + (mn.lastPing.nDaemonVersion > DEFAULT_DAEMON_VERSION ? GUIUtil::HtmlEscape(FormatVersion(mn.lastPing.nDaemonVersion)) : tr("Unknown")) + "<br>";
- strHTML += "<b>" + tr("Sentinel") + ": </b>" + (mn.lastPing.nSentinelVersion > DEFAULT_SENTINEL_VERSION ? GUIUtil::HtmlEscape(SafeIntVersionToString(mn.lastPing.nSentinelVersion)) : tr("Unknown")) + "<br>";
+ strHTML += "<b>" + tr("Version") + ": </b>" + (strDaemon == "Unknown" ? tr("Unknown") : strDaemon.c_str()) + "<br>";
+ strHTML += "<b>" + tr("Sentinel") + ": </b>" + (strDaemon == "Unknown" ? tr("Unknown") : strSentinel.c_str()) + "<br>";
strHTML += "<b>" + tr("Status") + ": </b>" + GUIUtil::HtmlEscape(CMasternode::StateToString(mn.nActiveState)) + "<br>";
strHTML += "<b>" + tr("Payee") + ": </b>" + GUIUtil::HtmlEscape(CBitcoinAddress(mn.pubKeyCollateralAddress.GetID()).ToString()) + "<br>";
strHTML += "<b>" + tr("Active") + ": </b>" + GUIUtil::HtmlEscape(DurationToDHMS(mn.lastPing.sigTime - mn.sigTime)) + "<br>";There was a problem hiding this comment.
@UdjinM6 I've been thinking a bit more on this... is "Unknown" really necessary at all? Can we use a "0" version string, or an empty string instead? Just seems weird that we have these special processing rules based on the string content.
That said, I fully understand the tr() for Qt only, but if we're putting in a either version or "Unknown", then it's obviously for human consumption, so why not make it more consistent and remove these conditionals?
There was a problem hiding this comment.
Agree, maybe we should get rid of "Unknown" (less strings to translate, yay! :) ) but not in this (refactoring) PR imo.
| const char *MNBUDGETPROPOSAL="mprop"; // deprecated since 12.1 | ||
| const char *MNBUDGETFINAL="fbs"; // deprecated since 12.1 | ||
| const char *MNBUDGETFINALVOTE="fbvote"; // deprecated since 12.1 | ||
| const char *MNQUORUM="mn quorum"; // not implemented |
There was a problem hiding this comment.
Yeah, I didn't realize they needed to be in-sync (esp these NsgMsgType constants here, since those aren't an enum and just directly assigned values). I mean, in theory we could replace these with a single const char *NOTIMPLEMENTED = "notimplemented";, right?
But for the sake of not making any functional changes I'll revert this commit.
There was a problem hiding this comment.
Yep, NOTIMPLEMENTED would work I guess.
|
Reverted Qt version handling change, should be ready for re-review for small refactoring / cleanup. |
* remove vector, extra loop in cleanup function This commit removes 2 loops and a vector which I don't believe are necessary in CMasternode::FlagGovernanceItemsAsDirty. I could be missing something, but can't think of any good reason why this was implemented this way. * use range operator to range over vectors * remove deprecated wire message types * mn: simplify govobj map mgmt a bit * remove extra semicolons * invert if/else condition and drop else * remove if/else logic from Qt This is the entire purpose of the Get<X>String methods on MNP class. * Revert "remove deprecated wire message types" This reverts commit 9de88a3. * Revert "remove if/else logic from Qt" This reverts commit c0f43c9.
See individual commits for more info on each one.