> 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. > > Cristian Oneț 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 ----------------------------------------------------------- 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ț > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
