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...) and [`gtk_file_chooser_set_current_folder()`](https://developer.gnome.org/gtk3/stable/GtkFileChooser.html#gtk-file-chooser...) take a [GLib-filename-encoded](https://developer.gnome.org/glib/stable/glib-Character-Set-Conversion.html#g...) 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#a143ba8805bd049f3fb...) 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-slis...) 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...) 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...) 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-slis...)

+/* 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-...)

• 			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-ke...) (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?

LarsGit223 commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

How do I do that? Just append more icon files to the makefile. How are the icons selected by the application? (The variable ```icondir``` doesn't seem to be used by the makefile???)

codebrainz commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

Automake uses the variable name, so `icondir` tells where to put the files in variable `icon_DATA` (or in the case here `dist_icon_DATA` - not sure what the `dist_` adds here since the stuff in `_DATA` should be distributed anyway).

Probably the best way to install multiple sizes is to do [like Geany does](https://github.com/geany/geany/tree/master/icons) and put them into separate subdirs for each size, and in the Makefile.am in each subdir, do like you did here.

b4n commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

Yeah, or use a single Makefile and the `nobase_` prefix, like:

```make iconsdir = $(datadir)/icons/hicolor nobase_dist_icons_DATA = \ 16x16/apps/workbench-project.png \ 32x32/apps/workbench-project.png \ scalable/apps/workbench-project.svg \ … ```

And of course have the different size in the corresponding folders.

not sure what the `dist_` adds here since the stuff in `_DATA` should be distributed anyway

No, [`_DATA` is not distributed by default](https://www.gnu.org/software/automake/manual/automake.html#index-_005fDATA), I guess because it's often generated out of templates and the like.

How are the icons selected by the application?

They are installed as part of the [icon theme](https://developer.gnome.org/gtk3/stable/GtkIconTheme.html#GtkIconTheme.descr...) so referencing them by name can automatically load them at the most appropriate size.

elextr commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

Makes sense, why would you want to distribute files that you've indicated need to be installed? </sarcasm>

Thats autofools for ya :stuck_out_tongue_winking_eye: , but it is documented behaviour for `_DATA`, installs but not dist unless its explicit.

LarsGit223 commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

Well, then I think I will go for the single makefile approach suggested by @b4n because it seems to be the simplest solution.

Another thing regarding icons: I noticed that there is no 16x16 icon for javascript files. That means if I have a javascript file listed in a workbench project directory, then it is bigger than the other icons. I assumed there would be references to icon files for each extension in geany but I did not find it. Is this coming from the OS? So in my case Ubuntu/Gnome. Should I open an issue there about the missing 16x16 icon for javascript?

elextr commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

The icons are obtained from GTK, so they are not in Geany, [see](https://github.com/geany/geany/blob/adc22a453b546b05f507fd6522ea13f3a92943c2...). And IIUC GTK gets them from the theme these days.

My Geany shows a JS icon in the documents tab for js files, so maybe its your theme thats deficient.

b4n requested changes on this pull request.

@@ -3405,7 +3405,7 @@ msgstr ""

msgid "Toggle Insert/Overwrite mode"

There is no relevant changes in this file

@@ -3344,7 +3344,7 @@ msgstr "Cancel·la la la Selecció"

msgid "Toggle Insert/Overwrite mode"

Nor in this one (and probably quite a few translation files)

+ +/** 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")); + gtk_widget_show(menu_data.item_new); + gtk_menu_append(GTK_MENU (menu_data.menu), menu_data.item_new);

[`gtk_menu_append()`](https://developer.gnome.org/gtk2/stable/GtkMenu.html#gtk-menu-append) is deprecated for a long time in favor to [`gtk_menu_shell_append()`](https://developer.gnome.org/gtk2/stable/GtkMenuShell.html#gtk-menu-shell-app...), and its use [breaks compilation under GTK3](https://travis-ci.org/geany/geany-plugins/jobs/266322308#L4962) which removed the former.

+} MatchType;

+ +typedef struct +{ + GeanyProject *project; + GPtrArray *expanded_paths; +} ExpandData; + +typedef struct SIDEBAR +{ + GtkWidget *file_view_vbox; + GtkWidget *file_view; + GtkTreeStore *file_store; + GtkWidget *file_view_label; +}SIDEBAR; +static SIDEBAR sidebar = {NULL, NULL, NULL, NULL, NULL};

[Excess initializer](https://travis-ci.org/geany/geany-plugins/jobs/266322308#L4946) (there are 4 fields, and you initialize 5).

• gtk_tree_view_column_pack_start(column, renderer, FALSE);

+ gtk_tree_view_column_add_attribute(column, renderer, "gicon", FILEVIEW_COLUMN_ICON); + + renderer = gtk_cell_renderer_text_new(); + gtk_tree_view_column_pack_start(column, renderer, TRUE); + gtk_tree_view_column_add_attribute(column, renderer, "markup", FILEVIEW_COLUMN_NAME); + + gtk_tree_view_append_column(GTK_TREE_VIEW(sidebar.file_view), column); + + gtk_tree_view_set_headers_visible(GTK_TREE_VIEW(sidebar.file_view), FALSE); + gtk_tree_view_set_enable_search(GTK_TREE_VIEW(sidebar.file_view), TRUE); + gtk_tree_view_set_search_column(GTK_TREE_VIEW(sidebar.file_view), FILEVIEW_COLUMN_NAME); + + pfd = pango_font_description_from_string(wb_globals.geany_plugin->geany_data->interface_prefs->tagbar_font); + gtk_widget_modify_font(sidebar.file_view, pfd); + pango_font_description_free(pfd);

you could use `ui_widget_modify_font_from_string()` from Geany's utility function (which also would avoid the [GTK3 deprecation warning](https://travis-ci.org/geany/geany-plugins/jobs/266322308#L4954))

LarsGit223 commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

I had a look again and changed my opinion: the directories in geany look clean and simple so I tried starting to do it that way, @codebrainz: thanks for the link.

But I can't get it to work. In ```workbench/icons/Makefile.am``` I have put: ``` SUBDIRS = 16x16 #24x24 32x32 48x48 scalable tango

gtk_update_icon_cache = gtk-update-icon-cache -f -t

install-data-hook: @-if test -z "$(DESTDIR)"; then \ echo "Updating Gtk icon cache."; \ $(gtk_update_icon_cache) "$(datadir)/icons/hicolor"; \ $(gtk_update_icon_cache) "$(datadir)/icons/Tango"; \ else \ echo "*** Icon cache not updated. Remember to run:"; \ echo "***"; \ echo "*** $(gtk_update_icon_cache) '$(datadir)/icons/hicolor'";\ echo "*** $(gtk_update_icon_cache) '$(datadir)/icons/Tango'";\ echo "***"; \ fi

uninstall-local: rm -f $(DESTDIR)$(datadir)/icons/hicolor/icon-theme.cache rm -f $(DESTDIR)$(datadir)/icons/Tango/icon-theme.cache ``` Right now I started with the original icons only, so only subdir ```16x16``` is used. In ```workbench/icons/16x16/Makefile.am``` I have put: ``` iconsdir = $(datadir)/icons/hicolor/16x16 icons_appsdir = $(iconsdir)/apps

dist_icons_apps_DATA = \ workbench-bookmark.png \ workbench-dir.png \ workbench-nodirs.png \ workbench-project.png \ workbench-project-error.png ``` But when I call ```make``` I get the following error: ``` Making all in src make[1]: Entering directory '/home/lars/DokuWiki/repos/gp-branch-workbench2/geany-plugins/workbench/src' make[1]: Nothing to be done for 'all'. make[1]: Leaving directory '/home/lars/DokuWiki/repos/gp-branch-workbench2/geany-plugins/workbench/src' Making all in icons make[1]: Entering directory '/home/lars/DokuWiki/repos/gp-branch-workbench2/geany-plugins/workbench/icons' cd ../.. && /bin/bash /home/lars/DokuWiki/repos/gp-branch-workbench2/geany-plugins/missing automake-1.15 --foreign workbench/icons/Makefile cd ../.. && /bin/bash ./config.status workbench/icons/Makefile config.status: creating workbench/icons/Makefile Making all in 16x16 make[2]: Entering directory '/home/lars/DokuWiki/repos/gp-branch-workbench2/geany-plugins/workbench/icons/16x16' make[2]: *** No rule to make target 'all'. Stop. make[2]: Leaving directory '/home/lars/DokuWiki/repos/gp-branch-workbench2/geany-plugins/workbench/icons/16x16' Makefile:509: recipe for target 'all-recursive' failed make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory '/home/lars/DokuWiki/repos/gp-branch-workbench2/geany-plugins/workbench/icons' Makefile:578: recipe for target 'all-recursive' failed make: *** [all-recursive] Error 1 ``` So everything looks fine until the point where directory ```16x16``` is entered. I know make but not automake and also not the details of the geany makefile. I do not see any target in the original geany 16x16/Makefile.am so I wonder why it's not working.

I need some help here please.

codebrainz commented on this pull request.

@@ -0,0 +1,10 @@

+AC_DEFUN([GP_CHECK_WORKBENCH], +[ + GP_ARG_DISABLE([Workbench], [auto]) + GP_COMMIT_PLUGIN_STATUS([Workbench]) + AC_CONFIG_FILES([ + workbench/Makefile + workbench/src/Makefile + workbench/icons/Makefile

Put the `16x16` Makefile here also, it gets the Makefile processed by Autoconf.

LarsGit223 commented on this pull request.

@@ -0,0 +1,10 @@

+AC_DEFUN([GP_CHECK_WORKBENCH], +[ + GP_ARG_DISABLE([Workbench], [auto]) + GP_COMMIT_PLUGIN_STATUS([Workbench]) + AC_CONFIG_FILES([ + workbench/Makefile + workbench/src/Makefile + workbench/icons/Makefile

Yes, that was the missing line. THANKS.

LarsGit223 commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

Yes, thanks. Works now. I added icons for 24x24, 32x32, 48x48 and scalable. The scalable icons are only symbolic which means they are painted in one color only (gray).

Now I got the problem the the icons for the first two levels (the project and the directories added to it) are much bigger than the icons for the folders and files contained below the directories. How can I control the size?

codebrainz commented on this pull request.

@@ -0,0 +1,20 @@

+icondir = $(datadir)/icons/hicolor/16x16/apps

I'm not sure I understand. If you mean the icons when loaded in to Geany, I believe the functions that load icons allow to specify the size (ex. `GTK_ICON_SIZE_MENU`, etc).

b4n requested changes on this pull request.

A few problems, with proposed fixes in https://github.com/b4n/geany-plugins/tree/LarsGit223/workbench%2Bfollow-up

@@ -0,0 +1,9 @@

+iconsdir = $(datadir)/icons/hicolor/16x16

wrong target directory, should be `24x24`

@@ -0,0 +1,9 @@

+iconsdir = $(datadir)/icons/hicolor/16x16

should be `32x32`

@@ -0,0 +1,9 @@

+iconsdir = $(datadir)/icons/hicolor/16x16

Should be `48x48`

@@ -0,0 +1,9 @@

+iconsdir = $(datadir)/icons/hicolor/16x16

should be `scalable`

• /**** tree view ****/

+ + sidebar.file_view = gtk_tree_view_new(); + g_signal_connect(sidebar.file_view, "row-activated", (GCallback)sidebar_filew_view_on_row_activated, NULL); + + sidebar.file_store = gtk_tree_store_new(FILEVIEW_N_COLUMNS, G_TYPE_ICON, G_TYPE_STRING, G_TYPE_UINT, G_TYPE_POINTER); + gtk_tree_view_set_model(GTK_TREE_VIEW(sidebar.file_view), GTK_TREE_MODEL(sidebar.file_store)); + + renderer = gtk_cell_renderer_pixbuf_new(); + column = gtk_tree_view_column_new(); + gtk_tree_view_column_pack_start(column, renderer, FALSE); + gtk_tree_view_column_add_attribute(column, renderer, "gicon", FILEVIEW_COLUMN_ICON); + + renderer = gtk_cell_renderer_text_new(); + gtk_tree_view_column_pack_start(column, renderer, TRUE); + gtk_tree_view_column_add_attribute(column, renderer, "markup", FILEVIEW_COLUMN_NAME);

you should use `"text"`" instead of `"markup"` as you pass raw filenames (and don't seem to need the markup features), otherwise it'll break with filenames containing `&`s, `<`s and the like.

+}

+ + +/* 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_dir_close(dir);

you should guard this call against `NULL` `dir`