name renew Qt GUI #376

JeremyRand · 2020-10-19T01:03:21Z

Port of the name renew Qt GUI to master branch.

Fixes #377 .

JeremyRand · 2020-10-19T01:07:54Z

@domob1812 I'm getting a Node context not found error when trying to call name_update in this code: https://github.com/namecoin/namecoin-core/pull/376/files#diff-25c46c9269e9ed05059493f6ac6057126358ee31e5cec55ab5c9726f326ba238R293-R309

I suspect that I'm creating the nameUpdateContext wrong (it looks like I'm supposed to use one that already exists, somewhere buried in a Node or Wallet object). Do you happen to know what I'm supposed to do here?

domob1812 · 2020-10-19T04:45:41Z

@JeremyRand You need to set that reference to an instance of NodeContext from node/context.h. Basically it contains a bunch of references to "globals" that have been refactored to no longer be globals (to improve modularity).

Most likely you are right and something should be readily available already somewhere, but I'm not 100% sure where. I guess you could look through the RPC handling code to find the place where normal RPC methods get it set.

JeremyRand · 2020-10-19T20:29:15Z

Okay, thanks @domob1812 . I'll see if I can trace where that's being stored.

JeremyRand · 2020-10-22T07:15:54Z

@JeremyRand You need to set that reference to an instance of NodeContext from node/context.h. Basically it contains a bunch of references to "globals" that have been refactored to no longer be globals (to improve modularity).

Most likely you are right and something should be readily available already somewhere, but I'm not 100% sure where. I guess you could look through the RPC handling code to find the place where normal RPC methods get it set.

@domob1812 I figured it out. Constructing a JSONRPCRequest manually is the wrong approach; the correct thing to do is call Node::executeRpc, which fills in the correct context automatically. In this case, the Node can be easily obtained via walletModel->node(), though it looks like a lot of the Qt classes have member variables for the Node. I pushed a fix to this PR, and everything appears to work properly now. I'll clean up the Git history, and then mark it as ready for review.

JeremyRand · 2020-10-22T11:32:04Z

Ready for review. Note that this PR includes a copy of #353 ; this PR should not be merged until #353 is merged and this PR is then rebased on top of master. But it should be fine to review now.

@randy-waterhouse You may be interested in reviewing this. Note that this PR only makes the Renew functionality accessible via the context menu; the button isn't added yet (that'll be in a subsequent PR). Also note that this PR supports renewing multiple names simultaneously; just select multiple names before you right-click.

randy-waterhouse · 2020-10-29T22:48:38Z

Ready for review. Note that this PR includes a copy of #353 ; this PR should not be merged until #353 is merged and this PR is then rebased on top of master. But it should be fine to review now.

@randy-waterhouse You may be interested in reviewing this. Note that this PR only makes the Renew functionality accessible via the context menu; the button isn't added yet (that'll be in a subsequent PR). Also note that this PR supports renewing multiple names simultaneously; just select multiple names before you right-click.

I've tested now and the crash on opening old wallet is fixed. I renewed a name with it and appears to work correctly. I think functionality is mostly there.

Things that need polishing:

expired names associated with keys in the wallet display in the Manage Names tab list, with a negative XX-XX-XX expiry date
in the Transactions tab "Label" column there is no information displayed about the name or name operation associated with a name operation transaction, only the public keys involved
Both of these issues were in Brandon's earlier version for name tab work.

JeremyRand · 2020-12-14T04:00:00Z

I fixed a minor bug in which the "Copy Name" and "Copy Value" context menu actions were still available when multiple names were selected (activating the action in this case would result in a randomly chosen name's data being copied); now those actions are disabled unless exactly one name is selected. Also rebased now that #353 is merged.

@domob1812 AFAICT this is ready to consider for merging. Can you review this?

JeremyRand · 2021-01-15T16:17:21Z

@domob1812 Any chance you could review this soon? I now have 2 other name management GUI PR's that are queued behind this PR; I'd prefer not to have the queue get any longer.

domob1812

Looks good, just a few minor things.

domob1812 · 2021-01-18T05:46:42Z

src/qt/managenamespage.cpp

+    QModelIndexList indexes = GUIUtil::getEntryData(ui->tableView, NameTableModel::Name);
+
+    bool singleNameSelected = indexes.size() == 1;
+    bool anyNamesSelected = indexes.size() >= 1;


Nit: These two can be marked const.

Good catch, will fix these shortly.

domob1812 · 2021-01-18T05:48:01Z

src/qt/managenamespage.cpp

+    if (!ctx.isValid ())
+        return;
+
+    for (QModelIndexList::iterator i = indexes.begin(); i != indexes.end(); i++)


Out of curiosity: Is it possible to use a C++11 "for each" loop here (I don't konw the details of QModelIndexList)?

Good catch. I just ran grep -r QModelIndex ./src/qt | grep for, and the existing three cases from upstream all use a for each loop. I'll adjust this code to match upstream style.

domob1812 · 2021-01-18T05:48:57Z

src/qt/nametablemodel.cpp

+       res = walletModel->node().executeRpc("name_update", params, walletURI);
+    }
+    catch (const UniValue& e) {
+        UniValue message = find_value( e, "message");


Nit: The whitespace is a bit messed up here (in particular the space between ( and e).

This style is copied from elsewhere in this file. I agree it should be fixed. Should I fix the rest of this file as part of this PR (in a separate commit) while we're thinking of it, or should I do that as a separate PR?

Really? I thought this was just a typo. I think just fixing the style here is fine for now.

JeremyRand · 2021-01-18T21:29:27Z

@domob1812 Fixes added as separate commits to make review easier; let me know when this passes review and I'll squash before you merge it.

domob1812 · 2021-01-19T05:25:30Z

Thanks for the fixes, ACK 3b349ee. When you squash them together, I will merge it.

Fixes namecoin#377 Based on namecoin#187 by Brandon Roberts.

JeremyRand · 2021-01-22T03:28:14Z

Thanks for the fixes, ACK 3b349ee. When you squash them together, I will merge it.

@domob1812 Squashed.

domob1812 · 2021-01-22T12:13:05Z

Thanks, merged.

JeremyRand mentioned this pull request Oct 19, 2020

name_list Qt GUI #353

Merged

JeremyRand force-pushed the qt-name-renew branch from e5fffd0 to cce3dc4 Compare October 22, 2020 11:01

JeremyRand force-pushed the qt-name-renew branch from cce3dc4 to 66c171d Compare October 22, 2020 16:35

JeremyRand added the qt label Oct 22, 2020

JeremyRand added the enhancement label Nov 2, 2020

JeremyRand force-pushed the qt-name-renew branch from 66c171d to a5f5ced Compare November 23, 2020 20:31

JeremyRand force-pushed the qt-name-renew branch 3 times, most recently from 0543958 to baa62b2 Compare December 14, 2020 03:52

JeremyRand changed the title ~~(WIP) name renew Qt GUI~~ name renew Qt GUI Dec 14, 2020

JeremyRand mentioned this pull request Jan 15, 2021

Add "Renew Names" button #386

Merged

domob1812 approved these changes Jan 18, 2021

View reviewed changes

JeremyRand force-pushed the qt-name-renew branch from 5ef3a7d to 3b349ee Compare January 18, 2021 21:26

Namecoin: Add Renew to Qt GUI context menu

e3506f4

Fixes namecoin#377 Based on namecoin#187 by Brandon Roberts.

JeremyRand force-pushed the qt-name-renew branch from 3b349ee to e3506f4 Compare January 22, 2021 03:27

domob1812 merged commit c007c77 into namecoin:master Jan 22, 2021

domob1812 added this to the nc0.22 milestone May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

name renew Qt GUI #376

name renew Qt GUI #376

JeremyRand commented Oct 19, 2020 •

edited

JeremyRand commented Oct 19, 2020

domob1812 commented Oct 19, 2020

JeremyRand commented Oct 19, 2020

JeremyRand commented Oct 22, 2020

JeremyRand commented Oct 22, 2020

randy-waterhouse commented Oct 29, 2020

JeremyRand commented Dec 14, 2020 •

edited

JeremyRand commented Jan 15, 2021

domob1812 left a comment

domob1812 Jan 18, 2021

JeremyRand Jan 18, 2021

domob1812 Jan 18, 2021

JeremyRand Jan 18, 2021

domob1812 Jan 18, 2021

JeremyRand Jan 18, 2021

domob1812 Jan 19, 2021

JeremyRand commented Jan 18, 2021

domob1812 commented Jan 19, 2021

JeremyRand commented Jan 22, 2021

domob1812 commented Jan 22, 2021

name renew Qt GUI #376

name renew Qt GUI #376

Conversation

JeremyRand commented Oct 19, 2020 • edited

JeremyRand commented Oct 19, 2020

domob1812 commented Oct 19, 2020

JeremyRand commented Oct 19, 2020

JeremyRand commented Oct 22, 2020

JeremyRand commented Oct 22, 2020

randy-waterhouse commented Oct 29, 2020

JeremyRand commented Dec 14, 2020 • edited

JeremyRand commented Jan 15, 2021

domob1812 left a comment

Choose a reason for hiding this comment

domob1812 Jan 18, 2021

Choose a reason for hiding this comment

JeremyRand Jan 18, 2021

Choose a reason for hiding this comment

domob1812 Jan 18, 2021

Choose a reason for hiding this comment

JeremyRand Jan 18, 2021

Choose a reason for hiding this comment

domob1812 Jan 18, 2021

Choose a reason for hiding this comment

JeremyRand Jan 18, 2021

Choose a reason for hiding this comment

domob1812 Jan 19, 2021

Choose a reason for hiding this comment

JeremyRand commented Jan 18, 2021

domob1812 commented Jan 19, 2021

JeremyRand commented Jan 22, 2021

domob1812 commented Jan 22, 2021

JeremyRand commented Oct 19, 2020 •

edited

JeremyRand commented Dec 14, 2020 •

edited