<p><b>@LarsGit223</b> requested changes on this pull request.</p>
<p>I tested the PR with gtk2 and 3 and so far it seems to work in both versions. The gtk3 build did not throw any gtk deprecation warnings. The gtk2 build did, but that comes from the old code so is out of scope of this PR I think. Some might be fixed if my suggestions in <code>tpage.c</code> will be applied.</p>
<p>But some code could be improved, see my comments.</p><hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236079074">debugger/src/btnpanel.c</a>:</p>
<pre style='color:#555'>> @@ -82,17 +82,28 @@ static void on_execute_until(GtkButton *button, gpointer user_data)
*/
GtkWidget* btnpanel_create(on_toggle cb)
{
+#if GTK_CHECK_VERSION(3, 0, 0)
+ GtkWidget *vbox = gtk_box_new(GTK_ORIENTATION_VERTICAL, CP_BUTTONS_PAD);
+
</pre>
<p>I know it was there in the original code already but I think readability would be improved if this empty line would be removed. Also for the non gtk3 code below (line 91).</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236079347">debugger/src/cell_renderers/cellrendererbreakicon.c</a>:</p>
<pre style='color:#555'>> calc_width = (gint) cell->xpad * 2 + pixbuf_width;
calc_height = (gint) cell->ypad * 2 + pixbuf_height;
+#endif
</pre>
<p>According to the gtk documentation the function <code>gtk_cell_renderer_get_padding()</code> exists since gtk 2.18. As geany itself requires 2.24 or higher this code could be improved by using the gtk3 part only - for gtk2 and 3.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236079521">debugger/src/cell_renderers/cellrendererbreakicon.c</a>:</p>
<pre style='color:#555'>> calc_width = (gint) cell->xpad * 2 + pixbuf_width;
calc_height = (gint) cell->ypad * 2 + pixbuf_height;
+#endif
</pre>
<p>The same is true for function <code>gtk_cell_renderer_get_alignment()</code>.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236079626">debugger/src/cell_renderers/cellrendererbreakicon.c</a>:</p>
<pre style='color:#555'>>
cell_renderer_break_icon_get_size (cell, widget, cell_area,
&pix_rect.x,
&pix_rect.y,
&pix_rect.width,
&pix_rect.height);
+#if GTK_CHECK_VERSION(3, 0, 0)
+ gtk_cell_renderer_get_padding(cell, &xpad, &ypad);
+ pix_rect.x += cell_area->x + xpad;
+ pix_rect.y += cell_area->y + ypad;
</pre>
<p>The 3 lines above could be used for gtk version 2 and 3.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236081045">debugger/src/cell_renderers/cellrendererframeicon.c</a>:</p>
<pre style='color:#555'>> @@ -136,21 +152,37 @@ static void cell_renderer_frame_icon_get_size(GtkCellRenderer *cell, GtkWidget *
pixbuf_height = MAX (pixbuf_height, gdk_pixbuf_get_height (cellframe->pixbuf_highlighted));
}
+#if GTK_CHECK_VERSION(3, 0, 0)
+ gtk_cell_renderer_get_padding(cell, &xpad, &ypad);
+ calc_width = xpad * 2 + pixbuf_width;
+ calc_height = ypad * 2 + pixbuf_height;
+
+ gtk_cell_renderer_get_alignment(cell, &xalign, &yalign);
+#else
</pre>
<p>As explained above the functions used in the code block above exist for both gtk versions. So IMHO all gtk3 code in this function could be used for both versions replacing the old gtk2 code.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236081097">debugger/src/cell_renderers/cellrendererframeicon.c</a>:</p>
<pre style='color:#555'>>
cell_renderer_frame_icon_get_size (cell, widget, cell_area,
&pix_rect.x,
&pix_rect.y,
&pix_rect.width,
&pix_rect.height);
+#if GTK_CHECK_VERSION(3, 0, 0)
+ gtk_cell_renderer_get_padding(cell, &xpad, &ypad);
+ pix_rect.x += cell_area->x + xpad;
+ pix_rect.y += cell_area->y + ypad;
+ pix_rect.width -= xpad * 2;
</pre>
<p>Here also, this 4 lines could be used in both gtk versions.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082068">debugger/src/dconfig.c</a>:</p>
<pre style='color:#555'>> @@ -67,8 +67,13 @@ static GKeyFile *keyfile_project = NULL;
static gboolean debug_config_loading = FALSE;
/* saving thread staff */
+#if GTK_CHECK_VERSION(3, 0, 0)
+static GMutex change_config_mutex;
+static GCond cond;
+#else
</pre>
<p>This is glib specific code and does not depend on gtk. So a check for the gtk version seems to be wrong here.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082121">debugger/src/dconfig.c</a>:</p>
<pre style='color:#555'>> @@ -269,8 +274,13 @@ static void save_to_keyfile(GKeyFile *keyfile)
*/
static gpointer saving_thread_func(gpointer data)
{
+#if GTK_CHECK_VERSION(3, 0, 0)
+ gint64 interval;
+ g_mutex_lock(&change_config_mutex);
+#else
</pre>
<p>I assume this code could be used for all gtk versions, again, not gtk but glib dependent.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082237">debugger/src/dconfig.c</a>:</p>
<pre style='color:#555'>> @@ -308,11 +318,20 @@ static gpointer saving_thread_func(gpointer data)
debug_config_changed = FALSE;
}
+#if GTK_CHECK_VERSION(3, 0, 0)
+ interval = g_get_monotonic_time() + SAVING_INTERVAL;
+#else
</pre>
<p>No gtk dependant code. It depends on the glib version. What is the reason for this change? If it is an improvement then the code could be used without any check as the function <code>g_get_monotonic_time()</code> exists since glib 2.28 and geany requires 2.32.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082293">debugger/src/dconfig.c</a>:</p>
<pre style='color:#555'>> }
+#if GTK_CHECK_VERSION(3, 0, 0)
+ while (!g_cond_wait_until(&cond, &change_config_mutex, interval));
+ g_mutex_unlock(&change_config_mutex);
</pre>
<p>This requires glib 2.32 at least, not gtk3.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082322">debugger/src/dconfig.c</a>:</p>
<pre style='color:#555'>> }
+#if GTK_CHECK_VERSION(3, 0, 0)
+ while (!g_cond_wait_until(&cond, &change_config_mutex, interval));
+ g_mutex_unlock(&change_config_mutex);
</pre>
<p>Sorry, I mean <code>g_cond_wait_until()</code> replacing <code>g_cond_timed_wait</code>.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082368">debugger/src/dconfig.c</a>:</p>
<pre style='color:#555'>> @@ -458,21 +493,36 @@ void config_init(void)
g_free(data);
}
+#if GTK_CHECK_VERSION(3, 0, 0)
+ g_mutex_init(&change_config_mutex);
+ g_cond_init(&cond);
+ saving_thread = g_thread_new(NULL, saving_thread_func, NULL);
+#else
</pre>
<p>Again, depends on glib 2.32 not gtk3.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082424">debugger/src/debug.c</a>:</p>
<pre style='color:#555'>> @@ -320,7 +324,11 @@ static void on_watch_dragged_callback(GtkWidget *wgt, GdkDragContext *context, i
gpointer userdata)
{
/* string that is dragged */
+#if GTK_CHECK_VERSION(3, 0, 0)
+ gchar *expression = (gchar*)gtk_selection_data_get_data(seldata);
</pre>
<p>The function <code>gtk_selection_data_get_data()</code> exists since gtk 2.14 and so could be used for both verions.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082602">debugger/src/debug.c</a>:</p>
<pre style='color:#555'>> @@ -1066,7 +1105,13 @@ void debug_init(void)
configfile = g_strconcat(geany_data->app->configdir, G_DIR_SEPARATOR_S, "geany.conf", NULL);
g_key_file_load_from_file(config, configfile, G_KEY_FILE_NONE, NULL);
font = utils_get_setting_string(config, "VTE", "font", "Monospace 10");
+#if GTK_CHECK_VERSION(3, 0, 0)
+ fontdesc = pango_font_description_from_string(font);
+ vte_terminal_set_font(VTE_TERMINAL(terminal), fontdesc);
+ pango_font_description_free(fontdesc);
+#else
</pre>
<p>If you like you can replace this lines with a call to function <code>gp_vtecompat_set_font_from_string()</code> from the geany-plugins util lib. Requires <code>#include <gp_vtecompat.h></code>.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082734">debugger/src/dpaned.c</a>:</p>
<pre style='color:#555'>> @@ -287,8 +288,13 @@ void dpaned_init(void)
/* setup notebooks */
gtk_notebook_set_scrollable(GTK_NOTEBOOK(debug_notebook_left), TRUE);
gtk_notebook_set_scrollable(GTK_NOTEBOOK(debug_notebook_right), TRUE);
+#if GTK_CHECK_VERSION(3, 0, 0)
+ gtk_notebook_set_group_name(GTK_NOTEBOOK(debug_notebook_left), NOTEBOOK_GROUP_NAME(NOTEBOOK_GROUP));
+ gtk_notebook_set_group_name(GTK_NOTEBOOK(debug_notebook_right), NOTEBOOK_GROUP_NAME(NOTEBOOK_GROUP));
+#else
</pre>
<p>It seems like the code above should be used for both versions gtk2 and 3 as <code>gtk_notebook_set_group_id()</code> is deprecated since version 2.10.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082808">debugger/src/stree.c</a>:</p>
<pre style='color:#555'>> @@ -197,7 +197,11 @@ static void on_render_line(GtkTreeViewColumn *tree_column, GtkCellRenderer *cell
g_object_set(cell, "text", "", NULL);
else
{
+#if GTK_CHECK_VERSION(3, 0, 0)
+ GValue value = G_VALUE_INIT;
</pre>
<p>Depends on glib 2.30, not on gtk3.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082863">debugger/src/tpage.c</a>:</p>
<pre style='color:#555'>> gtk_box_pack_start(GTK_BOX(root), hbox, FALSE, FALSE, 0);
gtk_box_pack_start(GTK_BOX(hbox), target_label, FALSE, FALSE, 0);
gtk_box_pack_start(GTK_BOX(hbox), target_name, TRUE, TRUE, 0);
gtk_box_pack_start(GTK_BOX(hbox), target_button_browse, FALSE, FALSE, 0);
/* lower hbox */
+#if GTK_CHECK_VERSION(3, 0, 0)
+ hbox = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, SPACING);
+ gtk_box_set_homogeneous(GTK_BOX(hbox), TRUE);
</pre>
<p>Looks like <code>gtk_box_set_homogeneous()</code> could be used in both gtk versions?</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236082884">debugger/src/tpage.c</a>:</p>
<pre style='color:#555'>> @@ -196,27 +223,45 @@ void tpage_pack_widgets(gboolean tabbed)
{
GtkWidget *lbox, *rbox, *hbox;
+#if GTK_CHECK_VERSION(3, 0, 0)
+ root = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, SPACING);
+ gtk_box_set_homogeneous(GTK_BOX(root), TRUE);
</pre>
<p>Here also the line could be used for both versions maybe.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236083002">debugger/src/tpage.c</a>:</p>
<pre style='color:#555'>> gtk_widget_set_size_request(target_button_browse, BROWSE_BUTTON_WIDTH, 0);
g_signal_connect(G_OBJECT(target_button_browse), "clicked", G_CALLBACK (on_target_browse_clicked), NULL);
/* debugger */
debugger_label = gtk_label_new(_("Debugger:"));
+#if GTK_CHECK_VERSION(3, 0, 0)
+ debugger_cmb = gtk_combo_box_text_new();
</pre>
<p>The function <code>gtk_combo_box_text_new()</code> exists since gtk version 2.24 so we could use that code for both verions (<code>gtk_combo_box_new_text()</code> is deprecated since 2.4).</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236083041">debugger/src/tpage.c</a>:</p>
<pre style='color:#555'>> modules = debug_get_modules();
for (iter = modules; iter; iter = iter->next)
{
+#if GTK_CHECK_VERSION(3, 0, 0)
+ gtk_combo_box_text_append_text(GTK_COMBO_BOX_TEXT(debugger_cmb), (gchar*)iter->data);
</pre>
<p>The function <code>gtk_combo_box_text_append_text()</code> exists since gtk version 2.24 so we could use that code for both verions (<code>gtk_combo_box_append_text()</code> is deprecated since 2.4).</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/791#discussion_r236083219">debugger/src/tpage.c</a>:</p>
<pre style='color:#555'>> @@ -366,7 +442,11 @@ int tpage_get_debug_module_index(void)
*/
gchar* tpage_get_debugger(void)
{
+#if GTK_CHECK_VERSION(3, 0, 0)
+ return gtk_combo_box_text_get_active_text(GTK_COMBO_BOX_TEXT(debugger_cmb));
</pre>
<p>The function <code>gtk_combo_box_text_get_active_text()</code> exists since gtk version 2.24 so we could use that code for both versions (<code>gtk_combo_box_get_active_text()</code> is deprecated since 2.6).</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/geany/geany-plugins/pull/791#pullrequestreview-178087309">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJ6R7aDFpwl0uHkCsG3lTX9HiA0D7ks5uytl5gaJpZM4YrdeP">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABDrJ9eLBhLYlPZQATIJ6THwrR_rVeadks5uytl5gaJpZM4YrdeP.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/geany/geany-plugins","title":"geany/geany-plugins","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/geany/geany-plugins"}},"updates":{"snippets":[{"icon":"PERSON","message":"@LarsGit223 requested changes on #791"}],"action":{"name":"View Pull Request","url":"https://github.com/geany/geany-plugins/pull/791#pullrequestreview-178087309"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/geany/geany-plugins/pull/791#pullrequestreview-178087309",
"url": "https://github.com/geany/geany-plugins/pull/791#pullrequestreview-178087309",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "@LarsGit223 requested changes on 791",
"sections": [
{
"text": "I tested the PR with gtk2 and 3 and so far it seems to work in both versions. The gtk3 build did not throw any gtk deprecation warnings. The gtk2 build did, but that comes from the old code so is out of scope of this PR I think. Some might be fixed if my suggestions in ```tpage.c``` will be applied.\r\n\r\nBut some code could be improved, see my comments.",
"activityTitle": "**LarsGit223**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@LarsGit223",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/geany/geany-plugins/pull/791#pullrequestreview-178087309"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 414046095\n}"
}
],
"themeColor": "26292E"
}
]</script>