Bug 109836 - Add Support for displaying Face: header
Summary: Add Support for displaying Face: header
Status: RESOLVED FIXED
Alias: None
Product: kmail
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR wishlist
Target Milestone: ---
Assignee: kdepim bugs
URL:
Keywords:
: 110057 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-07-29 15:55 UTC by Nick Brown
Modified: 2008-12-15 12:16 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
sample message with Face: header (4.10 KB, message/news)
2005-08-15 17:59 UTC, Nick Brown
Details
another sample message with Face: header (3.86 KB, message/news)
2005-08-15 18:01 UTC, Nick Brown
Details
Failed attempt add adding Face: header support (831 bytes, patch)
2005-08-15 18:03 UTC, Nick Brown
Details
working patch that adds support for Face: header (874 bytes, patch)
2005-08-16 14:58 UTC, Nick Brown
Details
working patch that adds support for Face: header with checks (1.58 KB, patch)
2005-08-16 15:49 UTC, Nick Brown
Details
updated patch (1.43 KB, patch)
2005-08-22 00:58 UTC, Nick Brown
Details
another update (1.59 KB, patch)
2005-08-22 01:05 UTC, Nick Brown
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Brown 2005-07-29 15:55:58 UTC
Version:            (using KDE Devel)
Installed from:    Compiled sources

This is not a request for X-Face: header support, kmail already supports setting and displaying that header.

http://quimby.gnus.org/circus/face/
The Face: header is similar. It is a 48x48 base64-encoded colour png image.

Adding support for displaying this should be straight forward.
Currently kmail displays the addressbook picture of contact if it exists, and if not then the X-Face: header.
The Face: header should take take precedence over X-Face but below the addressbook picture.

Adding support for setting the Face: header could come later.
Comment 1 Thiago Macieira 2005-07-30 05:52:30 UTC
Since it's not an X- header, this should be defined in an RFC, right?
Comment 2 Thiago Macieira 2005-08-03 04:58:58 UTC
*** Bug 110057 has been marked as a duplicate of this bug. ***
Comment 3 Nick Brown 2005-08-08 14:07:33 UTC
I'm a KDE/Qt programming novice, so tried this code.
Doesn't seem to work, but perhaps someone can fix it or expand on it.

Index: headerstyle.cpp
===================================================================
--- headerstyle.cpp     (revision 443530)
+++ headerstyle.cpp     (working copy)
@@ -525,6 +525,21 @@

     if( photoURL.isEmpty() )
     {
+      // no photo, look for a Face headerstyle
+      QString faceheader = message->headerField( "Face" );
+      if ( !faceheader.isEmpty() )
+      {
+        kdDebug( 5006 ) << "Found Face: header" << endl;
+        QImage faceimage;
+        faceimage.loadFromData( KCodecs::base64Decode(faceheader.latin1()), "png" );
+        photoURL = imgToDataUrl( faceimage );
+        photoWidth = 48;
+        photoHeight = 48;
+      }
+    }
+
+    if( photoURL.isEmpty() )
+    {
       // no photo, look for a X-Face header
       QString xfaceURL;
       QString xfhead = message->headerField( "X-Face" );
Comment 4 Nick Brown 2005-08-15 17:59:57 UTC
Created attachment 12229 [details]
sample message with Face: header
Comment 5 Nick Brown 2005-08-15 18:01:10 UTC
Created attachment 12230 [details]
another sample message with Face: header
Comment 6 Nick Brown 2005-08-15 18:03:44 UTC
Created attachment 12231 [details]
Failed attempt add adding Face: header support
Comment 7 Volker Krause 2005-08-15 22:50:26 UTC
The problem with your patch might be that you use a version of base64Decode() that returns a QCString. A QCString is 0-terminated and thus not very well suited for arbitrary binary data. You might want to try the version that puts the result into a QByteArray (see http://developer.kde.org/documentation/library/3.4-api/kdecore/html/classKCodecs.html#e17).
Comment 8 Nick Brown 2005-08-16 00:27:22 UTC
So I tried this patch, but it fails to compile with below error.
I can only get the header as QString and don't how to cast it to a QByteArray.

Index: headerstyle.cpp
===================================================================
--- headerstyle.cpp     (revision 443530)
+++ headerstyle.cpp     (working copy)
@@ -525,6 +525,25 @@

     if( photoURL.isEmpty() )
     {
+      // no photo, look for a Face headerstyle
+      QString faceheader = message->headerField( "Face" );
+      if ( !faceheader.isEmpty() )
+      {
+        QByteArray facearray;
+       QImage faceimage;
+
+       kdDebug( 5006 ) << "Found Face: header" << endl;
+
+       KCodecs::base64Decode(faceheader.latin1(), facearray);
+        faceimage.loadFromData( facearray, "png" );
+        photoURL = imgToDataUrl( faceimage );
+        photoWidth = 48;
+        photoHeight = 48;
+      }
+    }
+
+    if( photoURL.isEmpty() )
+    {
       // no photo, look for a X-Face header
       QString xfaceURL;
       QString xfhead = message->headerField( "X-Face" );




/home/kdedevel/kdesvn/kdepim/kmail/headerstyle.cpp: In member function `virtual\
 QString KMail::FancyHeaderStyle::format(const KMMessage*, const KMail::HeaderS\
trategy*, const QString&, bool) const':
/home/kdedevel/kdesvn/kdepim/kmail/headerstyle.cpp:537: error: invalid conversi\
on from `const char*' to `int'
/home/kdedevel/kdesvn/kdepim/kmail/headerstyle.cpp:537: error:   initializing a\
rgument 1 of `QMemArray<type>::QMemArray(int) [with type = char]'
Error creating ../kmail/headerstyle.lo. Exit status 1.
Comment 9 Volker Krause 2005-08-16 09:51:26 UTC
I used the following code in a similar situation:
    QByteArray decodedPicture;
    KCodecs::base64Decode( text.utf8(), decodedPicture );
    a.setPhoto( Picture( QImage( decodedPicture ) ) );

QString::utf8() returns a QCString which is a sub-class of QByteArray.
Comment 10 Nick Brown 2005-08-16 14:58:17 UTC
Created attachment 12233 [details]
working patch that adds support for Face: header

I've updated the patch, and it now works perfectly. Can I commit it?
Comment 11 Nick Brown 2005-08-16 15:49:10 UTC
Created attachment 12235 [details]
working patch that adds support for Face: header with checks

This patch adds lots of checks and debugs to make sure it we match the
specifcation for the header.
Comment 12 Nick Brown 2005-08-20 23:34:17 UTC
SVN commit 451500 by nickbroon:

FEATURE: 109836
Add Support for displaying Face: header


 M  +44 -0     headerstyle.cpp  


--- branches/KDE/3.5/kdepim/kmail/headerstyle.cpp #451499:451500
@@ -525,6 +525,50 @@
 
     if( photoURL.isEmpty() )
     {
+      // no photo, look for a Face header
+      QString faceheader = message->headerField( "Face" );
+      if ( !faceheader.isEmpty() )
+      {
+	QImage faceimage;
+
+	kdDebug( 5006 ) << "Found Face: header" << endl;
+
+	QCString facestring = faceheader.utf8();
+	// Spec says header should be no longer than 998 bytes
+	if ( facestring.length() < 998 )
+	  {
+	    QByteArray facearray;
+	    KCodecs::base64Decode(facestring, facearray);
+	    
+	    QImage faceimage;
+	    if ( TRUE == faceimage.loadFromData( facearray, "png" ) )
+	      {
+		// Spec says image must be 48x48 pixels
+		if ( (48 == faceimage.width()) && (48 == faceimage.height()) )
+		  {
+		    photoURL = imgToDataUrl( faceimage );
+		    photoWidth = 48;
+		    photoHeight = 48;
+		  }
+		else
+		  {
+		    kdDebug( 5006 ) << "Face: header image is" << faceimage.width() << "by" << faceimage.height() <<"not 48x48 Pixels" << endl;
+		  }
+	      }
+	    else
+	      {
+		kdDebug( 5006 ) << "Failed to load decoded png from Face: header" << endl;
+	      }
+	  }
+	else
+	  {
+	    kdDebug( 5006 ) << "Face: header too long at " << facestring.length() << endl;
+	  }
+      }
+    }
+
+    if( photoURL.isEmpty() )
+    {
       // no photo, look for a X-Face header
       QString xfaceURL;
       QString xfhead = message->headerField( "X-Face" );
Comment 13 Ingo Klöcker 2005-08-21 12:48:22 UTC
On Saturday 20 August 2005 23:34, Nick Brown wrote:
> SVN commit 451500 by nickbroon:
>
> FEATURE: 109836
> Add Support for displaying Face: header


We are in feature freeze for KDE 3.5 and this feature is not among the 
features which are listed in the feature plan. I'm sorry, but you will 
have to revert your commit.

> +	    if ( TRUE == faceimage.loadFromData( facearray, "png" ) )


TRUE doesn't exist. If you mean the C++ key word "true" then you should 
have simply written 
+	    if ( faceimage.loadFromData( facearray, "png" ) )

Also I notice tabs in your patch. Usage of tabs is not allowed! 
Moreover, the opening '{' has to be on the same line a 'if ( ... )'. 
Please read http://korganizer.kde.org/develop/hacking.html for more 
information about the kdepim coding style.

Regards,
Ingo
Comment 14 Nick Brown 2005-08-22 00:33:38 UTC
SVN commit 451833 by nickbroon:

Reverting 451500

Please accept my deepest apologies, I only over my KDE inexperience as
an excuse.
I was unaware of the feature freeze.
I had asked for and was given a review on the #kontact irc channel.

I will make the required changes and attach a new patch to bug report
for safe keeping. How do I add something to the feature list for a
future release as I am keen to get this into kmail at some point?

Sorry again.

CCBUG: 109836



 M  +0 -44     headerstyle.cpp  


--- branches/KDE/3.5/kdepim/kmail/headerstyle.cpp #451832:451833
@@ -525,50 +525,6 @@
 
     if( photoURL.isEmpty() )
     {
-      // no photo, look for a Face header
-      QString faceheader = message->headerField( "Face" );
-      if ( !faceheader.isEmpty() )
-      {
-	QImage faceimage;
-
-	kdDebug( 5006 ) << "Found Face: header" << endl;
-
-	QCString facestring = faceheader.utf8();
-	// Spec says header should be no longer than 998 bytes
-	if ( facestring.length() < 998 )
-	  {
-	    QByteArray facearray;
-	    KCodecs::base64Decode(facestring, facearray);
-	    
-	    QImage faceimage;
-	    if ( TRUE == faceimage.loadFromData( facearray, "png" ) )
-	      {
-		// Spec says image must be 48x48 pixels
-		if ( (48 == faceimage.width()) && (48 == faceimage.height()) )
-		  {
-		    photoURL = imgToDataUrl( faceimage );
-		    photoWidth = 48;
-		    photoHeight = 48;
-		  }
-		else
-		  {
-		    kdDebug( 5006 ) << "Face: header image is" << faceimage.width() << "by" << faceimage.height() <<"not 48x48 Pixels" << endl;
-		  }
-	      }
-	    else
-	      {
-		kdDebug( 5006 ) << "Failed to load decoded png from Face: header" << endl;
-	      }
-	  }
-	else
-	  {
-	    kdDebug( 5006 ) << "Face: header too long at " << facestring.length() << endl;
-	  }
-      }
-    }
-
-    if( photoURL.isEmpty() )
-    {
       // no photo, look for a X-Face header
       QString xfaceURL;
       QString xfhead = message->headerField( "X-Face" );
Comment 15 Nick Brown 2005-08-22 00:58:00 UTC
Created attachment 12315 [details]
updated patch

I've made the suggest changes.
The use of TRUE has been removed.
The header length check comment updated, and length of the header name taken
into account.
The code updated to meet the coding style guidelines, except for line length
but this seems to be widely ignored for debug lines. Please check for further
violations. (It should be noted I had simply been following the X-Face code in
the same file for previous patches.)
Comment 16 Nick Brown 2005-08-22 01:05:53 UTC
Created attachment 12316 [details]
another update

removed tabs.
Comment 17 Nick Brown 2006-04-26 23:54:50 UTC
This was originally added to 3.5 but backed out as it was in feature freeze.
However as the 3.5 branch now has less strict freeze, to allow small features in, due the delay until 4.0, can this now be committed?
Its a very small and safe addition.
Comment 18 Nick Brown 2006-04-28 16:28:47 UTC
There is only 4 days until the freeze for 3.5.3
Can someone commit this?
headerstyle.cpp has had no updates since this was reverted due to the feature freeze for 3.5.0 so it will apply cleanly. (and I made the suggest updates)
Comment 19 Allen Winter 2006-04-30 01:28:54 UTC
Nick,
I think we do meet all the requirements needed to include this patch in the KDE 3.5.3 release.  However, I spoke with danimo last night, and he also has concerns that it's not an X-header.

So, really Ingo needs to make the decision.
Comment 20 Nick Brown 2006-05-04 17:30:44 UTC
Although its true that a header like this should really be a X-header, theres a long history of many such non X-headers.
Further there 5 other mail clients that support this specific headers (See http://quimby.gnus.org/circus/face/).
Further, this patch does not generate the header, it simply renders the header in those mails that contain it.
Comment 21 Allen Winter 2006-10-17 00:00:15 UTC
SVN commit 596213 by winterz:

Nick's patch for displaying Face-formatted pictures in the header that has been
tested and lingering for a long time and keeps getting lost.
Ingo approved this a long time ago.

Thanks for the patch Nick, and apologies for the many delays.

BUGS: 109836


 M  +34 -0     headerstyle.cpp  


--- branches/KDE/3.5/kdepim/kmail/headerstyle.cpp #596212:596213
@@ -527,6 +527,40 @@
       userHTML = "&nbsp;";
     }
 
+    if( photoURL.isEmpty() ) {
+      // no photo, look for a Face header
+      QString faceheader = message->headerField( "Face" );
+      if ( !faceheader.isEmpty() ) {
+        QImage faceimage;
+
+        kdDebug( 5006 ) << "Found Face: header" << endl;
+
+        QCString facestring = faceheader.utf8();
+        // Spec says header should be less than 998 bytes
+        // Face: is 5 characters
+        if ( facestring.length() < 993 ) {
+          QByteArray facearray;
+          KCodecs::base64Decode(facestring, facearray);
+    
+          QImage faceimage;
+          if ( faceimage.loadFromData( facearray, "png" ) ) {
+            // Spec says image must be 48x48 pixels
+            if ( (48 == faceimage.width()) && (48 == faceimage.height()) ) {
+              photoURL = imgToDataUrl( faceimage );
+              photoWidth = 48;
+              photoHeight = 48;
+            } else {
+              kdDebug( 5006 ) << "Face: header image is" << faceimage.width() << "by" << faceimage.height() <<"not 48x48 Pixels" << endl;
+            }
+          } else {
+            kdDebug( 5006 ) << "Failed to load decoded png from Face: header" << endl;
+          }
+        } else {
+          kdDebug( 5006 ) << "Face: header too long at " << facestring.length() << endl;
+        }
+      }
+    }
+
     if( photoURL.isEmpty() )
     {
       // no photo, look for a X-Face header