Hi, The patches are available at my branch called "clean":
I can't log in into gitorious because my openid is down at mildred.fr. Also, don't try to contact me at mildred.fr for the next few hours. On 5 March 2012 18:54, Jan Kundrát <[email protected]> wrote: > - 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 just implemented that, feels great. > > - 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 check the fixes #8 has been added to the right commit > > - 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). > ok > > - The TagListWidget shall be hidden when there's no visible message > I did that, but I'm not sure it works. Please check. > > - There already is a Model* in MessageView.cpp, so there shall be no > need for another explicit casting > right, there is even an index right at hand > > - Please make the methods that accept a QString work with const& > (MessageView's newLabelAction and dleteLabelAction) > good point > > - 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 :) > I don't know what you're talking about, so i'm ignoring 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'll let you choose the license. There is just FlowLayout that I copied from the Qt examples, I hope you don't mind me doing that. > - Please do not align variable definitions like this: > > Foo foo; > BarBazXYZ barBazXYZ; > > Instead, please do use this: > > Foo foo; > BarBazXYZ barBazXYZ; > Ok, feels less readable, but why not. > > - 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. > Ok > > - Please split single-line constructs like this one into two lines: > > if(!tag.isEmpty()) emit tagAdded(tag); > Ok > > - When freeing a QList<Something*>, please just use qDeleteAll() and > .clear() > I didn't know about qDeleteAll, good point. > > - 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 understand > > - 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(). > Ok > > - 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) > I changed that, I didn't see any place where QLatin1String() or QString::fromAscii() was necessary though. > > - What do you think about providing localized names for some of the > well-known flags like \Seen, \Deleted, $Forwarded etc etc? > I thought that these special flags (starting with \ or $) should be hidden by default. But I did not implement that. > > I can't spot anything else right now, which is great -- please keep up > the good work! > Thank you for your interest. Mildred
