Skip to content

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

Merged
maflcko merged 1 commit intobitcoin:masterfrom
Sjors:2018/03/bump-fee-focus
Aug 20, 2018
Merged

[qt] TransactionView: highlight replacement tx after fee bump#12818
maflcko merged 1 commit intobitcoin:masterfrom
Sjors:2018/03/bump-fee-focus

Conversation

@Sjors
Copy link
Member

@Sjors Sjors commented Mar 28, 2018 •
edited by DrahtBot
Loading

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
Contributor

meshcollider commented Mar 28, 2018

Concept ACK

@Sjors
Copy link
Member Author

Sjors commented Mar 29, 2018 •
edited
Loading

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 2018/03/bump-fee-focus branch from c92e346 to 0fa52d3 Compare March 29, 2018 10:41
@promag
Copy link
Contributor

promag commented Mar 29, 2018

Concept ACK. NACK the timed single shot 😄

@Sjors Sjors force-pushed the 2018/03/bump-fee-focus branch 2 times, most recently from c601048 to bc48ac4 Compare April 3, 2018 16:18
jonasschnelli
jonasschnelli suggested changes Apr 10, 2018
View reviewed changes
src/qt/transactionview.cpp Outdated
Copy link
Contributor

@jonasschnelli jonasschnelli Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/qt/transactionview.cpp Outdated
Copy link
Contributor

@jonasschnelli jonasschnelli Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Apr 11, 2018

Needs rebase

@Sjors Sjors force-pushed the 2018/03/bump-fee-focus branch from bc48ac4 to 90c614c Compare April 11, 2018 10:35
@Sjors
Copy link
Member Author

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.

@DrahtBot DrahtBot mentioned this pull request Jul 22, 2018
Merged
maflcko
maflcko reviewed Jul 22, 2018
View reviewed changes
src/qt/transactionview.cpp Outdated
Copy link
Member

@maflcko maflcko Jul 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use the new connect syntax?

Copy link
Member Author

@Sjors Sjors Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ken2812221 ken2812221 Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sjors See #13529

Copy link
Contributor

@promag promag Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@Sjors Sjors Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 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 2018/03/bump-fee-focus branch from 90c614c to d795c61 Compare August 1, 2018 16:43
@Sjors
Copy link
Member Author

Sjors commented Aug 1, 2018

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

@maflcko
Copy link
Member

maflcko commented Aug 1, 2018

Tested ACK d795c61

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Aug 2, 2018

utACK d795c61, missed the 0.17 feature freeze though.

@maflcko maflcko added this to the 0.18.0 milestone Aug 2, 2018
@maflcko maflcko merged commit d795c61 into bitcoin:master Aug 20, 2018
maflcko pushed a commit that referenced this pull request Aug 20, 2018
Merge #12818: [qt] TransactionView: highlight replacement tx after fe…
4732fa1 
…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
@Sjors Sjors deleted the 2018/03/bump-fee-focus branch August 21, 2018 10:34
@Bushstar Bushstar mentioned this pull request Sep 4, 2018
Merged
@promag promag mentioned this pull request Sep 15, 2019
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

@ken2812221 ken2812221 ken2812221 left review comments

@jonasschnelli jonasschnelli jonasschnelli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

GUI

Projects

None yet

Milestone

0.18.0

Development

Successfully merging this pull request may close these issues.

8 participants

@Sjors @meshcollider @promag @jonasschnelli @DrahtBot @maflcko @ken2812221 @fanquake