From ddc6403aad27fe6aab6c7015eff98fe474181ecf Mon Sep 17 00:00:00 2001 From: Jim Nelson Date: Mon, 9 Sep 2013 15:05:22 -0700 Subject: [PATCH] Cleaned up Geary.Attachments implementation and interface While working on #7345, I grew dissatisfied with how Geary.Attachment was implemented. It knew too much about imap-db's internal workings, so I've broken it up a little bit. It's now an abstract base class completed by ImapDB.Attachment, a private implementation class. This corresponds with the coding patterns used throughout the Geary API. --- src/CMakeLists.txt | 1 + src/client/composer/composer-window.vala | 13 +--- src/client/geary-controller.vala | 10 +-- src/client/views/conversation-viewer.vala | 11 ++-- src/engine/api/geary-attachment.vala | 72 ++++++++++++++-------- src/engine/api/geary-email.vala | 4 +- src/engine/imap-db/imap-db-account.vala | 2 +- src/engine/imap-db/imap-db-attachment.vala | 35 +++++++++++ src/engine/imap-db/imap-db-folder.vala | 12 ++-- 9 files changed, 104 insertions(+), 56 deletions(-) create mode 100644 src/engine/imap-db/imap-db-attachment.vala diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index dcf237e5..b95653ac 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -166,6 +166,7 @@ engine/imap/transport/imap-deserializer.vala engine/imap/transport/imap-serializer.vala engine/imap-db/imap-db-account.vala +engine/imap-db/imap-db-attachment.vala engine/imap-db/imap-db-contact.vala engine/imap-db/imap-db-database.vala engine/imap-db/imap-db-email-identifier.vala diff --git a/src/client/composer/composer-window.vala b/src/client/composer/composer-window.vala index 55a5bc4d..a337142e 100644 --- a/src/client/composer/composer-window.vala +++ b/src/client/composer/composer-window.vala @@ -904,12 +904,8 @@ public class ComposerWindow : Gtk.Window { private void check_pending_attachments() { if (pending_attachments != null) { - Gee.Set filenames = new Gee.HashSet(); - foreach (File file in attachment_files) - filenames.add(file.get_path()); - foreach (Geary.Attachment attachment in pending_attachments) { - if (!filenames.contains(attachment.filepath)) { + if (!attachment_files.contains(attachment.file)) { pending_attachments_button.show(); return; } @@ -990,11 +986,8 @@ public class ComposerWindow : Gtk.Window { } private void add_attachments(Gee.List attachments, bool alert_errors = true) { - foreach(Geary.Attachment attachment in attachments) { - File? attachment_file = File.new_for_path(attachment.filepath); - if (attachment_file != null) - add_attachment(attachment_file, alert_errors); - } + foreach(Geary.Attachment attachment in attachments) + add_attachment(attachment.file, alert_errors); } private void remove_attachment(File file, Gtk.Box box) { diff --git a/src/client/geary-controller.vala b/src/client/geary-controller.vala index f49e4161..05536e29 100644 --- a/src/client/geary-controller.vala +++ b/src/client/geary-controller.vala @@ -1392,7 +1392,7 @@ public class GearyController : Geary.BaseObject { private void on_open_attachment(Geary.Attachment attachment) { if (GearyApplication.instance.config.ask_open_attachment) { QuestionDialog ask_to_open = new QuestionDialog.with_checkbox(main_window, - _("Are you sure you want to open \"%s\"?").printf(attachment.filename), + _("Are you sure you want to open \"%s\"?").printf(attachment.file.get_basename()), _("Attachments may cause damage to your system if opened. Only open files from trusted sources."), Stock._OPEN_BUTTON, Stock._CANCEL, _("Don't _ask me again"), false); if (ask_to_open.run() != Gtk.ResponseType.OK) @@ -1402,7 +1402,7 @@ public class GearyController : Geary.BaseObject { GearyApplication.instance.config.ask_open_attachment = !ask_to_open.is_checked; } - open_uri("file://" + attachment.filepath); + open_uri(attachment.file.get_uri()); } private bool do_overwrite_confirmation(File to_overwrite) { @@ -1434,7 +1434,7 @@ public class GearyController : Geary.BaseObject { if (last_save_directory != null) dialog.set_current_folder(last_save_directory.get_path()); if (attachments.size == 1) { - dialog.set_current_name(attachments[0].filename); + dialog.set_current_name(attachments[0].file.get_basename()); dialog.set_do_overwrite_confirmation(true); // use custom overwrite confirmation so it looks consistent whether one or many // attachments are being saved @@ -1458,8 +1458,8 @@ public class GearyController : Geary.BaseObject { // Save each one, checking for overwrite only if multiple attachments are being written foreach (Geary.Attachment attachment in attachments) { - File source_file = File.new_for_path(attachment.filepath); - File dest_file = (attachments.size == 1) ? destination : destination.get_child(attachment.filename); + File source_file = attachment.file; + File dest_file = (attachments.size == 1) ? destination : destination.get_child(attachment.file.get_basename()); if (attachments.size > 1 && dest_file.query_exists() && !do_overwrite_confirmation(dest_file)) return; diff --git a/src/client/views/conversation-viewer.vala b/src/client/views/conversation-viewer.vala index 18cb51e7..4cf0f84c 100644 --- a/src/client/views/conversation-viewer.vala +++ b/src/client/views/conversation-viewer.vala @@ -826,7 +826,7 @@ public class ConversationViewer : Gtk.Box { return null; try { - return email.get_attachment(int64.parse(element.get_attribute("data-attachment-id"))); + return email.get_attachment(element.get_attribute("data-attachment-id")); } catch (Geary.EngineError err) { return null; } @@ -1260,7 +1260,7 @@ public class ConversationViewer : Gtk.Box { } private void on_attachment_clicked_self(WebKit.DOM.Element element) { - int64 attachment_id = int64.parse(element.get_attribute("data-attachment-id")); + string attachment_id = element.get_attribute("data-attachment-id"); Geary.Email? email = get_email_from_element(element); if (email == null) return; @@ -1754,18 +1754,17 @@ public class ConversationViewer : Gtk.Box { } // 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) ? - _("none") : attachment.filename; + string filename = !attachment.has_supplied_filename ? _("none") : attachment.file.get_basename(); Util.DOM.select(attachment_table, ".info .filename") .set_inner_text(filename); Util.DOM.select(attachment_table, ".info .filesize") .set_inner_text(Files.get_filesize_as_string(attachment.filesize)); - attachment_table.set_attribute("data-attachment-id", "%s".printf(attachment.id.to_string())); + attachment_table.set_attribute("data-attachment-id", attachment.id); // Set the image preview and insert it into the container. WebKit.DOM.HTMLImageElement img = Util.DOM.select(attachment_table, ".preview img") as WebKit.DOM.HTMLImageElement; - web_view.set_image_src(img, attachment.mime_type, attachment.filepath, ATTACHMENT_PREVIEW_SIZE); + web_view.set_image_src(img, attachment.mime_type, attachment.file.get_path(), ATTACHMENT_PREVIEW_SIZE); attachment_container.append_child(attachment_table); } diff --git a/src/engine/api/geary-attachment.vala b/src/engine/api/geary-attachment.vala index ed1d707f..5f083f6c 100644 --- a/src/engine/api/geary-attachment.vala +++ b/src/engine/api/geary-attachment.vala @@ -4,9 +4,13 @@ * (version 2.1 or later). See the COPYING file in this distribution. */ -public class Geary.Attachment : BaseObject { - public const Email.Field REQUIRED_FIELDS = Email.REQUIRED_FOR_MESSAGE; - +/** + * An attachment that was a part of an {@link Email}. + * + * @see Email.get_attachment + */ + +public abstract class Geary.Attachment : BaseObject { // NOTE: These values are persisted on disk and should not be modified unless you know what // you're doing. public enum Disposition { @@ -43,36 +47,52 @@ public class Geary.Attachment : BaseObject { } } - public string? filename { get; private set; } - public string filepath { get; private set; } + /** + * An identifier that can be used to locate the {@link Attachment} in an {@link Email}. + * + * @see Email.get_attachment + */ + public string id { get; private set; } + + /** + * Returns true if the originating {@link Email} supplied a filename for the {@link Attachment}. + * + * Since all files must have a name, one is supplied for the Attachment by Geary if this is + * false. This is merely to indicate how the filename should be displayed, since Geary's will + * be an untranslated "none". + */ + public bool has_supplied_filename { get; private set; } + + /** + * The on-disk File of the {@link Attachment}. + */ + public File file { get; private set; } + + /** + * The MIME type of the {@link Attachment}. + */ public string mime_type { get; private set; } + + /** + * The file size (in bytes) if the {@link file}. + */ public int64 filesize { get; private set; } - public int64 id { get; private set; } + + /** + * The {@link Disposition} of the attachment, as specified by the {@link Email}. + * + * See [[https://tools.ietf.org/html/rfc2183]] + */ public Disposition disposition { get; private set; } - // TODO: Move some of this into ImapDB.Attachment - internal Attachment(File data_dir, string? filename, string mime_type, int64 filesize, - int64 message_id, int64 attachment_id, Disposition disposition) { - - this.filename = filename; + protected Attachment(string id, File file, bool has_supplied_filename, string mime_type, int64 filesize, + Disposition disposition) { + this.id = id; + this.file = file; + this.has_supplied_filename = has_supplied_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; } - - // TODO: Move this into ImapDB.Attachment - internal static string get_path(File data_dir, int64 message_id, int64 attachment_id, - string? filename) { - // "none" should not be translated, or the user will be unable to retrieve their - // attachments with no filenames after changing their language. - return "%s/attachments/%s/%s/%s".printf(data_dir.get_path(), message_id.to_string(), - attachment_id.to_string(), filename ?? "none"); - } - - internal static File get_attachments_dir(File data_dir) { - return data_dir.get_child("attachments"); - } } diff --git a/src/engine/api/geary-email.vala b/src/engine/api/geary-email.vala index cd9e3b6d..5e1f7458 100644 --- a/src/engine/api/geary-email.vala +++ b/src/engine/api/geary-email.vala @@ -253,7 +253,7 @@ public class Geary.Email : BaseObject { public string get_searchable_attachment_list() { StringBuilder search = new StringBuilder(); foreach (Geary.Attachment attachment in attachments) { - search.append(attachment.filename); + search.append(attachment.file.get_basename()); search.append("\n"); } return search.str; @@ -279,7 +279,7 @@ public class Geary.Email : BaseObject { * Requires the REQUIRED_FOR_MESSAGE fields be present; else * EngineError.INCOMPLETE_MESSAGE is thrown. */ - public Geary.Attachment? get_attachment(int64 attachment_id) throws EngineError { + public Geary.Attachment? get_attachment(string attachment_id) throws EngineError { if (!fields.fulfills(REQUIRED_FOR_MESSAGE)) throw new EngineError.INCOMPLETE_MESSAGE("Parsed email requires HEADER and BODY"); diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index 9469683b..87b16630 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -58,7 +58,7 @@ private class Geary.ImapDB.Account : BaseObject { public static void get_imap_db_storage_locations(File user_data_dir, out File db_file, out File attachments_dir) { db_file = ImapDB.Database.get_db_file(user_data_dir); - attachments_dir = Geary.Attachment.get_attachments_dir(user_data_dir); + attachments_dir = ImapDB.Attachment.get_attachments_dir(user_data_dir); } public async void open_async(File user_data_dir, File schema_dir, Cancellable? cancellable) diff --git a/src/engine/imap-db/imap-db-attachment.vala b/src/engine/imap-db/imap-db-attachment.vala new file mode 100644 index 00000000..2bf7b8d2 --- /dev/null +++ b/src/engine/imap-db/imap-db-attachment.vala @@ -0,0 +1,35 @@ +/* Copyright 2013 Yorba Foundation + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +private class Geary.ImapDB.Attachment : Geary.Attachment { + public const Email.Field REQUIRED_FIELDS = Email.REQUIRED_FOR_MESSAGE; + + private const string ATTACHMENTS_DIR = "attachments"; + + protected Attachment(File data_dir, string? filename, string mime_type, int64 filesize, + int64 message_id, int64 attachment_id, Geary.Attachment.Disposition disposition) { + base (generate_id(attachment_id),generate_file(data_dir, message_id, attachment_id, filename), + !String.is_empty(filename), mime_type, filesize, disposition); + } + + private static string generate_id(int64 attachment_id) { + return "imap-db:%s".printf(attachment_id.to_string()); + } + + public static File generate_file(File data_dir, int64 message_id, int64 attachment_id, + string? filename) { + // "none" should not be translated, or the user will be unable to retrieve their + // attachments with no filenames after changing their language. + return get_attachments_dir(data_dir) + .get_child(message_id.to_string()) + .get_child(attachment_id.to_string()) + .get_child(filename ?? "none"); + } + + public static File get_attachments_dir(File data_dir) { + return data_dir.get_child(ATTACHMENTS_DIR); + } +} diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 5a1fd96a..40cd7c3d 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -1329,7 +1329,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { internal static Geary.Email do_add_attachments(Db.Connection cx, Geary.Email email, int64 message_id, Cancellable? cancellable = null) throws Error { // Add attachments if available - if (email.fields.fulfills(Geary.Attachment.REQUIRED_FIELDS)) { + if (email.fields.fulfills(ImapDB.Attachment.REQUIRED_FIELDS)) { Gee.List? attachments = do_list_attachments(cx, message_id, cancellable); if (attachments != null) @@ -1772,9 +1772,9 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { Gee.List list = new Gee.ArrayList(); do { - list.add(new Geary.Attachment(cx.database.db_file.get_parent(), results.string_at(1), + list.add(new ImapDB.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), - Attachment.Disposition.from_int(results.int_at(4)))); + Geary.Attachment.Disposition.from_int(results.int_at(4)))); } while (results.next(cancellable)); return list; @@ -1814,12 +1814,12 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { 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)); + stmt.bind_int(4, Geary.Attachment.Disposition.from_string(disposition)); int64 attachment_id = stmt.exec_insert(cancellable); - File saved_file = File.new_for_path(Attachment.get_path(db.db_file.get_parent(), message_id, - attachment_id, filename)); + File saved_file = ImapDB.Attachment.generate_file(db.db_file.get_parent(), message_id, + attachment_id, filename); debug("Saving attachment to %s", saved_file.get_path());