Review Request 128817: Restructure pages in CSV Importer
Thomas Baumgart
tbaumgart at kde.org
Fri Sep 9 14:37:49 UTC 2016
> On Sept. 7, 2016, 8:07 vorm., Thomas Baumgart wrote:
> > kmymoney/plugins/csvimport/csvwizard.cpp, line 625
> > <https://git.reviewboard.kde.org/r/128817/diff/2/?file=475985#file475985line625>
> >
> > Doesn't that match an empty txt as well?
>
> Łukasz Wojniłowicz wrote:
> Yes it does and couple of lines above is my recipe for it
> ```c++
> QString txt = ui->tableWidget->item(row, col)->text();
> if (txt.isEmpty()) // nothing to process, so go to next row
> continue;
> ```
Never mind. I did not see that. Then it's not a problem.
> On Sept. 7, 2016, 8:07 vorm., Thomas Baumgart wrote:
> > kmymoney/plugins/csvimport/csvwizard.cpp, line 622
> > <https://git.reviewboard.kde.org/r/128817/diff/2/?file=475985#file475985line622>
> >
> > Don't we know about a lot more currency symbols in the engine (MyMoneyFile::instance()->currencyList())? The method would be tradingSymbol(). See KMyMoneyView::loadDefaultCurrencies() for the list of known currencies. Why do some of them are hard coded here?
>
> Łukasz Wojniłowicz wrote:
> I use d following code to harvest potential currencies.
> ```c++
> QList<MyMoneyAccount> accountList;
> MyMoneyFile::instance()->accountList(accountList);
>
> QList<MyMoneyAccount::accountTypeE> accountTypes;
> accountTypes << MyMoneyAccount::Checkings <<
> MyMoneyAccount::Savings <<
> MyMoneyAccount::Liability <<
> MyMoneyAccount::Checkings <<
> MyMoneyAccount::Savings <<
> MyMoneyAccount::Cash <<
> MyMoneyAccount::CreditCard <<
> MyMoneyAccount::Loan <<
> MyMoneyAccount::Asset <<
> MyMoneyAccount::Liability;
>
> QString filteredCurrencies;
> QList<MyMoneyAccount>::ConstIterator account;
> for (account = accountList.cbegin(); account != accountList.cend(); ++account)
> if (accountTypes.contains((*account).accountType()) && // account must acctually have currency property
> !filteredCurrencies.contains((*account).currencyId())) // currency must not be on the list already
> filteredCurrencies += (*account).currencyId();
> ```
>
> Łukasz Wojniłowicz wrote:
> Disregard previous response. It should be this.
> ```c++
> QList<MyMoneyAccount> accountList;
> MyMoneyFile::instance()->accountList(accountList);
>
> QList<MyMoneyAccount::accountTypeE> accountTypes;
> accountTypes << MyMoneyAccount::Checkings <<
> MyMoneyAccount::Savings <<
> MyMoneyAccount::Liability <<
> MyMoneyAccount::Checkings <<
> MyMoneyAccount::Savings <<
> MyMoneyAccount::Cash <<
> MyMoneyAccount::CreditCard <<
> MyMoneyAccount::Loan <<
> MyMoneyAccount::Asset <<
> MyMoneyAccount::Liability;
>
> QString filteredCurrencies;
> QList<MyMoneyAccount>::ConstIterator account;
> for (account = accountList.cbegin(); account != accountList.cend(); ++account) {
> if (accountTypes.contains((*account).accountType()) && // account must actually have currency property
> !filteredCurrencies.contains((*account).currencyId(), Qt::CaseInsensitive)) { // currency id must not be on the list already
> filteredCurrencies += (*account).currencyId();
> QString tradingSymbol = MyMoneyFile::instance()->currency((*account).currencyId()).tradingSymbol();
> if (!filteredCurrencies.contains(tradingSymbol, Qt::CaseInsensitive )) // currency trading symbol must not be on the list already
> filteredCurrencies += tradingSymbol;
> }
> }
> ```
> I think it's better to remove currency symbols based on what user has in his accounts, than to hunt for over 180 currency symbols/ids from (MyMoneyFile::instance()->currencyList()).
I'd use a QSet<QString> to add all trading symbols w/o the hassle of checking if it already exists. QSet does not create duplicates. At the end
QSet<QString> symbols;
// do the collection of the symbols over the accounts here
QString filteredCurrencies = QStringList(symbols.values()).join("");
Should make the code more readable.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128817/#review98964
-----------------------------------------------------------
On Sept. 7, 2016, 8:31 nachm., Łukasz Wojniłowicz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128817/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2016, 8:31 nachm.)
>
>
> Review request for KMymoney.
>
>
> Repository: kmymoney
>
>
> Description
> -------
>
> I actually decided to get rid of completion page and restructure LinesDate page. In my opinion it's more logical now and better structured for new feature I introduced ie. calculate fee column on fly.
>
> 1) shortened updateDecimalSymbol and renamed it to
> validateDecimalSymbol,
> 2) introduced detectDecimalSymbol for autodetecting decimal symbol,
> 3) simplified createStatement,
> 4) simplified and deduplicated slotImportClicked,
> 5) new decimal symbol option 'auto' for columns with various decimal
> symbols,
> 6) decimal symbol and date format get checked every time formats page is
> being opened,
> 7) removed completion page,
> 8) introduced rows page after separator page which is usefull for
> calculate fee
> 9) introduced formats page which groups all validity checks in one
> place,
> 10) checking securities existence is now part of
> InvestmentPage::validatePage(),
> 11) validateDateFormat operates on tableWidget and not on m_lines so it
> can be faster,
> 12) renamed page labels so they represent the content better,
> 13) Rows and Formats page have layout equal to Separator page, so look is more consistent.
>
>
> Diffs
> -----
>
> kmymoney/plugins/csvimport/CMakeLists.txt 216c79c
> kmymoney/plugins/csvimport/completionwizardpage.ui 616e0c3
> kmymoney/plugins/csvimport/csvdialog.h 59e6898
> kmymoney/plugins/csvimport/csvdialog.cpp ed9e1d8
> kmymoney/plugins/csvimport/csvwizard.h ecec5b0
> kmymoney/plugins/csvimport/csvwizard.cpp b576dea
> kmymoney/plugins/csvimport/csvwizard.ui 723e128
> kmymoney/plugins/csvimport/formatswizardpage.ui PRE-CREATION
> kmymoney/plugins/csvimport/investprocessing.h 902f9bb
> kmymoney/plugins/csvimport/investprocessing.cpp f050441
> kmymoney/plugins/csvimport/lines-datewizardpage.ui 84c1c91
> kmymoney/plugins/csvimport/rowswizardpage.ui PRE-CREATION
> kmymoney/plugins/csvimport/separatorwizardpage.ui 2ad5abd
>
> Diff: https://git.reviewboard.kde.org/r/128817/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Łukasz Wojniłowicz
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20160909/abfc8fee/attachment-0001.html>
More information about the KMyMoney-devel
mailing list