Merge lp:~shnatsel/granite/get_contracts_for_files into lp:~elementary-pantheon/granite/granite

[Sergey "Shnatsel" Davidoff]

files passed to get_contracts_for_file[s] don't have to be readable; update documentation accordingly

Add two convenience funtions on top of current Contractor API that allow querying available contracts for files instead of mimetypes. This spares developers of applications that deal with more or less arbitrary files the need to look up the mimetype themselves.

[Victor Martinez (victored)]

As you've mentioned, this will ease the addition of Contractor support for new (or lazy) developers.

The error-handling here is really decent IMO. If the code fails to determine the mime type of any given file, it's OK to let the caller know by aborting the method, instead of returning invalid contracts.

Needs fixing:

1) There's no need to add these new methods to the Contractor DBus interface (Diff lines 8 and 9), as they are not executed by the daemon, nor invoked through DBus.

Some things that could be changed:

2) Avoid code duplication in get_contracts_for_file by just calling "get_contracts_for_files" with a single-member array. Like this:

 public static Gee.List<Contract> get_contracts_for_file (File file) throws Error {
    File[] files = { file };
    return get_contracts_for_files (files);
 }

3) I guess there should be a single space after "get_content_type" (diff line 58)

Everything else looks good to me. Thanks for your work!

review: Needs Fixing

[Victor Martinez (victored)]

Hey, that was fast!

I'm not sure the comment on diff line 44 follows our coding style. It's usually better to place the comment on a separate line above the code, and leave a single space between the "//" and the text.

I'm approving the logic and actual code since it works as advertised.

review: Approve

[Sergey "Shnatsel" Davidoff (shnatsel)]

Thanks!
I've solved all the issues you pointed out, improved documentation and wrote a test client for these functions: lp:~shnatsel/noise/contractor-client-new-api
(The damn thing still corrupts ListView and crashes for God knows what reason, but that's a Noise bug unrelated to this merge).

This should be good to go now.

review: Approve

[Victor Martinez (victored)]

I like the changes you made to the documentation.

Regarding Valadoc, it's preferable to mention methods like:

"{@link GLib.File.query_info}"

instead of:

"GLib.File.query_info ()"

[RabbitBot (rabbitbot-a)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Victor Martinez (victored)]

review: Approve

[Sergey "Shnatsel" Davidoff (shnatsel)]

It occurred to me that this code will throw an error if the given file does not exist, but will not throw an error if the user is not permitted to read the file. Moreover, querying contracts will likely succeed based on the filename only. I'll update the documentation accordingly.

[lp:~shnatsel/granite/get_contracts_for_files]

645. By Sergey "Shnatsel" Davidoff on 2013-10-04

files passed to get_contracts_for_file[s] don't have to be readable; update documentation accordingly

=== modified file 'lib/Services/ContractorProxy.vala'
--- lib/Services/ContractorProxy.vala	2013-10-03 22:26:34 +0000
+++ lib/Services/ContractorProxy.vala	2013-10-04 12:04:58 +0000
@@ -230,7 +230,7 @@
          * Errors occurring in {@link GLib.File.query_info} method while looking up
          * the file (e.g. if the file is deleted) are forwarded to the caller.
          *
-         * @param file An existing file readable by the current user.
+         * @param file An existing file.
          * @return List of contracts applicable to the given file.
          */
         public static Gee.List<Contract> get_contracts_for_file (File file) throws Error {
@@ -245,7 +245,7 @@
          * Errors occurring in {@link GLib.File.query_info} method while looking up
          * the file (e.g. if the file is deleted) are forwarded to the caller.<<BR>>
          *
-         * @param files Array of existing files readable by the current user.
+         * @param files Array of existing files.
          * @return List of contracts applicable to any of the given files.
          */
         public static Gee.List<Contract> get_contracts_for_files (File[] files) throws Error {