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/
signature.asc
Description: OpenPGP digital signature
