Skip to content
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] TransactionView: highlight replacement tx after fee bump #12818

Merged
merged 1 commit into from Aug 20, 2018

Conversation

@Sjors
Copy link
Member

@Sjors Sjors commented Mar 28, 2018

Consistent with #12421 which highlights the transaction after send.

1

2

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.

@fanquake fanquake added the GUI label Mar 28, 2018
@meshcollider
Copy link
Member

@meshcollider meshcollider commented Mar 28, 2018

Concept ACK

@Sjors
Copy link
Member Author

@Sjors Sjors commented Mar 29, 2018

Travis fails with:

qt/transactionview.cpp: In member function ‘void TransactionView::bumpFee()’:
qt/transactionview.cpp:446:10: error: no matching function for call to ‘QTimer::singleShot(int, TransactionView::bumpFee()::__lambda0)’
         });
          ^
qt/transactionview.cpp:446:10: note: candidate is:
In file included from /usr/include/qt4/QtCore/QTimer:1:0,
                 from ./qt/sendcoinsdialog.h:13,
                 from qt/transactionview.cpp:13:
/usr/include/qt4/QtCore/qtimer.h:78:17: note: static void QTimer::singleShot(int, QObject*, const char*)
     static void singleShot(int msec, QObject *receiver, const char *member);

Seems to be a QT 4 issue. Will look into that later, since it might go away if there's a way to avoidsingleShot (or if #8263 happens first). For now I just skip it for QT < 5.4.

@Sjors Sjors force-pushed the Sjors:2018/03/bump-fee-focus branch Mar 29, 2018
@promag
Copy link
Member

@promag promag commented Mar 29, 2018

Concept ACK. NACK the timed single shot 😄

@Sjors Sjors force-pushed the Sjors:2018/03/bump-fee-focus branch 2 times, most recently Mar 29, 2018
src/qt/transactionview.cpp Outdated
model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, true);

#if QT_VERSION >= 0x050400
QTimer::singleShot(10, [=](){

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 10, 2018
Member

IMO just avoid the lambda and make it therefore Qt4 compile compatible.

src/qt/transactionview.cpp Outdated

#if QT_VERSION >= 0x050400
QTimer::singleShot(10, [=](){
Q_EMIT bumpedFee(newHash);

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 10, 2018
Member

Have you tried qApp->processEvents(); instead of the single shot dummy timer?

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Apr 11, 2018

Needs rebase

@Sjors Sjors force-pushed the Sjors:2018/03/bump-fee-focus branch Apr 11, 2018
@Sjors
Copy link
Member Author

@Sjors Sjors commented Apr 11, 2018

Rebased

@promag @jonasschnelli I replaced the single shot with qApp->processEvents(), which also removes the need for any Qt4 specific code.

src/qt/transactionview.cpp Outdated
@@ -202,6 +203,9 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
connect(copyTxPlainText, SIGNAL(triggered()), this, SLOT(copyTxPlainText()));
connect(editLabelAction, SIGNAL(triggered()), this, SLOT(editLabel()));
connect(showDetailsAction, SIGNAL(triggered()), this, SLOT(showDetails()));

// Highlight transaction after fee bump
connect(this, SIGNAL(bumpedFee(uint256)), this, SLOT(focusTransaction(uint256)));

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 22, 2018
Member

Could use the new connect syntax?

This comment has been minimized.

@Sjors

Sjors Jul 24, 2018
Author Member

@MarcoFalke not sure what you mean by new connect syntax: http://doc.qt.io/qt-5/signalsandslots.html

This comment has been minimized.

This comment has been minimized.

@promag

promag Jul 24, 2018
Member

Two options ATM:

connect(this, &TransactionView::bumpedFee, [this](const uint256& txid) {
    focusTransaction(txid);
});
connect(this, &TransactionView::bumpedFee, this, static_cast<void (TransactionView::*)(const uint256&)>(&TransactionView::focusTransaction));

Because TransactionView::focusTransaction is overloaded.

This comment has been minimized.

@Sjors

Sjors Aug 1, 2018
Author Member

@promag I find the first version more readable and less intimidating. I noticed the other version in your PR for a similar signal.

@DrahtBot DrahtBot closed this Jul 22, 2018
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jul 22, 2018

The last travis run for this pull request was 60 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
@DrahtBot DrahtBot reopened this Jul 22, 2018
@Sjors Sjors force-pushed the Sjors:2018/03/bump-fee-focus branch to d795c61 Aug 1, 2018
@Sjors
Copy link
Member Author

@Sjors Sjors commented Aug 1, 2018

Rebased and switched the to the QT5 connect() syntax.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Aug 1, 2018

Tested ACK d795c61

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Aug 2, 2018

utACK d795c61, missed the 0.17 feature freeze though.

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Aug 2, 2018
@MarcoFalke MarcoFalke merged commit d795c61 into bitcoin:master Aug 20, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
MarcoFalke added a commit that referenced this pull request Aug 20, 2018
…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
@Sjors Sjors deleted the Sjors:2018/03/bump-fee-focus branch Aug 21, 2018
Munkybooty added a commit to Munkybooty/dash that referenced this pull request Jun 30, 2021
…fter fee bump

d795c61 [qt] TransactionView: highlight replacement tx after fee bump (Sjors Provoost)

Pull request description:

  Consistent with bitcoin#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

# Conflicts:
#	src/qt/transactionview.cpp
#	src/qt/transactionview.h
#	src/qt/walletmodel.cpp
#	src/qt/walletmodel.h
Munkybooty added a commit to Munkybooty/dash that referenced this pull request Jun 30, 2021
…fter fee bump

d795c61 [qt] TransactionView: highlight replacement tx after fee bump (Sjors Provoost)

Pull request description:

  Consistent with bitcoin#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

# Conflicts:
#	src/qt/transactionview.cpp
#	src/qt/transactionview.h
#	src/qt/walletmodel.cpp
#	src/qt/walletmodel.h
Munkybooty added a commit to Munkybooty/dash that referenced this pull request Jun 30, 2021
…fter fee bump

d795c61 [qt] TransactionView: highlight replacement tx after fee bump (Sjors Provoost)

Pull request description:

  Consistent with bitcoin#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

# Conflicts:
#	src/qt/transactionview.cpp
#	src/qt/transactionview.h
#	src/qt/walletmodel.cpp
#	src/qt/walletmodel.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants