[Kmymoney-devel] Review Request 117620: Remove fixed layout values from the CSV importer UI.
Allan Anderson
agander93 at gmail.com
Wed May 28 10:05:54 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.
>
> 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.
>
> Allan Anderson wrote:
> 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 Anderson wrote:
> I've made some small changes to the various UIs, using one or two vertical spacers to avoid having all the widgets in a clump with empty space below. Also, on the Banking wizard page, I've set a minimum width on all the combos, which allows them to shrink and remain visible, in order to reduce the overall width of the UI, to help on smaller screen displays. I won't set any fixed sizes.
>
> Allan Anderson wrote:
> I've reduced the overall width, closer to its original value, in order to allow use on smaller screens.
> I've removed the inheritance from QDialog, replacing it with QWidget. I hadn't realised QWizard itself inherits
> from QDialog. I'm suspecting that that was actually the cause of some of the difficulties on different distros.
>
> One thing I'd like opinions on is the use of resize(int, int). I'm trying to keep the UI appearance 'tidy',
> avoiding the clipping of row height, athough obviously the user may choose to resize the plugin and please himself
> about the appearance. Anyway, I start off with a display of ten rows, which looks OK until the user back-tracks
> and loads another file with different column widths. This can result in the appearance/disappearance of the
> horizontal scroll bar, which in turn disrupts the table height and appearance. So, I've used a resize() to restore
> the original ten row display. However, this may not be what the user would want. Up to now, I've allowed for
> saving many user preferences, but haven't saved window size. So, I'm thinking to do that, and then the resize
> I've adopted would be of the saved values.
>
> Any opinions, anyone?
>
>
> Cristian Oneț wrote:
> Making the window size persistent would be very useful. Of course you would need to check that the saved size matched the available space on the desktop to avoid weird behavior when moving to a different resolution.
>
> Christian David wrote:
> "...I start off with a display of ten rows..." How about not fixing it to n rows. Instead you could create the window with the default size. Then calculate how many rows are shown and shrink or enlarge the window in a way that no row is shown half. This could avoid some issues which will be introduced by a fixed number.
>
> Also keep in mind that in near future high dpi displays will be common which will make the unit "pixel" useless for sizes.
>
> In my opinion it has no benefit not to show rows partly. This can actually clarify that there is more in the view than you can see directly (e.g. Mac OS X has no scroll bars anymore to indicate that).
>
> Allan Anderson wrote:
> Christian David 2 hours, 6 minutes ago (May 16, 2014, 7:39 p.m.)
>
> > "...I start off with a display of ten rows..." How about not fixing it to n rows. Instead you could create the window with the
> > default size. Then calculate how many rows are shown and shrink or enlarge the window in a way that no row is shown half. This could
> > avoid some issues which will be introduced by a fixed number.
>
> Allan Anderson
> Well, that is actually the process I'm using, albeit for a fixed number of rows, rather than for a variable number.
>
> > Also keep in mind that in near future high dpi displays will be common which will make the unit "pixel" useless for sizes.
>
> > In my opinion it has no benefit not to show rows partly. This can actually clarify that there is more in the view than you can see directly (e.g. Mac OS X has no scroll bars anymore to indicate that).
>
> But doesn't that imply that one then needs to ensure that there is always a part row at the foot to achieve that? Anyway, the scroll bars actually show that there is more to see. Even if the unit "pixel" becomes useless for sizes, some other measurement method would presumably still be required?
>
> But, more seriously, thanks for the input. Unfortunately, I have a 'thing' about such as verticals that aren't vertical, and other things that aren't quite right - to my eyes. Another for-instance is that when moving through the various wizard pages, the different wizard pages have different heights, and the table above them therefore has a tendancy to bob up and down, which would also offend.
>
> Never-the-less, it is becoming very debatable that the effort involved in avoiding such quirks is un-justifiable.
>
> Side-stepping all that for the moment, I'm experimenting with a splitter between the table and the wizard pages below. The thought was that it would reduce/eliminate interference between them. It also makes for easy height adjustment if say part of a wizard display , or a table row, gets clipped.
>
> On the question of Cristian's response - "Making the window size persistent would be very useful. Of course you would need to check that the saved size matched the available space on the desktop to avoid weird behavior when moving to a different resolution.", I've done some quick tests, right down to 800x640, without having to do any checks, and it looks fairly straight-forward.
>
>
>
>
>
> Christian David wrote:
> >> "...I start off with a display of ten rows..." How about not fixing it to n rows. Instead you could create the window with the
> >> default size. Then calculate how many rows are shown and shrink or enlarge the window in a way that no row is shown half. This could
> >> avoid some issues which will be introduced by a fixed number.
> > Well, that is actually the process I'm using, albeit for a fixed number of rows, rather than for a variable number.
>
> There is a misunderstanding. I wanted suggest *not* to use a fixed number and a variable number instead. This could solve some issues with strange resolutions.
>
>
> >> In my opinion it has no benefit not to show rows partly. This can actually clarify that there is more in the view than you can see directly (e.g. Mac OS X has no scroll bars anymore to indicate that).
>
> > But doesn't that imply that one then needs to ensure that there is always a part row at the foot to achieve that?
> Yes, you are right. But I would drop that to reduce work ;)
>
> > Anyway, the scroll bars actually show that there is more to see.
> Not all systems show scroll bars. E.g. iOS and Mac OS X show them only while you are scrolling. I think this will become more common.
>
> > Even if the unit "pixel" becomes useless for sizes, some other measurement method would presumably still be required?
> Sure, you are right. Just wanted to warn that using pixels will need some rework in near future.
>
> Allan Anderson wrote:
> > There is a misunderstanding. I wanted suggest *not* to use a fixed number and a variable number instead. This could solve some issues with
> > strange resolution.
>
> OK. So, we may have a file containing just one transaction, or a couple of hundred. How does the variable number get determined? If there are, say, 20 lines, part of the window is occupied by the wizard page where the user sets his selections. Above that is the table showing the actual data. The table window needs a number of lines. For the initial state, I am presently displaying 10 lines, which allows the importer to stay within a height of 640 px (for now), to allow for small screens. The user may choose to stretch or shrink the window to his choice. So actually, I don't have a fixed number of lines, just a starting number set to 10.
>
> This is not cast in stone and I am not being dogmatic. I just am trying to find out how to set this variable number. I hope the above throws a little more light, but if I am missing your point, then I'm happy to make changes.
>
>
> Christian David wrote:
> If you finished you work already, I suggest to use it as it is. We can gain experience with it and change it if it is really necessary (I could help then with code not only words ;) ).
>
> Also I noticed there was an error in my logic. So issues are less probable than I first thought. Thank you for your patience!
I've had to break off to attend to a bug (or to cope with an unexpected condition.) Should be able to get back to it pretty soon.
< Also I noticed there was an error in my logic. So issues are less probable than I first thought. Thank you for your patience!
No problem! I was just a bit concerned I was being dim and missing something obvious. Thanks for your interest, though.
- 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ț
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20140528/f4a8c0d2/attachment-0001.html>
More information about the KMyMoney-devel
mailing list