Merge lp:~ralsina/ubuntuone-client/fix_712674 into lp:ubuntuone-client

[Roberto Alsina]

Don't offer the "Publish" option in the context menu for files/folders that are being shared to the user, because it's not supported.

Adds a utility function to determine if a file or directory is inside a share, and uses that to modify the context menu so you can't try to publish those files/directories.

To fieldtest it, build it using these commands:

WARNING, this overwrites part of ubuntuone-client-gnome, you may need to reinstall it later

./autogen.sh
./configure --prefix=/usr
make
cd nautilus && sudo make install
pkill nautilus
nautilus nautilus ~/Ubuntu\ One/Shared\ With\ Me/

Then check the context menus, the "Ubuntu One->Publish" option should be disabled.
Check also in a subdirectory, with folders and files, and that the Publish option is enabled for contents outside the shares.

[Roberto Alsina (ralsina)]

grrr. "Check also in a subdirectory, with folders and files, and that the Publish option is enabled for contents outside the shares."

[Roman Yepishev (rye)]

Fails to build ATM:

cc1: warnings being treated as errors
utils.c: In function ‘ubuntuone_is_folder_shares’:
utils.c:144:15: error: unused variable ‘f’

review: Needs Fixing (fieldtest)

[lp:~ralsina/ubuntuone-client/fix_712674]

861. By Roberto Alsina on 2011-02-11

Remove unused variable

862. By Roberto Alsina on 2011-02-11

Simpler function (thx chipaca)

863. By Roberto Alsina on 2011-02-11

use the right prefix, explain corner case

864. By Roberto Alsina on 2011-02-11

nicer name for placeholder. Traditions matter

[dobey (dobey)]

Please rename the function for this to something more than one character different from the _is_shared method. Documenting API is good, but so is having sane method names. :)

Also, please use --fixes=lp:712674 on at least one commit, so that the bug is linked and gets marked as fix committed when the branch lands.

Also, set a more descriptive commit message than "Fix bug ####" please. We don't need to put bug #s in commit messages, as bzr/launchpad have useful metadata for them that we should use.

review: Needs Fixing

[lp:~ralsina/ubuntuone-client/fix_712674]

865. By Roberto Alsina on 2011-02-11

Better function name

[Roberto Alsina (ralsina)]

> Please rename the function for this to something more than one character
> different from the _is_shared method. Documenting API is good, but so is
> having sane method names. :)

Done.

> Also, please use --fixes=lp:712674 on at least one commit, so that the bug is
> linked and gets marked as fix committed when the branch lands.
>
> Also, set a more descriptive commit message than "Fix bug ####" please. We
> don't need to put bug #s in commit messages, as bzr/launchpad have useful
> metadata for them that we should use.

I am used to a system where the link is done by just mentioning the bugs in the commit messages, sorry.

Fixed!

[Roberto Alsina (ralsina)]

> Fails to build ATM:
>
> cc1: warnings being treated as errors
> utils.c: In function ‘ubuntuone_is_folder_shares’:
> utils.c:144:15: error: unused variable ‘f’

Missed a line I should have deleted.
Fixed!

[dobey (dobey)]

77 + gchar buf[PATH_MAX + 1];
78 + gchar prefix[PATH_MAX + 1];
79 + g_snprintf(prefix,PATH_MAX,"%s/ubuntuone/shares/",g_get_user_data_dir());

This is just bad C. :)

You should use g_build_filename (); instead to do this. Also, don't fear the space key.

gchar * full_path = g_build_filename (g_get_uer_data_dir (), "ubuntuone", "shares", "", NULL);

And then g_free (full_path); before the return;

Also, can you change the is_shares variable in the code to be is_sharetome or something a little more descriptive and more than a single character different from is_shared as well?

90 + gchar *res = realpath(path, buf);
91 + if (g_str_has_prefix (res, prefix)) {
92 + is_shares = TRUE;
93 + }

This is also wrong here. The gchar *res needs to be declared at the beginning of the function. Also please call it real_path or resolved_path instead. And you should call realpath () with NULL as the second argument to use the return value. You should then g_free () the variable assigned to before returning from the function.

It also looks like there are maybe some spaces vs tabs issues in the spacing of the code, in the diff.
Also please do multi-line comments in the following style:

/*
 * Start of text
 */

review: Needs Fixing

[Roberto Alsina (ralsina)]

> 77 + gchar buf[PATH_MAX + 1];
> 78 + gchar prefix[PATH_MAX + 1];
> 79 +
> g_snprintf(prefix,PATH_MAX,"%s/ubuntuone/shares/",g_get_user_data_dir());
>
> This is just bad C. :)
>
> You should use g_build_filename (); instead to do this. Also, don't fear the
> space key.

Ok, didn't know g_build_filename.

>
> gchar * full_path = g_build_filename (g_get_uer_data_dir (), "ubuntuone",
> "shares", "", NULL);
>
> And then g_free (full_path); before the return;
>
> Also, can you change the is_shares variable in the code to be is_sharetome or
> something a little more descriptive and more than a single character different
> from is_shared as well?

Sure.

> 90 + gchar *res = realpath(path, buf);
> 91 + if (g_str_has_prefix (res, prefix)) {
> 92 + is_shares = TRUE;
> 93 + }
>
> This is also wrong here. The gchar *res needs to be declared at the beginning
> of the function.

Yes. C++ism :-) Fixed.

> Also please call it real_path or resolved_path instead. And
> you should call realpath () with NULL as the second argument to use the return
> value.
> You should then g_free () the variable assigned to before returning
> from the function.

No. It always returns a pointer to the resolved path, the only difference is that since I used buf the pointer points to somewhere inside buf instead of allocating it using malloc. At least that's what the manual says.

I'll change it because you say it's ugly, but it's not *wrong* ;-)

> It also looks like there are maybe some spaces vs tabs issues in the spacing
> of the code, in the diff.
> Also please do multi-line comments in the following style:

Ok, fixed.

> /*
> * Start of text
> */

Ok, fixed.

[lp:~ralsina/ubuntuone-client/fix_712674]

866. By Roberto Alsina on 2011-02-11

Fixes for dobey's 'Needs Fixing'

867. By Roberto Alsina on 2011-02-11

use g_free for string allocated with g_malloc

[dobey (dobey)]

review: Approve

[Roman Yepishev (rye)]

Looks great, no publishing in shares.

review: Approve (fieldtest)

=== modified file 'nautilus/context-menu.c'
--- nautilus/context-menu.c	2010-10-18 11:05:29 +0000
+++ nautilus/context-menu.c	2011-02-11 15:30:10 +0000
@@ -182,11 +182,11 @@
 	NautilusMenuItem *root_item, *menu_item, *urlitem;
 	gchar *path, *item, *homedir_path;
 	gboolean is_managed, is_root, is_udf, is_public, is_shared, is_pending;
-	gboolean is_inhome, is_dir, is_regular;
+	gboolean is_shared_to_me, is_inhome, is_dir, is_regular;
 	MenuCallbackData *cb_data;
 
 	is_managed = is_root = is_udf = is_public = is_shared = is_pending = FALSE;
-	is_inhome = is_dir = is_regular = FALSE;
+	is_shared_to_me = is_inhome = is_dir = is_regular = FALSE;
 
 	if (g_list_length (files) != 1)
 		return NULL;
@@ -218,6 +218,9 @@
 	if (ubuntuone_is_folder_shared (uon, path))
 		is_shared = TRUE;
 
+	if (ubuntuone_is_inside_shares (uon, path))
+		is_shared_to_me = TRUE;
+
 	is_dir = nautilus_file_info_is_directory (file);
 	is_regular = nautilus_file_info_get_file_type (file) == G_FILE_TYPE_REGULAR;
 
@@ -351,7 +354,7 @@
 	/* public files */
 	menu_item = urlitem = NULL;
 
-	if (is_managed && is_regular) {
+	if (!is_shared_to_me && is_managed && is_regular) {
 		if (is_public) {
 			urlitem = nautilus_menu_item_new ("ubuntuone-geturl",
 							  _("Copy Web _Link"),

=== modified file 'nautilus/ubuntuone-nautilus.h'
--- nautilus/ubuntuone-nautilus.h	2010-10-18 11:05:29 +0000
+++ nautilus/ubuntuone-nautilus.h	2011-02-11 15:30:10 +0000
@@ -64,6 +64,7 @@
 
 /* Utility functions */
 gboolean ubuntuone_is_folder_shared (UbuntuOneNautilus *uon, const gchar *path);
+gboolean ubuntuone_is_inside_shares (UbuntuOneNautilus *uon, const gchar *path);
 gboolean ubuntuone_is_location_bar_enabled (void);
 
 gboolean ubuntuone_check_shares_and_public_files (UbuntuOneNautilus *uon,

=== modified file 'nautilus/utils.c'
--- nautilus/utils.c	2010-10-18 11:05:29 +0000
+++ nautilus/utils.c	2011-02-11 15:30:10 +0000
@@ -19,7 +19,11 @@
  *
  */
 
+#include <limits.h>
+#include <stdlib.h>
 #include <glib/gi18n.h>
+#include <glib/gutils.h>
+#include <glib/gfileutils.h>
 #include <libnautilus-extension/nautilus-file-info.h>
 #include <libsyncdaemon/libsyncdaemon.h>
 #include "ubuntuone-nautilus.h"
@@ -130,6 +134,37 @@
 	return is_shared;
 }
 
+/* 
+ * Similar to ubuntuone_is_folder_shared but it checks if the passed
+ * folder is inside a folder shared TO the user.
+ *
+ * Added to fix bug #712674
+ */
+gboolean
+ubuntuone_is_inside_shares (UbuntuOneNautilus *uon, const gchar *path)
+{
+	gboolean is_shared_to_me = FALSE;
+	gchar *resolved_path;
+	gchar *prefix = g_build_filename (g_get_user_data_dir (), "ubuntuone", "shares", G_DIR_SEPARATOR_S, NULL);
+
+	/* 
+	 *  Have to use realpath because path contains a symlink like
+	 * ~/Ubuntu One/Shared With Me -> ~/.local/share/ubuntuone/shares
+	 * 
+	 * This also claims ~/Ubuntu One/Shared with Me/foo is
+	 * in a share even if it's not true. that's intentional since
+	 * those files are never uploaded, and thus it makes no sense
+	 * to publish them.
+	 */
+	resolved_path = realpath(path, NULL);
+	if (g_str_has_prefix (resolved_path, prefix)) {
+		is_shared_to_me = TRUE;
+	}
+	free (resolved_path);
+	g_free (prefix);
+	return is_shared_to_me;
+}
+
 gboolean
 ubuntuone_is_location_bar_enabled (void)
 {