[qt] navigate to transaction history page after send by Sjors · Pull Request #12421 · bitcoin/bitcoin

Sjors · 2018-02-13T11:03:26Z

Before this change QT just remained on the Send tab, which I found confusing. Now it switches to the Transactions tab. This makes it more clear to the user that the send actually succeeded, and here they can monitor progress.

Ideally I would like to highlight the transaction, e.g. by refactoring TransactionView::focusTransaction(const QModelIndex &idx) to accept a transaction hash, but I'm not sure how to do that.

laanwj · 2018-02-13T11:19:54Z

Concept ACK

maflcko · 2018-02-13T13:10:21Z

utACK ddbc5aaee1bdb69583eb711ad3145fa5f8b67933

promag · 2018-02-13T18:04:47Z

just remained on the Send tab, which I found confusing

IMHO this is more confusing, having such sudden UI change. What if he wants to do multiple sends?

How about adding a checkbox like [x] View transaction after sending and remembering the checkbox state in settings?

promag · 2018-02-13T18:07:15Z

src/qt/sendcoinsdialog.h

If the idea is to highlight the transaction, adding the txid or like can help.

I can extract the tx id and pass it along as a string. The part I couldn't figure out is how to convert that to whatever focusTransaction(const QModelIndex &idx) needs.

Nit, rename to coinsSent().

Regarding row focus, see https://stackoverflow.com/a/35154534.

Sjors · 2018-02-14T09:28:28Z

@promag I don't think I've ever seen an online banking application that doesn't take the user to the transaction overview page after sending money.

In most financial applications Send is a modal experience, where the Send screen is pushed on top of the transaction screen and popped when the user is done. In a desktop browser that's usually a modal window, on an iPhone it's a window that animates in from the right, with an arrow on the top left to dismiss it. Doing another send is then simply a matter of clicking on Send again.

Making the experience modal might be too much of a change, but e.g. we could get rid of all tabs: merge the Overview and Transactions tab, Send and Receive open a new window.

I do think highlighting the new transaction would give a better visual clue to the user as to why they moved to this other tab.

jonasschnelli · 2018-02-14T09:33:39Z

utACK ddbc5aaee1bdb69583eb711ad3145fa5f8b67933

promag

@Sjors like you say the current layout is very different from those you mention. Anyway, ACK with the row focus.

promag · 2018-02-14T16:48:30Z

src/qt/sendcoinsdialog.cpp

Remove comment? It's only relevant below when connecting.

promag · 2018-02-14T16:49:31Z

src/qt/sendcoinsdialog.h

Nit, rename to coinsSent().

Regarding row focus, see https://stackoverflow.com/a/35154534.

promag · 2018-02-14T17:00:23Z

src/qt/sendcoinsdialog.cpp

BTW, add

ui->checkBoxCoinControlChange->setChecked(false); ui->lineEditCoinControlChange->clear();

like #12432?

Coin control fields, custom change address and selected inputs, are already cleared after you send.

Please verify custom change address checkbox and line edit because here they are not cleared.

You're right. Turns out #12432 fixes that, because SendCoinsDialog::accept() is called when the transaction is sent, which in turn calls clear().

practicalswift · 2018-02-14T17:55:50Z

Concept ACK

randolf · 2018-02-15T02:31:41Z

Concept ACK.

Sjors · 2018-02-15T17:57:32Z

It now highlights the transaction.

promag · 2018-02-15T18:21:26Z

src/qt/transactionview.cpp

if (results.isEmpty()) return;

promag · 2018-02-15T18:21:42Z

src/qt/transactionview.cpp

Space after if, one line.

promag · 2018-02-15T18:22:19Z

src/qt/sendcoinsdialog.h

const QString& txid so that it's clear what the argument is?

promag · 2018-02-15T18:29:04Z

src/qt/transactionview.cpp

IIRC this performs linear search. Probably something worth to optimise in case of bigger wallets in the future.

Are transactions in reverse chronological order? In that case it should be pretty quick, unless some race condition causes the new transaction to be missing.

Transactions are displayed in whatever order the user chooses. I'm not sure what order is internally used, however...

I created a fresh wallet and then called generate 10000. I didn't notice any delay when sending transactions, although my machine is relatively fast. I also tried reversing transaction order in the view.

jonasschnelli · 2018-02-17T11:00:28Z

src/qt/sendcoinsdialog.cpp

Meh on using strings for internal uint256 transport.
Why not uint256? Could also register the type if not yet supported.

I tried passing the uint256 directly and got a bloodbath of compiler errors. How do I go about registering the type?

With Q_DECLARE_METATYPE, see

bitcoin/src/qt/bitcoin.cpp

Lines 80 to 82 in dcfe218

// Declare meta types used for QMetaObject::invokeMethod

Q_DECLARE_METATYPE(bool*)

Q_DECLARE_METATYPE(CAmount)

Sjors · 2018-02-19T15:29:03Z

Now passing the transaction hash as uint256.

promag

Tested ACK fdea382.

promag · 2018-02-19T16:03:39Z

src/qt/sendcoinsdialog.cpp

Can be removed.

promag · 2018-02-19T16:04:22Z

src/qt/sendcoinsdialog.h

Use reference? const uint256& txid.

promag · 2018-02-19T16:04:41Z

src/qt/transactionview.cpp

Use reference? const uint256& txid.

promag · 2018-02-20T14:09:49Z

reACK b7ebe39, only changes are the suggested above.

luke-jr · 2018-02-27T15:25:11Z

What if he wants to do multiple sends?

@promag Then he should do them all at once...

luke-jr · 2018-02-27T15:27:42Z

src/qt/transactionview.cpp

+
+    if (results.isEmpty()) return;
+
+    focusTransaction(results[0]);


We support selecting multiple transactions, so this ought to select all of them...

I consider more than 1 result an error, given that we're passing a single tx id. I could replace the isEmpty() with a check that length is 1.

It's definitely not an error. The transaction list shows logical transactions, not blockchain transactions. There is 1 txid per blockchain transaction, but that could be many logical transactions.

Ah, I see what you mean. E.g. sending to two destinations in one transaction results in two rows in the transactions tab.

@luke-jr 👍

promag · 2018-02-27T15:33:41Z

@luke-jr agree, came to the same conclusion little after.

Sjors · 2018-02-27T17:39:18Z

I'd like to select both entries on the transaction screen when a user sends to two addresses. Unfortunately:

this->model->getTransactionTableModel()->match(... only returns one entry.
transactionView->selectRow(...) deselects whichever row was previously selected (even though the user can manually select multiple transactions)

luke-jr · 2018-02-27T18:34:13Z

https://doc.qt.io/qt-5/qtableview.html#setSelection with QItemSelectionModel::Rows (and probably QItemSelectionModel::ClearAndSelect).

Why would match only return one entry??

Sjors · 2018-02-28T09:37:18Z

Why would match only return one entry??

I don't know. Here's a WIP commit. The fprintf(stderr, "row: line is only called once.

I'm confused about the relation between this->model->getTransactionTableModel(), transactionView and transactionProxyModel. E.g. what does QModelIndex targetIdx = transactionProxyModel->mapFromSource(idx); do? My intuitive grasp is that there's somehow multiple representations of the same data. Maybe @laanwj can explain the big picture here?

Sjors · 2018-02-28T11:21:17Z

I added @promag's commit to enable multi row selection. Note that if the user scrolls down, it won't scroll up. And if the user orders transactions by ascending date and scrolls down, it won't scroll further down to reveal the new transaction.

promag

Tested ACK after squash and comments addressed.

promag · 2018-02-28T12:21:16Z

src/qt/transactionview.cpp

We could ensure first row is visible:

for (...) { if (index == results[0]) transactionView->scrollTo(index);

That worked (using transactionProxyModel->mapFromSource) when transactions are ordered by date. Unfortunately when they're ordered in reverse it doesn't scroll all the way to the bottom. Maybe we need to wait a little bit for transactionView to become aware of its new size?

promag · 2018-02-28T12:22:42Z

src/qt/transactionview.cpp

Nit, add space after if.

promag · 2018-02-28T12:23:53Z

src/qt/sendcoinsdialog.cpp

Avoid copy here:

const uint256& txid = ...

Or roll it into the Q_EMIT, as txid is not used after this. YMMV. For performance I'd be surprised it makes any difference.

Q_EMIT coinsSent(currentTransaction.getTransaction()->GetHash());

Rolled into Q_EMIT.

laanwj · 2018-02-28T12:42:59Z

@promag I don't think I've ever seen an online banking application that doesn't take the user to the transaction overview page after sending money.
@promag Then he should do them all at once...

+1

laanwj

utACK

laanwj · 2018-02-28T12:47:39Z

src/qt/sendcoinsdialog.cpp

Or roll it into the Q_EMIT, as txid is not used after this. YMMV. For performance I'd be surprised it makes any difference.

Q_EMIT coinsSent(currentTransaction.getTransaction()->GetHash());

promag · 2018-02-28T16:49:23Z

src/qt/transactionview.cpp

On my system calling transactionView->scrollTo(targetIndex) for every model index fixes it (not sure why). It also works in the default order because scroll hint is QAbstractItemView::EnsureVisible, so focusing the 2nd row will not remove the 1st from the viewport (the 2nd row is already visible).

But it's also possible to order by other column (for instance address), and in that case it can be impossible to show the whole selection in the table viewport.

I see 3 possible solutions for that:

do nothing ;

automatically sort by date descending (new rows will be the first);

automatically fill the filter field with the new txid.

When I call scrollTo for every model index and transactions are sort by ascending date, things get weird: if I send to two destinations it scrolls correctly, but if I send to one destination it won't scroll far enough.

Calling scollTo() twice "fixes" it. Seems suspiciously similar to this ~~I'll poke around a bit~~.

I prefer your first solution for the case where transactions are not ordered by date. At least one of them will be in view.

@promag

The transaction will be selected. When sending to multiple destinations, all will be selected (thanks @promag).

e7d9fc5 [qt] navigate to transaction history page after send (Sjors Provoost) Pull request description: Before this change QT just remained on the Send tab, which I found confusing. Now it switches to the Transactions tab. This makes it more clear to the user that the send actually succeeded, and here they can monitor progress. Ideally I would like to highlight the transaction, e.g. by refactoring `TransactionView::focusTransaction(const QModelIndex &idx)` to accept a transaction hash, but I'm not sure how to do that. Tree-SHA512: 8aa93e03874de8434e18951f8aec47377814c0bcaf7eda4766fc41d5a4e32806346e12e4139e4d45468dfdf0b786f5a7faa393a31b8cd6c65ccac21fb3782c33

…e bump d795c61 [qt] TransactionView: highlight replacement tx after fee bump (Sjors Provoost) Pull request description: Consistent with #12421 which highlights the transaction after send. <img width="747" alt="1" src="proxy.php?url=https://user-images.githubusercontent.com/10217/38036280-a7358ea4-32a6-11e8-8f92-417e9e1e3e8b.png"> <img width="685" alt="2" src="proxy.php?url=https://user-images.githubusercontent.com/10217/38036289-aac87040-32a6-11e8-9f94-81745ff6c592.png"> ~I'm not too proud of the `QTimer::singleShot(10` bit; any suggestions on how to properly wait for the transactions table to become aware of the new transaction?~ Although I could have called `focusTransaction()` directly from `TransactionView::bumpFee()` I'm using the same signal as the send screen. This should make it easier to move fee bump / transaction replacement functionality around later. Tree-SHA512: 242055b7c3d32c7b2cf871f5ceda2581221902fd53fa29e0b092713fc16d3191adbe8cbb28417d522dda9febec8cc05e07afe3489cd7caaecd33460c1dde6fbc

…send e7d9fc5 [qt] navigate to transaction history page after send (Sjors Provoost) Pull request description: Before this change QT just remained on the Send tab, which I found confusing. Now it switches to the Transactions tab. This makes it more clear to the user that the send actually succeeded, and here they can monitor progress. Ideally I would like to highlight the transaction, e.g. by refactoring `TransactionView::focusTransaction(const QModelIndex &idx)` to accept a transaction hash, but I'm not sure how to do that. Tree-SHA512: 8aa93e03874de8434e18951f8aec47377814c0bcaf7eda4766fc41d5a4e32806346e12e4139e4d45468dfdf0b786f5a7faa393a31b8cd6c65ccac21fb3782c33

laanwj added the GUI label Feb 13, 2018

promag reviewed Feb 13, 2018

View reviewed changes

promag reviewed Feb 14, 2018

View reviewed changes

Sjors force-pushed the 2018/02/qt-goto-transactions-after-send branch 3 times, most recently from 26fd965 to 44c7768 Compare February 15, 2018 17:56

promag reviewed Feb 15, 2018

View reviewed changes

Sjors force-pushed the 2018/02/qt-goto-transactions-after-send branch from 44c7768 to ea04610 Compare February 16, 2018 15:47

jonasschnelli reviewed Feb 17, 2018

View reviewed changes

Sjors force-pushed the 2018/02/qt-goto-transactions-after-send branch from ea04610 to fdea382 Compare February 19, 2018 15:27

promag reviewed Feb 19, 2018

View reviewed changes

Sjors force-pushed the 2018/02/qt-goto-transactions-after-send branch from fdea382 to b7ebe39 Compare February 20, 2018 12:21

luke-jr reviewed Feb 27, 2018

View reviewed changes

promag reviewed Feb 28, 2018

View reviewed changes

laanwj approved these changes Feb 28, 2018

View reviewed changes

Sjors force-pushed the 2018/02/qt-goto-transactions-after-send branch 2 times, most recently from f22196f to 48ef8d8 Compare February 28, 2018 14:47

promag reviewed Feb 28, 2018

View reviewed changes

[qt] navigate to transaction history page after send

e7d9fc5

The transaction will be selected. When sending to multiple destinations, all will be selected (thanks @promag).

Sjors force-pushed the 2018/02/qt-goto-transactions-after-send branch from 48ef8d8 to e7d9fc5 Compare March 1, 2018 09:40

laanwj merged commit e7d9fc5 into bitcoin:master Mar 1, 2018

Sjors deleted the 2018/02/qt-goto-transactions-after-send branch March 1, 2018 14:09

ryanofsky mentioned this pull request Mar 7, 2018

Unify CWalletTx construction #9680

Merged

Sjors mentioned this pull request Mar 28, 2018

[qt] TransactionView: highlight replacement tx after fee bump #12818

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

	// Declare meta types used for QMetaObject::invokeMethod
	Q_DECLARE_METATYPE(bool*)
	Q_DECLARE_METATYPE(CAmount)

Conversation

Sjors commented Feb 13, 2018

Uh oh!

laanwj commented Feb 13, 2018

Uh oh!

maflcko commented Feb 13, 2018

Uh oh!

promag commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 14, 2018

Uh oh!

jonasschnelli commented Feb 14, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 14, 2018

Uh oh!

randolf commented Feb 15, 2018

Uh oh!

Sjors commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 19, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

promag Feb 15, 2018 •

edited

Loading

Sjors commented Feb 27, 2018 •

edited

Loading

Sjors commented Feb 28, 2018 •

edited

Loading