On 07/07/12 01:10, Thomas Lübking wrote:
Just assume a lot of cheering for the approach of a simple but complete IMAP client and fancy stories about me ever once again looking at Trojitá here.
Hi Thomas, cheers received, thanks for them :).
Attached are 3 small patches, mostly regarding the UI (git format) Notice that the messageview patch has only partial impact on the oxygen style since it enforces window colors for tabwidgets etc. but it does work eg.with QtCurve, Skulpture and - surprise - Bespin =)
After having applied them, I see a few changes which I don't like:- Please use "0" instead of "NULL" (see [1], even though it isn't terribly explicit in there).
- The header of the message view has black background on my style (Oxygen) -- that's a no-go from my point of view.
- The spacing in the headers of the message view is still "wrong", ie. showing big spaces around the individual items. I don't see that in each and every mail, but it is clearly visible when using the "wide layout" and showing a "short mail"; doing that, the spacing around the From/To/Subject/... block and around the "tag list widget" is rather intrusive. As shown in the attached screenshot, it isn't an issue with certain messages. I don't think it's a regression from the previous state, but if you're touching this part, I'd prefer to have this one fixed, too, preferably in a separate patch. But that's certainly not a blocker, just something to have a look at in future.
- I don't like the lazy creating of the QTimer -- I prefer to instantiate it in the constructor.
- It could be just because I'm back from a well-deserved vacation, but it took me some time to figure out that the purpose of the QTimer is not to activate the search without having to press enter after the user has stopped typing for a brief moment, but to go back to unfiltered results after the user deletes her input -- I like that feature, but would appreciate a comment about that. In general, Trojita was criticized for lack of comments a few years ago which is something I'd like to fix.
- I like the idea of supporting the "advanced IMAP search" -- it's an interesting feature. In the long run, it shall probably be replaced by something more intuitive (I don't really know all supported IMAP search operators from the top of my head; I also managed to introduce a very embarrassing bug with OR in that code, so I don't really see people using that too much) -- it's an excellent idea for now.
- I don't like the implementation of its parser, though -- please don't use constructs like "if (!(inside = !inside))". The code doesn't properly handle escape sequences (\") and -- in my opinion -- would look better if it used better variable names than "inside", "k", "n". Furthermore, I wonder if the parsing actually has to be performed at all -- if we want to guard against malformed IMAP commands, the parser would have to be much, much more strict and deal with both low-level syntax and high-level meaning. Given that this is a feature clearly aimed at "advanced users", why not simply return a single-element QStringList() << foo.text()?
- Not sure if the "flat" presentation of the message parts is better -- I see that the old look is probably pretty cluttered up, but the new one looks weird on my style -- see the attached screenshot.
- Do you really like the buttons-right-from-the-qlineedit? I'm asking because I've grown used to the wide layout of Trojita (three full-sized columns) and in that configuration, the search input box is rather small after applying your changes -- to the extent that one cannot see the advice about the raw IMAP search with ":=". That's why I originally made the buttons hidden by default, appearing only after the user has typed something -- to conserve the precious screen space. I'm not sure what's the best approach here.
I'd also be interested in getting the composer (currently writing from a tiny msmtp frontend i wrote... so let's hope there actually is sth. attached) in shape and add multi-account support.
Well, the multiaccount support is going to be a rather big change -- doable, but intrusive into many parts of Trojitá. I'm very close to the deadline of my thesis, so I won't likely be able to spend any time on reviewing your patches before late September -- final exams and such stuff is more critical for me at this point. I'll do my best to escape from learning and text-producing mode every now and then :), but just don't assume quick replies during this time.
So, if you're interested in more patches (that is iff you accept patches from random external sources at all) just call.
Sure, patches are most welcome and I'm looking forward for more of them from you.
PS: is this the proper way to approach patches at all or do you handle them on reviewboard?
I don't care much -- but if you create a merge request on Gitorious, please send me a mail, they are notoriously bad at delivering notifications from there to my inbox. Having the discussion on the ML also gives a bit more visibility to the project, I guess, so in fact I'm inclined to prefer having them discussed over mail.
Thanks for your patches, I hope that we'll be able to reach consensus about some minor issues soon. I really appreciate your time here.
Cheers, Jan [1] https://projects.flaska.net/projects/trojita/wiki/Coding_Style -- Trojita, a fast e-mail client -- http://trojita.flaska.net/
<<attachment: trojita-thomas-msgview-patch-1.png>>
