GNOME Bugzilla – Bug 612693
Does not compile with -DGSEAL_ENABLE
Last modified: 2011-07-28 00:35:27 UTC
This module does not build with -DGSEAL_ENABLE. See http://live.gnome.org/GnomeGoals/UseGseal . Note that maybe this report cannot be fixed yet, as GTK+ still misses some accessor functions (see bug 588389, bug 597610) needed for sealing. Also see http://live.gnome.org/GTK%2B/3.0/PendingSealings for current status. The jhbuild output posted here of course only lists the very first error when trying to compile.
Created attachment 159051 [details] [review] Partial fix Patch fixing a few issues. Bumps gtk+ to 2.14
ping
Review of attachment 159051 [details] [review]: The patch looks good, only a minor comment Also, Id update the minimum required version to 2.20, to avoid #ifdef's on the code ::: src/gtkhex.c @@ +2048,3 @@ +#else + if (!GTK_WIDGET_HAS_FOCUS (gh)) +#endif This seems a typo, rigth? @@ +2051,1 @@ GTK_WIDGET_SET_FLAGS(gh, GTK_CAN_FOCUS); gtk_widget_set_can_focus (hg, TRUE);
Review of attachment 159051 [details] [review]: In addition to Javier's comments, there are two hunks where typecasting has gone wrong: > - gint source_min = ((gint)adj->value - gh->top_line) * gh->char_height; > + gint source_min = (gtk_adjustment_get_value((gint)adj) - gh->top_line) * gh->char_height; Should be: + gint source_min = ((gint)gtk_adjustment_get_value(adj) - gh->top_line) * gh->char_height; > - gh->top_line = (gint)adj->value; > + gh->top_line = gtk_adjustment_get_value((gint)adj); Should be: + gh->top_line = (gint)gtk_adjustment_get_value(adj);
Review of attachment 159051 [details] [review]: >@@ -840,11 +876,11 @@ static void recalc_displays(GtkHex *gh, guint width, guint height) { > gh->adj->value = MIN(gh->cursor_pos/gh->cpl, gh->lines - gh->vis_lines); > gh->adj->value = MAX(0, gh->adj->value); > } >- gh->adj->lower = 0; >- gh->adj->upper = gh->lines; >- gh->adj->step_increment = 1; >- gh->adj->page_increment = gh->vis_lines - 1; >- gh->adj->page_size = gh->vis_lines; >+ gtk_adjustment_set_upper(gh->adj, 0); >+ gtk_adjustment_set_upper(gh->adj, gh->lines); >+ gtk_adjustment_set_step_increment(gh->adj, 1); >+ gtk_adjustment_set_page_increment(gh->adj, gh->vis_lines - 1); >+ gtk_adjustment_set_page_size(gh->adj, gh->vis_lines); First gtk_adjustment_set_upper() call should instead be gtk_adjustment_set_lower()
Created attachment 192374 [details] [review] Updated Andre's patch per review comments Updated the patch as Andre and I discussed on IRC.
Created attachment 192530 [details] [review] Almost complete patch I worked over it for some time and here is almost complete port. Few more details in commit message
Review of attachment 192374 [details] [review]: Pushed to master as b24bf4e, thanks André.
Review of attachment 192530 [details] [review]: Thanks for the patch, Adrian. Could you update it to apply to current git master, now that Andre's changes have landed?
Created attachment 192654 [details] [review] Updated patch to apply with git master Here you have updated version.
Review of attachment 192654 [details] [review]: I would prefer if the first line of the commit message (also called the short commit message) described the commit in a way that would make it easy to understand what the commit changed, without having to reference the ticket here. Thus, I would change "Fix all issues connected with bug 612693" to "Fix build with GSEAL enabled" or similar, and put the link to the bug ticket in the body of the commit message. https://live.gnome.org/Git/CommitMessages I have some observations about the patch below; thanks for doing all this. Most of the in line comments also apply to other similar changes, but for the sake of brevity I didn't attempt to flag them all. ::: src/accessiblegtkhex.c @@ +209,3 @@ accessible = GTK_ACCESSIBLE (obj); g_return_if_fail (accessible != NULL); + gtk_accessible_set_widget(accessible, GTK_WIDGET (gtk_hex)); A space is missing before the opening parenthesis. I know the code in some of the ghex source files is a mess and varies from file to file, but this particular file has consistent style and it would nice to follow that. ::: src/findreplace.c @@ +111,2 @@ TRUE, TRUE, 0); + gtk_widget_set_can_default(dialog->f_next, 1); gtk_widget_set_can_default() takes gboolean instead of int, so it would be more natural to use gtk_widget_set_can_default(dialog->f_next, TRUE). ::: src/gtkhex.c @@ +610,3 @@ + GtkAllocation* allocation; + gtk_widget_get_allocation(w, allocation); This is going to lead to memory corruption, overwriting random memory that the uninitialized pointer points to. Instead allocate 'allocation' on the stack and do: GtkAllocation allocation; gtk_widget_get_allocation(w, &allocation); Also, don't mix variable declarations and code. All declarations should go to the beginning of the block. Although gcc allows mixed declarations and code, it's not valid C89 and some other compilers don't support it. @@ +786,3 @@ +#if GTK_CHECK_VERSION(3,0,0) + gtk_widget_get_preferred_width(gh->scrollbar, minimal_width, NULL); +#else I can see that you have used GTK+ 3 ifdefs in two places. Please make this patch to only fix build on gtk2 with gseal enabled and let's work on the gtk3 port separately. @@ +788,3 @@ +#else + GtkRequisition* sb_req; + gtk_widget_get_requisition(gh->scrollbar, sb_req); Same problem as with gtk_widget_get_allocation() use: it's going to overwrite random memory that *sb_req points to. Also don't mix variable declarations and code. @@ +873,3 @@ + gtk_adjustment_set_value(gh->adj, MAX(0, gtk_adjustment_get_value(gh->adj))); + } + gtk_adjustment_set_upper(gh->adj, 0); Looks like a bad merge. Should be gtk_adjustment_set_lower() instead of gtk_adjustment_set_upper. @@ +896,1 @@ gint source_min = ((gint)gtk_adjustment_get_value(adj) - gh->top_line) * gh->char_height; Mixed variable declarations and code. @@ +2045,3 @@ + if (!gtk_widget_has_focus (GTK_WIDGET (gh))) + if (!GTK_WIDGET_HAS_FOCUS (gh)) + gtk_widget_set_can_focus(GTK_WIDGET(gh), 1); Looks like a bad merge here.
On gtk+ 2 vs 3 topic: shall moving away from gdk_draw_* API to cairo be only for gtk+ 3 or both ?
Created attachment 192754 [details] [review] Updated patch according to Kevin's suggestions
Review of attachment 192754 [details] [review]: Looks very nice now. I fixed two minor problems (more on them down below) and pushed it to master as e2868ca2. Thanks! ::: configure.ac @@ +45,3 @@ gio-2.0 \ gtk+-2.0 >= 2.24.0 \ + gtk+-2.0 >= 2.22.0 \ ... ::: src/gtkhex.c @@ +896,3 @@ + gtk_widget_get_allocation(gh->xdisp, &xdisp_allocation); + gtk_widget_get_allocation(gh->adisp, &adisp_allocation); + gtk_widget_get_allocation(gh->offsets, &offsets_allocation); I think it would be better to check if gh->offsets isn't NULL -- it's possible to display the widget without having the offsets shown. Down below in the same function there are several 'if (gh->offsets)' checks to avoid dereferencing a possible NULL pointer.
(In reply to comment #12) > On gtk+ 2 vs 3 topic: shall moving away from gdk_draw_* API to cairo be only > for gtk+ 3 or both ? I'd like to land as many of the changes as possible without actually switching over to gtk3. That way we can test all the changes individually and keep the final gtk3 change reasonably small. Having said that, I already have local changes for switching over to cairo drawing and getting rid of bonobo.
Closing the ticket, thanks everybody for submitting patches!