[Kmymoney-devel] Review Request 117620: Remove fixed layout values from the CSV importer UI.
Chris
DeveloperChris at rebel.com.au
Sat Apr 19 00:49:32 UTC 2014
Hi
For what its worth I agree the patch is needed, as it stands the csv
importer is unusable in windows and problematic in other distro's
Chris
On 18/04/2014 11:08 PM, Allan Anderson wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117620/
>
>
> On April 18th, 2014, 11:34 a.m. UTC, *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.
>
> On April 18th, 2014, 12:19 p.m. UTC, *Cristian Onet,* 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.
>
> On April 18th, 2014, 12:21 p.m. UTC, *Cristian Onet,* wrote:
>
> Forgot to mention the fonts: don't set any fonts - let the user choose the fonts he desires using the system settings.
>
> On April 18th, 2014, 12:41 p.m. UTC, *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.
>
> On April 18th, 2014, 1:24 p.m. UTC, *Cristian Onet,* wrote:
>
> 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.
>
> No, your criticism was valid, if a little brusque.
> The patch is needed, so must go forward.
>
> > - try to use the available space as uniformly as possible (1 page with 2 combos vs. 1 page with 10 combos)
> I split up the wizard pages by logical function, and obviously some are much simpler than others. The investment
> page needed a lot of fields, others just a couple. Are you saying to combine some of the smaller pages, even though
> they may have no logical connection?
> > - now we have a dialog (QWizard) inside another dialog (CSVDialog - former widget) which seems a bad idea to me.
> Can you expand on this? I'm unclear.
>
> - Allan
>
>
> On April 17th, 2014, 9:23 p.m. UTC, Cristian Onet, wrote:
>
> Review request for KMymoney and Allan Anderson.
> By Cristian Onet,.
>
> /Updated April 17, 2014, 9:23 p.m./
>
> *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).
>
>
> Testing
>
> For me the importer looks/feel almost like without this.
>
>
> 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)
>
> View Diff <https://git.reviewboard.kde.org/r/117620/diff/>
>
>
>
> _______________________________________________
> KMyMoney-devel mailing list
> KMyMoney-devel at kde.org
> https://mail.kde.org/mailman/listinfo/kmymoney-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20140419/7809adbb/attachment-0001.html>
More information about the KMyMoney-devel
mailing list