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

[Wallet] Transaction View: LastMonth calculation fixed #7327

Merged
merged 1 commit into from Jan 13, 2016

Conversation

Projects
None yet
4 participants
@crowning-
Contributor

crowning- commented Jan 11, 2016

In January (in this example, January 2016), last month isn't from 2016-0-1 to 2016-1-1, it's from 2015-12-1 to 2016-1-1.

Edit: this means in each January the "Last Month" filter simply does not work.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jan 12, 2016

Contributor

Can you please rewrite this a bit? E.g. calculate prevMonthFirstDay first and then use it in one call.

Does January 1st belong to the previous month?

Contributor

paveljanik commented Jan 12, 2016

Can you please rewrite this a bit? E.g. calculate prevMonthFirstDay first and then use it in one call.

Does January 1st belong to the previous month?

@jonasschnelli jonasschnelli added the GUI label Jan 12, 2016

@crowning-

This comment has been minimized.

Show comment
Hide comment
@crowning-

crowning- Jan 12, 2016

Contributor

I'm not sure if it would be better readable, we would need an integer for the year (prevMonthYear) and an integer for the month (prevMonthFirstDay) and calculate both depending on current.month(), so the If/else statement wouldn't get shorter.
If general consensus is to implement it this way I'll change it of course, no problem.

Technically January 1st does not belong to the previous month, but since the old range-calculation in this method computed the last month from 1st to 1st I didn't want to change that logic. Maybe someone somewhere relies on this...

Contributor

crowning- commented Jan 12, 2016

I'm not sure if it would be better readable, we would need an integer for the year (prevMonthYear) and an integer for the month (prevMonthFirstDay) and calculate both depending on current.month(), so the If/else statement wouldn't get shorter.
If general consensus is to implement it this way I'll change it of course, no problem.

Technically January 1st does not belong to the previous month, but since the old range-calculation in this method computed the last month from 1st to 1st I didn't want to change that logic. Maybe someone somewhere relies on this...

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik
Contributor

paveljanik commented Jan 12, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 12, 2016

Member

Please don't implement your own logic here. Qt already has all of this built in. fa51939 is enough.

Member

MarcoFalke commented Jan 12, 2016

Please don't implement your own logic here. Qt already has all of this built in. fa51939 is enough.

@crowning-

This comment has been minimized.

Show comment
Hide comment
@crowning-

crowning- Jan 12, 2016

Contributor

Whenever I think I know (most of) Qt someone shows me that I don't...thanks @MarcoFalke 👍

I've changed and tested it, works fine.

Contributor

crowning- commented Jan 12, 2016

Whenever I think I know (most of) Qt someone shows me that I don't...thanks @MarcoFalke 👍

I've changed and tested it, works fine.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 12, 2016

Member

Offtopic: @crowning- I saw you implemented wallet maintenance (rescan, etc) over the GUI. I think this is considered useful for bitcoin core as well (ping @jonasschnelli ?)

Have you ever thought about porting it to bitcoin core?

Member

MarcoFalke commented Jan 12, 2016

Offtopic: @crowning- I saw you implemented wallet maintenance (rescan, etc) over the GUI. I think this is considered useful for bitcoin core as well (ping @jonasschnelli ?)

Have you ever thought about porting it to bitcoin core?

@jonasschnelli

View changes

Show outdated Hide outdated src/qt/transactionview.cpp
@@ -267,8 +267,8 @@ void TransactionView::chooseDate(int idx)
break;
case LastMonth:
transactionProxyModel->setDateRange(
QDateTime(QDate(current.year(), current.month()-1, 1)),
QDateTime(QDate(current.year(), current.month(), 1)));
QDateTime(QDate(current.year(), current.month(), 1).addMonths(-1)),

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 13, 2016

Member

nit: please realign with the other "case" structures (re-add the \t).

@jonasschnelli

jonasschnelli Jan 13, 2016

Member

nit: please realign with the other "case" structures (re-add the \t).

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 13, 2016

Member

Also squash, having two commits does not make sense here.

@MarcoFalke

MarcoFalke Jan 13, 2016

Member

Also squash, having two commits does not make sense here.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 13, 2016

Member

Tested ACK.
Thanks for fixing this.
Indeed: QDate(current.year(), current.month()-1, 1) does not work.

nit: code alignment.

Member

jonasschnelli commented Jan 13, 2016

Tested ACK.
Thanks for fixing this.
Indeed: QDate(current.year(), current.month()-1, 1) does not work.

nit: code alignment.

@crowning-

This comment has been minimized.

Show comment
Hide comment
@crowning-

crowning- Jan 13, 2016

Contributor

Alignment fixed, sorry.

Contributor

crowning- commented Jan 13, 2016

Alignment fixed, sorry.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 13, 2016

Member

Can you squash the commits into one?

Member

jonasschnelli commented Jan 13, 2016

Can you squash the commits into one?

@crowning-

This comment has been minimized.

Show comment
Hide comment
@crowning-

crowning- Jan 13, 2016

Contributor

Squashed!

Contributor

crowning- commented Jan 13, 2016

Squashed!

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 13, 2016

Member

Tested ACK 30cdace

Member

jonasschnelli commented Jan 13, 2016

Tested ACK 30cdace

@jonasschnelli jonasschnelli merged commit 30cdace into bitcoin:master Jan 13, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

jonasschnelli added a commit that referenced this pull request Jan 13, 2016

Merge pull request #7327
30cdace [Wallet] Transaction View: LastMonth calculation fixed (crowning-)

@crowning- crowning- deleted the crowning-:patch-2 branch Jan 13, 2016

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jan 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment