> On April 18, 2014, 1:34 p.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. >
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! - Christian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117620/#review56020 ----------------------------------------------------------- On April 17, 2014, 11: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, 11: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
