[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] X-Face support for kdepim
From:       Ingo =?utf-8?q?Kl=C3=B6cker?= <kloecker () kde ! org>
Date:       2004-11-14 22:22:21
Message-ID: 200411142322.30831 () erwin ! ingo-kloecker ! de
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Saturday 13 November 2004 01:53, Jakob Schroeter wrote:
> Hi,
>
> the author of the compface library agreed to change its license to
> LGPL. So it should be possible to add my patches for kmail and knode
> to kdepim. This is wished for in BR28319 and BR92261.
> Please find the patch at
> http://camaya.net/download/kdepim-xface-v1.diff
>
> The KXFace class is added to libkdepim. It is mostly based on
> libcompface. In KMail, X-Faces can be set per identity, with preview.
> They are displayed in the message reader alongside the headers for
> the fancy and standard header styles. It is currently not yet
> possible to disable the X-Face display as wished for on kmail-devel.
> The KNode part adds support for displaying X-Faces only, you would
> have to manually create an appropriate X-Face header.
>
> I would like to commit by next friday if there are no serious
> objections by then.
> What do you think?

I general the patch is okay and it works quite nicely. But it's not 
ready for committing. Let's go into the details:

In kmail/headerstyle.cpp:
- The X-Face image should only be shown with the fancy header style. In 
particular this means that moving FancyHeaderStyle::imgToDataUrl to 
HeaderStyle::imgToDataUrl is not necessary.

- The X-Face image should only be shown if there's no image for the 
sender in the address book, i.e. if photoURL.isEmpty(). See 
headerstyle.cpp around line 460.


In kmail/identitydialog.cpp:
- I think "X-Face" is a bad description for the tab. I suggest using 
"Photo" instead. On the tab there should then be some text which 
explains that currently only black-white photos are supported.


In kmail/xfaceconfigurator.cpp:
- The license of this file should be GPL + Qt exception as for example 
in folderstorage.cpp.

- It's not acceptable (usability-wise) that the user has to create the 
X-Face header himself. The user should just have to select an image 
file. Then an X-Face should be created from this image (i.e. scale it 
to 48x48 and then make it black-white). Internally the X-Face should be 
stored.

- The links are not necessary when the user can simply select an image 
file.


In libkdepim/Makefile.am:
- It doesn't seem to be necessary to link against libkmime and to add 
-I$(top_srcdir)/libkmime.


In libkdepim/kxface.*:
- Please give comp and uncomp more meaningful names. From the name it's 
completely unclear to me what those methods do.

- I don't like all the #defines in kxface.h. This will probably lead to 
conflicts. Therefore please move all defines which are not needed in 
kxface.h to kxface.cpp and replace the remaining defines by private 
static const members of KXFace or by a private typedefs (in case of 
WORD).

- Pass QString as 'const QString &' in comp, uncomp and toPixmap.

- KXFace::toPixmap doesn't look very efficient. tmp should be char 
instead of QString. And the long if-else statement should be a switch 
statement.

- KXFace::comp doesn't guard against a possible buffer overflow.
+  char *fbuf = (char *)malloc(MAX_XFACE_LENGTH);
+  memset(fbuf, '\0', MAX_XFACE_LENGTH);
+  strncpy(fbuf, str.latin1(), str.length());
You have to add a str.truncate( MAX_XFACE_LENGTH ) in the first line. 
Interestingly KXFace::uncomp checks the length of the parameter.

- KXFace::x and KXFace::y should be static methods and the parameter 
should be passed as 'const int pos'.

=====

Now a few things which could be done to improve the feature:
- In the configuration: Add the possibility to load the X-Face from the 
image in the user's own address book entry.

- Add the possibility to store the X-Face in the address book entry of 
the sender (via an action in the context menu).

Regards,
Ingo

[Attachment #5 (application/pgp-signature)]

_______________________________________________
kde-pim mailing list
kde-pim@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
kde-pim home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread]