Overhauled the Buy Names Page with the Electrum-NMC version#558
Overhauled the Buy Names Page with the Electrum-NMC version#558junekomeiji wants to merge 1 commit intonamecoin:masterfrom
Conversation
src/qt/forms/buynamespage.ui
Outdated
| <resources> | ||
| <include location="../bitcoin.qrc"/> | ||
| </resources> | ||
| <resources/> |
There was a problem hiding this comment.
This is an Electrum-ism, we already have a (non-empty) resources element (above).
|
OK so initial reaction: the XML form changes here are really hard to review because there's too much happening in a single commit. For example, the text I think one way to improve this would be to split the XML form diff into two commits: one that exclusively tweaks whitespace on existing lines, and one that adds/removes lines. This should make it a lot easier for Git to tell which lines correspond in the diff. |
junekomeiji
force-pushed
the
domainforms
branch
2 times, most recently
from
8fb3d7f to
7c1b67d
Compare
|
@domob1812 Updated the codebase. |
junekomeiji
changed the title
|
I'd like @JeremyRand to review this, as he is the one who did the Qt code and I'm not really proficient with it. |
src/names/applications.cpp
Outdated
| return v.write(0,0); | ||
| } | ||
|
|
||
| //remind me to write test cases for them |
There was a problem hiding this comment.
It looks like this comment is no longer relevant?
src/qt/buynamespage.cpp
Outdated
| #include <string> | ||
| #include <algorithm> | ||
| #include <exception> |
There was a problem hiding this comment.
What are we using from these three headers?
There was a problem hiding this comment.
(To be clear, I'm not saying they are definitely superfluous, I'm just asking for info.)
src/qt/buynamespage.cpp
Outdated
| return NameTableModel::asciiToHex(name); | ||
| } | ||
|
|
||
| void BuyNamesPage::availableError() |
There was a problem hiding this comment.
It looks like this function has the same name as a variable inside it. The function name is also a bit confusing (it could be interpreted as something that you call when there's an error).
Can we rename this function to refreshAvailableError or something similar?
src/qt/buynamespage.cpp
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Is there a reason we have two blank lines here rather than one?
src/qt/buynamespage.cpp
Outdated
| } | ||
| else | ||
| { | ||
| ui->statusLabel->setText(tr("%1 is not a valid Namecoin domain!").arg(name)); |
There was a problem hiding this comment.
Might be worth putting %1 inside double-quotes so that it's a bit less ambiguous. Also can we make the text a bit more helpful? Is this just checking for the .bit suffix? If so, maybe say in the error text that the .bit suffix is expected?
|
Based on code review of 7c1b67d, only a few small issues (see review above). Also please change the commit message to something more descriptive. I'll do a final round of testing in the next 24 hours but I'm not expecting to see any major problems there. |
|
Should I fix the issues highlighted above now, or is it better if I wait for your final checks on the PR? |
@junekomeiji Feel free to fix them now. |
src/qt/buynamespage.cpp
Outdated
| { | ||
| return QString::fromStdString(ASCIIFromDomain(name.toStdString())); | ||
| } | ||
| else return QString(""); |
There was a problem hiding this comment.
else isn't needed here, since the then clause of this if statement will always return before we reach this point. Please remove else and add a blank line between the if statement and the return.
There was a problem hiding this comment.
The change here isn't correct -- return is still needed, just not else.
src/qt/buynamespage.cpp
Outdated
| { | ||
| return QString::fromStdString(DescFromName(DecodeName(name.toStdString(), NameEncoding::ASCII), NameNamespace::Domain)); | ||
| } | ||
| else return QString(""); |
There was a problem hiding this comment.
The change here isn't correct -- return is still needed, just not else.
src/qt/buynamespage.cpp
Outdated
|
|
||
| const QString availableError = name_available(name); | ||
| QString BuyNamesPage::DomainToASCII(const QString &name){ | ||
|
|
There was a problem hiding this comment.
I personally think this blank line can be removed, but if it's not removed, at least remove the spaces from this line.
src/names/applications.cpp
Outdated
| } | ||
|
|
||
| bool | ||
| IsPurportedNamecoinDomain (const std::string& domain){ |
There was a problem hiding this comment.
Please put a space between the close-parenthesis and the open-brace.
src/names/applications.cpp
Outdated
| } | ||
|
|
||
| std::string | ||
| ASCIIFromDomain(const std::string& domain){ |
There was a problem hiding this comment.
Please put a space between the close-parenthesis and the open-brace.
src/qt/buynamespage.cpp
Outdated
| return; | ||
|
|
||
| const QString availableError = name_available(name); | ||
| QString BuyNamesPage::DomainToASCII(const QString &name){ |
There was a problem hiding this comment.
Please put a space between the close-parenthesis and the open-brace.
src/qt/buynamespage.cpp
Outdated
| } | ||
|
|
||
|
|
||
| QString BuyNamesPage::ASCIIToDomain(const QString &name){ |
There was a problem hiding this comment.
Please put a space between the close-parenthesis and the open-brace.
src/qt/buynamespage.cpp
Outdated
| } | ||
|
|
||
|
|
||
| QString BuyNamesPage::HexToASCII(const QString &name){ |
There was a problem hiding this comment.
Please put a space between the close-parenthesis and the open-brace.
src/qt/buynamespage.cpp
Outdated
| } | ||
|
|
||
|
|
||
| QString BuyNamesPage::ASCIIToHex(const QString &name){ |
There was a problem hiding this comment.
Please put a space between the close-parenthesis and the open-brace.
src/qt/buynamespage.cpp
Outdated
|
|
||
| } | ||
|
|
||
| void BuyNamesPage::onDomainNameEdited(const QString &name){ |
There was a problem hiding this comment.
Please put a space between the close-parenthesis and the open-brace.
src/qt/buynamespage.cpp
Outdated
| } | ||
| catch(InvalidNameString &e) | ||
| { | ||
| ui->statusLabel->setText(tr("Not a valid entry!")); |
There was a problem hiding this comment.
Maybe say hex entry instead of entry for consistency with the ASCII codepath?
There was a problem hiding this comment.
Looks like you missed this one?
src/qt/buynamespage.cpp
Outdated
| if (!walletModel) | ||
| return; | ||
|
|
||
| QString input, name; |
There was a problem hiding this comment.
I think input is unused here?
Since it's unused, and name is the only remaining variable being declared here, I think the next line (initializing name) can be combined with this line.
src/qt/buynamespage.cpp
Outdated
| } | ||
|
|
||
| if (availableError == "") | ||
| void BuyNamesPage::refreshAvailableError() |
There was a problem hiding this comment.
Per the Developer Notes docs, please name this method via UpperCamelCase.
|
Quite a few of the added lines have trailing whitespace. If you run |
|
Tested b035e5c, all functionality works fine for me. Just some small code style issues (see review above). |
|
Responded to the review above. |
src/qt/buynamespage.cpp
Outdated
| if (!walletModel) | ||
| return; | ||
|
|
||
| try{ |
There was a problem hiding this comment.
Please either put the brace on a separate line, or add a space between try and the brace.
There's still one in |
junekomeiji
force-pushed
the
domainforms
branch
2 times, most recently
from
8e50de5 to
c73cc2e
Compare
|
ACK 558bad4. Code review looks OK to me, unit tests pass, and manual testing of the GUI doesn't show any problems. |
|
@domob1812 Feel free to merge. |
3 participants
Overhauled the Buy Names Page with the Electrum-NMC version and added error boxes for invalid domain names.