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

Fix regression with initial sorting after pr205 #229

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 27, 2021

Unfortunately, #205 introduced a regression. After opening the "Receive" or "Transaction" tab at first time despite of the "Date" header is marked as sorted, table rows are not sorted actually:

Screenshot from 2021-02-27 17-49-54

It appears that sorting the table must be triggered after the QTableView::setModel call.

With this PR (and pre-#205):

Screenshot from 2021-02-27 17-48-40

QTableView widget must be explicitly sorted after the setModel call.
@hebasto
Copy link
Member Author

hebasto commented Feb 27, 2021

Many thanks to @Geremia who reports about this regression.

Copy link

@leonardojobim leonardojobim left a comment

Choose a reason for hiding this comment

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

Tested ACK c524dc5 on Ubuntu 20.04.2 Qt 5.12.8

I can confirm the "Transactions" table rows are not initially ordered on the master branch. And this PR fixes this.

However, in my node, the "Receive" table rows are correctly ordered, both on the master and on this PR branch.

Transactions table on master branch Transactions table on this PR branch Both ("Receive" table)
Screen Shot 2021-02-27 at 6 06 35 PM Screen Shot 2021-02-27 at 6 07 09 PM Screen Shot 2021-02-27 at 6 07 38 PM

@Talkless
Copy link

tACK c524dc5, tested on Debian Sid with Qt 5.15.2. I can confirm @leonardojobim observations.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK c524dc5

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK c524dc5, tested on macOS 11.1 Qt 5.15.2

Confirming this regression on master and that this PR fixes it. I see the same behavior as shown in @leonardojobim screenshots.

This regression can actually be seen in my review of the PR that introduced the bug: #205 (review). It's not immediately obvious that the PR introduced this bug because all of the transactions occurred under the same minute. The maturity indicator is the clue that shows that the transactions we're not sorted by date under this PR.

This highlights the need for increased test coverage of the GUI.

@hebasto hebasto added the Bug Something isn't working label Mar 2, 2021
@maflcko maflcko merged commit 63314b8 into bitcoin-core:master Mar 10, 2021
@hebasto hebasto deleted the 210227-sort branch March 10, 2021 16:06
@laanwj
Copy link
Member

laanwj commented Mar 10, 2021

Thanks for fixing this, can confirm that it works.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 10, 2021
… pr205

c524dc5 qt: Fix regression with initial sorting after pr205 (Hennadii Stepanov)

Pull request description:

  Unfortunately, #205 introduced a regression. After opening the "Receive" or "Transaction" tab at first time despite of the "Date" header is marked as sorted, table rows are not sorted actually:

  ![Screenshot from 2021-02-27 17-49-54](https://user-images.githubusercontent.com/32963518/109392491-f7e9a480-7924-11eb-96cc-98b6f932e18e.png)

  It appears that sorting the table must be triggered _after_ the `QTableView::setModel` call.

  With this PR (and pre-#205):

  ![Screenshot from 2021-02-27 17-48-40](https://user-images.githubusercontent.com/32963518/109392505-08018400-7925-11eb-8107-8f8685744b83.png)

ACKs for top commit:
  Talkless:
    tACK c524dc5, tested on Debian Sid with Qt 5.15.2. I can confirm @leonardojobim observations.
  leonardojobim:
    Tested ACK bitcoin-core/gui@c524dc5 on Ubuntu 20.04.2 Qt 5.12.8
  jonatack:
    ACK c524dc5
  jarolrod:
    ACK c524dc5, tested on macOS 11.1 Qt 5.15.2

Tree-SHA512: e370229979a70d63a0b64dbc11c4eca338695a070881d4d8f015644617f180e6accc24d6bdf98a75e7c9ba9be2a0ace9a2b7eb9c783ebb2992c3b2c3b3deb408
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants