[Github-comments] [geany/geany] Rpg sidebar tree (see #259) (#1813)

[Colomban Wendling]

b4n requested changes on this pull request.

First pass looking at the code.  Not tested yet, but the feature looks pretty cool indeed :)

>  
-	if (!documents_show_paths)
-		return NULL;
+	path = gtk_tree_model_get_path(GTK_TREE_MODEL(store_openfiles), iter);
+	gtk_tree_view_expand_row(GTK_TREE_VIEW(tv.tree_openfiles), path, TRUE);
+	gtk_tree_path_free(path);
+}

style: One more blank line below this

> +	if (parent_len)
+	{
+		gsize len;
+		dirname = get_doc_folder(path);
+		len = strlen(dirname);
+		/* Maybe parent is /home but dirname is ~ (after substitution from /home/user) */
+		if (pathname[0] == dirname[0])
+			memmove(dirname, dirname + parent_len, len - parent_len + 1);
+	}
+
+	g_free(pathname);
+
+	return dirname;
+}
+
+static void tree_copy_item(GtkTreeIter *parent, GtkTreeIter *parent_old, GtkTreeIter *parent_new)

this is not intuitively named, and would benefit renaming or documenting.
IIUC, it *copies* data pointed to by `parent_old` to where a new child of `parent_new`, returning that child iter in `parent`.  Why everything's called parent?
Actually at first not remembering the order of parameters to `gtk_tree_store_append()` I thought it added a child to `parent` and stored it in `parent_new`…

> +}
+
+
+/*
+ * Recursively copy all nodes from old parent to new parent
+ * */
+static void tree_copy_recursive(GtkTreeIter *parent_old, GtkTreeIter *parent_new)
+{
+	gint i;
+	GtkTreeIter child;
+	GtkTreeIter parent;
+	GtkTreePath *path;
+	GtkTreeModel *model = GTK_TREE_MODEL(store_openfiles);
+
+	path = gtk_tree_model_get_path(model, parent_old);
+	gtk_tree_path_free(path);

These two lines seem useless -- well, the whole `path` seems not used.

> +/*
+ * Recursively copy all nodes from old parent to new parent
+ * */
+static void tree_copy_recursive(GtkTreeIter *parent_old, GtkTreeIter *parent_new)
+{
+	gint i;
+	GtkTreeIter child;
+	GtkTreeIter parent;
+	GtkTreePath *path;
+	GtkTreeModel *model = GTK_TREE_MODEL(store_openfiles);
+
+	path = gtk_tree_model_get_path(model, parent_old);
+	gtk_tree_path_free(path);
+	tree_copy_item(&parent, parent_old, parent_new);
+	i = gtk_tree_model_iter_n_children(model, parent_old) - 1;
+	while (i >= 0 && gtk_tree_model_iter_nth_child(model, &child, parent_old, i))

Why walking children from the last one to the first?  This would reverse the order, as `tree_copy_item()` appends to the model, wouldn't it? Or is it to allow `parent_old == parent_new`?
OIn nay case, if possible it sounds like a better job for `gtk_tree_model_iter_next()`/`previous`, doesn't it?

> +	GtkTreePath *path;
+	GtkTreeModel *model = GTK_TREE_MODEL(store_openfiles);
+
+	path = gtk_tree_model_get_path(model, parent_old);
+	gtk_tree_path_free(path);
+	tree_copy_item(&parent, parent_old, parent_new);
+	i = gtk_tree_model_iter_n_children(model, parent_old) - 1;
+	while (i >= 0 && gtk_tree_model_iter_nth_child(model, &child, parent_old, i))
+	{
+		tree_copy_recursive(&child, &parent);
+		i--;
+	}
+}
+
+
+static void tree_add_new_dir(GtkTreeIter *child, GtkTreeIter *parent, gchar *file)

`file` should be `const`, shouldn't it?

> +	return i;
+}
+
+
+typedef struct TreeForeachData {
+	gchar *needle;
+	gsize best_len;
+	gsize needle_len;
+	GtkTreeIter best_iter;
+	enum {
+		TREE_CASE_NONE,
+		TREE_CASE_EQUALS,
+		TREE_CASE_CHLID_OF,
+		TREE_CASE_PARENT_OF,
+		TREE_CASE_HAVE_SAME_PARENT
+	} best_case;

you could make this a proper type so it can be used in the different places instead of int

> +		}
+		case TREE_CASE_PARENT_OF:
+		{
+			/* More complicated logic. This dir should be a parent
+			 * of existing, so reparent existing dir.  */
+			has_parent = gtk_tree_model_iter_parent(model, &iter, &data.best_iter);
+			tree_add_new_dir(parent, has_parent ? &iter : NULL, path);
+			tree_copy_recursive(&data.best_iter, parent);
+			gtk_tree_store_remove(store_openfiles, &data.best_iter);
+			break;
+		}
+		case TREE_CASE_HAVE_SAME_PARENT:
+		{
+			/* Even more complicated logic. Both dirs have same
+			 * parent, so create new parent and reparent them */
+			GtkTreeIter parent_buf;

`parent_buf`: why "buf"?

> +			tree_copy_recursive(&data.best_iter, &parent_buf);
+			unfold_iter(&parent_buf);
+			gtk_tree_store_remove(store_openfiles, &data.best_iter);
+			tree_add_new_dir(parent, &parent_buf, path);
+
+			g_free(newpath);
+			break;
+		}
+		default:
+		{
+			tree_add_new_dir(parent, NULL, path);
+			break;
+		}
+	}
+	if (data.needle != path)
+		g_free(data.needle);

the conditional is both confusing and useless: `data.needle` comes from `get_doc_folder(path)` which will always return a new string.

BTW, if my reasoning is wrong and `data.needle` can be equal to `path` this should be changed IMO, because usually the confusion introduced by this kind of memory management is not worth the gain.  This is not true for very specific cases simple enough or that give enough gain to warrant the extra complexity, but I doubt this is the case here, and if it is it should be better documented.

> -		gtk_tree_store_remove(store_openfiles, iter);
+	if (gtk_tree_model_iter_parent(model, &parent, iter) &&
+	    gtk_tree_model_iter_n_children(model, &parent) == 1)
+	{
+		GtkTreeIter buf = parent;
+
+		/* walk on top and close all orphaned parents */
+		while (gtk_tree_model_iter_parent(model, &parent, &buf) &&
+		       gtk_tree_model_iter_n_children(model, &parent) == 1)
+		{
+			buf = parent;
+		}
+		gtk_tree_store_remove(store_openfiles, &buf);
+		return;
+	}
+	gtk_tree_store_remove(store_openfiles, iter);

If I ain't mistaken, this could be simplified a little like this:
```C
	GtkTreeIter iter = doc->priv->iter;
	GtkTreeIter parent;

	/* walk up to remove all parents that would end up empty */
	while (gtk_tree_model_iter_parent(model, &parent, &iter) &&
	       gtk_tree_model_iter_n_children(model, &parent) == 1)
	{
		iter = parent;
	}
	gtk_tree_store_remove(store_openfiles, &iter);
```

> @@ -1100,7 +1412,7 @@ void sidebar_init(void)
 	StashGroup *group;
 
 	group = stash_group_new(PACKAGE);
-	stash_group_add_boolean(group, &documents_show_paths, "documents_show_paths", TRUE);
+	stash_group_add_integer(group, (gint*)&documents_show_paths, "documents_show_paths", SHOW_PATHS_LIST);

beware! this doesn't restrict the value of `documents_show_paths` from an corrupted/invalid configuration file, which can lead to a crash (accessing outside the array).  It might be worth sanitizing the value when using it (not sure what we do or if we do that already for other or not, but it wouldn't hurt to start).

> +			break;
+		}
+		case TREE_CASE_CHLID_OF:
+		{
+			/* This dir is longer than existing so just add child */
+			tree_add_new_dir(parent, &data.best_iter, path);
+			break;
+		}
+		case TREE_CASE_PARENT_OF:
+		{
+			/* More complicated logic. This dir should be a parent
+			 * of existing, so reparent existing dir.  */
+			has_parent = gtk_tree_model_iter_parent(model, &iter, &data.best_iter);
+			tree_add_new_dir(parent, has_parent ? &iter : NULL, path);
+			tree_copy_recursive(&data.best_iter, parent);
+			gtk_tree_store_remove(store_openfiles, &data.best_iter);

Looking at how it's used, it could be nice to make a `tree_reparent()` or something, which would do this plus removing the old parent.  That the implementation requires copying is sad, but would be encapsulated and wouldn't get people like me to look suspiciously at the calling code before going further :)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1813#pullrequestreview-176064362
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20181124/58315e1c/attachment.html>