[Wallet] Transaction View: LastMonth calculation fixed #7327

pull crowning- wants to merge 1 commits into bitcoin:master from crowning-:patch-2 changing 1 files +1 −1

crowning- commented at 9:58 PM on January 11, 2016: contributor

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 commented at 7:31 PM on January 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?
jonasschnelli added the label GUI on Jan 12, 2016
crowning- commented at 9:01 PM on January 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...
paveljanik commented at 9:03 PM on January 12, 2016: contributor

ping @jonasschnelli
MarcoFalke commented at 9:32 PM on January 12, 2016: member

Please don't implement your own logic here. Qt already has all of this built in. fa51939 is enough.
crowning- commented at 9:50 PM on January 12, 2016: contributor

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

I've changed and tested it, works fine.
MarcoFalke commented at 9:55 PM on January 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?

in src/qt/transactionview.cpp:None in 9e6d1ca991 outdated

 266 | @@ -267,8 +267,8 @@ void TransactionView::chooseDate(int idx)
 267 |          break;
 268 |      case LastMonth:
 269 |          transactionProxyModel->setDateRange(
 270 | -                QDateTime(QDate(current.year(), current.month()-1, 1)),
 271 | -                QDateTime(QDate(current.year(), current.month(), 1)));
 272 | +           QDateTime(QDate(current.year(), current.month(), 1).addMonths(-1)),

jonasschnelli commented at 7:22 PM on January 13, 2016:

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

MarcoFalke commented at 7:25 PM on January 13, 2016:

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

jonasschnelli commented at 7:23 PM on January 13, 2016: contributor

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

nit: code alignment.
crowning- commented at 7:27 PM on January 13, 2016: contributor

Alignment fixed, sorry.
jonasschnelli commented at 7:29 PM on January 13, 2016: contributor

Can you squash the commits into one?
[Wallet] Transaction View: LastMonth calculation fixed 30cdacea3c
crowning- force-pushed on Jan 13, 2016
crowning- commented at 8:23 PM on January 13, 2016: contributor

Squashed!
jonasschnelli commented at 8:24 PM on January 13, 2016: contributor

Tested ACK 30cdacea3c356acda32ab77238f07c1c40b1f1b5
jonasschnelli merged this on Jan 13, 2016
jonasschnelli closed this on Jan 13, 2016
jonasschnelli referenced this in commit c079d79c9a on Jan 13, 2016
crowning- deleted the branch on Jan 13, 2016
MarcoFalke referenced this in commit a74fa1f06b on Jan 18, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels