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

[qt] navigate to transaction history page after send #12421

Merged
merged 1 commit into from Mar 1, 2018

Conversation

Projects
None yet
8 participants
@Sjors
Member

Sjors commented Feb 13, 2018

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 laanwj added the GUI label Feb 13, 2018

@laanwj

This comment has been minimized.

Member

laanwj commented Feb 13, 2018

Concept ACK

@MarcoFalke

This comment has been minimized.

Member

MarcoFalke commented Feb 13, 2018

utACK ddbc5aa

@promag

This comment has been minimized.

Member

promag commented Feb 13, 2018

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?

@@ -54,6 +54,9 @@ public Q_SLOTS:
void setBalance(const CAmount& balance, const CAmount& unconfirmedBalance, const CAmount& immatureBalance,
const CAmount& watchOnlyBalance, const CAmount& watchUnconfBalance, const CAmount& watchImmatureBalance);
Q_SIGNALS:
void transactionSent();

This comment has been minimized.

@promag

promag Feb 13, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors Feb 14, 2018

Member

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.

This comment has been minimized.

@promag

promag Feb 14, 2018

Member

Nit, rename to coinsSent().

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

This comment has been minimized.

@Sjors

Sjors Feb 15, 2018

Member

Done

@Sjors

This comment has been minimized.

Member

Sjors commented Feb 14, 2018

@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

This comment has been minimized.

Member

jonasschnelli commented Feb 14, 2018

utACK ddbc5aa

@promag

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

@@ -369,6 +369,8 @@ void SendCoinsDialog::on_sendButton_clicked()
accept();
CoinControlDialog::coinControl()->UnSelectAll();
coinControlUpdateLabels();
// Navigate to transaction history page:

This comment has been minimized.

@promag

promag Feb 14, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors Feb 15, 2018

Member

Removed

@@ -54,6 +54,9 @@ public Q_SLOTS:
void setBalance(const CAmount& balance, const CAmount& unconfirmedBalance, const CAmount& immatureBalance,
const CAmount& watchOnlyBalance, const CAmount& watchUnconfBalance, const CAmount& watchImmatureBalance);
Q_SIGNALS:
void transactionSent();

This comment has been minimized.

@promag

promag Feb 14, 2018

Member

Nit, rename to coinsSent().

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

@@ -369,6 +369,8 @@ void SendCoinsDialog::on_sendButton_clicked()
accept();
CoinControlDialog::coinControl()->UnSelectAll();
coinControlUpdateLabels();
// Navigate to transaction history page:

This comment has been minimized.

@promag

promag Feb 14, 2018

Member

BTW, add

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

like #12432?

This comment has been minimized.

@Sjors

Sjors Feb 15, 2018

Member

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

This comment has been minimized.

@promag

promag Feb 15, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors Feb 15, 2018

Member

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

@practicalswift

This comment has been minimized.

Member

practicalswift commented Feb 14, 2018

Concept ACK

@randolf

This comment has been minimized.

Contributor

randolf commented Feb 15, 2018

Concept ACK.

@Sjors

This comment has been minimized.

Member

Sjors commented Feb 15, 2018

It now highlights the transaction.

TransactionTableModel::TxHashRole,
hash);
if (!results.length())

This comment has been minimized.

@promag

promag Feb 15, 2018

Member
if (results.isEmpty()) return;

This comment has been minimized.

@Sjors

Sjors Feb 16, 2018

Member

Fixed.

@@ -592,6 +592,22 @@ void TransactionView::focusTransaction(const QModelIndex &idx)
transactionView->setFocus();
}
void TransactionView::focusTransaction(const QString &hash)
{
if(!transactionProxyModel)

This comment has been minimized.

@promag

promag Feb 15, 2018

Member

Space after if, one line.

This comment has been minimized.

@Sjors

Sjors Feb 16, 2018

Member

Fixed.

@@ -54,6 +54,9 @@ public Q_SLOTS:
void setBalance(const CAmount& balance, const CAmount& unconfirmedBalance, const CAmount& immatureBalance,
const CAmount& watchOnlyBalance, const CAmount& watchUnconfBalance, const CAmount& watchImmatureBalance);
Q_SIGNALS:
void coinsSent(QString);

This comment has been minimized.

@promag

promag Feb 15, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors Feb 16, 2018

Member

Done.

if(!transactionProxyModel)
return;
const QModelIndexList results = this->model->getTransactionTableModel()->match(

This comment has been minimized.

@promag

promag Feb 15, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors Feb 16, 2018

Member

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.

This comment has been minimized.

@luke-jr

luke-jr Feb 27, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

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.

@@ -369,6 +370,8 @@ void SendCoinsDialog::on_sendButton_clicked()
accept();
CoinControlDialog::coinControl()->UnSelectAll();
coinControlUpdateLabels();
const uint256 txid = currentTransaction.getTransaction()->GetHash();
Q_EMIT coinsSent(QString::fromStdString(txid.ToString()));

This comment has been minimized.

@jonasschnelli

jonasschnelli Feb 17, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors Feb 19, 2018

Member

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

This comment has been minimized.

@promag

promag Feb 19, 2018

Member

With Q_DECLARE_METATYPE, see

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

@Sjors

This comment has been minimized.

Member

Sjors commented Feb 19, 2018

Now passing the transaction hash as uint256.

@@ -13,6 +13,7 @@
#include <qt/optionsmodel.h>
#include <qt/platformstyle.h>
#include <qt/sendcoinsentry.h>
#include <qt/transactiontablemodel.h>

This comment has been minimized.

@promag

promag Feb 19, 2018

Member

Can be removed.

This comment has been minimized.

@Sjors

Sjors Feb 20, 2018

Member

Fixed

@@ -54,6 +54,9 @@ public Q_SLOTS:
void setBalance(const CAmount& balance, const CAmount& unconfirmedBalance, const CAmount& immatureBalance,
const CAmount& watchOnlyBalance, const CAmount& watchUnconfBalance, const CAmount& watchImmatureBalance);
Q_SIGNALS:
void coinsSent(const uint256 txid);

This comment has been minimized.

@promag

promag Feb 19, 2018

Member

Use reference? const uint256& txid.

This comment has been minimized.

@Sjors

Sjors Feb 20, 2018

Member

Fixed

@@ -592,6 +591,21 @@ void TransactionView::focusTransaction(const QModelIndex &idx)
transactionView->setFocus();
}
void TransactionView::focusTransaction(const uint256 txid)

This comment has been minimized.

@promag

promag Feb 19, 2018

Member

Use reference? const uint256& txid.

@promag

This comment has been minimized.

Member

promag commented Feb 20, 2018

reACK b7ebe39, only changes are the suggested above.

@luke-jr

This comment has been minimized.

Member

luke-jr commented Feb 27, 2018

What if he wants to do multiple sends?

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

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

This comment has been minimized.

@luke-jr

luke-jr Feb 27, 2018

Member

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

This comment has been minimized.

@Sjors

Sjors Feb 27, 2018

Member

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.

This comment has been minimized.

@luke-jr

luke-jr Feb 27, 2018

Member

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.

This comment has been minimized.

@Sjors

Sjors Feb 27, 2018

Member

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

This comment has been minimized.

@promag
@promag

This comment has been minimized.

Member

promag commented Feb 27, 2018

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

@Sjors

This comment has been minimized.

Member

Sjors commented Feb 27, 2018

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

This comment has been minimized.

Member

luke-jr commented Feb 27, 2018

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

Why would match only return one entry??

@Sjors

This comment has been minimized.

Member

Sjors commented Feb 28, 2018

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

This comment has been minimized.

Member

Sjors commented Feb 28, 2018

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.

transactionView->setFocus();
transactionView->selectionModel()->clearSelection();
for (const QModelIndex& index : results) {

This comment has been minimized.

@promag

promag Feb 28, 2018

Member

We could ensure first row is visible:

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

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

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?

@@ -592,6 +591,25 @@ void TransactionView::focusTransaction(const QModelIndex &idx)
transactionView->setFocus();
}
void TransactionView::focusTransaction(const uint256& txid)
{
if(!transactionProxyModel)

This comment has been minimized.

@promag

promag Feb 28, 2018

Member

Nit, add space after if.

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

Fixed

@@ -369,6 +369,8 @@ void SendCoinsDialog::on_sendButton_clicked()
accept();
CoinControlDialog::coinControl()->UnSelectAll();
coinControlUpdateLabels();
const uint256 txid = currentTransaction.getTransaction()->GetHash();

This comment has been minimized.

@promag

promag Feb 28, 2018

Member

Avoid copy here:

const uint256& txid = ...

This comment has been minimized.

@laanwj

laanwj Feb 28, 2018

Member

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());

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

Rolled into Q_EMIT.

@laanwj

This comment has been minimized.

Member

laanwj commented Feb 28, 2018

@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

laanwj approved these changes Feb 28, 2018

utACK

@@ -369,6 +369,8 @@ void SendCoinsDialog::on_sendButton_clicked()
accept();
CoinControlDialog::coinControl()->UnSelectAll();
coinControlUpdateLabels();
const uint256 txid = currentTransaction.getTransaction()->GetHash();

This comment has been minimized.

@laanwj

laanwj Feb 28, 2018

Member

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());
transactionView->selectionModel()->select(
targetIndex,
QItemSelectionModel::Rows | QItemSelectionModel::Select);
if (index == results[0]) transactionView->scrollTo(targetIndex);

This comment has been minimized.

@promag

promag Feb 28, 2018

Member

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 :trollface:;
  • automatically sort by date descending (new rows will be the first);
  • automatically fill the filter field with the new txid.

This comment has been minimized.

@Sjors

Sjors Mar 1, 2018

Member

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.

[qt] navigate to transaction history page after send
The transaction will be selected. When sending to multiple
destinations, all will be selected (thanks @promag).

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 1, 2018

Merge #12421: [qt] navigate to transaction history page after 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

@Sjors Sjors deleted the Sjors:2018/02/qt-goto-transactions-after-send branch Mar 1, 2018

MarcoFalke added a commit that referenced this pull request Aug 20, 2018

Merge #12818: [qt] TransactionView: highlight replacement tx after fe…
…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="https://user-images.githubusercontent.com/10217/38036280-a7358ea4-32a6-11e8-8f92-417e9e1e3e8b.png">

  <img width="685" alt="2" src="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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment