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@kde.org
https://mail.kde.org/mailman/listinfo/kmymoney-devel
