qt: Disable requests context menu actions when appropriate #214

jarolrod commented at 6:31 AM on February 19, 2021: member

The recent requests table will allow you to copy data points even if they do not exist. This PR implements checks to disable the copy label, copy message, and copy amount context menu actions if the respective fields are empty. This brings the recent requests table context menu behavior closer to the behavior seen in the transaction view.

On a payment request entry which does not have a value for label, message, or amount:

Master	PR
<img width="169" alt="Screen Shot 2021-02-19 at 1 22 28 AM" src="https://user-images.githubusercontent.com/23396902/108466086-167adc00-7251-11eb-8bd6-13984042bdb3.png">	<img width="169" alt="Screen Shot 2021-02-19 at 1 21 49 AM" src="https://user-images.githubusercontent.com/23396902/108466185-3e6a3f80-7251-11eb-9dd8-492ed07fd638.png">

copy URI never needs to be disabled as an entry in the recent requests table must have a URI even if it doesn't have a label, message, or amount. #213 will add a copy address context menu action. This also does not need a check as an entry must be associated with an address.

Below are some more examples of how this PR will behave: Has Label, Message, and Amount <img width="780" alt="Screen Shot 2021-02-19 at 12 05 38 AM" src="https://user-images.githubusercontent.com/23396902/108466507-c18b9580-7251-11eb-8875-f3aeb9c4c8e9.png">

Has Label and Amount, but no Message <img width="780" alt="Screen Shot 2021-02-19 at 12 05 58 AM" src="https://user-images.githubusercontent.com/23396902/108466421-9b65f580-7251-11eb-97eb-a3bfaa21fa7d.png">

jonasschnelli commented at 7:58 AM on February 19, 2021: contributor

utACK 69915b80ccf853b66345b3d612d24eddd3411a2a - I guess comparing against zero for an "empty" amount is acceptable here.

jarolrod commented at 8:13 AM on February 19, 2021: member

utACK 69915b8 - I guess comparing against zero for an "empty" amount is acceptable here.

Comparing against zero is what's used to determine (no amount requested) in recentrequeststablemodel.cpp Line 84

Talkless commented at 5:04 PM on February 20, 2021: none

Concept ACK.

in src/qt/receivecoinsdialog.cpp:278 in 69915b80cc outdated

 274 |      }
 275 | +
 276 | +    // disable context menu actions when appropriate
 277 | +    const RecentRequestsTableModel * const submodel = model->getRecentRequestsTableModel();
 278 | +    const RecentRequestEntry *req = &submodel->entry(sel.row());
 279 | +    copyLabelAction->setEnabled(!req->recipient.label.isEmpty());

Talkless commented at 5:08 PM on February 20, 2021:

nit: negations could be avoided (simplified) by using setDisabled instead (same for all three similar lines).

jarolrod commented at 9:46 PM on February 21, 2021:

addressed in 336a643

in src/qt/receivecoinsdialog.cpp:270 in 69915b80cc outdated

 266 | @@ -267,9 +267,18 @@ void ReceiveCoinsDialog::copyColumnToClipboard(int column)
 267 |  // context menu
 268 |  void ReceiveCoinsDialog::showMenu(const QPoint &point)
 269 |  {
 270 | -    if (!selectedRow().isValid()) {
 271 | +    QModelIndex sel = selectedRow();

Talkless commented at 5:08 PM on February 20, 2021:

nit: sel Could be const.

jarolrod commented at 9:46 PM on February 21, 2021:

addressed in 336a643

in src/qt/receivecoinsdialog.cpp:277 in 69915b80cc outdated

 273 |          return;
 274 |      }
 275 | +
 276 | +    // disable context menu actions when appropriate
 277 | +    const RecentRequestsTableModel * const submodel = model->getRecentRequestsTableModel();
 278 | +    const RecentRequestEntry *req = &submodel->entry(sel.row());

Talkless commented at 5:09 PM on February 20, 2021:

nit: Pointer req is not change within function, could be const RecentRequestEntry * const req (same as submodel above).

jarolrod commented at 9:45 PM on February 21, 2021:

addressed in 336a643

Talkless changes_requested

jarolrod force-pushed on Feb 21, 2021

jarolrod commented at 9:49 PM on February 21, 2021: member

updated 69915b8 -> 336a643

Addressed @Talkless suggestions, thanks for the review!

Changes:

variables that can be const are made const
refactor context menu action disable logic from using setEnabled with negations to using setDisabled without negations

in src/qt/receivecoinsdialog.cpp:277 in 336a643596 outdated

 273 |          return;
 274 |      }
 275 | +
 276 | +    // disable context menu actions when appropriate
 277 | +    const RecentRequestsTableModel * const submodel = model->getRecentRequestsTableModel();
 278 | +    const RecentRequestEntry * const req = &submodel->entry(sel.row());

MarcoFalke commented at 7:41 AM on February 22, 2021:

wouldn't a const reference make more sense?

    const RecentRequestEntry& req = submodel->entry(sel.row());

jarolrod commented at 2:17 AM on February 23, 2021:

addressed in 3f83f05f52be8f3bac2267d447b5037409cde17d

MarcoFalke commented at 7:42 AM on February 22, 2021: contributor

tested ACK 336a6435965dc813bdc2bf11398bce564e52c984

didn't review code

hebasto commented at 4:59 PM on February 22, 2021: member

Concept ACK.

in src/qt/receivecoinsdialog.h:59 in 336a643596 outdated

  53 | @@ -54,6 +54,9 @@ public Q_SLOTS:
  54 |      GUIUtil::TableViewLastColumnResizingFixer *columnResizingFixer;
  55 |      WalletModel *model;
  56 |      QMenu *contextMenu;
  57 | +    QAction *copyLabelAction;
  58 | +    QAction *copyMessageAction;
  59 | +    QAction *copyAmountAction;

hebasto commented at 5:10 PM on February 22, 2021:

style nit (see clang-format-diff.py):

    QAction* copyLabelAction;
    QAction* copyMessageAction;
    QAction* copyAmountAction;

jarolrod commented at 2:17 AM on February 23, 2021:

Addressed in 3f83f05f52be8f3bac2267d447b5037409cde17d

in src/qt/receivecoinsdialog.cpp:276 in 336a643596 outdated

 272 | +    if (!sel.isValid()) {
 273 |          return;
 274 |      }
 275 | +
 276 | +    // disable context menu actions when appropriate
 277 | +    const RecentRequestsTableModel * const submodel = model->getRecentRequestsTableModel();

hebasto commented at 5:11 PM on February 22, 2021:

style nit (see clang-format-diff.py):

    const RecentRequestsTableModel* const submodel = model->getRecentRequestsTableModel();

hebasto approved

hebasto commented at 5:12 PM on February 22, 2021: member

ACK 336a6435965dc813bdc2bf11398bce564e52c984, tested on Linux Mint 20.1 (Qt 5.12.8).

jarolrod force-pushed on Feb 23, 2021

jarolrod commented at 2:20 AM on February 23, 2021: member

updated from 336a643 -> 3f83f05f52be8f3bac2267d447b5037409cde17d, thanks @hebasto @MarcoFalke for the review

Changes:

Adhere to clang-format
req is made to be a const reference as that makes more sense

in src/qt/receivecoinsdialog.cpp:282 in 3f83f05f52 outdated

 275 | +    // disable context menu actions when appropriate
 276 | +    const RecentRequestsTableModel* const submodel = model->getRecentRequestsTableModel();
 277 | +    const RecentRequestEntry& req = submodel->entry(sel.row());
 278 | +    copyLabelAction->setDisabled(req.recipient.label.isEmpty());
 279 | +    copyMessageAction->setDisabled(req.recipient.message.isEmpty());
 280 | +    copyAmountAction->setDisabled(req.recipient.amount == 0);

hebasto commented at 7:27 AM on February 23, 2021:

    const auto& r = model->getRecentRequestsTableModel()->entry(sel.row()).recipient;
    copyLabelAction->setDisabled(r.label.isEmpty());
    copyMessageAction->setDisabled(r.message.isEmpty());
    copyAmountAction->setDisabled(r.amount == 0);

jarolrod commented at 5:23 PM on February 23, 2021:

Since 213 was merged with the style used in this PR, I'll close this for consistency

hebasto approved

hebasto commented at 7:28 AM on February 23, 2021: member

re-ACK 3f83f05f52be8f3bac2267d447b5037409cde17d, only suggested changes and rebased since my previous review, verified with

$ git range-diff master 336a6435965dc813bdc2bf11398bce564e52c984 3f83f05f52be8f3bac2267d447b5037409cde17d

DrahtBot added the label Needs rebase on Feb 23, 2021

qt: Disable requests context menu actions when appropriate

The recent requests table will allow you to copy data points even if they do not exist.
This PR implements checks to disable the 'copy label', 'copy message', and 'copy amount' context menu action if the respective fields are empty.

bb3da8fe41

jarolrod force-pushed on Feb 23, 2021

jarolrod commented at 5:24 PM on February 23, 2021: member

Updated from 3f83f05 -> bb3da8f

Changes: Resolve conflict with #213

DrahtBot removed the label Needs rebase on Feb 23, 2021

hebasto approved

hebasto commented at 6:41 PM on February 23, 2021: member

re-ACK bb3da8fe410285887f22679c6f08a1f40bcfac8f

luke-jr approved

luke-jr commented at 4:59 PM on February 24, 2021: member

utACK

MarcoFalke merged this on Feb 25, 2021

MarcoFalke closed this on Feb 25, 2021

jarolrod deleted the branch on Mar 8, 2021

gwillen referenced this in commit 1bbd69a6af on Jun 28, 2022

bitcoin-core locked this on Aug 16, 2022