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: Make it possible to show details for multiple transactions #7939

Merged
merged 2 commits into from Apr 28, 2016

Conversation

Projects
None yet
3 participants
@laanwj
Member

laanwj commented Apr 25, 2016

A small GUI annoyance for me has always been that it's impossible to have multiple transaction detail windows open, for example to compare transactions.

This patch makes the window non-modal so that it is possible to open transaction details at will.

@laanwj laanwj added the GUI label Apr 25, 2016

qt: Make it possible to show details for multiple transactions
A small GUI annoyance for me has always been that it's impossible to
have multiple transaction detail windows open, for example to compare
transactions.

This patch makes the window non-modal so that it is possible to open
transaction details at will.
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 25, 2016

Member

Nice! Also though about this. Thanks.

utACK 1f3b3fd48bb6c5787893529862430bd77b2c7fb3

Member

jonasschnelli commented Apr 25, 2016

Nice! Also though about this. Thanks.

utACK 1f3b3fd48bb6c5787893529862430bd77b2c7fb3

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 25, 2016

Member

Another thing to consider here now may be to add some information to the window title. After all if there are multiple open it may be useful to distinguish them in taskbars etc.

I'm not sure on what to add though; transaction id seems to be of limited usefulness. Maybe date/recipient/amount.

Member

laanwj commented Apr 25, 2016

Another thing to consider here now may be to add some information to the window title. After all if there are multiple open it may be useful to distinguish them in taskbars etc.

I'm not sure on what to add though; transaction id seems to be of limited usefulness. Maybe date/recipient/amount.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 25, 2016

Contributor

Numbering the windows?

Contributor

paveljanik commented Apr 25, 2016

Numbering the windows?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 25, 2016

Member

Numbering doesn't seem more useful to me than just keeping them with the same window title. It needs to actually identify the transaction.
(also this opens up issues with reusing or not reusing numbers, I'd rather just add (part of) the transaction id in that case)

Member

laanwj commented Apr 25, 2016

Numbering doesn't seem more useful to me than just keeping them with the same window title. It needs to actually identify the transaction.
(also this opens up issues with reusing or not reusing numbers, I'd rather just add (part of) the transaction id in that case)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 25, 2016

Member

What about the shortened transaction id (middle truncated). Something like 4d8fad...8dd9f9 for 4d8fad408da55d4b54e3be97960db06d78489f5984e0a6e8ada082295e8dd9f9?

Member

jonasschnelli commented Apr 25, 2016

What about the shortened transaction id (middle truncated). Something like 4d8fad...8dd9f9 for 4d8fad408da55d4b54e3be97960db06d78489f5984e0a6e8ada082295e8dd9f9?

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 25, 2016

Contributor

Full tx ID. window manager/... will shorten it if needed (ie. Transaction details: tx_id).

Contributor

paveljanik commented Apr 25, 2016

Full tx ID. window manager/... will shorten it if needed (ie. Transaction details: tx_id).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 25, 2016

Member

To make this work on OSX (not sure about Windows) you need to make the dialog non-modal.

diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
index eb6111e..bfdd7a4 100644
--- a/src/qt/transactionview.cpp
+++ b/src/qt/transactionview.cpp
@@ -480,6 +480,7 @@ void TransactionView::showDetails()
     {
         TransactionDescDialog *dlg = new TransactionDescDialog(selection.at(0));
         dlg->setAttribute(Qt::WA_DeleteOnClose);
+        dlg->setModal(false);
         dlg->show();
     }
 }
Member

jonasschnelli commented Apr 25, 2016

To make this work on OSX (not sure about Windows) you need to make the dialog non-modal.

diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
index eb6111e..bfdd7a4 100644
--- a/src/qt/transactionview.cpp
+++ b/src/qt/transactionview.cpp
@@ -480,6 +480,7 @@ void TransactionView::showDetails()
     {
         TransactionDescDialog *dlg = new TransactionDescDialog(selection.at(0));
         dlg->setAttribute(Qt::WA_DeleteOnClose);
+        dlg->setModal(false);
         dlg->show();
     }
 }
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 25, 2016

Member

I tested with the full transaction-ID as window title. LGTM.
bildschirmfoto 2016-04-25 um 21 27 50

diff --git a/src/qt/transactiondescdialog.cpp b/src/qt/transactiondescdialog.cpp
index f7b6995..5c10d84 100644
--- a/src/qt/transactiondescdialog.cpp
+++ b/src/qt/transactiondescdialog.cpp
@@ -15,6 +15,7 @@ TransactionDescDialog::TransactionDescDialog(const QModelIndex &idx, QWidget *pa
 {
     ui->setupUi(this);
     QString desc = idx.data(TransactionTableModel::LongDescriptionRole).toString();
+    setWindowTitle(tr("Details for %1").arg(idx.data(TransactionTableModel::TxHashRole).toString()));
     ui->detailText->setHtml(desc);
 }
Member

jonasschnelli commented Apr 25, 2016

I tested with the full transaction-ID as window title. LGTM.
bildschirmfoto 2016-04-25 um 21 27 50

diff --git a/src/qt/transactiondescdialog.cpp b/src/qt/transactiondescdialog.cpp
index f7b6995..5c10d84 100644
--- a/src/qt/transactiondescdialog.cpp
+++ b/src/qt/transactiondescdialog.cpp
@@ -15,6 +15,7 @@ TransactionDescDialog::TransactionDescDialog(const QModelIndex &idx, QWidget *pa
 {
     ui->setupUi(this);
     QString desc = idx.data(TransactionTableModel::LongDescriptionRole).toString();
+    setWindowTitle(tr("Details for %1").arg(idx.data(TransactionTableModel::TxHashRole).toString()));
     ui->detailText->setHtml(desc);
 }
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 26, 2016

Member

@jonasschnelli: Interesting, do you know why is this necessary specifically here, how about the other places where we use setAttribute(Qt::WA_DeleteOnClose)?

  • src/qt/receivecoinsdialog.cpp: on_receiveButton_clicked, on_recentRequestsView_doubleClicked
  • src/qt/walletview.cpp: WalletView::gotoSignMessageTab, WalletView::gotoVerifyMessageTab

(excluding the splash window and the shutdown window as they are special cases anyway)

Member

laanwj commented Apr 26, 2016

@jonasschnelli: Interesting, do you know why is this necessary specifically here, how about the other places where we use setAttribute(Qt::WA_DeleteOnClose)?

  • src/qt/receivecoinsdialog.cpp: on_receiveButton_clicked, on_recentRequestsView_doubleClicked
  • src/qt/walletview.cpp: WalletView::gotoSignMessageTab, WalletView::gotoVerifyMessageTab

(excluding the splash window and the shutdown window as they are special cases anyway)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 26, 2016

Member

From the QDialog documentation:
http://doc.qt.io/qt-5/qdialog.html#modal-prop "By default, this property is false"

It's only dlg->exec() which sets it to true, which we don't use anymore (here). I don't think we need to set it.

Edit: screenshot looks good, I do think we should prefix the title Transaction details: though so we don't get an all-hex title :)

Member

laanwj commented Apr 26, 2016

From the QDialog documentation:
http://doc.qt.io/qt-5/qdialog.html#modal-prop "By default, this property is false"

It's only dlg->exec() which sets it to true, which we don't use anymore (here). I don't think we need to set it.

Edit: screenshot looks good, I do think we should prefix the title Transaction details: though so we don't get an all-hex title :)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 26, 2016

Contributor

Works OK here, even without setModal(false).

Contributor

paveljanik commented Apr 26, 2016

Works OK here, even without setModal(false).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 26, 2016

Member

Right. setModal() is not required. Was a local issue here.

Member

jonasschnelli commented Apr 26, 2016

Right. setModal() is not required. Was a local issue here.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 26, 2016

Member

Using "Details for" instead of "Transaction Details" would not truncate the window title (at least on OSX).

bildschirmfoto 2016-04-26 um 08 13 21

Tested ACK 17a6a21.

Member

jonasschnelli commented Apr 26, 2016

Using "Details for" instead of "Transaction Details" would not truncate the window title (at least on OSX).

bildschirmfoto 2016-04-26 um 08 13 21

Tested ACK 17a6a21.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 26, 2016

Member

Yes, Abbreviating to "Details for %s" makes sense, will do that, it was just that having the whole title hex was a bit too cypherpunk even for me :)

Member

laanwj commented Apr 26, 2016

Yes, Abbreviating to "Details for %s" makes sense, will do that, it was just that having the whole title hex was a bit too cypherpunk even for me :)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 26, 2016

Member

Added a new commit that puts the tx id in the window title (as well as removes the old "Transaction details" translation string).

Member

laanwj commented Apr 26, 2016

Added a new commit that puts the tx id in the window title (as well as removes the old "Transaction details" translation string).

@laanwj laanwj merged commit f135e3c into bitcoin:master Apr 28, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Apr 28, 2016

Merge #7939: qt: Make it possible to show details for multiple transa…
…ctions

f135e3c qt: Add transaction hash to details window title (Wladimir J. van der Laan)
17a6a21 qt: Make it possible to show details for multiple transactions (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7939: qt: Make it possible to show details for multiple transa…
…ctions

f135e3c qt: Add transaction hash to details window title (Wladimir J. van der Laan)
17a6a21 qt: Make it possible to show details for multiple transactions (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7939: qt: Make it possible to show details for multiple transa…
…ctions

f135e3c qt: Add transaction hash to details window title (Wladimir J. van der Laan)
17a6a21 qt: Make it possible to show details for multiple transactions (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Dec 20, 2017

Merge #7939: qt: Make it possible to show details for multiple transa…
…ctions

f135e3c qt: Add transaction hash to details window title (Wladimir J. van der Laan)
17a6a21 qt: Make it possible to show details for multiple transactions (Wladimir J. van der Laan)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment