General discussion about an IMAP e-mail client Trojita ()

Jan Kundrát | 5 Mar 2012 18:54

Merging Mildred's Trojita work for 0.3

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

(Continue reading)

headers

Mildred Ki'Lya | 13 Mar 2012 09:29

Re: Merging Mildred's Trojita work for 0.3

On 5 March 2012 18:54, Jan Kundrát <jkt <at> flaska.net> wrote:

- 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"

I'd choose anything you want. For mersonal projects, I like the MIT/X11 license but I will stick with what you choose.

Mildred

headers

Mildred Ki'Lya | 13 Mar 2012 11:37

Re: Merging Mildred's Trojita work for 0.3

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 <jkt <at> flaska.net> 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).

- 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.

- 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()

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().

- 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

headers

Jan Kundrát | 13 Mar 2012 16:21

Re: Merging Mildred's Trojita work for 0.3

On 03/13/12 11:37, Mildred Ki'Lya wrote:
> The patches are available at my branch called "clean":

Applied, thanks!

> Please check the fixes #8 has been added to the right commit

Looks good.

>     - The TagListWidget shall be hidden when there's no visible message
> 
> I did that, but I'm not sure it works. Please check.

Checked, works fine.

>     - 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 

I was speaking about this code, which IMHO looks ugly:

>     method->insertItem( 0, tr("TCP"), QVariant( TCP ) );
>     method->insertItem( 1, tr("SSL"), QVariant( SSL ) );
>     method->insertItem( 2, tr("Local Process"), QVariant( PROCESS ) );
>     using Common::SettingsNames;
>     if ( QSettings().value( SettingsNames::imapMethodKey ).toString() == SettingsNames::methodTCP ) {
>         method->setCurrentIndex( 0 );
>     } else if ( QSettings().value( SettingsNames::imapMethodKey ).toString() ==
SettingsNames::methodSSL ) {
>         method->setCurrentIndex( 1 );
>     } else {
>         method->setCurrentIndex( 2 );
>     }

...but as it's my code, I shall probably not complain too much.

> I'll let you choose the license.

If you're fine with the BSD license, I'd go for that.

> There is just FlowLayout that I copied from the Qt examples, I hope you
> don't mind me doing that.

It's all right.

>     - 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.

There's an optional macro QT_NO_CAST_FROM_ASCII which -- if defined --
disables constructing QStrings directly from string literals or char *.
The idea is to make sure that any user-facing strings either came
through tr() and are therefore translatable, or that the programmer made
an explicit effort indicating that a conversion shall take place (by
using QLatin1String, the preferred lightweight form, or through stuff
like QString::fromAscii). The end goal is to make sure that the
application can be properly translated and no English strings remain in
the UI.

Trojita cannot be compiled in this mode yet, but I do try to make sure
that the new code is "safe" in this regard. In practice, it means that:

- Any user-facing strings are wrapped in tr()
- QStrings are initialized either through QLatin1String("some literal"),
or via QString::fromAscii() (including the implicit conversions from
QByteArray)

That's what I'd like to have in all new code, but it isn't a real
problem at this point.

>     - 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. 

That'd be great -- if you have time in future, I'll be happy to merge a
patch doing something similar.

Cheers,
Jan
--

-- 
Trojita, a fast e-mail client -- http://trojita.flaska.net/

headers

Jan Kundrát | 23 Mar 2012 10:56

Re: Merging Mildred's Trojita work for 0.3

On 03/13/12 11:37, Mildred Ki'Lya 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.

Hi Mildred, there turned out to be a performance problem with this
approach. The code did not check whether the index which got changed was
actually the one of the current message, and would update the
TagListWidget anyway. This was not an issue before, but now I've added
an explicit dataChanged() when flags of any message change (which is
required for proper operation of the GUI when displaying threads),
including during the mailbox sync. As a result of that, when I opened a
message, left it visible in the message view and switched to another
mailbox with, say, 10k of messages, the TagListWidget would be updated
10k times. Just FYI; I haven't noticed that when reviewing, either.
Changed in 344bc91b25e9b0a9c28bcd017ade191e8a1ac32f.

> I thought that these special flags (starting with \ or $) should be
> hidden by default. But I did not implement that. 

Not that this is a huge issue, but it enables users to click on the [x]
button for system flags like \Recent which leads to the IMAP server
responding with a BAD response (which essentially means "hey you,
client, you've sent some garbage here"). I've opened a bugreport [1] for
that.

Would you like to change that, or would you prefer me to finalize this?
I'd like to release 0.3 in a week, and I'd prefer to have this fix included.

With kind regards,
Jan

[1] https://projects.flaska.net/issues/462

--

-- 
Trojita, a fast e-mail client -- http://trojita.flaska.net/

headers

Mildred Ki'Lya | 23 Mar 2012 11:06

Re: Merging Mildred's Trojita work for 0.3

On 23 March 2012 10:56, Jan Kundrát <jkt <at> flaska.net> wrote:

Hi Mildred, there turned out to be a performance problem with this
approach. The code did not check whether the index which got changed was
actually the one of the current message, and would update the
TagListWidget anyway. This was not an issue before, but now I've added
an explicit dataChanged() when flags of any message change (which is
required for proper operation of the GUI when displaying threads),
including during the mailbox sync. As a result of that, when I opened a
message, left it visible in the message view and switched to another
mailbox with, say, 10k of messages, the TagListWidget would be updated
10k times. Just FYI; I haven't noticed that when reviewing, either.
Changed in 344bc91b25e9b0a9c28bcd017ade191e8a1ac32f.

I thought it could happen in theory, but never in practice. I already developped an application that made heavy use of dataChanged() signals, and over time, we switched to make sure we only updated the view when necessary. I just didn't think of the case where the message is opened in a new window.

It shouldn't be very difficult, please feel free to change that.
I'd be more confortable not to have to do this soon because I have a lot of thigs to do. I first need to create an electronic watchdog to reboot my server when (like now) it shuts down for no obvious reason. And I am pretty busy on other fronts as well.

If I have time next week, I might give a hand though.

> I thought that these special flags (starting with \ or $) should be
> hidden by default. But I did not implement that.

Not that this is a huge issue, but it enables users to click on the [x]
button for system flags like \Recent which leads to the IMAP server
responding with a BAD response (which essentially means "hey you,
client, you've sent some garbage here"). I've opened a bugreport [1] for
that.

With the current state of the flag view, I consider the responsability of the user to deal with these flags with care.
It was also good to test that the flags were actually changed.

As I said, don't wait for me. The fix is not very difficult either.

Thank you,

Mildred

PS: If I don't respond to the e-mails, don't hesitate to send a mail at mildred-pub <at> mildred.fr (you can remove the -pub, it's for spam bots mostly)

headers

Jan Kundrát | 23 Mar 2012 14:52

Re: Merging Mildred's Trojita work for 0.3

On 03/23/12 11:06, Mildred Ki'Lya wrote:
> It shouldn't be very difficult, please feel free to change that.
> I'd be more confortable not to have to do this soon because I have a lot
> of thigs to do. I first need to create an electronic watchdog to reboot
> my server when (like now) it shuts down for no obvious reason. And I am
> pretty busy on other fronts as well.

I've changed that one already. Enjoy your soldering break :).

> With the current state of the flag view, I consider the responsability
> of the user to deal with these flags with care.
> It was also good to test that the flags were actually changed.
> 
> As I said, don't wait for me. The fix is not very difficult either.

Will do, thanks for letting me know.

Cheers,
Jan

--

-- 
Trojita, a fast e-mail client -- http://trojita.flaska.net/

headers

Markus Böök | 23 Mar 2012 11:41

Re: Merging Mildred's Trojita work for 0.3

Please remove me from the mailing list, I no longer have the N900 I used Trojita on!

All the best & keep up the good work!

--Markus

On Fri, Mar 23, 2012 at 11:56 AM, Jan Kundrát <jkt <at> flaska.net> wrote:

On 03/13/12 11:37, Mildred Ki'Lya 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.

Hi Mildred, there turned out to be a performance problem with this
approach. The code did not check whether the index which got changed was
actually the one of the current message, and would update the
TagListWidget anyway. This was not an issue before, but now I've added
an explicit dataChanged() when flags of any message change (which is
required for proper operation of the GUI when displaying threads),
including during the mailbox sync. As a result of that, when I opened a
message, left it visible in the message view and switched to another
mailbox with, say, 10k of messages, the TagListWidget would be updated
10k times. Just FYI; I haven't noticed that when reviewing, either.
Changed in 344bc91b25e9b0a9c28bcd017ade191e8a1ac32f.

> I thought that these special flags (starting with \ or $) should be
> hidden by default. But I did not implement that.

Not that this is a huge issue, but it enables users to click on the [x]
button for system flags like \Recent which leads to the IMAP server
responding with a BAD response (which essentially means "hey you,
client, you've sent some garbage here"). I've opened a bugreport [1] for
that.

Would you like to change that, or would you prefer me to finalize this?
I'd like to release 0.3 in a week, and I'd prefer to have this fix included.

With kind regards,
Jan

[1] https://projects.flaska.net/issues/462

--
Trojita, a fast e-mail client -- http://trojita.flaska.net/