Merge lp:~florian-angermeier/contractor/filter-functions-based-on-file-size into lp:contractor/0.3

[Florian Angermeier]

Implement filter functions based on file size:
* Get the max file size (int64, size in bytes) from a .contract file (optional key MaxFileSize)
* Add methods to the D-Bus service:
  - get_contracts_by_file_size
  - get_contracts_by_mime_and_file_size
  - get_contracts_by_mimelist_and_file_size

Add documentation

Implement filter functions based on file size:
* Get the max file size (int64, size in bytes) from a .contract file (optional key MaxFileSize)
* Add methods to the D-Bus service:
  - get_contracts_by_file_size
  - get_contracts_by_mime_and_file_size
  - get_contracts_by_mimelist_and_file_size

Add documentation

[xapantu (xapantu)]

Nice work, thanks :)

See below a few comments about code style and some other things.

We also need documentations for the public methods to get this merged, which will be used to generate the api docs (see other files (e.g. the widgets are well documented) if you are looking for examples).

review: Needs Fixing

[Florian Angermeier (florian-angermeier)]

> Nice work, thanks :)
>
> See below a few comments about code style and some other things.
>
> We also need documentations for the public methods to get this merged, which
> will be used to generate the api docs (see other files (e.g. the widgets are
> well documented) if you are looking for examples).

=== modified file 'src/Contract.vala'
 --- src/Contract.vala 2013-05-20 04:55:11 +0000
 +++ src/Contract.vala 2015-09-13 00:54:10 +0000
 @@ -72,6 +77,12 @@
              } catch (Error err) {
                  warning ("Contract '%s' does not provide an icon (%s)", id, err.message);
              }
 +
 + try {
 + max_file_size = keyfile.get_max_file_size ();
 + } catch (Error err) {
 + warning ("Contract '%s' does not provide a max file size (%s)", id, err.message);

Should I change the warning outputs of all optional contract fields to debug outputs?

[xapantu (xapantu)]

Well, for now, we can let them here. For instance, for the icon one, while it is strictly speaking optional, we do want to force developers to put an icon. Whereas for the max file size, it does not make sense for a lot of contracts (print for instance), so it is really an optional field (and not an optional-if-one-really-is-too-lazy-to-write-it one).

[kay van der Zander (kay20)]

Hey try to keep the unmerged revisions to one. not 11 ;)
use the uncommit command. http://doc.bazaar.canonical.com/latest/en/user-guide/undoing_mistakes.html#undoing-multiple-commits

[xapantu (xapantu)]

Thanks for everything, including the documentation. It may need some rephrasing/fomating, but that's beyond the scope of this merge request.

review: Approve

=== modified file 'src/Contract.vala'
--- src/Contract.vala	2013-05-20 04:55:11 +0000
+++ src/Contract.vala	2015-09-15 23:13:26 +0000
@@ -16,15 +16,39 @@
  */
 
 namespace Contractor {
+    /**
+     * a contract which defines an action available for certain files
+     */
     public class Contract : Object {
+        /**
+         * the contracts ID based on the file name, e.g. file-roller-compress
+         * (file-roller-compress.contract)
+         */
         public string id { get; private set; }
+        /**
+         * the name displayed in the GUI
+         */
         public string name { get; private set; }
         public string icon { get; private set; default = ""; }
         public string description { get; private set; default = ""; }
+        /**
+         * the maximal file size a file or list of files are allowed to have to
+         * be applicable for this contract
+         */
+        public int64 max_file_size { get; private set; default = -1; }
 
         private MimeTypeManager mimetype_manager;
+        /**
+         * the object used to get individual fields from the .contract file
+         */
         private ContractKeyFile keyfile;
 
+        /**
+         * the constructor used to create a Contract object containing a
+         * ContractFile object based on the passed File object
+         *
+         * @param file the file of which a ContractFile object should be created
+         */
         public Contract (File file) throws Error {
             var contract_file = new ContractFile (file);
             keyfile = new ContractKeyFile (contract_file);
@@ -35,15 +59,39 @@
             load_non_mandatory_fields ();
         }
 
+        /**
+         * returns true if the MIME type is supported by this contract; false
+         * otherwise
+         *
+         * @param mime_type the MIME type of the file or list of files on which the contract should be applied
+         *
+         * @return true if the MIME type is supported by this contract; false otherwise
+         */
         public bool supports_mime_type (string mime_type) {
             return mimetype_manager.is_type_supported (mime_type);
         }
 
+        /**
+         * returns true if the file size is supported by this contract; false
+         * otherwise
+         *
+         * @param file_size the file size of the file or list of files on which the contract should be applied
+         *
+         * @return true if the file size is supported by this contract; false otherwise
+         */
+        public bool supports_file_size (int64 file_size) {
+            return file_size == -1 || file_size <= max_file_size;
+        }
+
         public void launch_uris (string[] uris) throws Error {
             var uri_list = String.array_to_list (uris);
             keyfile.get_app_info ().launch_uris (uri_list, null);
         }
 
+        /**
+         * creates and returns a new GenericContract object and fills it with
+         * data from this Contract object (id, name, description, icon)
+         */
         public GenericContract get_generic () {
             return GenericContract () {
                 id = id,
@@ -53,6 +101,9 @@
             };
         }
 
+        /**
+         * loads mandatory fields from the key file
+         */
         private void load_mandatory_fields () throws Error {
             name = keyfile.get_name ();
 
@@ -60,6 +111,9 @@
             mimetype_manager = new MimeTypeManager (mimetypes);
         }
 
+        /**
+         * loads non-mandatory fields from the key file
+         */
         private void load_non_mandatory_fields () {
             try {
                 description = keyfile.get_description ();
@@ -72,6 +126,12 @@
             } catch (Error err) {
                 warning ("Contract '%s' does not provide an icon (%s)", id, err.message);
             }
+
+            try {
+                max_file_size = keyfile.get_max_file_size ();
+            } catch (Error err) {
+                debug ("Contract '%s' does not provide a max file size (%s)", id, err.message);
+            }
         }
     }
 }

=== modified file 'src/ContractDirectory.vala'
--- src/ContractDirectory.vala	2013-05-11 03:05:47 +0000
+++ src/ContractDirectory.vala	2015-09-15 23:13:26 +0000
@@ -50,8 +50,9 @@
     }
 
     private async void on_change_event (File file, File? other_file, FileMonitorEvent event) {
-        if (update_pending)
+        if (update_pending) {
             return;
+        }
 
         update_pending = true;
 

=== modified file 'src/ContractFile.vala'
--- src/ContractFile.vala	2013-04-30 16:32:09 +0000
+++ src/ContractFile.vala	2015-09-15 23:13:26 +0000
@@ -15,32 +15,73 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+/**
+ * used to access the Contracts File object and read its content
+ */
 public class Contractor.ContractFile : Object {
+    /**
+     * contract files filename extension
+     */
     private const string EXTENSION = ".contract";
 
+    /**
+     * the File object used to access its content
+     */
     private File file;
 
+    /**
+     * the constructor to create ContractFile object which contains the passed
+     * File object
+     *
+     * @param file the file to contain
+     */
     public ContractFile (File file) {
         this.file = file;
     }
 
+    /**
+     * get the contract ID from the filename, e.g. file-roller-compress
+     * (file-roller-compress.contract)
+     *
+     * @return the contracts ID, e.g. file-roller-compress
+     */
     public string get_id () {
         return remove_extension (file.get_basename ());
     }
 
+    /**
+     * loads and returns the internally stored files content
+     *
+     * @return the files content as string
+     */
     public string get_contents () throws Error {
         uint8[] file_data;
 
-        if (file.load_contents (null, out file_data, null))
+        if (file.load_contents (null, out file_data, null)) {
             return (string) file_data;
+        }
 
         return "";
     }
 
+    /**
+     * checks if the filename extension is '.contract'
+     *
+     * @param filename the full filename incl. the filename extension
+     *
+     * @return true if the filename extension is '.contract'; false otherwise
+     */
     public static bool is_valid_filename (string filename) {
         return filename[- EXTENSION.length : filename.length] == EXTENSION;
     }
 
+    /**
+     * removes the filename extension and returns the result
+     *
+     * @param file_name the filename incl. the filename extesnion
+     *
+     * @return the filename without the filename extension
+     */
     private static string remove_extension (string file_name) {
         return file_name[0 : - EXTENSION.length];
     }

=== modified file 'src/ContractKeyFile.vala'
--- src/ContractKeyFile.vala	2013-05-19 20:19:40 +0000
+++ src/ContractKeyFile.vala	2015-09-15 23:13:26 +0000
@@ -23,6 +23,7 @@
     private const string DESCRIPTION_KEY = "Description";
     private const string ICON_KEY = KeyFileDesktop.KEY_ICON;
     private const string MIMETYPE_KEY = KeyFileDesktop.KEY_MIME_TYPE;
+    private const string MAX_FILE_SIZE_KEY = "MaxFileSize";
     private const string EXEC_KEY = KeyFileDesktop.KEY_EXEC;
     private const string TRY_EXEC_KEY = KeyFileDesktop.KEY_TRY_EXEC;
 
@@ -35,6 +36,13 @@
     private string text_domain;
     private KeyFile keyfile;
 
+    /**
+     * the constructor to create a ContractKeyFile object which loads the
+     * content of the passed ContractFile object and sets up an internally
+     * stored KeyFile object to access individual contract fields
+     *
+     * @param contract_file ContractFile object from which the content should be loaded
+     */
     public ContractKeyFile (ContractFile contract_file) throws Error {
         string contract_file_contents = contract_file.get_contents ();
         string contents = preprocess_contents (contract_file_contents);
@@ -57,28 +65,59 @@
     public AppInfo get_app_info () throws Error {
         var app_info = new DesktopAppInfo.from_keyfile (keyfile);
 
-        if (app_info == null)
+        if (app_info == null) {
             throw new FileError.NOENT ("%s's file is probably missing.", TRY_EXEC_KEY);
+        }
 
         return app_info;
     }
 
+    /**
+     * gets the contracts name from the key file
+     *
+     * @return the contracts name
+     */
     public string get_name () throws Error {
         return get_locale_string (NAME_KEY);
     }
 
+    /**
+     * gets the contracts description from the key file
+     *
+     * @return the contracts description
+     */
     public string get_description () throws Error {
         return get_locale_string (DESCRIPTION_KEY);
     }
 
+    /**
+     * gets the contracts icon from the key file
+     *
+     * @return the contracts icon, e.g. add-files-to-archive
+     */
     public string get_icon () throws Error {
         return keyfile.get_string (DESKTOP_GROUP, ICON_KEY);
     }
 
+    /**
+     * gets the contracts supported MIME types from the key file
+     *
+     * @return an array of MIME type strings, e.g. text, image
+     */
     public string[] get_mimetypes () throws Error {
         return keyfile.get_string_list (DESKTOP_GROUP, MIMETYPE_KEY);
     }
 
+    /**
+     * gets the contracts supported maximal file size from the key file
+     * the return value can be used directly in GLib
+     *
+     * @return the maximal file size in bytes as int64
+     */
+    public int64 get_max_file_size () throws Error {
+        return keyfile.get_int64 (DESKTOP_GROUP, MAX_FILE_SIZE_KEY);
+    }
+
     private void verify_exec () throws Error {
         string exec = keyfile.get_string (DESKTOP_GROUP, EXEC_KEY);
         verify_string (exec, EXEC_KEY);
@@ -86,8 +125,9 @@
 
     private string get_text_domain () throws Error {
         foreach (var domain_key in SUPPORTED_GETTEXT_DOMAIN_KEYS) {
-            if (keyfile.has_key (DESKTOP_GROUP, domain_key))
+            if (keyfile.has_key (DESKTOP_GROUP, domain_key)) {
                 return keyfile.get_string (DESKTOP_GROUP, domain_key);
+            }
         }
 
         return "";
@@ -100,8 +140,9 @@
     }
 
     private static void verify_string (string? str, string key) throws Error {
-        if (String.is_empty (str))
+        if (String.is_empty (str)) {
             throw new KeyFileError.INVALID_VALUE ("%s key is empty.", key);
+        }
     }
 
     private static string preprocess_contents (string contents) {

=== modified file 'src/ContractMatcher.vala'
--- src/ContractMatcher.vala	2013-10-03 08:37:57 +0000
+++ src/ContractMatcher.vala	2015-09-15 23:13:26 +0000
@@ -16,14 +16,23 @@
  */
 
 namespace Contractor.ContractMatcher {
+    /**
+     * get contracts which support the passed MIME types
+     *
+     * @param mime_types the MIME types which have to be supported by the returned contracts
+     * @param contracts_to_filter a list of contracts to filter
+     *
+     * @return a Collection of Contract objects which support the file size
+     */
     public Gee.Collection<Contract> get_contracts_for_types (string[] mime_types,
         Gee.Collection<Contract> contracts_to_filter) throws ContractorError
     {
         var valid_contracts = new Gee.LinkedList<Contract> ();
         var valid_mime_types = String.clean_array (mime_types);
 
-        if (valid_mime_types.length == 0)
+        if (valid_mime_types.length == 0) {
             throw new ContractorError.NO_MIMETYPES_GIVEN ("No mimetypes were provided.");
+        }
 
         foreach (var contract in contracts_to_filter) {
             // Check if the contract supports ALL the types listed in mime_types
@@ -36,9 +45,55 @@
                 }
             }
 
-            if (all_types_supported)
-                valid_contracts.add (contract);
-        }
+            if (all_types_supported) {
+                valid_contracts.add (contract);
+            }
+        }
+
+        return valid_contracts;
+    }
+
+    /**
+     * get contracts which support the passed file size
+     *
+     * @param file_size the file size which has to be supported by the returned contracts
+     * @param contracts_to_filter a list of contracts to filter
+     *
+     * @return a Collection of Contract objects which support the file size
+     */
+    public Gee.Collection<Contract> get_contracts_for_file_size (int64 file_size,
+        Gee.Collection<Contract> contracts_to_filter) throws ContractorError
+    {
+        var valid_contracts = new Gee.LinkedList<Contract> ();
+
+        foreach (var contract in contracts_to_filter) {
+            bool file_size_supported = true;
+
+            if (!contract.supports_file_size (file_size)) {
+                file_size_supported = false;
+            }
+
+            if (file_size_supported)
+                valid_contracts.add (contract);
+        }
+
+        return valid_contracts;
+    }
+
+    /**
+     * get contracts which support the passed MIME types and file size
+     *
+     * @param mime_types the MIME types which have to be supported by the returned contracts
+     * @param file_size the file size which has to be supported by the returned contracts
+     * @param contracts_to_filter a list of contracts to filter
+     *
+     * @return a Collection of Contract objects which support the MIME types and the file size
+     */
+    public Gee.Collection<Contract> get_contracts_for_types_and_file_size (string[] mime_types,
+        int64 file_size, Gee.Collection<Contract> contracts_to_filter) throws ContractorError
+    {
+        var contracts_for_types = get_contracts_for_types (mime_types, contracts_to_filter);
+        var valid_contracts = get_contracts_for_file_size (file_size, contracts_for_types);
 
         return valid_contracts;
     }

=== modified file 'src/ContractSorter.vala'
--- src/ContractSorter.vala	2013-05-25 08:08:58 +0000
+++ src/ContractSorter.vala	2015-09-15 23:13:26 +0000
@@ -17,11 +17,13 @@
 
 namespace Contractor.ContractSorter {
     public int compare_func (Contract? a, Contract? b) {
-        if (a == null)
+        if (a == null) {
             return (b == null) ? 0 : -1;
+        }
 
-        if (b == null)
+        if (b == null) {
             return 1;
+        }
 
         return strcmp (a.name.collate_key (), b.name.collate_key ());
     }

=== modified file 'src/ContractSource.vala'
--- src/ContractSource.vala	2013-05-20 04:55:11 +0000
+++ src/ContractSource.vala	2015-09-15 23:13:26 +0000
@@ -39,8 +39,9 @@
     public Contract lookup_by_id (string contract_id) throws Error {
         var contract = contracts.get (contract_id);
 
-        if (contract == null)
+        if (contract == null) {
             throw new IOError.NOT_FOUND ("Requested invalid contract: %s", contract_id);
+        }
 
         return contract;
     }
@@ -50,8 +51,9 @@
 
         var contract_files_to_load = file_service.load_contract_files ();
 
-        foreach (var contract_file in contract_files_to_load)
+        foreach (var contract_file in contract_files_to_load) {
             load_contract (contract_file);
+        }
 
         changed ();
     }

=== modified file 'src/DBusService.vala'
--- src/DBusService.vala	2013-10-03 08:37:57 +0000
+++ src/DBusService.vala	2015-09-15 23:13:26 +0000
@@ -24,32 +24,113 @@
 
 namespace Contractor {
     [DBus (name = "org.elementary.ContractorError")]
+    /**
+     * Errors specific to the Contractor D-Bus service.
+     */
     public errordomain ContractorError {
+        /**
+         * Error if no MIME type was passed where mandatory.
+         */
         NO_MIMETYPES_GIVEN
     }
 
+    /**
+     * A D-Bus service handling requests for Contracts defined by .contract
+     * files.
+     */
     [DBus (name = "org.elementary.Contractor")]
     public class DBusService : Object {
+        /**
+         * Signal which gets sent when Contracts change.
+         */
         public signal void contracts_changed ();
 
+        /**
+         * Source from where .Contracts are loaded.
+         */
         private ContractSource contract_source;
 
+        /**
+         * Constructor to create a DBusService object and setup a
+         * ContractSource.
+         */
         public DBusService () {
             contract_source = new ContractSource ();
             contract_source.changed.connect (() => contracts_changed ());
         }
 
+        /**
+         * This method gets an array of GenericContracts filtered by a
+         * MIME type.
+         *
+         * @param mime_type a MIME type string, e.g. text, image
+         *
+         * @return an array of GenericContracts
+         */
         public GenericContract[] get_contracts_by_mime (string mime_type) throws Error {
             string[] mime_types = { mime_type };
             return get_contracts_by_mimelist (mime_types);
         }
 
+        /**
+         * This method gets an array of GenericContracts filtered by an array of
+         * MIME types.
+         *
+         * @param mime_types an array of MIME type strings, e.g. text, image
+         *
+         * @return an array of GenericContracts
+         */
         public GenericContract[] get_contracts_by_mimelist (string[] mime_types) throws Error {
             var all_contracts = contract_source.get_contracts ();
             var contracts = ContractMatcher.get_contracts_for_types (mime_types, all_contracts);
             return convert_to_generic_contracts (contracts);
         }
 
+        /**
+         * This method gets an array of GenericContracts filtered by the file
+         * size in bytes.
+         *
+         * @param file_size the file size in bytes, e.g. from FileInfo.get_size ()
+         *
+         * @return an array of GenericContracts
+         */
+        public GenericContract[] get_contracts_by_file_size (int64 file_size) throws Error {
+            var all_contracts = contract_source.get_contracts ();
+            var contracts = ContractMatcher.get_contracts_for_file_size (file_size, all_contracts);
+            return convert_to_generic_contracts (contracts);
+        }
+
+        /**
+         * This method gets an array of GenericContracts filtered by a MIME type
+         * and the file size in bytes.
+         *
+         * @param mime_type a MIME type string, e.g. text, image
+         * @param file_size the file size in bytes, e.g. from FileInfo.get_size ()
+         *
+         * @return an array of GenericContracts
+         */
+        public GenericContract[] get_contracts_by_mime_and_file_size (string mime_type, int64 file_size) throws Error {
+            string[] mime_types = { mime_type };
+            return get_contracts_by_mimelist_and_file_size (mime_types, file_size);
+        }
+
+        /**
+         * This method gets an array of GenericContracts filtered by an array of
+         * MIME types and the file size in bytes.
+         *
+         * The file size should probably be the sum of all files file size.
+         *
+         * @param mime_types an array of MIME type strings, e.g. text, image
+         * @param file_size the file size in bytes, e.g. from FileInfo.get_size ()
+         *
+         * @return an array of GenericContracts
+         */
+        public GenericContract[] get_contracts_by_mimelist_and_file_size (string[] mime_types, int64 file_size) throws Error {
+            var all_contracts = contract_source.get_contracts ();
+            var contracts = ContractMatcher.get_contracts_for_types_and_file_size (mime_types, file_size, all_contracts);
+            return convert_to_generic_contracts (contracts);
+        }
+
         public void execute_with_uri (string id, string uri) throws Error {
             string[] uris = { uri };
             execute_with_uri_list (id, uris);
@@ -60,16 +141,29 @@
             contract.launch_uris (uris);
         }
 
+        /**
+         * This method gets an array of GenericContracts containing all
+         * contracts that exist.
+         *
+         * @return an array of GenericContracts containing all contracts
+         */
         public GenericContract[] list_all_contracts () {
             var contracts = contract_source.get_contracts ();
             return convert_to_generic_contracts (contracts);
         }
 
+        /**
+         * Converts a Collection of Contracts into an array of
+         * GenericContracts.
+         *
+         * @return an array of GenericContracts
+         */
         private static GenericContract[] convert_to_generic_contracts (Gee.Collection<Contract> contracts) {
             var generic_contracts = new GenericContract[0];
 
-            foreach (var contract in contracts)
+            foreach (var contract in contracts) {
                 generic_contracts += contract.get_generic ();
+            }
 
             return generic_contracts;
         }

=== modified file 'src/FileEnumerator.vala'
--- src/FileEnumerator.vala	2013-06-20 21:10:16 +0000
+++ src/FileEnumerator.vala	2015-09-15 23:13:26 +0000
@@ -49,8 +49,9 @@
                 var child = directory.get_child (name);
 
                 if (file_type == FileType.REGULAR) {
-                    if (ContractFile.is_valid_filename (name))
+                    if (ContractFile.is_valid_filename (name)) {
                         files.add (child);
+                    }
                 } else {
                     warning ("'%s' is not a regular file. Skipping it...", child.get_path ());
                 }

=== modified file 'src/FileService.vala'
--- src/FileService.vala	2013-05-11 03:05:47 +0000
+++ src/FileService.vala	2015-09-15 23:13:26 +0000
@@ -43,8 +43,9 @@
         public Gee.List<File> load_contract_files () {
             var contract_files = new Gee.LinkedList<File> ();
 
-            foreach (var directory in directories)
+            foreach (var directory in directories) {
                 contract_files.add_all (directory.lookup_contract_files ());
+            }
 
             return contract_files;
         }
@@ -55,8 +56,9 @@
             // The user's data dir takes priority over system-wide data directories
             data_dir_paths += Environment.get_user_data_dir ();
 
-            foreach (string data_dir in Environment.get_system_data_dirs ())
+            foreach (string data_dir in Environment.get_system_data_dirs ()) {
                 data_dir_paths += data_dir;
+            }
 
             foreach (var path in data_dir_paths) {
                 var directory = File.new_for_path (path).get_child (CONTRACT_DATA_DIR_NAME);

=== modified file 'src/MimeTypeManager.vala'
--- src/MimeTypeManager.vala	2013-06-12 21:50:38 +0000
+++ src/MimeTypeManager.vala	2015-09-15 23:13:26 +0000
@@ -27,8 +27,9 @@
 
         values = String.clean_array (mimetypes);
 
-        if (values.length == 0)
+        if (values.length == 0) {
             throw new KeyFileError.INVALID_VALUE ("No values specified for MimeType.");
+        }
     }
 
     public bool is_type_supported (string mime_type) {
@@ -38,8 +39,9 @@
 
     private bool contains_mimetype (string mime_type) {
         foreach (string local_mime_type in values) {
-            if (compare (mime_type, local_mime_type))
+            if (compare (mime_type, local_mime_type)) {
                 return true;
+            }
         }
 
         return false;

=== modified file 'src/String.vala'
--- src/String.vala	2013-05-20 04:55:11 +0000
+++ src/String.vala	2015-09-15 23:13:26 +0000
@@ -26,8 +26,9 @@
         if (array != null && array.length > 0) {
             list = new List<string> ();
 
-            foreach (var str in array)
+            foreach (var str in array) {
                 list.prepend (str);
+            }
 
             list.reverse ();
         }
@@ -45,8 +46,9 @@
             if (str != null) {
                 string clean_str = str.strip ();
 
-                if (clean_str != "" && !container.contains (clean_str))
+                if (clean_str != "" && !container.contains (clean_str)) {
                     container.add (clean_str);
+                }
             }
         }
 

=== modified file 'src/Translations.vala'
--- src/Translations.vala	2013-05-20 06:07:44 +0000
+++ src/Translations.vala	2015-09-15 23:13:26 +0000
@@ -36,8 +36,9 @@
     }
 
     private static void add_domain (string domain) {
-        if (domains.contains (domain))
+        if (domains.contains (domain)) {
             return;
+        }
 
         domains.add (domain);
         Intl.textdomain (domain);