[Kmymoney-devel] Review Request 122364: CSV Importer - Fix display on high-definition displays. Fix interaction between QTable and QWizard in same window.
Allan Anderson
agander93 at gmail.com
Sun Feb 22 23:01:06 UTC 2015
> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment357>
> >
> > This should be done in a more modular fashion (general annotation).
>
> Allan Anderson wrote:
> Sorry, can you explain.
>
> Christian David wrote:
> Imagine you want to change or work with the object pointed to by ```m_pageCompletion```. Then you have to look at ```m_wiz``` as well – from experience I know this is forgotten easily. Also it makes it hard to understand the code. The chance a new developer will give up after a couple of these constructs is high.
>
> This line is in a method called ```init()```. So I assume it is called once to initialize the widget. So I would move the initializing into the class of ```m_pageCompletion```. Actually you can set this simply in Qt Designer and do not need any C++ code.
m_pageCompletion is the final page of the importer wizard. Each of the six pages has its own ui and has to be accessed via the wizard itself, which is m_wiz. I'm not aware of any mechanism that allows a wizard ui component to be directly accessed.
Then, in this most recent incarnation of the importer, the wizard and its various uis were split off into a separate class, from the main importer which handles the preview QWidgetTable. The preview table is used by both the banking side and the investment side, both of which display the data in the table, according to the fields selected by the user. So, both of them need to access the wizard pages, each having a pointer to the wizard, which in turn manipulates the various pages.
This function is not used just at startup, but also during program exection. If you move the initializing of the combobox into the class of m_pageCompletion, the decision/need to clear it or reset it still has to come from the banking or investment class. So I don't think it would achieve anything.
As ever, I'm willing to learn.
> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment358>
> >
> > This part can be removed, the flag is set and removed again. Also you call show() twice.
>
> Allan Anderson wrote:
> As above.
>
> Christian David wrote:
> Without seeing the code: I think, you should just drop the part where the flag is removed again and the second ```show()```.
This is used in three places, and in two of them problems ensue. In the first one, the wizard window stays on top and clicking on the table window does not bring it to the front. It is necessary to start to manipulate the wizard before the table wakes up. In the second one, it is possible for the table to completely cover the wizard, and to circumvent this, I allow a ight click on the table to raise the wizard. With those three lines removed, the wizard stays hidden. In the third case, it might be possible to remove all that code, with no problem apparent. I'll keep my eye on all this.
> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment359>
> >
> > You are calling setCurrentIndex() later again. So these lines are not needed as their change is overwritten anyway (or do you need the change signal to get emitted?).
>
> Allan Anderson wrote:
> If the second setCurrentIndex() doesn't change the setting, there will be no connect, which is needed to allow other settings
> to be checked for conflict, etc.. So, yes, the first is to ensure the second does cause a connect.
>
> Christian David wrote:
> Do you mean a second "connect" or "ensure it causes an emit of the signal"?
Perhaps there's a fine point I'm missing here. The second setCurrentIndex() will cause an emit, provided the value has changed. The first setCurrentIndex() clears the setting, and ensures that the second one will get triggered.
> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment360>
> >
> > Why do you need the ui to change?
>
> Allan Anderson wrote:
> As above, really.
The program detects the change and starts a validation process.
> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment361>
> >
> > Why do you disconnect and reconnect later? Also this connection should be done in CSVWizard.
>
> Allan Anderson wrote:
> This goes back to when I was originally working on the plugins, and if I remember, just above these re-connects, I populate all the comboboxes with their items, which was causing multiple connects and upsetting the indexes. There is a brief note there.
> Agreed about moving them, but do we want more code at the moment? Certainly, I'm happy to move them. They too predate this patch.
>
> Christian David wrote:
> You can prevent multiple connections with ```connect( sender, signal, receiver, slot, Qt::UniqueConnection)```.
>
> "Agreed about moving them, but do we want more code at the moment?" - I agree.
The multiple connections were from the multiple combos, one from each, not just from one.
> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment362>
> >
> > ```QStringList m_columnTypeList; // holds field types - date, payee, etc.```
> >
> > to
> >
> > ```/** holds field types - date, payee, etc. */
> > QStringList m_columnTypeList;```
> >
> > then doxygen can help you
>
> Allan Anderson wrote:
> Oh, I hadn't come across that. If I've used it, it's by accident.
It actually looks like putting the cart before the horse. Does doxygen need it that way?
> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment363>
> >
> > I am not sure here; do these values depend on a user selection? If so it is usualy better to "calculate" them on request. Having a copy quickly leads to inconsistent data.
>
> Allan Anderson wrote:
> Yes, for the Banking and Investment wizards, etc, a group of similar, required settings are tested to ensure all required (and optional alternatives) fields have been selected. So, you're saying not to keep these results, but to perform the tests as needed? I was using them as a cache, to reduce the library overheads.
>
> Christian David wrote:
> If the value is not requested each millisecond the overhead won't be notable (under certain circumstances it can be even faster not to use this cache).
Just one point. A couple of tomes, readability has been touched upon. In some places a combination of such values determines whether or not all necessary conditions have been met, and that code can get a bit unwieldy. If it is required to replace them with the current lengthy value from the UI, it will get quite a bit more convoluted or obscure.
> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment364>
> >
> > This method looks very similar to the following ```…ColumnSelected()``` methods. They could be compacted with some template magic.
>
> Allan Anderson wrote:
> Hmmm. I'll have a play with that later.
>
> Christian David wrote:
> Can I offer my help?
Thanks. Certainly later I'll look into this.
- Allan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122364/#review75942
-----------------------------------------------------------
On Feb. 3, 2015, 12:27 a.m., Allan Anderson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122364/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 12:27 a.m.)
>
>
> Review request for KMymoney.
>
>
> Bugs: 340656
> http://bugs.kde.org/show_bug.cgi?id=340656
>
>
> Repository: kmymoney
>
>
> Description
> -------
>
> Fix display on high-definition monitors. Fix interaction between the import preview table widget and the parameter entry wizard, caused by them both resizing dynamically within the same window. This was achieved by creating a new class and UI for the wizard and transferring most of the existing relevant code out of the large csvdialog.cpp file into the new class and file. Unfortunately, because 11 UIs are affected and the transfer of existing logic into a new class, the patch is quite large, although there is little new code involved.
>
>
> Diffs
> -----
>
> kmymoney/plugins/csvimport/csvwizard.h PRE-CREATION
> kmymoney/plugins/csvimport/csvwizard.cpp PRE-CREATION
> kmymoney/plugins/csvimport/csvwizard.ui PRE-CREATION
> kmymoney/plugins/csvimport/introwizardpage.ui 2bf93f0
> kmymoney/plugins/csvimport/investmentdlg.cpp 5e7ce06
> kmymoney/plugins/csvimport/investmentwizardpage.ui d3f2857
> kmymoney/plugins/csvimport/investprocessing.h 3c08dee
> kmymoney/plugins/csvimport/investprocessing.cpp 2b6b2d1
> kmymoney/plugins/csvimport/lines-datewizardpage.ui 01d7253
> kmymoney/plugins/csvimport/redefinedlgdecl.ui 26d8b62
> kmymoney/plugins/csvimport/separatorwizardpage.ui 21136f2
> kmymoney/plugins/csvimport/CMakeLists.txt 36e5afc
> kmymoney/plugins/csvimport/bankingwizardpage.ui 9e1b5cb
> kmymoney/plugins/csvimport/completionwizardpage.ui ce61e89
> kmymoney/plugins/csvimport/csvdialog.h 780329d
> kmymoney/plugins/csvimport/csvdialog.cpp b986317
> kmymoney/plugins/csvimport/csvdialog.ui 166b04a
>
> Diff: https://git.reviewboard.kde.org/r/122364/diff/
>
>
> Testing
> -------
>
> Tested on 96 DPI and 160 DPI. Also, the OP of the bug has tested the patch and confirms its performance.
> There is one remaining UI not yet included. I've completed its testing, but as it's a little-used window, I've held it back to avoid making this patch even larger.
> I'm unable to test on Windows.
>
>
> File Attachments
> ----------------
>
> Updated patch
> https://git.reviewboard.kde.org/media/uploaded/files/2015/02/03/6e4dad98-936c-4ca0-8804-c4abb9b51438__0002-BUG-340656.patch
>
>
> Thanks,
>
> Allan Anderson
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20150222/468d6bb9/attachment-0001.html>
More information about the KMyMoney-devel
mailing list