[Github-comments] [geany/geany-plugins] Workbench (#598)
Colomban Wendling
notifications at xxxxx
Fri Aug 18 05:38:22 UTC 2017
b4n requested changes on this pull request.
Here are some comment after reviewing the diff (no actual testing of the plugin or compilation on my end yet).
Apart from those, you've got conflicts on the translation files. Basically you shouldn't update translations you don't actually change manually (e.g. don't merely commit the result of `make update-po`), because it leads to many conflicts for no real benefit. For the translations you actually did update it's trickier, but you could probably isolate the part you actually added from the automated result of make `update-po`.
> @@ -0,0 +1,20 @@
+icondir = $(datadir)/icons/hicolor/16x16/apps
You might wanna install other size of icons, especially in the wake of HiDPI screens and awareness. Just a suggestion though.
> +/** Shows the dialog "Create new workbench".
+ *
+ * The dialog lets the user create a new workbench file (filter *.geanywb).
+ *
+ * @return The filename
+ *
+ **/
+gchar *dialogs_create_new_workbench(void)
+{
+ gchar *utf8_filename = NULL;
+ GtkWidget *dialog;
+
+ dialog = gtk_file_chooser_dialog_new(_("Create new workbench"),
+ GTK_WINDOW(wb_globals.geany_plugin->geany_data->main_widgets->window), GTK_FILE_CHOOSER_ACTION_SAVE,
+ _("_Cancel"), GTK_RESPONSE_CANCEL,
+ _("Create"), GTK_RESPONSE_ACCEPT, NULL);
Having mnemonics (the `_` marker) on buttons is very handy for everyone, and especially disabled people that rely mostly on keyboard navigation. I don't know if there's a common one for *Create*, but `r` sounds like a first reasonable choice to me (so `_("C_reate")`)
> +gchar *dialogs_create_new_workbench(void)
+{
+ gchar *utf8_filename = NULL;
+ GtkWidget *dialog;
+
+ dialog = gtk_file_chooser_dialog_new(_("Create new workbench"),
+ GTK_WINDOW(wb_globals.geany_plugin->geany_data->main_widgets->window), GTK_FILE_CHOOSER_ACTION_SAVE,
+ _("_Cancel"), GTK_RESPONSE_CANCEL,
+ _("Create"), GTK_RESPONSE_ACCEPT, NULL);
+ gtk_file_chooser_set_current_name(GTK_FILE_CHOOSER(dialog), "new.geanywb");
+ gtk_file_chooser_set_do_overwrite_confirmation (GTK_FILE_CHOOSER(dialog), TRUE);
+
+ if (gtk_dialog_run(GTK_DIALOG(dialog)) == GTK_RESPONSE_ACCEPT)
+ {
+ gchar *locale_filename = gtk_file_chooser_get_filename(GTK_FILE_CHOOSER(dialog));
+ utf8_filename = utils_get_utf8_from_locale(locale_filename);
It will probably work just fine, but usually you would prefer not convert encoding, and especially filename encoding, unless absolutely necessary. The reason is that you cannot be 100% sure the filename is neither valid characters, nor a fixed encoding you can go back to (e.g. on Unices GLib/GTK use the raw string used by the OS, which is basically arbitrary bytes, and the encoding can be different from one filesystem po the next).
So if you can afford it, only ever convert it to UTF-8 when you actually have to display it to the user, and otherwise keep the plain "data" string you got from the lowest level.
BTW, "locale" naming isn't quite correct even, because GLib has a special "encoding" for those filename as said, which is raw data on Unices and UTF-8 on Windows (but you shouldn't really have to worry about that in your own code most of the time, just treating it as arbitrary data is fine). GLib uses the term "filename encoding".
Anyway, to summarize my point, don't do conversion on filenames but for display purposes. Keep the most unmodified version of it you can around and use that.
> + gtk_widget_destroy(dialog);
+
+ return utf8_filename;
+}
+
+
+/** Shows the dialog "Open workbench".
+ *
+ * The dialog lets the user choose an existing workbench file (filter *.geanywb).
+ *
+ * @return The filename
+ *
+ **/
+gchar *dialogs_open_workbench(void)
+{
+ gchar *utf8_filename = NULL;
odd indentation
> + *
+ * The dialog lets the user choose an existing workbench file (filter *.geanywb).
+ *
+ * @return The filename
+ *
+ **/
+gchar *dialogs_open_workbench(void)
+{
+ gchar *utf8_filename = NULL;
+ GtkWidget *dialog;
+ GtkFileFilter *filter;
+
+ dialog = gtk_file_chooser_dialog_new(_("Open workbench"),
+ GTK_WINDOW(wb_globals.geany_plugin->geany_data->main_widgets->window), GTK_FILE_CHOOSER_ACTION_OPEN,
+ _("_Cancel"), GTK_RESPONSE_CANCEL,
+ _("Open"), GTK_RESPONSE_ACCEPT, NULL);
Mnemonic on the "O"
> + *
+ * @return The filename
+ *
+ **/
+gchar *dialogs_open_workbench(void)
+{
+ gchar *utf8_filename = NULL;
+ GtkWidget *dialog;
+ GtkFileFilter *filter;
+
+ dialog = gtk_file_chooser_dialog_new(_("Open workbench"),
+ GTK_WINDOW(wb_globals.geany_plugin->geany_data->main_widgets->window), GTK_FILE_CHOOSER_ACTION_OPEN,
+ _("_Cancel"), GTK_RESPONSE_CANCEL,
+ _("Open"), GTK_RESPONSE_ACCEPT, NULL);
+
+ filter = gtk_file_filter_new ();
nitpick: you've got mixed style for the parentheses here, with and without a space between the function name and the opening parenthesis.
> + _("_Cancel"), GTK_RESPONSE_CANCEL,
+ _("Open"), GTK_RESPONSE_ACCEPT, NULL);
+
+ filter = gtk_file_filter_new ();
+ gtk_file_filter_set_name (filter, _("Workbench files (.geanywb)"));
+ gtk_file_filter_add_pattern (filter, "*.geanywb");
+ gtk_file_chooser_add_filter (GTK_FILE_CHOOSER(dialog), filter);
+ filter = gtk_file_filter_new ();
+ gtk_file_filter_set_name (filter, _("All Files"));
+ gtk_file_filter_add_pattern (filter, "*");
+ gtk_file_chooser_add_filter (GTK_FILE_CHOOSER(dialog), filter);
+
+ if (gtk_dialog_run(GTK_DIALOG(dialog)) == GTK_RESPONSE_ACCEPT)
+ {
+ gchar *locale_filename = gtk_file_chooser_get_filename(GTK_FILE_CHOOSER(dialog));
+ utf8_filename = utils_get_utf8_from_locale(locale_filename);
Same thing about converting filenames. I'll stop here and let you see whether you would like to take action or not.
> +
+ return utf8_filename;
+}
+
+
+/** Shows the dialog "Add project".
+ *
+ * The dialog lets the user choose an existing project file
+ * (filter *.geany or *).
+ *
+ * @return The filename
+ *
+ **/
+gchar *dialogs_add_project(void)
+{
+ gchar *utf8_filename = NULL;
odd indentation again
> + * The dialog lets the user choose an existing project file
+ * (filter *.geany or *).
+ *
+ * @return The filename
+ *
+ **/
+gchar *dialogs_add_project(void)
+{
+ gchar *utf8_filename = NULL;
+ GtkWidget *dialog;
+ GtkFileFilter *filter;
+
+ dialog = gtk_file_chooser_dialog_new(_("Add project"),
+ GTK_WINDOW(wb_globals.geany_plugin->geany_data->main_widgets->window), GTK_FILE_CHOOSER_ACTION_OPEN,
+ _("_Cancel"), GTK_RESPONSE_CANCEL,
+ _("Add"), GTK_RESPONSE_ACCEPT, NULL);
Mnemonic on the "A"
> +/** Shows the dialog "Add directory".
+ *
+ * The dialog lets the user choose an existing folder.
+ *
+ * @return The filename
+ *
+ **/
+gchar *dialogs_add_directory(WB_PROJECT *project)
+{
+ gchar *utf8_filename = NULL;
+ GtkWidget *dialog;
+
+ dialog = gtk_file_chooser_dialog_new(_("Add directory"),
+ GTK_WINDOW(wb_globals.geany_plugin->geany_data->main_widgets->window), GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER,
+ _("_Cancel"), GTK_RESPONSE_CANCEL,
+ _("Add"), GTK_RESPONSE_ACCEPT, NULL);
here too
> + GtkWidget *dialog;
+
+ dialog = gtk_file_chooser_dialog_new(_("Add directory"),
+ GTK_WINDOW(wb_globals.geany_plugin->geany_data->main_widgets->window), GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER,
+ _("_Cancel"), GTK_RESPONSE_CANCEL,
+ _("Add"), GTK_RESPONSE_ACCEPT, NULL);
+ if (project != NULL)
+ {
+ gchar *path;
+
+ /* Set the current folder to the location of the project file */
+ path = wb_project_get_filename(project);
+ if (path != NULL)
+ {
+ path = g_path_get_dirname(path);
+ gtk_file_chooser_set_current_folder(GTK_FILE_CHOOSER(dialog), path);
Both [`g_path_get_dirname()`](https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-path-get-dirname) and [`gtk_file_chooser_set_current_folder()`](https://developer.gnome.org/gtk3/stable/GtkFileChooser.html#gtk-file-chooser-set-current-folder) take a [GLib-filename-encoded](https://developer.gnome.org/glib/stable/glib-Character-Set-Conversion.html#g-filename-to-utf8) string. Does `wb_project_get_filename()` return this?
> + **/
+gboolean dialogs_directory_settings(WB_PROJECT_DIR *directory)
+{
+ GtkWidget *w_file_patterns, *w_ignored_dirs_patterns, *w_ignored_file_patterns;
+ GtkWidget *dialog, *label, *content_area;
+ GtkWidget *vbox, *hbox, *hbox1, *table;
+ GtkDialogFlags flags;
+ gchar *file_patterns_old, *ignored_file_patterns_old, *ignored_dirs_patterns_old;
+ gboolean changed;
+
+ /* Create the widgets */
+ flags = GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT;
+ dialog = gtk_dialog_new_with_buttons (_("Directory settings"),
+ GTK_WINDOW(wb_globals.geany_plugin->geany_data->main_widgets->window),
+ flags,
+ GTK_STOCK_CANCEL,
You're using stock items here, but not on various other dialogs, is there a reason for either of those?
I do like stock items myself because for several reasons, but they have been deprecated in GTK3. Either way, you might like to be consistent on this.
> + sidebar_update(SIDEBAR_CONTEXT_WB_CLOSED, NULL);
+}
+
+
+/** Setup the workbench menu.
+ *
+ **/
+gboolean menu_init(void)
+{
+ /* Create menu and root item/label */
+ menu_data.menu = gtk_menu_new();
+ menu_data.root_item = gtk_menu_item_new_with_label(_("Workbench"));
+ gtk_widget_show(menu_data.root_item);
+
+ /* Create new menu item "New Workbench" */
+ menu_data.item_new = gtk_menu_item_new_with_mnemonic(_("New"));
Would be useful to have mnemonics in the menu too (e.g. `"_New"`, `"_Open"`, `"_Save"`, etc.)
> + *
+ **/
+gboolean menu_init(void)
+{
+ /* Create menu and root item/label */
+ menu_data.menu = gtk_menu_new();
+ menu_data.root_item = gtk_menu_item_new_with_label(_("Workbench"));
+ gtk_widget_show(menu_data.root_item);
+
+ /* Create new menu item "New Workbench" */
+ menu_data.item_new = gtk_menu_item_new_with_mnemonic(_("New"));
+ gtk_widget_show(menu_data.item_new);
+ gtk_menu_append(GTK_MENU (menu_data.menu), menu_data.item_new);
+ g_signal_connect(menu_data.item_new, "activate",
+ G_CALLBACK(item_new_workbench_activate_cb), NULL);
+ geany_plugin_set_data(wb_globals.geany_plugin, menu_data.item_new, NULL);
Hum, there's something fishy here. [`geany_plugin_set_data()`](http://www.geany.org/manual/reference/plugindata_8h.html#a143ba8805bd049f3fb13037fae0eb30a) is supposed to be called only once by a plugin. And I'm really not sure what this is supposed to achieve here anyway?
> +
+ /* Create new menu item "New Workbench" */
+ menu_data.item_new = gtk_menu_item_new_with_mnemonic(_("New"));
+ gtk_widget_show(menu_data.item_new);
+ gtk_menu_append(GTK_MENU (menu_data.menu), menu_data.item_new);
+ g_signal_connect(menu_data.item_new, "activate",
+ G_CALLBACK(item_new_workbench_activate_cb), NULL);
+ geany_plugin_set_data(wb_globals.geany_plugin, menu_data.item_new, NULL);
+
+ /* Create new menu item "Open Workbench" */
+ menu_data.item_open = gtk_menu_item_new_with_mnemonic(_("Open"));
+ gtk_widget_show(menu_data.item_open);
+ gtk_menu_append(GTK_MENU (menu_data.menu), menu_data.item_open);
+ g_signal_connect(menu_data.item_open, "activate",
+ G_CALLBACK(item_open_workbench_activate_cb), NULL);
+ geany_plugin_set_data(wb_globals.geany_plugin, menu_data.item_open, NULL);
And here, etc.
> + sidebar_update(SIDEBAR_CONTEXT_WB_CLOSED, NULL);
+}
+
+
+/** Setup the workbench menu.
+ *
+ **/
+gboolean menu_init(void)
+{
+ /* Create menu and root item/label */
+ menu_data.menu = gtk_menu_new();
+ menu_data.root_item = gtk_menu_item_new_with_label(_("Workbench"));
+ gtk_widget_show(menu_data.root_item);
+
+ /* Create new menu item "New Workbench" */
+ menu_data.item_new = gtk_menu_item_new_with_mnemonic(_("New"));
Also, most interface guidelines tell you to add an ellipsis at the end of a menu item label that opens a dialog (to suggest it won't perform an action right away but leads to more steps). Geany follows this in its own UI, so it would also be better for integration.
> + G_CALLBACK(item_workbench_settings_activate_cb), NULL);
+ geany_plugin_set_data(wb_globals.geany_plugin, menu_data.item_settings, NULL);
+
+ /* Create new menu item "Close Workbench" */
+ menu_data.item_close = gtk_menu_item_new_with_mnemonic(_("Close"));
+ gtk_widget_show(menu_data.item_close);
+ gtk_menu_append(GTK_MENU (menu_data.menu), menu_data.item_close);
+ g_signal_connect(menu_data.item_close, "activate",
+ G_CALLBACK(item_close_workbench_activate_cb), NULL);
+ geany_plugin_set_data(wb_globals.geany_plugin, menu_data.item_close, NULL);
+
+ /* Add our menu to the main window (left of the help menu) */
+ gtk_menu_item_set_submenu(GTK_MENU_ITEM(menu_data.root_item), menu_data.menu);
+ gtk_menu_shell_insert
+ (GTK_MENU_SHELL (ui_lookup_widget(wb_globals.geany_plugin->geany_data->main_widgets->window, "menubar1")),
+ menu_data.root_item, 8);
I have no real good suggestion here, but inserting at a fixed position is unlikely to play well with future changes and other plugins doing similar things.
> +/** Cleanup menu data/mem.
+ *
+ **/
+void menu_cleanup (void)
+{
+ gtk_container_remove (GTK_CONTAINER(ui_lookup_widget(wb_globals.geany_plugin->geany_data->main_widgets->window, "menubar1")),
+ menu_data.root_item);
+}
+
+
+/** Activate menu item "New".
+ *
+ **/
+void menu_item_new_activate (void)
+{
+ gtk_widget_set_sensitive(menu_data.item_new, TRUE);
Your call, but these flow of functions just wrapping one call seem weird to me. At least I'd have a single function for each menu item that has a boolean flag for activation/deactivation, rather than 2 separate functions. But that's a pure question of style.
> + GError *error = NULL;
+
+ filename = dialogs_create_new_workbench();
+ if (filename == NULL)
+ {
+ return;
+ }
+ wb_globals.opened_wb = workbench_new();
+ workbench_set_filename(wb_globals.opened_wb, filename);
+ if (workbench_save(wb_globals.opened_wb, &error))
+ {
+ menu_item_new_deactivate();
+ menu_item_open_deactivate();
+ menu_item_save_activate();
+ menu_item_settings_activate();
+ menu_item_close_activate();
This looks hard to maintain; couldn't you have a more generic wrapper for doing these kind of things? I would guess there is only so many combinations of menu items sensitivity that make sense you could have the generic states wrapped up in a helper.
> + gtk_menu_item_set_submenu(GTK_MENU_ITEM(menu_data.root_item), menu_data.menu);
+ gtk_menu_shell_insert
+ (GTK_MENU_SHELL (ui_lookup_widget(wb_globals.geany_plugin->geany_data->main_widgets->window, "menubar1")),
+ menu_data.root_item, 8);
+
+ return TRUE;
+}
+
+
+/** Cleanup menu data/mem.
+ *
+ **/
+void menu_cleanup (void)
+{
+ gtk_container_remove (GTK_CONTAINER(ui_lookup_widget(wb_globals.geany_plugin->geany_data->main_widgets->window, "menubar1")),
+ menu_data.root_item);
Couldn't you simply `gtk_widget_destroy(menu_data.root_item)`? Should work just fine and be a lot simpler to maintain.
> +/* Cleanup plugin */
+static void plugin_workbench_cleanup(GeanyPlugin *plugin, gpointer pdata)
+{
+ menu_cleanup();
+ sidebar_cleanup();
+}
+
+
+/* Show help */
+static void plugin_workbench_help (GeanyPlugin *plugin, gpointer pdata)
+{
+ utils_open_browser("http://plugins.geany.org/workbench.html");
+}
+
+
+PluginCallback plugin_workbench_callbacks[] = {
should be `static` if I'm not mistaken.
> + /* Set metadata */
+ plugin->info->name = _("Workbench");
+ plugin->info->description = _("Manage and customize multiple projects.");
+ plugin->info->version = "1.0";
+ plugin->info->author = "LarsGit223";
+
+ /* Set functions */
+ plugin->funcs->init = plugin_workbench_init;
+ plugin->funcs->cleanup = plugin_workbench_cleanup;
+ plugin->funcs->help = plugin_workbench_help;
+ plugin->funcs->callbacks = plugin_workbench_callbacks;
+
+ /* Register! */
+ if (GEANY_PLUGIN_REGISTER(plugin, 225))
+ {
+ return;
Cute code path :)
> + wb_project_add_directory(project, dirname);
+ sidebar_update(SIDEBAR_CONTEXT_DIRECTORY_ADDED, &context);
+ g_free(dirname);
+ }
+ }
+}
+
+
+/* Handle popup menu item "Remove directory" */
+static void popup_menu_on_remove_directory(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data)
+{
+ SIDEBAR_CONTEXT context;
+
+ if (sidebar_file_view_get_selected_context(&context)
+ &&
+ context.project != NULL && context.directory != NULL)
Nitpick: line splitting style is kind of odd here IMO.
> + wb_project_remove_bookmark(context.project, context.prj_bookmark);
+ sidebar_update(SIDEBAR_CONTEXT_PRJ_BOOKMARK_REMOVED, &context);
+ }
+}
+
+
+/** Setup/Initialize the popup menu.
+ *
+ **/
+void popup_menu_init(void)
+{
+ GtkWidget *item;
+
+ s_popup_menu.widget = gtk_menu_new();
+
+ item = gtk_menu_item_new_with_mnemonic(_("Add project"));
Mnemonics, and ellipsis where appropriate
> +}
+
+
+/** Setup/Initialize the popup menu.
+ *
+ **/
+void popup_menu_init(void)
+{
+ GtkWidget *item;
+
+ s_popup_menu.widget = gtk_menu_new();
+
+ item = gtk_menu_item_new_with_mnemonic(_("Add project"));
+ gtk_widget_show(item);
+ gtk_container_add(GTK_CONTAINER(s_popup_menu.widget), item);
+ g_signal_connect((gpointer) item, "activate", G_CALLBACK(popup_menu_on_add_project), NULL);
cast to `gpointer` is not required/useful here in C
> +
+ foreach_slist (elem, lst)
+ {
+ gchar **path_split;
+
+ path_split = g_strsplit_set(elem->data, "/\\", 0);
+ path_list = g_slist_prepend(path_list, path_split);
+ }
+
+ if (path_list != NULL)
+ sidebar_create_branch(0, abs_base_dir, path_list, parent);
+
+ g_slist_foreach(lst, (GFunc) g_free, NULL);
+ g_slist_free(lst);
+ g_slist_foreach(path_list, (GFunc) g_strfreev, NULL);
+ g_slist_free(path_list);
there is [`g_slist_free_full()`](https://developer.gnome.org/glib/stable/glib-Singly-Linked-Lists.html#g-slist-free-full) if you like
> + }
+
+ dirs = wb_project_get_directories(project);
+ if (dirs != NULL)
+ {
+ icon = g_icon_new_for_string("workbench-dir", NULL);
+
+ foreach_slist (elem, dirs)
+ {
+ gtk_tree_store_insert_with_values(sidebar.file_store, &iter, parent, *position,
+ FILEVIEW_COLUMN_ICON, icon,
+ FILEVIEW_COLUMN_NAME, wb_project_dir_get_name(elem->data),
+ FILEVIEW_COLUMN_DATA_ID, DATA_ID_DIRECTORY,
+ FILEVIEW_COLUMN_ASSIGNED_DATA_POINTER, elem->data,
+ -1);
+ position++;
this is **WRONG**, it increments the pointer, not the value pointed to. Thus next iterations will try and dereference a highly likely invalid pointer, and result is undefined at best (given you seem to pass as a pointer to an integer as the `position` parameter rather than an array of integers).
I guess you mean `(*position)++`?
> + FILEVIEW_COLUMN_ASSIGNED_DATA_POINTER, elem->data,
+ -1);
+ position++;
+ sidebar_insert_project_directory(project, elem->data, &iter);
+ }
+ }
+ else
+ {
+ icon = g_icon_new_for_string("workbench-nodirs", NULL);
+
+ gtk_tree_store_insert_with_values(sidebar.file_store, &iter, parent, *position,
+ FILEVIEW_COLUMN_ICON, icon,
+ FILEVIEW_COLUMN_NAME, _("No directories"),
+ FILEVIEW_COLUMN_DATA_ID, DATA_ID_NO_DIRS,
+ -1);
+ position++;
ditto
> + position++;
+ sidebar_insert_project_directory(project, elem->data, &iter);
+ }
+ }
+ else
+ {
+ icon = g_icon_new_for_string("workbench-nodirs", NULL);
+
+ gtk_tree_store_insert_with_values(sidebar.file_store, &iter, parent, *position,
+ FILEVIEW_COLUMN_ICON, icon,
+ FILEVIEW_COLUMN_NAME, _("No directories"),
+ FILEVIEW_COLUMN_DATA_ID, DATA_ID_NO_DIRS,
+ -1);
+ position++;
+ }
+ g_object_unref(icon);
This should be guarded in theory because [`g_icon_new_for_string()`](https://developer.gnome.org/gio/stable/GIcon.html#g-icon-new-for-string) can return `NULL`.
> + icon = g_icon_new_for_string("workbench-bookmark", NULL);
+ for (index = 0 ; index < max ; index++)
+ {
+ gchar *file, *name;
+
+ file = wb_project_get_bookmark_at_index(project, index);
+ name = get_any_relative_path(wb_project_get_filename(project), file);
+ gtk_tree_store_insert_with_values(sidebar.file_store, &iter, parent, *position,
+ FILEVIEW_COLUMN_ICON, icon,
+ FILEVIEW_COLUMN_NAME, name,
+ FILEVIEW_COLUMN_DATA_ID, DATA_ID_PRJ_BOOKMARK,
+ FILEVIEW_COLUMN_ASSIGNED_DATA_POINTER, file,
+ -1);
+ (*position)++;
+ }
+ g_object_unref(icon);
ditto
> + GtkTreeIter iter;
+
+ if (wb_globals.opened_wb == NULL)
+ return;
+
+ if (sidebar_get_project_iter(project, &iter))
+ {
+ gint length;
+ gchar text[200];
+
+ length = g_snprintf(text, sizeof(text), "%s", wb_project_get_name(project));
+ if (length < (sizeof(text)-1) && wb_project_is_modified(project))
+ {
+ text [length] = '*';
+ text [length+1] = '\0';
+ }
this looks like awfully more complex and less reliable code than the naive, more costly but cheap enough version using a dynamically allocated string, and even e.g. [GString](https://developer.gnome.org/glib/stable/glib-Strings.html):
```c
GString *name = g_string_new(wb_project_get_name(project));
if (wb_project_is_modified(project))
g_string_append_c(name, '*');
/* use name->str */
g_string_free(name, TRUE);
```
> + return;
+
+ if (sidebar_get_project_iter(project, &iter))
+ {
+ gint length, position;
+ gchar text[200];
+
+ length = g_snprintf(text, sizeof(text), "%s", wb_project_get_name(project));
+ if (length < (sizeof(text)-1) && wb_project_is_modified(project))
+ {
+ text [length] = '*';
+ text [length+1] = '\0';
+ }
+ gtk_tree_store_set(sidebar.file_store, &iter,
+ FILEVIEW_COLUMN_NAME, text,
+ -1);
same, and looks like most of the code could be shared with the previous function
> + ==
+ PROJECT_ENTRY_STATUS_OK)
+ {
+ icon = icon_ok;
+ }
+ else
+ {
+ icon = icon_ko;
+ }
+
+ length = g_snprintf(text, sizeof(text), "%s", wb_project_get_name(project));
+ if (length < (sizeof(text)-1) && wb_project_is_modified(project))
+ {
+ text [length] = '*';
+ text [length+1] = '\0';
+ }
ditto
> + FILEVIEW_COLUMN_ICON, icon,
+ FILEVIEW_COLUMN_NAME, text,
+ FILEVIEW_COLUMN_DATA_ID, DATA_ID_PROJECT,
+ FILEVIEW_COLUMN_ASSIGNED_DATA_POINTER, project,
+ -1);
+
+ child_position = 0;
+ /* Not required here as we build a completely new tree
+ sidebar_remove_children(&iter); */
+ sidebar_insert_project_bookmarks(project, iter, &child_position);
+ sidebar_insert_project_directories(project, iter, &child_position);
+ }
+ gtk_tree_view_expand_all(GTK_TREE_VIEW(sidebar.file_view));
+
+ g_object_unref(icon_ok);
+ g_object_unref(icon_ko);
same theoretical need for guarding
> + sidebar_insert_project_directories(project, &iter, &position);
+ }
+}
+
+
+/* Insert all projects into the sidebar file tree */
+static void sidebar_insert_all_projects(GtkTreeIter *iter, gint *position)
+{
+ GIcon *icon_ok, *icon_ko, *icon;
+ guint index, max;
+
+ if (wb_globals.opened_wb == NULL)
+ return;
+
+ icon_ok = g_icon_new_for_string("workbench-project", NULL);
+ icon_ko = g_icon_new_for_string("workbench-project-error", NULL);
suggestion: using so close variable names is likely to create confusion, even though it looks like it makes sense it could be easily mistaken one for the other when not paying enough attention
> + sidebar_show_intro_message(_("Create or open a workbench\nusing the workbench menu."), FALSE);
+ sidebar_deactivate();
+ }
+ else
+ {
+ gint length;
+ gchar text[200];
+
+ count = workbench_get_project_count(wb_globals.opened_wb);
+ length = g_snprintf(text, sizeof(text), "%s: %u %s",
+ workbench_get_name(wb_globals.opened_wb), count, _("Projects"));
+ if (length < (sizeof(text)-1) && workbench_is_modified(wb_globals.opened_wb))
+ {
+ text [length] = '*';
+ text [length+1] = '\0';
+ }
this too could benefit from GString
> +
+ if (wb_globals.opened_wb == NULL)
+ {
+ gtk_label_set_text (GTK_LABEL(sidebar.file_view_label), _("No workbench opened."));
+ gtk_tree_store_clear(sidebar.file_store);
+ sidebar_show_intro_message(_("Create or open a workbench\nusing the workbench menu."), FALSE);
+ sidebar_deactivate();
+ }
+ else
+ {
+ gint length;
+ gchar text[200];
+
+ count = workbench_get_project_count(wb_globals.opened_wb);
+ length = g_snprintf(text, sizeof(text), "%s: %u %s",
+ workbench_get_name(wb_globals.opened_wb), count, _("Projects"));
the format string should be translatable (e.g. in French you'd have a space before the colon), and the "Project" string should then likely be in it directly
> + case DATA_ID_PROJECT:
+ info = wb_project_get_info((WB_PROJECT *)address);
+ if (workbench_get_project_status_by_address(wb_globals.opened_wb, address)
+ ==
+ PROJECT_ENTRY_STATUS_OK)
+ {
+ dialogs_show_msgbox(GTK_MESSAGE_INFO, "%s", info);
+ }
+ else
+ {
+ dialogs_show_msgbox(GTK_MESSAGE_ERROR, _("%s\nProject file not found!"), info);
+ }
+ g_free(info);
+ break;
+ case DATA_ID_DIRECTORY:
+ info = wb_project_dir_get_info((WB_PROJECT_DIR *)address);
weird indentation
> + if (context.prj_bookmark != NULL)
+ {
+ popup_context = POPUP_CONTEXT_PRJ_BOOKMARK;
+ }
+ if (context.directory != NULL)
+ {
+ popup_context = POPUP_CONTEXT_DIRECTORY;
+ }
+ if (context.folder != NULL)
+ {
+ popup_context = POPUP_CONTEXT_FOLDER;
+ }
+ if (context.file != NULL)
+ {
+ popup_context = POPUP_CONTEXT_FILE;
+ }
it is kind of weird to have these list of `if`s that override one another. If it's the goal, I would rather inverse the order and make theme `else if`s so the indent is clearer, and the code more efficient at the same time.
> + basedir_end--;
+ }
+ if (*basedir_end == G_DIR_SEPARATOR)
+ {
+ *basedir_end = '\0';
+ }
+ else
+ {
+ break;
+ }
+ goback--;
+ }
+
+ length = strlen(basedir)+strlen(start);
+ result = g_new(char, length+1);
+ if (result == NULL)
[`g_new()`](https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-new) will never return on allocation failure, so the check is not very useful (it can only return `NULL` if you try and allocate 0 bytes, which you don't).
Also, you could use [`g_strconcat(basedir, start, NULL)`](https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strconcat) to simplify the code.
> + * string with g_free().
+ *
+ * @param base The absolute path.
+ * @param relative The relative path.
+ * @return Combined path or NULL if no memory is available
+ *
+ **/
+gchar *get_combined_path(const gchar *base, const gchar *relative)
+{
+ gchar *basedir, *basedir_end;
+ const gchar *start;
+ gchar *result;
+ guint length;
+ gint goback;
+
+ if (relative[0] != '.')
`foo/bar` is technically a relative path, maybe you should add it to the description of the function if it requires relative paths to start with `./`
> + * @param relative The relative path.
+ * @return Combined path or NULL if no memory is available
+ *
+ **/
+gchar *get_combined_path(const gchar *base, const gchar *relative)
+{
+ gchar *basedir, *basedir_end;
+ const gchar *start;
+ gchar *result;
+ guint length;
+ gint goback;
+
+ if (relative[0] != '.')
+ {
+ /* Not a relative directory. Simply return it. */
+ return strdup(relative);
you should use [`g_strdup()`](https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup) which is more portable and uses GLib's allocator and so you're sure the memory returned by your function is always valid for [`g_free()`](https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-free)
> + return NULL;
+ }
+ g_snprintf(result, length+1, "%s%s", basedir, start);
+
+ return result;
+}
+
+
+/** Get the relative path.
+ *
+ * The function examines the relative path of @a target in relation to
+ * @a base. It is not required that target is a sub-directory of base.
+ *
+ * The function can only properly handle pathes which consist of a maximum of
+ * 29 sub-directories. If the pathes exceed this maximum the function aborts
+ * and NULL is returned.
It looks like a weird limitation, but why not. Though, if you're just limiting because of a static array, you could consider using [`GPtrArray`](https://developer.gnome.org/glib/stable/glib-Pointer-Arrays.html)
> + if (strlen(splitv_base[index]) > 0)
+ {
+ equal++;
+ equal_index = index;
+ }
+ }
+ else
+ {
+ break;
+ }
+ index++;
+ }
+
+ pos = 0;
+ length = 0;
+ relative = g_new (char *, 30);
looks like `relative` is leaked. And if you're using a small fixed-size array anyway, why not use one on the stack?
> + * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include "wb_globals.h"
+
+struct S_WB_GLOBALS
+{
+ GeanyPlugin *geany_plugin;
+ WORKBENCH *opened_wb;
+};
this struct seems unused (the one from the header is used instead)
> +
+/** Set the filename of a project.
+ *
+ * @param prj The project
+ * @param filename The filename
+ *
+ **/
+void wb_project_set_filename(WB_PROJECT *prj, gchar *filename)
+{
+ if (prj != NULL)
+ {
+ guint offset;
+ gchar *ext;
+
+ prj->filename = g_strdup(filename);
+ prj->name = g_path_get_basename (filename);
shouldn't the previous values of those 2 be freed first?
> +{
+ if (prj != NULL)
+ {
+ return prj->filename;
+ }
+ return NULL;
+}
+
+
+/** Get the name of a project.
+ *
+ * @param prj The project
+ * @return The project name
+ *
+ **/
+gchar *wb_project_get_name(WB_PROJECT *prj)
ditto
> + /* Strip of file extension by overwriting
+ '.' with string terminator. */
+ prj->name[offset] = '\0';
+ }
+ }
+ }
+}
+
+
+/** Get the filename of a project.
+ *
+ * @param prj The project
+ * @return The filename
+ *
+ **/
+gchar *wb_project_get_filename(WB_PROJECT *prj)
I would return a `const` value if you don't mean callers to modify the value directly in place
> +
+/* Clear idle queue */
+static void wb_project_clear_idle_queue(GSList **queue)
+{
+ GSList *elem;
+
+ if (queue == NULL || *queue == NULL)
+ {
+ return;
+ }
+
+ foreach_slist(elem, *queue)
+ {
+ g_free(elem->data);
+ }
+ g_slist_free(*queue);
possible use for [`g_slist_free_full(*queue, g_free)`](https://developer.gnome.org/glib/stable/glib-Singly-Linked-Lists.html#g-slist-free-full)
> +
+/* Get the list of files for root */
+static GSList *wb_project_dir_get_file_list(WB_PROJECT_DIR *root, const gchar *utf8_path, GSList *patterns,
+ GSList *ignored_dirs_patterns, GSList *ignored_file_patterns, GHashTable *visited_paths)
+{
+ GSList *list = NULL;
+ GDir *dir;
+ gchar *locale_path = utils_get_locale_from_utf8(utf8_path);
+ gchar *real_path = tm_get_real_path(locale_path);
+
+ dir = g_dir_open(locale_path, 0, NULL);
+ if (!dir || !real_path || g_hash_table_lookup(visited_paths, real_path))
+ {
+ g_free(locale_path);
+ g_free(real_path);
+ return NULL;
`dir` is possibly leaked
> + g_free(locale_path);
+
+ return list;
+}
+
+
+/* Create a new project dir with base path "utf8_base_dir" */
+static WB_PROJECT_DIR *wb_project_dir_new(const gchar *utf8_base_dir)
+{
+ guint offset;
+
+ if (utf8_base_dir == NULL)
+ {
+ return NULL;
+ }
+ WB_PROJECT_DIR *dir = (WB_PROJECT_DIR *) g_new0(WB_PROJECT_DIR, 1);
cast is not useful in C
> +static void wb_project_dir_collect_source_files(gchar *filename, TMSourceFile *sf, gpointer user_data)
+{
+ GPtrArray *array = user_data;
+
+ if (sf != NULL)
+ g_ptr_array_add(array, sf);
+}
+
+
+/** Get the name of a project dir.
+ *
+ * @param directory The project dir
+ * @return The name
+ *
+ **/
+gchar *wb_project_dir_get_name (WB_PROJECT_DIR *directory)
ditto about `const`ness of the return value
> + GSList *ignored_dirs_list = NULL;
+ GSList *ignored_file_list = NULL;
+ GHashTable *visited_paths;
+ GSList *lst;
+ GSList *elem;
+ guint filenum = 0;
+ gchar *searchdir;
+
+ wb_project_dir_remove_from_tm_workspace(root);
+ g_hash_table_remove_all(root->file_table);
+
+ if (!root->file_patterns || !root->file_patterns[0])
+ {
+ gchar **all_pattern = g_strsplit ("*", " ", -1);
+ pattern_list = get_precompiled_patterns(all_pattern);
+ g_strfreev(all_pattern);
any reason not to use a simple `gchar *all_patterns[] = { "*", NULL };` instead of splitting to a known result and freeing right after?
> + }
+ return FALSE;
+}
+
+
+/** Get an info string for the project dir.
+ *
+ * @param dir The project dir
+ * @return The info string
+ *
+ **/
+gchar *wb_project_dir_get_info (WB_PROJECT_DIR *dir)
+{
+ guint pos = 0;
+ gchar *str;
+ gchar text[2048];
Looks like [`GString`](https://developer.gnome.org/glib/stable/glib-Strings.html) could help
> + pos += g_snprintf(&(text[pos]), sizeof(text)-pos, _("Number of Files: %u\n"), dir->file_count);
+
+ return g_strdup(text);
+}
+
+
+/** Get an info string for the project.
+ *
+ * @param prj The project
+ * @return The info string
+ *
+ **/
+gchar *wb_project_get_info (WB_PROJECT *prj)
+{
+ guint pos = 0;
+ gchar text[2048];
ditto
> + contents = g_key_file_to_data (kf, &length, error);
+ g_key_file_free(kf);
+
+ /* Save to file */
+ success = g_file_set_contents (prj->filename, contents, length, error);
+ if (success)
+ {
+ prj->modified = FALSE;
+ }
+ g_free (contents);
+ }
+ else if (error != NULL)
+ {
+ g_set_error (error, 0, 0,
+ "Internal error: param missing (file: %s, line %d)",
+ __FILE__, __LINE__);
looks like it should rather be [`g_return_if_fail()`](https://developer.gnome.org/glib/stable/glib-Warnings-and-Assertions.html#g-return-if-fail)/[`g_warn_if_reached()`](https://developer.gnome.org/glib/stable/glib-Warnings-and-Assertions.html#g-warn-if-reached)
> + abs_path = get_combined_path(prj->filename, *file);
+ if (abs_path != NULL)
+ {
+ wb_project_add_bookmark_int(prj, abs_path);
+ g_free(abs_path);
+ }
+ file++;
+ }
+ g_strfreev(bookmarks_strings);
+ }
+
+ /* Load project dirs */
+ for (index = 1 ; index < 1025 ; index++)
+ {
+ g_snprintf(key, sizeof(key), "Dir%u-BaseDir", index);
+ if (!g_key_file_has_key (kf, "Workbench", key, NULL))
this isn't really useful as [`g_key_file_get_string()`](https://developer.gnome.org/glib/stable/glib-Key-value-file-parser.html#g-key-file-get-string) (below) returns `NULL` if the key is not found
> + splitv = g_strsplit (str, ";", -1);
+ wb_project_dir_set_ignored_file_patterns(new_dir, splitv);
+ }
+ g_free(str);
+ }
+ }
+
+ g_key_file_free(kf);
+ g_free (contents);
+ success = TRUE;
+ }
+ else if (error != NULL)
+ {
+ g_set_error (error, 0, 0,
+ "Internal error: param missing (file: %s, line %d)",
+ __FILE__, __LINE__);
Here again, it probably rather calls for a `g_return_val_if_fail(prf != NULL, FALSE)` at the start of the function. `GErrors` are supposed to carry runtime errors (like IO error, file missing, etc.), not programming errors (invalid parameters, etc.).
> +
+ return success;
+}
+
+
+/** Create a new empty project.
+ *
+ * @return Address of the new structure.
+ *
+ **/
+WB_PROJECT *wb_project_new(gchar *filename)
+{
+ WB_PROJECT *new_prj;
+
+ new_prj = g_new(WB_PROJECT, 1);
+ memset(new_prj, 0, sizeof(*new_prj));
You could use [`g_new0()`](https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-new0) (or my favorite `new_prj = g_malloc0(sizeof *new_prj)` which doesn't repeat the type, but that's another story 😄)
> +
+
+/** Free a project.
+ *
+ * @param prj Adress of structure to free
+ *
+ **/
+void wb_project_free(WB_PROJECT *prj)
+{
+ GSList *elem;
+
+ /* Free directories first */
+ foreach_slist(elem, prj->directories)
+ {
+ wb_project_dir_free(elem->data);
+ }
shouldn't you `g_slist_free(prj->directories)`?
> +{
+ if (wb != NULL)
+ {
+ return wb->rescan_projects_on_open;
+ }
+ return FALSE;
+}
+
+
+/** Set the filename.
+ *
+ * @param wb The workbench
+ * @param filename Name of the workbench file
+ *
+ **/
+void workbench_set_filename(WORKBENCH *wb, gchar *filename)
`filename` argument could be `const`
> + return entry->status;
+ }
+ }
+ }
+ return PROJECT_ENTRY_STATUS_UNKNOWN;
+}
+
+
+/** Add a project to the workbench.
+ *
+ * @param wb The workbench
+ * @param filename Project file
+ * @return TRUE on success, FALSE otherwise
+ *
+ **/
+gboolean workbench_add_project(WORKBENCH *wb, gchar *filename)
Same, `filename` could be `const`
> + }
+ }
+ }
+ return NULL;
+}
+
+
+/* Add a workbench bookmark */
+static gboolean workbench_add_bookmark_int(WORKBENCH *wb, gchar *filename)
+{
+ if (wb != NULL)
+ {
+ gchar *new;
+
+ new = g_strdup(filename);
+ if (new != NULL)
one more place where, if `filename` is not `NULL`, the test is not relevant as `g_strdup()` aborts on allocation failure
> + {
+ g_free (bookmarks_strings[index]);
+ }
+ g_free(bookmarks_strings);
+ }
+
+ /* Save projects data */
+ for (index = 0 ; index < wb->projects->len ; index++)
+ {
+ entry = g_ptr_array_index(wb->projects, index);
+ g_snprintf(group, sizeof(group), "Project-%u", (index+1));
+ g_key_file_set_string(kf, group, "AbsFilename", entry->abs_filename);
+ g_key_file_set_string(kf, group, "RelFilename", entry->rel_filename);
+ g_key_file_set_boolean(kf, group, "UseAbsFilename", entry->use_abs);
+ }
+ contents = g_key_file_to_data (kf, &length, error);
in the (unlikely) event `g_key_file_to_data()` fails, you should make sure to properly handle it, because it is invalid to set a `GError` on top of another, and you reuse `error` in the `g_file_set_contents()` call below.
> + check = g_key_file_get_string (kf, "General", "filetype", error);
+ if (check == NULL || g_strcmp0(check, "workbench") != 0)
+ {
+ valid = FALSE;
+ }
+ g_free(check);
+ }
+ else
+ {
+ valid = FALSE;
+ }
+
+ if (!valid)
+ {
+ g_set_error (error, 0, 0,
+ "File: %s is not a valid workbench file!",
probably worth translating?
--
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-plugins/pull/598#pullrequestreview-56801445
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20170818/b50319ac/attachment.html>
More information about the Github-comments
mailing list