[Kmymoney-devel] Review Request 117620: Remove fixed layout values from the CSV importer UI.

Fri Apr 18 13:24:34 UTC 2014

> On April 18, 2014, 11:34 a.m., Allan Anderson wrote:
> > Hi Cristian
> > Many thanks.  It had been starting to dawn on me that, when dealing with different distros, it shouldn't be necessary to fiddle about with margins
> > to get things to look right.  I'd suspected that I needed to make some fundamental changes, but wasn't really ready to take the plunge.  Anyway,
> > you've done it for me, so again, thank you.
> > The question now is, where do we go from here?
> > Here, I'd already removed a chunk of code and have been working on details of the UI appearance.  For instance, I don't like to see the size of the
> > table changing height between the different wizard pages, and I don't like to see just partial lines displayed.  Obviously, if the user decides to do
> > a resize, then the decision is his, but without that happening, I don't want to see half lines displayed.  This is mainly to do with whether a
> > horizontal scroll-bar is visible or not.  I have not been able to get that to work correctly.  Sometimes, it is visible but isVisible() returns
> > false, and vice versa I think.  So I've had to calculate the diplayed width and adjust the containing rectangle.  A little work is still needed here.
> > In your pruning, I think you have removed some vertical spacers, which is causing the combo-boxes to shift upwards, leaving a lot of space below.
> > I wish to revert that.  In the separators wizard page, the two combo-boxes are now different sizes, I think because you've removed the form layout I was
> > using.  In the banking wizard, the combo-box sizes are significantly different in width between Linux Mint and Ubuntu.  I don't know about on Windows.
> > These are very minor points, but I'd like to tweak them.
> > I'd decided on a default window height of 10 rows, which has been changed.  Was there a particular reason for that?
> > Between Mint and Ubuntu, the margin values are still different, but now that has no effect on the appearance.  The font sizes are differ too, 9 point
> > against 11 point.  That does affect the appearance.  Should I leave that as is, do you think?  I think probably yes.
> > I can't add a revised patch to your review, but if you agree, I can describe any proposed changes.  I don't want to stray away from your major changes,
> > though, obviously.
> 
> Cristian Oneț wrote:
>     "where do we go from here?"
>     
>     I don't know, it's up to you to decide. If you ask me (after 2 days of going trough csvdialog.cpp) I would suggest to rewrite everything. I know that seems harsh but frankly I didn't see such "tangled up" code in a while. One reading it couldn't quite tell what is essential (necessary) in there and what is trial/error leftover code.
>     
>     For the UI I would have the following guide:
>     - keep it simple (your layouts were really, really complicated)
>     - don't center everything (including messages)
>     - don't set fix sizes (fixed size policies are OK in the appropriate place - line edits, combos should have a fixed vertical size hint)
>     - try to use the available space as uniformly as possible (1 page with 2 combos vs. 1 page with 10 combos)
>     - now we have a dialog (QWizard) inside another dialog (CSVDialog - former widget) which seems a bad idea to me
>     
>     For the code:
>     - don't keep everything together (like a big ball of spaghetti), try to break it up into smaller components
>     - only keep the essential code otherwise you won't be able to spot it later from the clutter
>     
>     Until the csv importer plugin will not be cleaned up, clean reviewable patches can't be provided because the patches will always look like the code that is patched.
> 
> Cristian Oneț wrote:
>     Forgot to mention the fonts: don't set any fonts - let the user choose the fonts he desires using the system settings.
> 
> Allan Anderson wrote:
>     I've never made any claim to be a programmer!  This all started off as a script to produce a QIF file, which I was advised to expand, first into
>     tabs, and later into a wizard.  It has grown, like Topsy.  I've learned a lot along the way, but I'm still not a programmer.
>     In the light of your comments, there seems little point now wasting time tuning what we have, so I'll leave it as is.
>     When/if a rewrite occurs will depend upon my available time.

Sorry if I offended you, I know to well how one feels about his work made voluntarily in his free time. I'm just asking that you never stop learning and improving the work you do. I even have the same critical view of my own code that I wrote back when I started contributing to kmymoney. I did ask for a rewrite but didn't say when so whenever you feel like it seems fine. Meanwhile we can leave this patch in "suspension" if it's too much.

- Cristian

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117620/#review56020
-----------------------------------------------------------

On April 17, 2014, 9:23 p.m., Cristian Oneț wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117620/
> -----------------------------------------------------------
> 
> (Updated April 17, 2014, 9:23 p.m.)
> 
> 
> Review request for KMymoney and Allan Anderson.
> 
> 
> Repository: kmymoney
> 
> 
> Description
> -------
> 
> Remove fixed layout values from the CSV importer UI.
> 
> Also removed a lot of code (a lot still remains) that I think should
> not be in there (like UI control size fine tuning).
> 
> 
> Diffs
> -----
> 
>   kmymoney/plugins/csvimport/bankingwizardpage.ui d2179bff0504a67a22efbb364f90e089443d8239 
>   kmymoney/plugins/csvimport/completionwizardpage.ui 99db07576265b68c764308bbb3da3cae38b151bd 
>   kmymoney/plugins/csvimport/csvdialog.h 6716ca412db036384d9d8d65f0d1ab40efbe443f 
>   kmymoney/plugins/csvimport/csvdialog.cpp 3ab7f9537acdb369b0c5b781827564e30d9ad396 
>   kmymoney/plugins/csvimport/csvdialog.ui 36e7fd51159a1f7391394b5f00bc22387b0bd8f5 
>   kmymoney/plugins/csvimport/csvimporterplugin.h d2d2f6ca9aeea0709512afcffbad17abea6a315a 
>   kmymoney/plugins/csvimport/csvimporterplugin.cpp 602b4d924c8afc0b34819e47b03b88776319bf0c 
>   kmymoney/plugins/csvimport/introwizardpage.ui 9bd29f5f4339add8790e032f5613ec46c675634d 
>   kmymoney/plugins/csvimport/investmentwizardpage.ui 3744963e5e9a91ad36b8d0fa78839b296c5810f8 
>   kmymoney/plugins/csvimport/investprocessing.cpp d9df90d6b9a6500b499e52ccb3e8734d163765eb 
>   kmymoney/plugins/csvimport/lines-datewizardpage.ui fb10a0e228d7bdee9ca1162edcd0d4b69225d68b 
>   kmymoney/plugins/csvimport/separatorwizardpage.ui 30b2dc7b22ad24b7b2df96332deb25f9c8c76b89 
> 
> Diff: https://git.reviewboard.kde.org/r/117620/diff/
> 
> 
> Testing
> -------
> 
> For me the importer looks/feel almost like without this.
> 
> 
> Thanks,
> 
> Cristian Oneț
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20140418/b987c290/attachment-0001.html>