Non-image inline attachments not shown in client: Closes #5748

This processes existing email for inline attachments and adds them
to the attachments table, doing the same for new mail as it arrives.

sql/CMakeLists.txt

@@ -12,3 +12,4 @@ install(FILES version-009.sql DESTINATION ${SQL_DEST})
 install(FILES version-010.sql DESTINATION ${SQL_DEST})
 install(FILES version-011.sql DESTINATION ${SQL_DEST})
 install(FILES version-012.sql DESTINATION ${SQL_DEST})
+install(FILES version-013.sql DESTINATION ${SQL_DEST})

sql/version-013.sql

@@ -0,0 +1,9 @@
+--
+-- Add the disposition column as a string so the client can decide which attachments to show.
+-- Since all attachments up to this point have been non-inline, set it to that value (which
+-- is defined in src/engine/api/geary-attachment.vala
+--
+
+ALTER TABLE MessageAttachmentTable ADD COLUMN disposition INTEGER;
+UPDATE MessageAttachmentTable SET disposition=0;
+

src/client/views/conversation-viewer.vala

@@ -15,6 +15,9 @@ public class ConversationViewer : Gtk.Box {
         | Geary.Email.Field.FLAGS
         | Geary.Email.Field.PREVIEW;
     
+    public const string INLINE_MIME_TYPES =
+        "image/png image/gif image/jpeg image/pjpeg image/bmp image/x-icon image/x-xbitmap image/x-xbm";
+    
     private const int ATTACHMENT_PREVIEW_SIZE = 50;
     private const int SELECT_CONVERSATION_TIMEOUT_MSEC = 100;
     private const string MESSAGE_CONTAINER_ID = "message_container";
@@ -473,9 +476,10 @@ public class ConversationViewer : Gtk.Box {
             }
         }
         
-        // Set attachment icon and add the attachments container if we have any attachments.
+        // Set attachment icon and add the attachments container if there are displayed attachments.
-        set_attachment_icon(div_message, email.attachments.size > 0);
+        int displayed = displayed_attachments(email);
-        if (email.attachments.size > 0) {
+        set_attachment_icon(div_message, displayed > 0);
+        if (displayed > 0) {
             insert_attachments(div_message, email.attachments);
         }
         
@@ -1284,7 +1288,7 @@ public class ConversationViewer : Gtk.Box {
         save_attachment_item.activate.connect(() => save_attachment(attachment));
         menu.append(save_attachment_item);
         
-        if (email.attachments.size > 1) {
+        if (displayed_attachments(email) > 1) {
             Gtk.MenuItem save_all_item = new Gtk.MenuItem.with_mnemonic(_("Save All A_ttachments..."));
             save_all_item.activate.connect(() => save_attachments(email.attachments));
             menu.append(save_all_item);
@@ -1303,9 +1307,10 @@ public class ConversationViewer : Gtk.Box {
         Gtk.Menu menu = new Gtk.Menu();
         menu.selection_done.connect(on_message_menu_selection_done);
         
-        if (email.attachments.size > 0) {
+        int displayed = displayed_attachments(email);
+        if (displayed > 0) {
             string mnemonic = ngettext("Save A_ttachment...", "Save All A_ttachments...",
-                email.attachments.size);
+                displayed);
             Gtk.MenuItem save_all_item = new Gtk.MenuItem.with_mnemonic(mnemonic);
             save_all_item.activate.connect(() => save_attachments(email.attachments));
             menu.append(save_all_item);
@@ -1588,6 +1593,29 @@ public class ConversationViewer : Gtk.Box {
         header_text += create_header_row(Geary.HTML.escape_markup(title), value, important);
     }
     
+    private static bool should_show_attachment(Geary.Attachment attachment) {
+        switch (attachment.disposition) {
+            case Geary.Attachment.Disposition.ATTACHMENT:
+                return true;
+            
+            case Geary.Attachment.Disposition.INLINE:
+                return !(attachment.mime_type in INLINE_MIME_TYPES);
+            
+            default:
+                assert_not_reached();
+        }
+    }
+    
+    private static int displayed_attachments(Geary.Email email) {
+        int ret = 0;
+        foreach (Geary.Attachment attachment in email.attachments) {
+            if (should_show_attachment(attachment)) {
+                ret++;
+            }
+        }
+        return ret;
+    }
+    
     private void insert_attachments(WebKit.DOM.HTMLElement email_container,
         Gee.List<Geary.Attachment> attachments) {
 
@@ -1618,6 +1646,9 @@ public class ConversationViewer : Gtk.Box {
 
             // Create an attachment table for each attachment.
             foreach (Geary.Attachment attachment in attachments) {
+                if (!should_show_attachment(attachment)) {
+                    continue;
+                }
                 // Generate the attachment table.
                 WebKit.DOM.HTMLElement attachment_table = Util.DOM.clone_node(attachment_template);
                 string filename = Geary.String.is_empty_or_whitespace(attachment.filename) ?

src/engine/api/geary-attachment.vala

@@ -7,20 +7,58 @@
 public class Geary.Attachment : BaseObject {
     public const Email.Field REQUIRED_FIELDS = Email.REQUIRED_FOR_MESSAGE;
     
+    // NOTE: These values are persisted on disk and should not be modified unless you know what
+    // you're doing.
+    public enum Disposition {
+        ATTACHMENT = 0,
+        INLINE = 1;
+        
+        public static Disposition? from_string(string? str) {
+            // Returns null to indicate an unknown disposition
+            if (str == null) {
+                return null;
+            }
+            
+            switch (str.down()) {
+                case "attachment":
+                    return ATTACHMENT;
+                
+                case "inline":
+                    return INLINE;
+                
+                default:
+                    return null;
+            }
+        }
+        
+        public static Disposition from_int(int i) {
+            switch (i) {
+                case INLINE:
+                    return INLINE;
+                
+                case ATTACHMENT:
+                default:
+                    return ATTACHMENT;
+            }
+        }
+    }
+    
     public string? filename { get; private set; }
     public string filepath { get; private set; }
     public string mime_type { get; private set; }
     public int64 filesize { get; private set; }
     public int64 id { get; private set; }
+    public Disposition disposition { get; private set; }
     
     internal Attachment(File data_dir, string? filename, string mime_type, int64 filesize,
-        int64 message_id, int64 attachment_id) {
+        int64 message_id, int64 attachment_id, Disposition disposition) {
 
         this.filename = filename;
         this.mime_type = mime_type;
         this.filesize = filesize;
         this.filepath = get_path(data_dir, message_id, attachment_id, filename);
         this.id = attachment_id;
+        this.disposition = disposition;
     }
     
     internal static string get_path(File data_dir, int64 message_id, int64 attachment_id,

src/engine/db/db-versioned-database.vala

@@ -52,7 +52,7 @@ public class Geary.Db.VersionedDatabase : Geary.Db.Database {
             check_cancelled("VersionedDatabase.open", cancellable);
             
             try {
-                debug("Upgrading database to to version %d with %s", db_version, upgrade_script.get_path());
+                debug("Upgrading database to version %d with %s", db_version, upgrade_script.get_path());
                 cx.exec_transaction(TransactionType.EXCLUSIVE, (cx) => {
                     cx.exec_file(upgrade_script, cancellable);
                     cx.set_user_version_number(db_version);

src/engine/imap-db/imap-db-account.vala

@@ -1065,7 +1065,7 @@ private class Geary.ImapDB.Account : BaseObject {
     
     // For a message row id, return a set of all folders it's in, or null if
     // it's not in any folders.
-    private Gee.Set<Geary.FolderPath>? do_find_email_folders(Db.Connection cx, int64 message_id,
+    private static Gee.Set<Geary.FolderPath>? do_find_email_folders(Db.Connection cx, int64 message_id,
         Cancellable? cancellable) throws Error {
         Db.Statement stmt = cx.prepare("SELECT folder_id FROM MessageLocationTable WHERE message_id=?");
         stmt.bind_int64(0, message_id);
@@ -1090,7 +1090,7 @@ private class Geary.ImapDB.Account : BaseObject {
     // For a folder row id, return the folder path (constructed with default
     // separator and case sensitivity) of that folder, or null in the event
     // it's not found.
-    private Geary.FolderPath? do_find_folder_path(Db.Connection cx, int64 folder_id,
+    private static Geary.FolderPath? do_find_folder_path(Db.Connection cx, int64 folder_id,
         Cancellable? cancellable) throws Error {
         Db.Statement stmt = cx.prepare("SELECT parent_id, name FROM FolderTable WHERE id=?");
         stmt.bind_int64(0, folder_id);

src/engine/imap-db/imap-db-database.vala

@@ -43,6 +43,10 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase {
             case 12:
                 post_upgrade_populate_internal_date_time_t();
             break;
+            
+            case 13:
+                post_upgrade_populate_inline_attachments();
+            break;
         }
     }
     
@@ -186,7 +190,46 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase {
                 return Db.TransactionOutcome.COMMIT;
             });
         } catch (Error e) {
-            debug("Error populating internaldate_time_t column during upgrade to database schema 11: %s",
+            debug("Error populating internaldate_time_t column during upgrade to database schema 12: %s",
+                e.message);
+        }
+    }
+    
+    // Version 13.
+    private void post_upgrade_populate_inline_attachments() {
+        try {
+            exec_transaction(Db.TransactionType.RO, (cx) => {
+                Db.Statement stmt = cx.prepare("""
+                    SELECT id, header, body
+                    FROM MessageTable
+                    WHERE (fields & ?) = ?
+                    """);
+                stmt.bind_int(0, Geary.Email.REQUIRED_FOR_MESSAGE);
+                stmt.bind_int(1, Geary.Email.REQUIRED_FOR_MESSAGE);
+                Db.Result select = stmt.exec();
+                while (!select.finished) {
+                    int64 id = select.rowid_at(0);
+                    Geary.Memory.Buffer header = select.string_buffer_at(1);
+                    Geary.Memory.Buffer body = select.string_buffer_at(2);
+                    
+                    try {
+                        Geary.RFC822.Message message = new Geary.RFC822.Message.from_parts(
+                            new RFC822.Header(header), new RFC822.Text(body));
+                        Geary.ImapDB.Folder.do_save_attachments_db(cx, id,
+                            message.get_attachments(Geary.Attachment.Disposition.INLINE),
+                            this, null);
+                    } catch (Error e) {
+                        debug("Error fetching inline Mime parts: %s", e.message);
+                    }
+                    
+                    select.next();
+                    pump_event_loop();
+                }
+                
+                return Db.TransactionOutcome.COMMIT;
+            });
+        } catch (Error e) {
+            debug("Error populating old inline attachments during upgrade to database schema 13: %s",
                 e.message);
         }
     }

src/engine/imap-db/imap-db-folder.vala

@@ -1523,9 +1523,12 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
     
     private static Gee.List<Geary.Attachment>? do_list_attachments(Db.Connection cx, int64 message_id,
         Cancellable? cancellable) throws Error {
-        Db.Statement stmt = cx.prepare(
+        Db.Statement stmt = cx.prepare("""
-            "SELECT id, filename, mime_type, filesize FROM MessageAttachmentTable WHERE message_id=? "
+            SELECT id, filename, mime_type, filesize, disposition
-            + "ORDER BY id");
+            FROM MessageAttachmentTable
+            WHERE message_id = ?
+            ORDER BY id
+            """);
         stmt.bind_rowid(0, message_id);
         
         Db.Result results = stmt.exec(cancellable);
@@ -1535,20 +1538,27 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
         Gee.List<Geary.Attachment> list = new Gee.ArrayList<Geary.Attachment>();
         do {
             list.add(new Geary.Attachment(cx.database.db_file.get_parent(), results.string_at(1),
-                results.string_at(2), results.int64_at(3), message_id, results.rowid_at(0)));
+                results.string_at(2), results.int64_at(3), message_id, results.rowid_at(0),
+                Attachment.Disposition.from_int(results.int_at(4))));
         } while (results.next(cancellable));
         
         return list;
     }
-    
+
     private void do_save_attachments(Db.Connection cx, int64 message_id,
         Gee.List<GMime.Part>? attachments, Cancellable? cancellable) throws Error {
+        do_save_attachments_db(cx, message_id, attachments, db, cancellable);
+    }
+    
+    public static void do_save_attachments_db(Db.Connection cx, int64 message_id,
+        Gee.List<GMime.Part>? attachments, ImapDB.Database db, Cancellable? cancellable) throws Error {
         // nothing to do if no attachments
         if (attachments == null || attachments.size == 0)
             return;
         
         foreach (GMime.Part attachment in attachments) {
             string mime_type = attachment.get_content_type().to_string();
+            string disposition = attachment.get_disposition();
             string? filename = attachment.get_filename();
             if (String.is_empty(filename)) {
                 /// Placeholder filename for attachments with no filename.
@@ -1565,13 +1575,15 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
             uint filesize = byte_array.len;
             
             // Insert it into the database.
-            Db.Statement stmt = cx.prepare(
+            Db.Statement stmt = cx.prepare("""
-                "INSERT INTO MessageAttachmentTable (message_id, filename, mime_type, filesize) " +
+                INSERT INTO MessageAttachmentTable (message_id, filename, mime_type, filesize, disposition)
-                "VALUES (?, ?, ?, ?)");
+                VALUES (?, ?, ?, ?, ?)
+                """);
             stmt.bind_rowid(0, message_id);
             stmt.bind_string(1, filename);
             stmt.bind_string(2, mime_type);
             stmt.bind_uint(3, filesize);
+            stmt.bind_int(4, Attachment.Disposition.from_string(disposition));
             
             int64 attachment_id = stmt.exec_insert(cancellable);
             

src/engine/rfc822/rfc822-message.vala

@@ -576,29 +576,48 @@ public class Geary.RFC822.Message : BaseObject {
         }
         return null;
     }
-
+    
-    internal Gee.List<GMime.Part> get_attachments() throws RFC822Error {
+    internal Gee.List<GMime.Part> get_attachments(Geary.Attachment.Disposition? disposition = null)
+        throws RFC822Error {
+        // A null disposition means "return all Mime parts recognized by Geary.Attachment.Disposition"
         Gee.List<GMime.Part> attachments = new Gee.ArrayList<GMime.Part>();
-        find_attachments(attachments, message.get_mime_part() );
+        get_attachments_recursively(attachments, message.get_mime_part(), disposition);
         return attachments;
     }
-
+    
-    private void find_attachments(Gee.List<GMime.Part> attachments, GMime.Object root)
+    private void get_attachments_recursively(Gee.List<GMime.Part> attachments, GMime.Object root,
-        throws RFC822Error {
+        Geary.Attachment.Disposition? requested_disposition) throws RFC822Error {
-
         // If this is a multipart container, dive into each of its children.
-        if (root is GMime.Multipart) {
+        GMime.Multipart? multipart = root as GMime.Multipart;
-            GMime.Multipart multipart = root as GMime.Multipart;
+        if (multipart != null) {
             int count = multipart.get_count();
             for (int i = 0; i < count; ++i) {
-                find_attachments(attachments, multipart.get_part(i));
+                get_attachments_recursively(attachments, multipart.get_part(i), requested_disposition);
             }
             return;
         }
-
+        
-        // Otherwise see if it has a content disposition of "attachment."
+        // Otherwise, check if this part should be an attachment
-        if (root is GMime.Part && String.nullable_stri_equal(root.get_disposition(), "attachment")) {
+        GMime.Part? part = root as GMime.Part;
-            attachments.add(root as GMime.Part);
+        if (part == null) {
+            return;
+        }
+        
+        Geary.Attachment.Disposition? part_disposition = Geary.Attachment.Disposition.from_string(
+            part.get_disposition());
+        if (part_disposition == null) {
+            // The part disposition was unknown to Geary.Attachment.Disposition
+            return;
+        }
+        
+        if (requested_disposition == null) {
+            // Return any attachment whose disposition is recognized by Geary.Attachment.Disposition
+            attachments.add(part);
+            return;
+        }
+        
+        if (part_disposition == requested_disposition) {
+            attachments.add(part);
         }
     }