5 Mar 2012 18:54
Merging Mildred's Trojita work for 0.3
Jan Kundrát <jkt <at> flaska.net>
2012-03-05 17:54:24 GMT
2012-03-05 17:54:24 GMT
Hi Mildred (and hi the Cc-ed list), I'd like to release Trojita 0.3 soon, and I'd love to include your changes in that. I took a look on what you've done so far, and here are my comments that I promised. They're mostly aimed at being consistent with (my vision of) Trojita; if you don't like them, just talk to me, they're meant as suggestions. - I *really* like the flag management GUI, the rounded borders and the colored background look just nice. I want to have that in 0.3 :). - As you said, the code needs some more work (the tokens shall autoupdate, for example) -- I know that you're aware of that, I just want to document this. It should also be pretty easy, the Imap::Mailbox::Model will emit a dataChanged() for the message's index when the flags change (and also under other circumstances). - I'll be happy to merge (and test) the TLS/SMTP bits as they are, but I'd like to have them as a standalone patch series. Could you please do that in an extra branch and also add an extra "fixes #8" line at the end of the last commit for Redmine to auto-close that bugreport? - Please be careful when QtAssistant/Creator touches the .ui files, it has a habit of adding an explicit geometry property to some of the elements. I'm not sure if it actually matters, but I tend to remove these changes and not commit them (I think that not having them makes the GUI more size-independent, but I could be mistaken of course). - The TagListWidget shall be hidden when there's no visible message - There already is a Model* in MessageView.cpp, so there shall be no need for another explicit casting - Please make the methods that accept a QString work with const& (MessageView's newLabelAction and dleteLabelAction) - Gui/SettingsDialog.cpp's OutgoingPage constructor uses magic numbers for populating a combobox -- yes, that's my code, but I'd say that it must be possible to do that in some better form? Or mayeb not, please feel free to ignore this point :) - Please add proper copyright header to your .cpp/.h files; these should follow the templates in the rest of Trojita's code. I can accept anything which is licensed under either: 1) BSD 2) "LGPLv2 or later" 3) "GPLv2 or later, at your option" 4) "GPLv2 or GPLv3, at your option" The two companies I've worked with have no problem having a GPLed code in their product, so not contributing under the BSD is fine. Most code of Trojita is currently under GPLv2 or GPLv3, that's a decision I've made some time ago which might need revisiting now that more people are contributing; it would be a bit awkward for me to insist on people allowing for (possible) GPLv4 while not doing the same thing myself. Anyway, any of 1-2-3 is absolutely OK for the project as a whole, I'd prefer not to have #4 as that'd complicate the matter even more than it already is. - Please do not align variable definitions like this: Foo foo; BarBazXYZ barBazXYZ; Instead, please do use this: Foo foo; BarBazXYZ barBazXYZ; - More bits about the preferred coding style are at [1] - Please sort your #includes -- in .h, the order is system - Qt - Trojita and all groups are sorted alphabetically. In .cpp, the only exception is the the file's own .h which goes first, the rest is the same. - Please split single-line constructs like this one into two lines: if(!tag.isEmpty()) emit tagAdded(tag); - When freeing a QList<Something*>, please just use qDeleteAll() and .clear() - In src/Gui/TagWidget.cpp's TagWidget::TagWidget, please initialize the m_tagName member in the initializer list, not inside the constructor's body - I'd prefer to have commit 8ec2854a6626202efa0321d76574fb0e204103b2 split into one providing the Model::markFlag method and switching the markMessages...() to use that and another commit for the rest of the changes. Also, I'd strongly prefer name like setMessageFlags(). - Strings shall be either wrapped in tr() for user-facing ones or in QLatin1String() or QString::fromAscii() for those which are "magic" (like the IMAP flag names) - What do you think about providing localized names for some of the well-known flags like \Seen, \Deleted, $Forwarded etc etc? I can't spot anything else right now, which is great -- please keep up the good work! Cheers, Jan [1] https://projects.flaska.net/projects/trojita/wiki/Coding_Style -- -- Trojita, a fast e-mail client -- http://trojita.flaska.net/
RSS Feed