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: Sort transactions by date #8014

Merged
merged 1 commit into from May 23, 2016

Conversation

Projects
None yet
6 participants
@Tyler-Hardin
Contributor

Tyler-Hardin commented May 6, 2016

Conflicted transactions can get stuck at the top. This fixes that.

I'm willing to add a setting to allow the user to pick a default sort if necessary.

I got the idea for this from Theymos: https://www.reddit.com/r/Bitcoin/comments/4i396u/bitcoin_qt_feature_suggestions/d2uowpt

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr May 6, 2016

Member

You should be able to just click the heading you want to sort by... maybe this makes sense as a default though.

Member

luke-jr commented May 6, 2016

You should be able to just click the heading you want to sort by... maybe this makes sense as a default though.

@Tyler-Hardin

This comment has been minimized.

Show comment
Hide comment
@Tyler-Hardin

Tyler-Hardin May 6, 2016

Contributor

Yeah, you can click to sort by column. The idea is that date makes a better default.

Contributor

Tyler-Hardin commented May 6, 2016

Yeah, you can click to sort by column. The idea is that date makes a better default.

@jonasschnelli jonasschnelli added the GUI label May 6, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 6, 2016

Member

Yes. This makes sense.
utACK 650222badaf95b7a91ce0c8aa50623359fa03380

Member

jonasschnelli commented May 6, 2016

Yes. This makes sense.
utACK 650222badaf95b7a91ce0c8aa50623359fa03380

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 9, 2016

Contributor

With this, we will have a different default sort order in the main window (Overview - Recent transactions) and in the Transactions tab. This is not very intuitive.

Can you change the sort order there too?

Contributor

paveljanik commented May 9, 2016

With this, we will have a different default sort order in the main window (Overview - Recent transactions) and in the Transactions tab. This is not very intuitive.

Can you change the sort order there too?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 9, 2016

Member

With this, we will have a different default sort order in the main window (Overview - Recent transactions) and in the Transactions tab. This is not very intuitive.

Good point. I agree with @paveljanik.

Member

jonasschnelli commented May 9, 2016

With this, we will have a different default sort order in the main window (Overview - Recent transactions) and in the Transactions tab. This is not very intuitive.

Good point. I agree with @paveljanik.

Qt: Sort transactions by date
Conflicted transactions can get stuck at the top. This fixes that.
@Tyler-Hardin

This comment has been minimized.

Show comment
Hide comment
@Tyler-Hardin

Tyler-Hardin May 10, 2016

Contributor

@paveljanik, good catch! Thanks!

And yeah, it was also trivial to change.

Contributor

Tyler-Hardin commented May 10, 2016

@paveljanik, good catch! Thanks!

And yeah, it was also trivial to change.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake
Member

fanquake commented May 10, 2016

utACK 2d5603c

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 10, 2016

Contributor

ACK 2d5603c

Contributor

paveljanik commented May 10, 2016

ACK 2d5603c

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 14, 2016

Contributor

Ready for merge.

Contributor

paveljanik commented May 14, 2016

Ready for merge.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 14, 2016

Member

On regtest this can result in a strange table ordering. If you generate a couple of blocks, mostly some (or all) of them are generated during the same second which makes them equal for the date sort order. My tests showed me, that I had transactions table entries (coinbase) with more confirmations on top of entries with fever confirmations.

I hope this is a regtest only problem (I can't imagine a mainnet use case where this issue could play a role).

Is there a possibility to use the amount of confirmations as second sort attribute (if the first sort attribute is equal)?

Member

jonasschnelli commented May 14, 2016

On regtest this can result in a strange table ordering. If you generate a couple of blocks, mostly some (or all) of them are generated during the same second which makes them equal for the date sort order. My tests showed me, that I had transactions table entries (coinbase) with more confirmations on top of entries with fever confirmations.

I hope this is a regtest only problem (I can't imagine a mainnet use case where this issue could play a role).

Is there a possibility to use the amount of confirmations as second sort attribute (if the first sort attribute is equal)?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 14, 2016

Member

Conflicted transactions can get stuck at the top. This fixes that.

Wouldn't it be better to give an option to wipe such transactions from the db (and thus gui) entirely?

Member

MarcoFalke commented May 14, 2016

Conflicted transactions can get stuck at the top. This fixes that.

Wouldn't it be better to give an option to wipe such transactions from the db (and thus gui) entirely?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 14, 2016

Member

You can use the new abandon function (context menu) #7707. But it will not remove the transaction. Removing is somehow against the bookkeeping rules. But I agree it could be useful sometimes.

For this PR: I think it is useful. But confirmation count should be the 2nd sort attribute.

Member

jonasschnelli commented May 14, 2016

You can use the new abandon function (context menu) #7707. But it will not remove the transaction. Removing is somehow against the bookkeeping rules. But I agree it could be useful sometimes.

For this PR: I think it is useful. But confirmation count should be the 2nd sort attribute.

@Tyler-Hardin

This comment has been minimized.

Show comment
Hide comment
@Tyler-Hardin

Tyler-Hardin May 16, 2016

Contributor

@jonasschnelli there's no builtin way to do that. I could add a method to do it, but I'm not sure how to expose the functionality API-wise.

What do you think about having a sortSecondary(int col, Qt::SortOrder ord) method that is called on a TransactionFilterProxy that has already been sorted by the primary key. This is the simplest way to implement it that I can think of.

Contributor

Tyler-Hardin commented May 16, 2016

@jonasschnelli there's no builtin way to do that. I could add a method to do it, but I'm not sure how to expose the functionality API-wise.

What do you think about having a sortSecondary(int col, Qt::SortOrder ord) method that is called on a TransactionFilterProxy that has already been sorted by the primary key. This is the simplest way to implement it that I can think of.

@jonasschnelli jonasschnelli merged commit 2d5603c into bitcoin:master May 23, 2016

1 check passed

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

jonasschnelli added a commit that referenced this pull request May 23, 2016

Merge #8014: Qt: Sort transactions by date
2d5603c Qt: Sort transactions by date (Tyler Hardin)
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 23, 2016

Member

Tested ACK.
Further improvements (second sort attribute) can be done later.

Member

jonasschnelli commented May 23, 2016

Tested ACK.
Further improvements (second sort attribute) can be done later.

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

Merge bitcoin#8014: Qt: Sort transactions by date
2d5603c Qt: Sort transactions by date (Tyler Hardin)

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

Merge bitcoin#8014: Qt: Sort transactions by date
2d5603c Qt: Sort transactions by date (Tyler Hardin)

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

Merge bitcoin#8014: Qt: Sort transactions by date
2d5603c Qt: Sort transactions by date (Tyler Hardin)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment