Add toolbar button, popup menu item, and keybinding to open file manager with folder of currently opened document or selected item in sidebar. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/1126
-- Commit Summary --
* <a href="https://github.com/geany/geany-plugins/pull/1126/commits/8a37d0a7dcfe16a6ed81694188e6e17dc806a1f2">Project Organizer: Add option to open current folder in file manager</a>
-- File Changes --
M projectorganizer/src/prjorg-menu.c (4) M projectorganizer/src/prjorg-sidebar.c (68) M projectorganizer/src/prjorg-sidebar.h (1)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/1126.patch https://github.com/geany/geany-plugins/pull/1126.diff
@techee commented on this pull request.
- }
+ + if (!gtk_tree_model_iter_has_child(model, &iter)) + { + if (gtk_tree_model_iter_parent(model, &parent, &iter)) + utf8_path = build_path(&parent); + else + utf8_path = build_path(NULL); + } + else + utf8_path = build_path(&iter); + + locale_path = utils_get_locale_from_utf8(utf8_path); + if (locale_path) + { + command = g_strconcat ("xdg-open "", locale_path, """, NULL);
This probably doesn't work on macOS or Windows. Isn't there any GTK function for this that is multi-platform?
@xiota commented on this pull request.
- }
+ + if (!gtk_tree_model_iter_has_child(model, &iter)) + { + if (gtk_tree_model_iter_parent(model, &parent, &iter)) + utf8_path = build_path(&parent); + else + utf8_path = build_path(NULL); + } + else + utf8_path = build_path(&iter); + + locale_path = utils_get_locale_from_utf8(utf8_path); + if (locale_path) + { + command = g_strconcat ("xdg-open "", locale_path, """, NULL);
On Windows, the equivalent of `xdg-open` is `start`. I don't know about MacOS. I'll search around.
I think Geany has some utility functions for running external programs if `system()` is not portable.
@techee commented on this pull request.
- }
+ + if (!gtk_tree_model_iter_has_child(model, &iter)) + { + if (gtk_tree_model_iter_parent(model, &parent, &iter)) + utf8_path = build_path(&parent); + else + utf8_path = build_path(NULL); + } + else + utf8_path = build_path(&iter); + + locale_path = utils_get_locale_from_utf8(utf8_path); + if (locale_path) + { + command = g_strconcat ("xdg-open "", locale_path, """, NULL);
On macOS it is `open`. So yeah, you could add ifdefs for the individual platforms.
@xiota pushed 1 commit.
e659f8735a20ed9c094e4c8d8e2ee9548c6b0698 add ifdefs to select open-folder program based on platform
@xiota commented on this pull request.
- }
+ + if (!gtk_tree_model_iter_has_child(model, &iter)) + { + if (gtk_tree_model_iter_parent(model, &parent, &iter)) + utf8_path = build_path(&parent); + else + utf8_path = build_path(NULL); + } + else + utf8_path = build_path(&iter); + + locale_path = utils_get_locale_from_utf8(utf8_path); + if (locale_path) + { + command = g_strconcat ("xdg-open "", locale_path, """, NULL);
I added ifdefs to change the command based on platform. I don't have a Mac or Windows to test.
Left the `system()` command alone because it's supposed to be standard C. Geany has [`spawn_async()`](https://www.geany.org/manual/reference/spawn_8h.html), but it looks more complicated to use.
@xiota pushed 1 commit.
5635662280f117843a02e564fc00fdd84c8c52b7 print error to status window when unable to open folder
@xiota pushed 1 commit.
5c8952d0f39c3130d47998112137738d5b8ff982 Add option to preferences to configure folder open command
@techee I added an option so the user can change the open folder command in case the defaults (start for windows, open for mac, otherwise xdg-open) aren't to the user's liking. I'm thinking about adding options to open the file directly (eg, glade files) and to open a terminal (the geany vte doesn't work well for me). Should I add those to this PR or wait and add them later?
@xiota pushed 1 commit.
0b8d506cfdf6a6670651ecbeb8dc4ebc03f93137 Add ability to run terminal and open files with external programs
So far I just looked at the functionality and not the code much yet so just a few observations based on what these patches do:
1. I like the "open directory" and "run terminal" features (maybe I'd just prefer "open terminal" so it's clear that when you invoke it on some script, it won't run the script), but I'm not so sure about the "open externally" feature. The "open externally" feature could potentially execute some code from e.g. scripts and I'd like to avoid such things from inside the plugin, especially when it can happen by accidentally selecting a wrong row in the menu. 2. The configuration option (especially for the terminal) is probably useful, it would just be better to have this as a plugin configuration (the one you specify in Edit->Plugin Preferences or in the plugin manager) and not a per-project configuration option - you want to set this once for all your projects and not on a per-project basis. The plugin currently doesn't use this configuration window - if you want to implement it, you can check some other plugins how they do it. 3. I'd prefer having the toolbar in the sidebar simple and having these new features only accessible from the popup menu. 4. I'd prefer moving these new popup menu items to the very bottom (below "Delete", above "hide sidebar", separated by a separator above and below)
1. I added open externally to make it easier to open the `geany.glade` file. What actually happens depends on what command the user specifies. I don't know what `xdg-open` does with scripts. I'll check when I'm at a real computer.
2. I agree the commands would be better as general plugin configs, but project organizer doesn't currently have a general config. Adding it would have been too much effort, especially if the feature ends up being rejected.
3. I can remove the toolbar buttons. Maybe "add external folder" can also be removed from the toolbar since it's probably infrequently used. (The only button I use much is the reload button.)
4. Why this preference? While I don't necessarily object to moving them, next to "Delete" is not a good place because of the potential for accidental data loss, even when the Trash is used. "Open" menu items are also almost universally placed at the beginning of menus.
@xiota pushed 1 commit.
35962b1589f9c1b3cd2080f12e8d416035fb3464 Project Organizer: Add ability to open file manager and terminal
Combined commits and made the following changes:
1. Removed open externally. Renamed options to "File Manager" and "Terminal". 2. Will leave creating a general configuration for another PR, since it potentially affects another PR. 3. Removed the folder and terminal toolbar buttons. 4. Moved "File Manager" and "Terminal" options lower in the menu, but away from "Delete".
@xiota just a warning, every time you force push you are likely to break the copy others are using for testing your changes, which is likely to annoy them.
Just normal push should work almost all the time. If you think you need to force push, think what you might have done wrong.
There is no need for PRs to branch from git HEAD, and nice github will flag any conflicts between your changes and changes in the main branch that happened after you branched.
PS The travis failure isn't you, geany_lua is broken and that stops the build. Probably the build should be fixed one day to continue to other plugins even if one fails.
@elextr
just a warning, every time you force push you are likely to break the copy others are using for testing your changes, which is likely to annoy them.
I force pushed here because @techee asked on another PR for me to combine commits. So I assumed that was his preference generally. How would I do that without a force push?
For Geany, a couple were because of conflicts that would need to be resolved manually. The rest were just because it's fun to rebase. I'll refrain from any more of those.
The travis failure isn't you, geany_lua is broken and that stops the build. Probably the build should be fixed one day to continue to other plugins even if one fails.
#1123 fixes the build issue. @Skif-off Seems to think it's prudent to wait for geany/geany#2930 (Scintilla 5.1.3), but in that thread there's talk of waiting for 5.1.4.
If there are no new data types, the changes are auto-generated. So if it's holding other things back, the only real downside to just merging and regenerating the file again later is another PR. If scintilla only add symbols, without removing any, the next update could be delayed further with the only consequence being that new messages can't be used.
@xiota it wasn't a comment about this particular rebase, just that the fact you had been doing a lot of them bubbled up my consciousness at this point so I mentioned it whilst I remembered.
I'll refrain from any more of those.
Yeah those are the ones I mean.
waiting for 5.1.4.
5.1.4 came out a day or two ago, it just needs someone to have time to update [geany/geany/#2930](https://github.com/geany/geany/pull/2930) (or make a new one). And 5.1.4 fixes bugs found in Geany so we want it.
@techee requested changes on this pull request.
- static gchar *open_command = NULL;
+ + if (!open_command) + open_command = g_strdup(PRJORG_COMMAND_TERMINAL); + + if (prj_org) + { + g_free(open_command); + open_command = g_strdup(prj_org->command_terminal); + } + + locale_path = get_folder_for_selection(); + if (!spawn_async(locale_path, open_command, NULL, NULL, NULL, NULL)) + msgwin_status_add("Unable open terminal: %s", open_command); + + if (locale_path)
g_free() works on NULL pointers, the if is unnecessary here
+}
+ + +void on_open_terminal(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data) +{ + gchar *locale_path = NULL; + static gchar *open_command = NULL; + + if (!open_command) + open_command = g_strdup(PRJORG_COMMAND_TERMINAL); + + if (prj_org) + { + g_free(open_command); + open_command = g_strdup(prj_org->command_terminal); + }
Personally I'd just avoid the static variable and would do ``` open_command = prj_org ? g_strdup(prj_org->command_terminal) : g_strdup(PRJORG_COMMAND_TERMINAL); ``` and would free this at the end of the function.
+}
+ + +void on_open_file_manager(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data) +{ + gchar *locale_path = NULL; + static gchar *open_command = NULL; + + if (!open_command) + open_command = g_strdup(PRJORG_COMMAND_OPEN); + + if (prj_org) + { + g_free(open_command); + open_command = g_strdup(prj_org->command_file_manager); + }
like in on_open_terminal(), avoid the static and extra allocation.
}
+ } + + /* Try home folder */ + if (!locale_path) + locale_path = g_strdup(g_get_home_dir()); + + /* Make sure path exists */ + if(locale_path && !g_file_test(locale_path, G_FILE_TEST_IS_DIR)) + { + g_free(locale_path); + locale_path = NULL; + } + + return locale_path; +}
I think both get_fullpath_for_selection() and get_folder_for_selection() are an overkill. I think you could just simply get the path like in on_delete() and then test whether the resulting path is a file or directory, remove the file part if it's a file and use the resulting path to open file manager or terminal. I don't think any super-strict checks are necessary here - in the worst case the terminal or the file manager get opened at a wrong path.
@@ -1653,6 +1847,20 @@ void prjorg_sidebar_init(void)
gtk_widget_show(item); gtk_container_add(GTK_CONTAINER(s_popup_menu.widget), item);
+ item = menu_item_new("folder-open", _("File Manager"));
The name should be an action, so better to call it "Open File Manager".
@@ -1653,6 +1847,20 @@ void prjorg_sidebar_init(void)
gtk_widget_show(item); gtk_container_add(GTK_CONTAINER(s_popup_menu.widget), item);
+ item = menu_item_new("folder-open", _("File Manager")); + gtk_container_add(GTK_CONTAINER(s_popup_menu.widget), item); + g_signal_connect((gpointer) item, "activate", G_CALLBACK(on_open_file_manager), NULL); + s_popup_menu.find_in_directory = item; + + item = menu_item_new("terminal", _("Terminal"));
"Open Terminal"
- ui_entry_add_clear_icon(GTK_ENTRY(e->command_file_manager));
+ gtk_widget_set_tooltip_text(e->command_file_manager, + _("The command used to open folders in the file manager.")); + gtk_entry_set_text(GTK_ENTRY(e->command_file_manager), prj_org->command_file_manager); + + label = gtk_label_new(_("Terminal:")); + gtk_misc_set_alignment(GTK_MISC(label), 0, 0); + e->command_terminal = gtk_entry_new(); + ui_table_add_row(GTK_TABLE(table), 2, label, e->command_terminal, NULL); + ui_entry_add_clear_icon(GTK_ENTRY(e->command_terminal)); + gtk_widget_set_tooltip_text(e->command_terminal, + _("The terminal emulator.")); + gtk_entry_set_text(GTK_ENTRY(e->command_terminal), prj_org->command_terminal); + + gtk_box_pack_start(GTK_BOX(vbox), table, FALSE, FALSE, 6); +
I'd really like this to be a plugin configuration instead of a project configuration or not configurable at all. If you don't want to make it part of this pull request, I'd actually prefer if you could remove the configuration code from this pull request.
+}
+ + +void on_open_terminal(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data) +{ + gchar *locale_path = NULL; + static gchar *open_command = NULL; + + if (!open_command) + open_command = g_strdup(PRJORG_COMMAND_TERMINAL); + + if (prj_org) + { + g_free(open_command); + open_command = g_strdup(prj_org->command_terminal); + }
In fact, the g_strdup() seems to be unnecessary so it could be just ``` open_command = prj_org ?prj_org->command_terminal : PRJORG_COMMAND_TERMINAL; ```
Combined commits and made the following changes: Removed open externally. Renamed options to "File Manager" and "Terminal".
Better call it "Open File Manager" and "Open Terminal".
Will leave creating a general configuration for another PR, since it potentially affects another PR.
I was going to suggest that myself. But since I really don't like having this configuration inside the project configuration, I'd prefer if the configurability of these commands could be removed completely for now.
Removed the folder and terminal toolbar buttons.
Good.
Moved "File Manager" and "Terminal" options lower in the menu, but away from "Delete".
Good point about the Delete, I just wanted to have the "find" commands at the top of the menu.
Check the review comments for some more details.
@xiota pushed 1 commit.
d10bdaff25c1cab97099dd99d109bec43201e8eb remove config, use x-terminal when available
@techee I've made the changes you've requested where applicable. Please let me know if there are any other changes or if you'd like the commits squashed.
* removed the null check before g_free * kept g_strdup because it's needed to prevent segfault on g_free * combined get_fullpath and get_folder * added "open" to the menu item labels * removed config
Also, added use of `x-terminal` when available. When this is used, basename from the real_name is used instead of the symlink because of an issue that affects only some terminals (related to PWD detection).
@techee requested changes on this pull request.
@@ -300,6 +300,181 @@ static void on_follow_active(GtkToggleToolButton *button, G_GNUC_UNUSED gpointer
}
+/* returns parent folder of path from get_full_path_for_selection() + * if unable to find parent folder, returns home folder + * otherwise, returns NULL + * return path is in locale encoding */ +static gchar *get_folder_for_selection(void)
Once again, I don't understand why this function has so many tests and is so complicated. Basically, when you right-click some item, there should be a selection; if there's no selection, fall back just to whatever, e.g. the project base path. Basically, just call `parent_dir_for_create()` (maybe could be renamed to something different as it would be used here now) - use the return value when not NULL, otherwise fall back to project base path.
@@ -0,0 +1,72 @@
+/*
What is this file? Was it committed by mistake?
+ +void on_open_terminal(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data) +{ + gchar *locale_path, *open_command; + + if(g_file_test(PRJORG_COMMAND_TERMINAL_ALT, G_FILE_TEST_EXISTS)) + { + gchar *alt_command; + alt_command = utils_get_real_path(PRJORG_COMMAND_TERMINAL_ALT); + open_command = g_path_get_basename(alt_command); + g_free(alt_command); + } + else + { + /* g_strdup is needed here to prevent segfault on g_free */
This comment isn't really necessary.
+ g_free(command); + g_free(locale_path); + } + else + { + msgwin_status_add(_("Unable to find folder.")); + } +} + + +void on_open_terminal(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data) +{ + gchar *locale_path, *open_command; + + if(g_file_test(PRJORG_COMMAND_TERMINAL_ALT, G_FILE_TEST_EXISTS))
Space after 'if'.
- if(locale_path && !g_file_test(locale_path, G_FILE_TEST_IS_DIR))
+ { + g_free(locale_path); + locale_path = NULL; + } + + return locale_path; +} + + +void on_open_file_manager(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data) +{ + gchar *locale_path; + gchar *open_command; + + open_command = PRJORG_COMMAND_OPEN;
Can be moved inside the `if` below and the variable can be `const`.
@xiota commented on this pull request.
@@ -0,0 +1,72 @@
+/*
yes, a mistake. will remove.
@xiota commented on this pull request.
+ +void on_open_terminal(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data) +{ + gchar *locale_path, *open_command; + + if(g_file_test(PRJORG_COMMAND_TERMINAL_ALT, G_FILE_TEST_EXISTS)) + { + gchar *alt_command; + alt_command = utils_get_real_path(PRJORG_COMMAND_TERMINAL_ALT); + open_command = g_path_get_basename(alt_command); + g_free(alt_command); + } + else + { + /* g_strdup is needed here to prevent segfault on g_free */
Will remove.
@xiota commented on this pull request.
+ g_free(command); + g_free(locale_path); + } + else + { + msgwin_status_add(_("Unable to find folder.")); + } +} + + +void on_open_terminal(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data) +{ + gchar *locale_path, *open_command; + + if(g_file_test(PRJORG_COMMAND_TERMINAL_ALT, G_FILE_TEST_EXISTS))
Thanks. I'm used to just letting `clang-format` clean up code.
@xiota commented on this pull request.
@@ -300,6 +300,181 @@ static void on_follow_active(GtkToggleToolButton *button, G_GNUC_UNUSED gpointer
}
+/* returns parent folder of path from get_full_path_for_selection() + * if unable to find parent folder, returns home folder + * otherwise, returns NULL + * return path is in locale encoding */ +static gchar *get_folder_for_selection(void)
I didn't know about `parent_dir_for_create()`. Renaming to `get_dir_of_selection()`. Changing `get_folder_for_selection()` to `get_fallback_dir_of_selection()`.
@xiota pushed 1 commit.
555692da5b5bd6243ae7e6b52a518a21e2fecf7d Project Organizer: Add ability to open file manager and terminal
@techee I've made the requested changes. Please let me know if there is anything else or if I've forgotten something.
@techee requested changes on this pull request.
}
+ } + return path; +} + + +/* if get_dir_of_selection() fails, + * returns parent of current document, project folder, or home folder */ +static gchar *get_fallback_dir_of_selection(void) +{ + gchar *locale_path; + + /* get path from treeview selection */ + locale_path = get_dir_of_selection(); + + /* get path from current document */
I still don't understand why there are so many fallbacks tested - the purpose of the added functionality is to open terminal/file browser at the path from the sidebar - if the path for some reason doesn't exist (probably the directory was deleted and project not refreshed), the functionality basically cannot be performed. One option would be not opening the terminal at all which would be weird for the user though because he expects something happens. The other option is to open the terminal at some path we know that exists (but which is "wrong" because it doesn't correspond to the path in the sidebar). There's no "better" or "worse" path in this case - yes, e.g. project base dir or document dir are closer to what the user wanted but they aren't what the user wanted anyway. It's true that the base directory may also not exist so I'd just perform a single fallback to the home directory that you have at the very end of this function.
@xiota commented on this pull request.
}
+ } + return path; +} + + +/* if get_dir_of_selection() fails, + * returns parent of current document, project folder, or home folder */ +static gchar *get_fallback_dir_of_selection(void) +{ + gchar *locale_path; + + /* get path from treeview selection */ + locale_path = get_dir_of_selection(); + + /* get path from current document */
they aren't what the user wanted anyway
There is a keybinding to open the terminal, so the treeview doesn't necessarily have a selection when the terminal is opened. While I have been using this, most of the time, it's the path to the document that I want. The exception is if something is selected in the treeview. New documents don't have an assigned path, so the fallback is the project path. If the project path is invalid (which could happen when the project folder is moved without updating preferences) the user home folder is used.
the purpose of the added functionality is to open terminal/file browser at the path from the sidebar...
@techee The selection of a fallback path is necessary because of the keybinding. The treeview doesn't necessarily have a selection when the file manager or terminal is opened.
The purpose of the feature is convenience. Forcing the user to take corrective action to avoid a few checks that take only nanoseconds is not convenient.
@techee The selection of a fallback path is necessary because of the keybinding. The treeview doesn't necessarily have a selection when the file manager or terminal is opened.
Oh yes, that makes sense now, I forgot about the keybinding possibility - the checks are definitely useful in this case.
The pull request looks good to me now. Thanks for your contributions, I think both of the features you implemented are nice additions to the plugin!
@techee approved this pull request.
@techee Thank you for the reviews and helping me improve the PRs. I will wait for the current ones to be merged before working on a general config. My current thoughts are that it may include:
* Configure default options for the current per-project settings. This could be used to add git and build junk that aren't in the hard-coded defaults. * Configure file manager and terminal commands
It could be implemented by modifying the current function(s) to take a flag that indicates whether it is working on general or project-specific preferences.
@techee Thank you for the reviews and helping me improve the PRs. I will wait for the current ones to be merged before working on a general config. My current thoughts are that it may include:
- Configure default options for the current per-project settings. This could be used to add git and build junk that aren't in the hard-coded defaults.
- Configure file manager and terminal commands
It could be implemented by modifying the current function(s) to take a flag that indicates whether it is working on general or project-specific preferences.
Yeah, sounds good to me (I'd just skip adding the "Various" preferences, I think there don't have to be defaults for these).
@xiota pushed 1 commit.
9517c5c26535092a720d5f1bfbadc87035b5ac69 Project Organizer: Add ability to open file manager and terminal
@techee It's been brought to my attention that whenever I think "home", I really should be thinking PWD... (On my computer, PWD ends up being home when Geany is launched from shortcut.) So I've changed `g_get_home_dir()` to `g_get_current_dir()`. Thanks for your understanding.
What does `g_get_current_dir()` return when you don't launch Geany using terminal but from some menu/launcher of your desktop? Is it something sensible?
On my computer, when I launch from a launcher, `g_get_current_dir()` gives me the home directory. When the current directory is invalid (eg, deleted), it goes to `/`... so maybe bring back `g_get_home_dir()` as the final fallback?
On my computer, when I launch from a launcher, g_get_current_dir() gives me the home directory. When the current directory is invalid (eg, deleted), it goes to /
Sounds good to me.
so maybe bring back g_get_home_dir() as the final fallback?
I think this is just fine, in such cases it doesn't return the "right" directory anyway.
@xiota pushed 1 commit.
6cd488ae7a872e0812c19257488dcb24f598ec20 Project Organizer: Add ability to open file manager and terminal
Fixed incorrect comment and added home as final fallback (it was bugging me even though it covers a rare scenario).
@xiota pushed 1 commit.
7e86828684c07622f2c2387db1785c73a2b04c50 Project Organizer: Add ability to open file manager and terminal
Force push to rebase on master and remove some extraneous files from unrelated plugins that were accidentally added to the commit.
Merged #1126 into master.
github-comments@lists.geany.org