gui: Disable unavailable context menu items in transactions tab

kristapsk commented at 1:01 pm on January 18, 2020: contributor

Fixes #9192.

fanquake added the label GUI on Jan 18, 2020

hebasto commented at 2:54 pm on January 18, 2020: member

@kristapsk screenshots “before” and “after” are welcome for GUI-related PRs ;)

kristapsk commented at 3:01 pm on January 18, 2020: contributor

@hebasto Functionality seemed so simple and obvious (just menu items being disabled) that there isn’t need for screenshots. But could add. All four pairs of possible before and after screenshots?

in src/qt/transactionview.cpp:397 in 5ce6599f70 outdated

395@@ -395,6 +396,8 @@ void TransactionView::contextualMenu(const QPoint &point)
396     hash.SetHex(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString());
397     abandonAction->setEnabled(model->wallet().transactionCanBeAbandoned(hash));
398     bumpFeeAction->setEnabled(model->wallet().transactionCanBeBumped(hash));
399+    copyAddressAction->setEnabled(GUIUtil::hasEntryData(transactionView, 0, TransactionTableModel::AddressRole));

hebasto commented at 4:13 pm on January 18, 2020:

How to test this line of the code?

kristapsk commented at 4:24 pm on January 18, 2020:

I tested with some JoinMarket transactions, where I had partially imported JM wallet addresses as watchonly (some of tx inputs belonged to the wallet, but no output addresses).

jonasschnelli commented at 5:40 am on January 20, 2020: contributor

Nice! Concept ACK.

in src/qt/guiutil.cpp:256 in 5ce6599f70 outdated

249@@ -250,6 +250,15 @@ QList<QModelIndex> getEntryData(QAbstractItemView *view, int column)
250     return view->selectionModel()->selectedRows(column);
251 }
252 
253+bool hasEntryData(QAbstractItemView *view, int column, int role)
254+{
255+    QModelIndexList selection = getEntryData(view, column);
256+    if (!selection.isEmpty()) {

promag commented at 7:17 pm on January 26, 2020:

nit,

0if (selection.isEmpty()) return false
1return !selection.at(0).data(role).toString().isEmpty();

in src/qt/transactionview.h:80 in 5ce6599f70 outdated

76@@ -77,6 +77,8 @@ class TransactionView : public QWidget
77     QDateTimeEdit *dateTo;
78     QAction *abandonAction;
79     QAction *bumpFeeAction;
80+    QAction *copyAddressAction;

promag commented at 7:18 pm on January 26, 2020:

prefer QAction* copyAddressAction{nullptr}; here and below and drop changes to constructor initializer list.

in src/qt/guiutil.h:86 in 5ce6599f70 outdated

81+        nothing is selected.
82+       @param[in] column  Data column to extract from the model
83+       @param[in] role    Data role to extract from the model
84+       @see  TransactionView::contextualMenu
85+     */
86+    bool hasEntryData(QAbstractItemView *view, int column, int role);

promag commented at 7:21 pm on January 26, 2020:

Looks like this should be const but getEntryData is not. Would that be a big change?

promag commented at 7:22 pm on January 26, 2020: member

Concept ACK.

kristapsk commented at 8:17 pm on January 26, 2020: contributor

Looks like I fucked this up. Any hints how to undo this git merge mess?

kristapsk force-pushed on Jan 26, 2020

kristapsk commented at 8:21 pm on January 26, 2020: contributor

Ok, figured out myself, addressed comments by @promag.

nopara73 approved

nopara73 commented at 1:55 pm on February 8, 2020: none

Untested concept ACK, my original suggestion was the removal of non-functional context menu entries, but disabling is better, as it’s used already:

DrahtBot commented at 4:59 am on March 3, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

promag commented at 7:53 am on March 3, 2020: member

Concept ACK.

laanwj added the label Bug on Mar 27, 2020

in src/qt/guiutil.h:81 in f780d5a588 outdated

78      */
79-    QList<QModelIndex> getEntryData(QAbstractItemView *view, int column);
80+    QList<QModelIndex> getEntryData(const QAbstractItemView *view, int column);
81+
82+    /** Check whether a field of the currently selected entry of a view is not empty. Does nothing if
83+        nothing is selected.

jonatack commented at 1:16 pm on April 16, 2020:

This doc is for a boolean, so remove “Does nothing if nothing is selected.”

Suggested change (or something similar/simpler):

0-    /** Check whether a field of the currently selected entry of a view is not empty. Does nothing if
1-        nothing is selected.
2+    /** Returns true if the specified field of the currently selected view entry is not empty.

jonatack commented at 1:19 pm on April 16, 2020: member

Thanks, good to see user-facing improvements like this.

Tested ACK f780d5a588075efdcd6d6fd097b251190c8d5e7e modulo docstring fix.

Disable unavailable context menu items in transactions tab 2b18fd2242

kristapsk force-pushed on Apr 16, 2020

jonatack commented at 4:20 pm on April 16, 2020: member

Re-ACK 2b18fd2242a5899

git diff f780d5a 2b18fd2 shows the only change since the previous review is the bool docstring fixup

0-/** Check whether a field of the currently selected entry of a view is not empty. Does nothing if
1-    nothing is selected.
2+/** Returns true if the specified field of the currently selected view entry is not empty.

kristapsk commented at 7:56 pm on May 15, 2020: contributor

Is there anything anyone expects me to do more here or this is just forgotten in a long list of pending pull requests?

fanquake requested review from jonasschnelli on May 15, 2020

fanquake requested review from promag on May 15, 2020

jonatack commented at 8:43 am on May 16, 2020: member

@kristapsk the latter, I think. I agree this would be good, for now it needs a couple more reviewers’ eyes on it.

hebasto commented at 8:57 am on May 16, 2020: member

Concept ACK.

May I suggest to:

split out refactor-only changes into a separate commit
use standard prefixes in commit messages, e.g., refactor: ... and qt: ...
apply clang-format-diff.py for each commit

?

jonasschnelli commented at 8:28 am on May 29, 2020: contributor

codereview utACK 2b18fd2242a589988fbb68205dae4afa0b8b3d34

jonasschnelli merged this on May 29, 2020

jonasschnelli closed this on May 29, 2020

kristapsk deleted the branch on May 29, 2020

sidhujag referenced this in commit cf64aeca6b on May 31, 2020

Fabcien referenced this in commit 624c94dcaf on Aug 24, 2021

DrahtBot locked this on Feb 15, 2022

gui: Disable unavailable context menu items in transactions tab #17956

Conflicts