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

[Enrico Tröger]

@eht16 requested changes on this pull request.

Looks good. Had only a few remarks.

> @@ -1105,8 +1591,6 @@ void sidebar_init(void)
 		G_CALLBACK(sidebar_tabs_show_hide), NULL);
 	g_signal_connect_after(main_widgets.sidebar_notebook, "switch-page",
 		G_CALLBACK(on_sidebar_switch_page), NULL);
-
-	sidebar_tabs_show_hide(GTK_NOTEBOOK(main_widgets.sidebar_notebook), NULL, 0, NULL);

Removed because `sidebar_tabs_show_hide()` is called in `on_load_settings()` already?

> @@ -1018,20 +1018,12 @@ static const gchar *get_locale(void)
 
 
 GEANY_EXPORT_SYMBOL
-gint main_lib(gint argc, gchar **argv)
+void main_init_headless(void)

While this sounds like a good idea, AFAIU this is completely unrelated in this PR?

> @@ -0,0 +1,33 @@
+dnl GEANY_CHECK_GTK
+dnl Checks whether the GTK stack is available and new enough. Sets GTK_CFLAGS and GTK_LIBS.
+AC_DEFUN([GEANY_CHECK_GTK],

Great to move those checks into here.

But pretty much unrelated to this PR, I think.
Wouldn't it be better to do such refactorings on its own?

> @@ -187,7 +187,7 @@ static void init_pref_groups(void)
 	stash_group_add_toggle_button(group, &file_prefs.tab_close_switch_to_mru,
 		"tab_close_switch_to_mru", FALSE, "check_tab_close_switch_to_mru");
 	stash_group_add_integer(group, &interface_prefs.tab_pos_sidebar, "tab_pos_sidebar", GTK_POS_TOP);
-	stash_group_add_integer(group, &interface_prefs.documents_show_paths, "documents_show_paths", SHOW_PATHS_LIST);
+	stash_group_add_integer(group, &interface_prefs.openfiles_path_mode, "openfiles_path_mode", -1);

Since we rename the existing setting, users' Document List will be reset to the default.
I think this is OK in this case as it is quickly changed again. Still, we could add an item to the `NEWS` file so it won't be forgotten and we can add it to the highlight items in the release announcement.

> @@ -468,7 +468,7 @@ gdouble utils_scale_round(gdouble val, gdouble factor)
 
 /* like g_utf8_strdown() but if @str is not valid UTF8, convert it from locale first.

Since `utf8_strdown()` is now public, it should be renamed to `utils_utf8_strdown()` or maybe better `utils_str_utf8_strdown()`.

> @@ -68,7 +68,7 @@
 	"filetype: %f      " \
 	"scope: %S")
 
-GeanyInterfacePrefs	interface_prefs;
+GEANY_EXPORT_SYMBOL GeanyInterfacePrefs interface_prefs;

Why is this necessary?

>  
 	return cmp;
 }
 
+GEANY_EXPORT_SYMBOL

Why is this necessary?
AFAIU `sidebar_create_store_openfiles()` could be even `static`.

>  }
 
 
 /* Also sets doc->priv->iter.
  * This is called recursively in sidebar_openfiles_update_all(). */
+GEANY_EXPORT_SYMBOL

Why is `GEANY_EXPORT_SYMBOL` necessary here?

-- 
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1813#pullrequestreview-980989213
You are receiving this because you are subscribed to this thread.

Message ID: <geany/geany/pull/1813/review/980989213 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20220522/c3fafbfd/attachment.htm>