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

name_update Qt GUI #394

Merged
merged 2 commits into from
May 11, 2021
Merged

name_update Qt GUI #394

merged 2 commits into from
May 11, 2021

Conversation

JeremyRand
Copy link
Member

@JeremyRand JeremyRand commented Jan 17, 2021

This PR adds the name_update GUI.

@JeremyRand
Copy link
Member Author

@randy-waterhouse You may be interested in reviewing this.

@JeremyRand
Copy link
Member Author

JeremyRand commented Jan 25, 2021

Updated PR to use Optional in a few places where it makes sense. Will squash after review (prior to a merge).

@JeremyRand JeremyRand force-pushed the qt-name-update branch 2 times, most recently from 1ec36f1 to c6baa03 Compare January 29, 2021 01:55
@JeremyRand JeremyRand changed the title (WIP) name_update Qt GUI name_update Qt GUI Jan 29, 2021
@JeremyRand
Copy link
Member Author

@domob1812 This PR is ready for review.

@JeremyRand
Copy link
Member Author

@domob1812 Any chance you could review this?

Copy link

@domob1812 domob1812 left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay and thanks for this PR! Looks good (with the usual caveat that I'm not really a Qt expert so could only review those parts lightly), I've just marked some tiny nits.

src/qt/forms/configurenamedialog.ui Outdated Show resolved Hide resolved
src/qt/managenamespage.cpp Outdated Show resolved Hide resolved
src/qt/managenamespage.cpp Outdated Show resolved Hide resolved
@JeremyRand
Copy link
Member Author

JeremyRand commented May 8, 2021

@domob1812 Addressed feedback (and also fixed some minor formatting issues in configurenamedialog.ui, most notably a bug that broke the hyperlink coloring in Dark Mode). Let me know if this is good, and I'll squash before you merge it.

Copy link

@domob1812 domob1812 left a comment

Choose a reason for hiding this comment

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

ACK d0bb93d. Please squash the commits, then I'll merge the PR.

@JeremyRand
Copy link
Member Author

One more quick fix before we merge this; the latest commit prevents the name from being displayed as HTML in the Configure dialog. This is probably not easy for an attacker to exploit (they'd need to send you a name, and get you to try to update it, and then they'd have to hope that you don't notice that the Manage Names list shows the name differently from the Configure dialog), but still a good idea to fix.

@domob1812 Let me know if this final commit is okay, then I'll squash everything.

@domob1812
Copy link

One more quick fix before we merge this; the latest commit prevents the name from being displayed as HTML in the Configure dialog. This is probably not easy for an attacker to exploit (they'd need to send you a name, and get you to try to update it, and then they'd have to hope that you don't notice that the Manage Names list shows the name differently from the Configure dialog), but still a good idea to fix.

@domob1812 Let me know if this final commit is okay, then I'll squash everything.

Yes, that is ok of course.

@JeremyRand
Copy link
Member Author

@domob1812 Squashed. Left the registerName commit separate since it's atomic to the main commit.

@domob1812 domob1812 merged commit 5db3842 into namecoin:master May 11, 2021
@domob1812
Copy link

domob1812 commented May 11, 2021

For the record, this broke the build for me:

In file included from qt/configurenamedialog.cpp:1:
./qt/configurenamedialog.h:4:10: fatal error: optional.h: No such file or directory
 #include <optional.h>
          ^~~~~~~~~~~~

I guess that's trivial to fix, will do now. @JeremyRand did it build for you (I assume so)?

I believe upstream Bitcoin recently removed their custom Optional, switching to C++17's std::optional.

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request May 11, 2021
namecoin#394 (name_update Qt UI)
was still using the old, custom Optional implementation.  This fixes the
code to use std::optional, which has been introduced into the main
codebase some time ago.
@domob1812
Copy link

Fixed in 67a184a.

@JeremyRand
Copy link
Member Author

For the record, this broke the build for me:

In file included from qt/configurenamedialog.cpp:1:
./qt/configurenamedialog.h:4:10: fatal error: optional.h: No such file or directory
 #include <optional.h>
          ^~~~~~~~~~~~

I guess that's trivial to fix, will do now. @JeremyRand did it build for you (I assume so)?

I believe upstream Bitcoin recently removed their custom Optional, switching to C++17's std::optional.

@domob1812 It did build for me, but I hadn't rebased for a couple of months. Usually this is a good thing since frequent rebasing makes review more difficult, and typically any issues caused by not rebasing will be caught by Cirrus. Alas, Cirrus is routinely failing on master for other reasons, which I guess caused us to miss this one. I suppose this is a good reason to repair the other failures.

@domob1812 domob1812 added this to the nc0.22 milestone May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants