Continuation of geany#259 since I can't push to @scriptum branch (I suggest you allow that).
I added some cleanup commits on top of @scriptum work and rebased onto 1.33.
@elextr @b4n Please give feedback (considering the history in geany#259). I think I'll merge this in maybe 10 days or so.
You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1813
-- Commit Summary --
* Add sidebar tree * Add documentation for document list * sidebar: optimized documents_sort_func() a bit * sidebar: refactor get_doc_parent() * sidebar: various minor changes
-- File Changes --
M doc/geany.txt (29) A doc/images/sidebar_documents_only.png (0) A doc/images/sidebar_show_paths.png (0) A doc/images/sidebar_show_tree.png (0) M src/sidebar.c (571) M src/utils.c (2) M src/utils.h (2)
-- Patch Links --
https://github.com/geany/geany/pull/1813.patch https://github.com/geany/geany/pull/1813.diff
@codebrainz too
elextr requested changes on this pull request.
Code was reviewed too, but I couldn't find anything to complain about!!! :)
+ .. image:: ./images/sidebar_documents_only.png + +Show Paths + Show opened documents as two-level tree in which first level is a full + directory path to document and second level is a file name. + + .. image:: ./images/sidebar_show_paths.png + +Show Tree + Show opened documents as tree similar to directory tree, but it shows only + directories that contains opened files. Directories that doesn't contain + files are shown as paths (similar to *Show Paths* mode). Home directory + always shortened to ``~``. If there is an active project, it makes a tree + of project documents inside project name. +
By "project name" do you mean "project base directory"?
And what does it do with documents that are outside the project base dir?
+Documents Only
+ Show only ordered document's file names. + + .. image:: ./images/sidebar_documents_only.png + +Show Paths + Show opened documents as two-level tree in which first level is a full + directory path to document and second level is a file name. + + .. image:: ./images/sidebar_show_paths.png + +Show Tree + Show opened documents as tree similar to directory tree, but it shows only + directories that contains opened files. Directories that doesn't contain + files are shown as paths (similar to *Show Paths* mode). Home directory + always shortened to ``~``. If there is an active project, it makes a tree
is always
@kugel thanks for making a testable version of #259, reviewed but unfortunately I can't test ATM, maybe someone else will test in the meantime.
kugel- commented on this pull request.
+ .. image:: ./images/sidebar_documents_only.png + +Show Paths + Show opened documents as two-level tree in which first level is a full + directory path to document and second level is a file name. + + .. image:: ./images/sidebar_show_paths.png + +Show Tree + Show opened documents as tree similar to directory tree, but it shows only + directories that contains opened files. Directories that doesn't contain + files are shown as paths (similar to *Show Paths* mode). Home directory + always shortened to ``~``. If there is an active project, it makes a tree + of project documents inside project name. +
The node in the tree view is labeled after the project. While normally folders are combined if no files in parent folders are open, this is not done for projects.
e.g. if I open document.c it shows normally as ~/dev/geany.git/src -- document.c
If I make a project it shows as geany -- src ---- document.c
Documents outside the project base dir are not affected (i.e. the nodes are labeled after the paths)
elextr commented on this pull request.
+ .. image:: ./images/sidebar_documents_only.png + +Show Paths + Show opened documents as two-level tree in which first level is a full + directory path to document and second level is a file name. + + .. image:: ./images/sidebar_show_paths.png + +Show Tree + Show opened documents as tree similar to directory tree, but it shows only + directories that contains opened files. Directories that doesn't contain + files are shown as paths (similar to *Show Paths* mode). Home directory + always shortened to ``~``. If there is an active project, it makes a tree + of project documents inside project name. +
Oh, ok, so if I read that right only the project base dir and below shows as a tree?
Maybe the selection should be called "Show Project Tree"
Just a final check, commonly my directory layouts look like:
``` /data -- the raid array mount point foo -- the project name foo -- the git tree and the project base dir in foo.geany src foo.cpp lots more source foo.geany dev_build -- out of tree build dir foo -- the executable test_output -- unit test output file lots more stuff the build makes ``` So if I had project foo open and foo.cpp and test_output open I would get
``` /data/foo/dev_build test_output foo src foo.cpp ```
hey sorry to dissapoint, I dont know how my email got linked to this project but the Kugel you are looking for doesnt use this email (ive been getting random updates from github every once in a while for years), sorry =( but hey i hope you guys work out those bugs and keep plugging away at it!
On Mon, Mar 26, 2018 at 5:47 PM, elextr notifications@github.com wrote:
*@elextr* commented on this pull request.
In doc/geany.txt https://github.com/geany/geany/pull/1813#discussion_r177245479:
- .. image:: ./images/sidebar_documents_only.png
+Show Paths
- Show opened documents as two-level tree in which first level is a full
- directory path to document and second level is a file name.
- .. image:: ./images/sidebar_show_paths.png
+Show Tree
- Show opened documents as tree similar to directory tree, but it shows only
- directories that contains opened files. Directories that doesn't contain
- files are shown as paths (similar to *Show Paths* mode). Home directory
- always shortened to ``~``. If there is an active project, it makes a tree
- of project documents inside project name.
Oh, ok, so if I read that right only the project base dir and below shows as a tree?
Maybe the selection should be called "Show Project Tree"
Just a final check, commonly my directory layouts look like:
/data -- the raid array mount point foo -- the project name foo -- the git tree and the project base dir in foo.geany src foo.cpp lots more source foo.geany dev_build -- out of tree build dir foo -- the executable test_output -- unit test output file lots more stuff the build makes
So if I had project foo open and foo.cpp and test_output open I would get
/data/foo/dev_build test_output foo src foo.cpp
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/geany/geany/pull/1813#discussion_r177245479, or mute the thread https://github.com/notifications/unsubscribe-auth/AFh94vi_hC13eNhoFXodrBn1SOdsmxTLks5tiWIKgaJpZM4SyLKn .
@kugel sorry, I meant @kugel- but your user comes up first on the autocomplete, and hey us humans make mistakes.
hehe its fine, i get these every once in a while, just wanted to make sure the real kugel gets your message, eve if he stole my nickname =D
On Tue, Mar 27, 2018 at 2:19 AM, elextr notifications@github.com wrote:
@Kugel https://github.com/Kugel sorry, I meant @kugel- https://github.com/kugel- but your user comes up first on the autocomplete, and hey us humans make mistakes.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/geany/geany/pull/1813#issuecomment-376410113, or mute the thread https://github.com/notifications/unsubscribe-auth/AFh94ipxXxoJMJTTXUVW5B-aPy40D7GDks5tidnggaJpZM4SyLKn .
Oh, ok, so if I read that right only the project base dir and below shows as a tree?
No, all open files are tree'ified. Its just that once a project is open it is ensured that its base path gets its own tree node (labeled after the project)
Maybe the selection should be called "Show Project Tree"
No, see above.
Just a final check, commonly my directory layouts look like:
/data -- the raid array mount point foo -- the project name foo -- the git tree and the project base dir in foo.geany src foo.cpp lots more source foo.geany dev_build -- out of tree build dir foo -- the executable test_output -- unit test output file lots more stuff the build makesSo if I had project foo open and foo.cpp and test_output open I would get
/data/foo/dev_build test_output foo src foo.cpp
Yes. But if you also had a /data/bar folder then it'd look like
``` /data foo/dev_build test_output bar file.txt foo src foo.cpp ```
all open files are tree'ified. Its just that once a project is open it is ensured that its base path gets its own tree node (labeled after the project)
Oh, ok, thats sort of unexpected, so it needs to be explained more clearly (if only so we can close issues and point people to the manual that says thats how its meant to work :)
Please suggest a better description, I'm not so good at this.
@kugel- yep, will propose something as soon as I'm happy I understand :)
But on looking at the above example again.
No, all open files are tree'ified.
But in your additional example you have shown `foo/dev_build` as not being expanded the way `foo/src` is?
But in your additional example you have shown foo/dev_build as not being expanded the way foo/src is?
foo/dev_build is one tree node because no file in foo itself is open. foo->src is two tree nodes because foo represents the project (therefore always gets its own top-level node) and src is a subdirectory (with an open file).
Might be easier if you just try the patch and see yourself, Github/mail kind of sucks for showing examples.
Might be easier if you just try the patch and see yourself, Github/mail kind of sucks for showing examples.
Two things:
1. I said above I can't try it ATM
2. guessing how its meant to work from experiments is a poor way of getting an explanation for users
@elextr do you need more information on how it's supposed to work?
Yes, above I said:
But on looking at the above example again.
No, all open files are tree'ified.
But in your additional example you have shown foo/dev_build as not being expanded the way foo/src is?
I had previously asked if only the project tree was expanded and you said no as quoted above, so to be clear the question is, if "all open files are treeified" then why isn't `foo/dev_build` expanded but `foo/src` is expanded?
This proves that I'm not good at documenting such things for end users :-)
There is additional confusion since your example has two foo folders and the project is named foo as well. I'll rename to illustrate:
``` /data -- the raid array mount point foo bar -- the git tree and the project base dir in baz.geany src xx.cpp lots more source baz.geany -- project name renamed from foo to baz dev_build -- out of tree build dir foo -- the executable test_output -- unit test output file lots more stuff the build makes ```
So accordingly, in your next example foo/src becomes bar/src.
If xx.cpp and test_output are open, then it's displayed as follows:
``` baz src xx.cpp /data/foo/dev_build test_output ```
This is because of the following rules: - The directory of a filename is folded as much as possible - if two or more documents have common parents, then a tree is formed at that parent (repeat for any common parent encountered) - If a project is open, then the project's base path will get a separate tree node at the root (labeled after the project). This happens regardless of whether parts of the project base path has common parents with open documents outside of the project. Folding of subdirectories (below the project base path) is performed if possible.
Compare to the situation when there is no project open:
This proves that I'm not good at documenting such things for end users :-)
Thats ok, thats why I'm asking all these annoying questions :-)
I think the confusion is that I understood that the difference between this option and the existing option was that all directories on the path to open files expanded, but in fact they only expand if more than one thing inside them is open.
To confirm by example, if my project `src` directory had three sub-directories `front_end`, `middle_end` and `back_end` then if only a file was open in `front_end` the result for the project tree would be:
``` baz src/front_end xxx.cpp ```
but if I had a file open in each of `front_end` and `back_end` the result would be:
``` baz src back_end yyy.cpp front_end xxx.cpp ``` with `back_end` and `front_end` sorted and `middle_end` not showing.
Correct
@kugel- I still can't do anything with github, so I have posted my suggested text below.
``` Document list views ^^^^^^^^^^^^^^^^^^^
There are three different ways to display documents on the sidebar if *Show documents list* is active. To switch between views press the right mouse button on documents list and select one of those radio buttons:
Documents Only Show only file names of open documents in sorted order.
.. image:: ./images/sidebar_documents_only.png
Show Paths Show open documents as a two-level tree in which first level is the paths of directories containing open files and the second level is the file names of the documents open in that path. All documents with the same path are grouped together under the same first level item. Paths are in sorted order and documents are sorted within each group.
.. image:: ./images/sidebar_show_paths.png
Show Tree Show paths as above, but as a multiple level partial tree. The tree is only expanded at positions where two or more directory paths to open documents share the same prefix. The common prefix is shown as a parent level, and the remainer of those paths are shown as child levels. This applies recursively down the paths making a tree to the file names of open documents, which are grouped in sorted order as an additional level below the last path segment.
For convenience two common file locations are handled specifically, open files below the users home directory, and open files below an open project base path. Each of these is moved to its own top level tree instead of being in place in the normal tree. The top level of these trees are each labelled differently. For the home directory tree the path of the home directory is shown as ``~``, and for the project tree the path to the project base path is shown simply as the project name.
.. image:: ./images/sidebar_show_tree.png
In all cases paths and file names that do not fit in the width available are ellipsised. ```
Works for me, thanks.
@kugel- pushed 1 commit.
5c79661 improve manual text for the document listing methods in the sidebar
elextr approved this pull request.
LGTM
I get the following warning when building the manual:
``` [WARNING] image.py:463 image /home/kugel/dev/geany.git/doc/./images/sidebar_documents_only.png is too tall for the frame, rescaling [WARNING] image.py:463 image /home/kugel/dev/geany.git/doc/./images/sidebar_documents_only.png is too tall for the frame, rescaling ```
In the generated pdf sidebar_documents_only.png appears very small. The HTML manual is fine though. Does anyone know what to do about it?
@elextr Actually, the home directory is merely abbreviated (using ~), it doesn't get an individual root node unlike the project base path. See the doc/sidebar_show_tree.png
@kugel, erm, ok, what happens if there are two directories open along the path to `~` eg if HOME is `/file_server/home/this_user` and I have the files `/file_server/home/this_user/my_file.cpp`, `/file_server/home/this_user/a_dir/also_mine.cpp` and `/file_servert/home/another_user/their_file.cpp` open?
``` ~ a_dir also_mine.cpp my_file.cpp /file_server/home/another_user their_file.cpp ``` or ``` /file_server/home ~ a_dir also_mine.cpp my_file.cpp another_user their_file.cpp ``` or no `~` ``` /file_server/home this_user a_dir also_mine.cpp my_file.cpp another_user their_file.cpp ```
changed which @kugel- :)
I think the second. However, upon trying I found there is a crash (but only when there are open files of another user AND a file located in ~ itself). I'll dig into it.
On a side note, having a file of another user open seems unlikely, as Geany is a poor program for pair programming :-)
@kugel- various daemons and databases run under other users and have their config files in that home dir as another use-case. But even if we can't think of a use-case, if its at all possible, _somebody_ __will__ do it :smile:
I amended the commit message of 7552d5f due to typos. There are no code changes, except the three new commits on top of @elextr change.
@elextr Upon looking at the code handling the home directory, I think the intention was to match your first case, and it was a bit buggy. Since I think this is the most useful behavior, so I fixed the code to match that behavior.
@elextr is your approval still valid even after my changes? I'd like to merge this finally.
@kugel- I'm fine for you to merge it.
@kugel- pushed 1 commit.
7b10e1e sidebar: simplify code by handling projects and home directory equally
@elextr I'm sorry. I found that I have forgotten to publish one more commit I made. Please review this one, though I hope it's welcome one since it simplifies things.
@elextr ping
Has anyone (apart from @kugel-) tested this yet?
@scriptum did you give it a try?
@elextr @b4n I really want this in the release so please give it a last look / test or an OK for me to merge it. String freeze is soon. Please!
@kugel- I'm in the process of reviewing the code, but I might not have a lot of time before Friday.
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 :)
Bug: when I open Geany after this change, so with the default setting: if I right-click on the documents sidebar, every element gets folded. Yeah, even before I click on any action.
More generally fold state seems pretty random when switching between modes.
I also get all the tree folding up when right clicking.
To be clear, this is unacceptable, I can't close a file without losing my fold state.
This should be merged as is.
This PR has a history of more bugs being found each time its "ready" probably because @kugel- has only had the chance to work on it intermittently, so although I like the intent I, don't think the implementation is suitable for last minute merging.
It should be held over and merged after 1.34 (and after its fixed) so any further bugs can be worked out before a release.
For some reason I wasn't notified about your updates. I was still thinking review is ongoing. Given the bugs that appeared I agree to post-pone. Thanks for finally testing this
@scriptum did you give it a try?
I tested forked Geany 4 or 3 years ago with this patch. But I didn't tested it on latest Geany versions.
I remember a fold state issue but it is probably common for all GTK applications - GTK doesn't remember children fold state unlike Qt. This feature was planned in GTK 4 or later.
@scriptum Yes, GTK doesn't maintain tree view fold state below folded elements for you. However, that doesn't mean elements get folded seemingly randomly :) The issue I mentioned is not that folding an element and unfolding it doesn't restore children's fold state, but that for some reason when right-clicking (which opens a contextual menu) rows get folded. Also, keeping fold state when changing mode is something the app will likely have to do, because it changes the hierarchy, so there's no real magic that can be done, it's up to the app to know which new row corresponds to which old one, and thus map fold states if wanted.
Also, keeping fold state when changing mode is something the app will likely have to do
Especially so in the cases where a buffer close changes the visible hierarchy in one of the "smart" modes, thats really something that only Geany can do, not GTK.
kugel- commented on this pull request.
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);
No, you are right, might be a leftover from my modifications over the original version.
kugel- commented on this pull request.
- 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)
will fix
kugel- commented on this pull request.
+}
+ + +/* + * 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);
good catch, seems pretty obvious
Trying to finish this up...I could locate and fix the unfold bug and I'm currently work through the other review comments.
kugel- commented on this pull request.
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 this, your suggestion sounds good and would also make the intent of the function more clear.
The copy is currently mainly done because the shortname of a directory node may change (if a new parent is added in-between then child nodes could change from a/b/c/d to a->b/c/d, for example (after opening a/x.c))
kugel- commented on this pull request.
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);
Oh, and because gtk_tree_store_move_after/_before don't work on sorted tree stores.
kugel- commented on this pull request.
+/*
+ * 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))
The original author probably copied the loop from `on_openfiles_document_action_apply()`. I changed it your suggestion which is a more natual fit.
kugel- commented on this pull request.
}
+ 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;
Will rename
kugel- commented on this pull request.
@@ -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);
Do you suggest to check every single usage of the variable or is there some kind of point in time after which we can assume the value is sanitized?
@kugel- good to see you get some time, ping when you think its testable.
@kugel- pushed 5 commits.
17d06db1ac63277581fdb71c17b66cd8e3aa799b Merge tag '1.35.0' into rpg-sidebar-tree-fixed ef6d6817e1214bd098d000850ed906ac3f41dcdf sidebar: resolve some review comments 15582e2a440220607e80449162e51b810fbf3252 sidebar: fix for new documents being added unfolded when tree view is not enabled a172c5d61db0af082b7290315bb2627d7604e668 refactor and rename tree_copy_recursive and tree_copy_item 1d28d2a7833c7a858ab4c00aed345d3d8c1abbb3 sidebar: implement merging rows back when closing documents and
@elextr ping :-) @b4n hopefully I adressed the review comments and didn't forget one. Please test and review.
PS: I merged 1.35 to be up-to-date. I plan to rebase and consolidate a bit before merging (but no complete squash).
@kugel- I no longer use geany all day every day, so it might be a delay before I can test it, hopefully not too long.
@b4n @elextr please test and/or review, thanks :-)
@b4n @elextr @codebrainz please test and/or review, thanks :-)
@b4n your review is still pending. Please have a look again. I would like to merge this early in the cycle.
ntrel commented on this pull request.
- 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;
Also, typo CHILD
ntrel commented on this pull request.
Haven't tested, but the idea seems great. (To be honest if this works well I don't know why we need the other 2 options besides *Show Tree*).
I've reviewed the doc changes and made suggestions, not a blocker. If I have time maybe I'll fix those.
@@ -530,6 +530,48 @@ order. It is not alphabetical as shown in the documents list
See the `Notebook tab keybindings`_ section for useful shortcuts including for Most-Recently-Used document switching.
+Document list views +^^^^^^^^^^^^^^^^^^^ + +There are three different ways to display documents on the sidebar if *Show +documents list* is active. To switch between views press the right mouse button +on documents list and select one of those radio buttons:
on *the* documents list
those radio buttons -> these items (Users might not know what radio buttons are)
- .. image:: ./images/sidebar_documents_only.png
+ +Show Paths + Show open documents as a two-level tree in which first level is the paths + of directories containing open files and the second level is the file names of + the documents open in that path. All documents with the same path are grouped + together under the same first level item. Paths are in sorted order and + documents are sorted within each group. + + .. image:: ./images/sidebar_show_paths.png + +Show Tree + Show paths as above, but as a multiple level partial tree. The tree is only + expanded at positions where two or more directory paths to open documents + share the same prefix. The common prefix is shown as a parent level, and + the remainer of those paths are shown as child levels. This applies
remainder
- the documents open in that path. All documents with the same path are grouped
+ together under the same first level item. Paths are in sorted order and + documents are sorted within each group. + + .. image:: ./images/sidebar_show_paths.png + +Show Tree + Show paths as above, but as a multiple level partial tree. The tree is only + expanded at positions where two or more directory paths to open documents + share the same prefix. The common prefix is shown as a parent level, and + the remainer of those paths are shown as child levels. This applies + recursively down the paths making a tree to the file names of open documents, + which are grouped in sorted order as an additional level below the last path + segment. + + For convenience two common file locations are handled specifically, open
specifically, -> specially:
@kugel- pushed 1 commit.
f15336e0ef057947ba6d1d74d78d7d87952b8e06 typo and doc fixes
Thanks for the heads up and review!
To be honest if this works well I don't know why we need the other 2 options besides Show Tree.
Once I used "show tree" I never went back to the other methods. It's really great IMO, especially since it allows to easily close all files within a directory with a simple middle mouse click.
I could imagine that someone might be concerned that the tree gets too many levels, making leafs harder to see (might need to scroll sideways) but I haven't had this problem in multiple years.
@kugel- pushed 2 commits.
67aa6b95c05514fd5e963204a24b786db007219a session.conf split follow-up: sidebar_page ba971c39a277d86ae46a47937ff1fc4658b09f1f Add (and default to) tree view of open documents in the sidebar
@kugel- pushed 1 commit.
0af4dadab45fbfec85475d2c4f13e8f6654012fc Add a unit test program to check the new sidebar documents tree view
I did a lot of cleanup and added a logic to remember the fold state of children rows when one of their parents are folded. I would love to merge this soon as I've been using this feature for years now.
I squashed all the commits. The original work of @scriptum is barely recognizable by now so I removed him from the commit history.
@kugel- commented on this pull request.
@@ -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);
I added sanitizing in the "load-settings" signal handler.
@kugel- pushed 2 commits.
be4f083a3de1a06e0562e30db5540b1c16ebd957 Add (and default to) tree view of open documents in the sidebar 3f67ae4aaffabc82771437053d45426041aec244 Add a unit test program to check the new sidebar documents tree view
@kugel- pushed 2 commits.
3906e1ab30f91c70900eaf05f0d706db143f572e Add (and default to) tree view of open documents in the sidebar 1ee3a3273d659658a72cb84f0b1c5eb41f232bfc Add a unit test program to check the new sidebar documents tree view
@kugel- pushed 2 commits.
438b608a0e136410a1ef4454dca8358de968e1ae Add (and default to) tree view of open documents in the sidebar 5066844f7ec400caf648e9f7cdc1c96c14769a0e Add a unit test program to check the new sidebar documents tree view
@kugel- pushed 1 commit.
dcef9be2910edc301005f0f056b142e5628f7a2e Add a unit test program to check the new sidebar documents tree view
@kugel- pushed 1 commit.
af7636ac6aca2d4bd0517d3e1a92bc9b4b867a85 Add a unit test program to check the new sidebar documents tree view
@kugel- pushed 1 commit.
ce29af68f26dd7ebc8f5267d34fcb15b7ca9b8d7 Add a unit test program to check the new sidebar documents tree view
@kugel- pushed 1 commit.
24555399b26eeca046689fd732b693f81889b9e9 Add a unit test program to check the new sidebar documents tree view
@kugel- pushed 2 commits.
0f77012c58082b1f08d50e5b79188620fb57a090 Add (and default to) tree view of open documents in the sidebar 2474712b08cb15127a5f61e5caf5e91d6b54582c Add a unit test program to check the new sidebar documents tree view
@b4n @elextr @eht16 please review. Without any further feedback I'll probably go ahead and merge in a week or so
@kugel- sorry, I totally lost track of this PR. Tested, some observations below, not sure if they are deliberate? But they were surprising, appeared inconsistent, and in at least one case annoying.
Initial layout:
``` V /data/nexciidoc/attic old_spec.edoc V ~/tmpgeany/geany V data geany-3.0.css V src document.c > tagmanager ```
The `V` means unrolled, and `>` rolled up, there is a file open in tagmanager but I folded it up.
Note the path starting with `~` sorts after `/data` which is correct because `~` is second last ASCII character
Opened `~/atest.edoc` got:
``` V ~ atest.edoc V /tmpgeany/geany V data geany-3.0.css V src document.c > tagmanager V /data/nexciidoc/attic old_spec.edoc ```
Note the resorting of `/data` when `~` became a top level, but the previous top level path still started with `~` and sorted after `/data`.
When I closed `~/atest.edoc`:
``` V ~ V /tmpgeany/geany V data geany-3.0.css V src document.c > tagmanager V /data/nexciidoc/attic old_spec.edoc ```
The `~` stayed separated even though it now had only one entry open.
When I closed `geany-3.0.css`:
I expected:
``` V ~ V /tmpgeany/geany/src document.c > tagmanager V /data/nexciidoc/attic old_spec.edoc ```
but got:
``` V ~ > /tmpgeany/geany/src V /data/nexciidoc/attic old_spec.edoc ```
Note `/tmpgeany/geany/src` folded uncommanded, that would get annoying if it kept happening.
Also `~` still doesn't recombine, but when I closed and reopened Geany it combined and sorted after `/data`.
Thanks, I'll check. It's quite hard to get GTK to do what you want. Remembering fold state is not supported by GTK directly so I have to code around it, basically using a hammer so long until it fits.
Do you have comments aside the fold state memory?
Do you have comments aside the fold state memory?
The resorting of ~ when it became a standalone level
Sorting weirdness seems to be caused by `g_utf8_collate_key_for_filename()` simply ignoring `~`. However, I an acceptable workaround (tread ~ as . for sorting). Now I'm looking why nodes below `~` aren't merged back properly.
Sorting weirdness seems to be caused by g_utf8_collate_key_for_filename() simply ignoring ~.
Makes sense, it probably is intended to work on absolute filenames, or at least filenames with the same root (ie directory).
Now I'm looking why nodes below ~ aren't merged back properly.
Getting ordering and merging stable will probably make saved fold state work a lot easier.
@kugel- pushed 4 commits.
97333cb2da19910d388680f9c629e29fa8e65c02 fixup! Add (and default to) tree view of open documents in the sidebar e2631c752ac0317835bf8a7ffa1469800751a856 fixup! Add (and default to) tree view of open documents in the sidebar 54745524cec31445dbbf00876ababe2ac14acc63 fixup! Add (and default to) tree view of open documents in the sidebar 85fac3741badc8897968864cf70609d0a6eb4423 fixup! Add (and default to) tree view of open documents in the sidebar
@kugel- pushed 1 commit.
4b3df388d9932552ec8951109e3b5c1567f65a66 fixup! Add a unit test program to check the new sidebar documents tree view
@elextr I think I got this right now. Please check.
@eht16 requested changes on this pull request.
Looks good. Had only a few remarks.
@@ -1105,8 +1591,6 @@ void sidebar_init(void)
G_CALLBACK(sidebar_tabs_show_hide), NULL); g_signal_connect_after(main_widgets.sidebar_notebook, "switch-page", G_CALLBACK(on_sidebar_switch_page), NULL); - - sidebar_tabs_show_hide(GTK_NOTEBOOK(main_widgets.sidebar_notebook), NULL, 0, NULL);
Removed because `sidebar_tabs_show_hide()` is called in `on_load_settings()` already?
@@ -1018,20 +1018,12 @@ static const gchar *get_locale(void)
GEANY_EXPORT_SYMBOL -gint main_lib(gint argc, gchar **argv) +void main_init_headless(void)
While this sounds like a good idea, AFAIU this is completely unrelated in this PR?
@@ -0,0 +1,33 @@
+dnl GEANY_CHECK_GTK +dnl Checks whether the GTK stack is available and new enough. Sets GTK_CFLAGS and GTK_LIBS. +AC_DEFUN([GEANY_CHECK_GTK],
Great to move those checks into here.
But pretty much unrelated to this PR, I think. Wouldn't it be better to do such refactorings on its own?
@@ -187,7 +187,7 @@ static void init_pref_groups(void)
stash_group_add_toggle_button(group, &file_prefs.tab_close_switch_to_mru, "tab_close_switch_to_mru", FALSE, "check_tab_close_switch_to_mru"); stash_group_add_integer(group, &interface_prefs.tab_pos_sidebar, "tab_pos_sidebar", GTK_POS_TOP); - stash_group_add_integer(group, &interface_prefs.documents_show_paths, "documents_show_paths", SHOW_PATHS_LIST); + stash_group_add_integer(group, &interface_prefs.openfiles_path_mode, "openfiles_path_mode", -1);
Since we rename the existing setting, users' Document List will be reset to the default. I think this is OK in this case as it is quickly changed again. Still, we could add an item to the `NEWS` file so it won't be forgotten and we can add it to the highlight items in the release announcement.
@@ -468,7 +468,7 @@ gdouble utils_scale_round(gdouble val, gdouble factor)
/* like g_utf8_strdown() but if @str is not valid UTF8, convert it from locale first.
Since `utf8_strdown()` is now public, it should be renamed to `utils_utf8_strdown()` or maybe better `utils_str_utf8_strdown()`.
@@ -68,7 +68,7 @@
"filetype: %f " \ "scope: %S")
-GeanyInterfacePrefs interface_prefs; +GEANY_EXPORT_SYMBOL GeanyInterfacePrefs interface_prefs;
Why is this necessary?
return cmp; }
+GEANY_EXPORT_SYMBOL
Why is this necessary? AFAIU `sidebar_create_store_openfiles()` could be even `static`.
}
/* Also sets doc->priv->iter. * This is called recursively in sidebar_openfiles_update_all(). */ +GEANY_EXPORT_SYMBOL
Why is `GEANY_EXPORT_SYMBOL` necessary here?
@kugel- commented on this pull request.
@@ -1105,8 +1591,6 @@ void sidebar_init(void)
G_CALLBACK(sidebar_tabs_show_hide), NULL); g_signal_connect_after(main_widgets.sidebar_notebook, "switch-page", G_CALLBACK(on_sidebar_switch_page), NULL); - - sidebar_tabs_show_hide(GTK_NOTEBOOK(main_widgets.sidebar_notebook), NULL, 0, NULL);
yea
@kugel- commented on this pull request.
@@ -0,0 +1,33 @@
+dnl GEANY_CHECK_GTK +dnl Checks whether the GTK stack is available and new enough. Sets GTK_CFLAGS and GTK_LIBS. +AC_DEFUN([GEANY_CHECK_GTK],
I can do a separate PR if you want. The new `GEANY_CHECK_GTK_FUNCS` is only required for the unit test though
@kugel- commented on this pull request.
@@ -187,7 +187,7 @@ static void init_pref_groups(void)
stash_group_add_toggle_button(group, &file_prefs.tab_close_switch_to_mru, "tab_close_switch_to_mru", FALSE, "check_tab_close_switch_to_mru"); stash_group_add_integer(group, &interface_prefs.tab_pos_sidebar, "tab_pos_sidebar", GTK_POS_TOP); - stash_group_add_integer(group, &interface_prefs.documents_show_paths, "documents_show_paths", SHOW_PATHS_LIST); + stash_group_add_integer(group, &interface_prefs.openfiles_path_mode, "openfiles_path_mode", -1);
I agree. Should I do this as part of this PR?
@kugel- commented on this pull request.
@@ -468,7 +468,7 @@ gdouble utils_scale_round(gdouble val, gdouble factor)
/* like g_utf8_strdown() but if @str is not valid UTF8, convert it from locale first.
OK
@kugel- commented on this pull request.
@@ -68,7 +68,7 @@
"filetype: %f " \ "scope: %S")
-GeanyInterfacePrefs interface_prefs; +GEANY_EXPORT_SYMBOL GeanyInterfacePrefs interface_prefs;
The unit test tests each of the modes, thus needs to modify `interface_prefs.openfiles_path_mode`
@kugel- commented on this pull request.
return cmp; }
+GEANY_EXPORT_SYMBOL
Again, for the unit test
@kugel- commented on this pull request.
}
/* Also sets doc->priv->iter. * This is called recursively in sidebar_openfiles_update_all(). */ +GEANY_EXPORT_SYMBOL
Again, for the unit test
@kugel- commented on this pull request.
@@ -1018,20 +1018,12 @@ static const gchar *get_locale(void)
GEANY_EXPORT_SYMBOL -gint main_lib(gint argc, gchar **argv) +void main_init_headless(void)
This and other changes are necessary for the unit test that I added (2474712b08cb15127a5f61e5caf5e91d6b54582c)
@eht16 commented on this pull request.
@@ -1018,20 +1018,12 @@ static const gchar *get_locale(void)
GEANY_EXPORT_SYMBOL -gint main_lib(gint argc, gchar **argv) +void main_init_headless(void)
Ah, I see.
@eht16 commented on this pull request.
@@ -187,7 +187,7 @@ static void init_pref_groups(void)
stash_group_add_toggle_button(group, &file_prefs.tab_close_switch_to_mru, "tab_close_switch_to_mru", FALSE, "check_tab_close_switch_to_mru"); stash_group_add_integer(group, &interface_prefs.tab_pos_sidebar, "tab_pos_sidebar", GTK_POS_TOP); - stash_group_add_integer(group, &interface_prefs.documents_show_paths, "documents_show_paths", SHOW_PATHS_LIST); + stash_group_add_integer(group, &interface_prefs.openfiles_path_mode, "openfiles_path_mode", -1);
I think so, yes. It's just an item in the `NEWS` file.
@eht16 commented on this pull request.
@@ -68,7 +68,7 @@
"filetype: %f " \ "scope: %S")
-GeanyInterfacePrefs interface_prefs; +GEANY_EXPORT_SYMBOL GeanyInterfacePrefs interface_prefs;
Ah, I see.
@eht16 commented on this pull request.
@@ -0,0 +1,33 @@
+dnl GEANY_CHECK_GTK +dnl Checks whether the GTK stack is available and new enough. Sets GTK_CFLAGS and GTK_LIBS. +AC_DEFUN([GEANY_CHECK_GTK],
Maybe it's also OK then here.
@kugel- pushed 2 commits.
025e22f8cba9c86ab8e26a3c44b294e81df1e427 fixup! Add (and default to) tree view of open documents in the sidebar c4bc6eaed6fe6b30329e290bb8231268cc39f7f8 Add item to NEWS for tree view of open documents
LGTM now! Thanks.
Great, thanks! I'll merge in a few days then. @elextr do you plan more tests and/or review or are you fine with this now as well?
@kugel- Intending to test this weekend.
@kugel- its better, ~ handling seems more correct, successfully recombines ~ and child when reduced from two children to one.
But folding saving doesn't seem to be right yet.
Initial state
``` v ~ v old_geany/geany/src document.c
rpggeany/geany/src // note this is folded
v /data/misc parser.cpp ```
close and re-open Geany gives
```
~ // now whole of ~ is folded
v /data/misc parser.cpp ```
set back to same initial state and close and re-open, Geany opens with everything unfolded and from then on always seemed to re-open with everything unfolded, no matter what I did.
Thanks for testing. I'll check.
Just leaving the note here that folding state is not stored to disk anywhere so re-opening Geany begins anew.
Just leaving the note here that folding state is not stored to disk anywhere so re-opening Geany begins anew.
Ok, so opening all unfolded is fine then, so the only issue is the ~ opening folded one time. Since it didn't repeat the few times I tried maybe its an uninitialised variable that is mostly the correct value, but occasionally bad.
@kugel- pushed 1 commit.
833c2c54bd6f7f60dbfbfc223db3691d2f8321db fixup! Add (and default to) tree view of open documents in the sidebar
I made a minor change that should fix startup (the expectation is that everything is unfolded on startup). Please check again. I couldn't reproduce your second example even without the last correction.
Ok, now seems consistent, can't cause any bad behaviour, always opens unfolded, ~ coalescing seems to work. :-)
@kugel- pushed 3 commits.
d41812ffa621b53956df6eab7327cf1f17d50414 Revert "fixup! Add (and default to) tree view of open documents in the sidebar" 965840d785b2fc31f2306961e54211a046757a14 Revert "Revert "fixup! Add (and default to) tree view of open documents in the sidebar"" b4adf7e1f591b60817792e84048f00ff5324bc45 fixup! Add (and default to) tree view of open documents in the sidebar
I could reproduce the your second example (all under `~` starts up folded). Depends on the order on which documents are opened. I could fix that with another minor tweak.
So if nothing comes up last minute I will merge by the weekend or so.
Depends on the order on which documents are opened. I could fix that with another minor tweak.
Would have been useful to detail the reproducer for testers.
Anyway I found an order that reliably reproduced the problem and your latest push seems to fix it.
So if nothing comes up last minute I will merge by the weekend or so.
As I have said before, its important to allow a whole weekend before merging, I just happened to have time today, but many people only have weekends to try anything.
Merging after CI is done. The last push was just a squash of the fixup (as well as splitting the unit test commit into two). No diff to the previous state.
Merged #1813 into master.
github-comments@lists.geany.org