> 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.
Forgot to mention the fonts: don't set any fonts - let the user choose the fonts he desires using the system settings. - 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ț > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
