After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 639073 - Many issues in gtkmm 3 affect the custom widget example program
Many issues in gtkmm 3 affect the custom widget example program
Status: RESOLVED FIXED
Product: gtkmm
Classification: Bindings
Component: general
2.91.x
Other Linux
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on: 639754
Blocks:
 
 
Reported: 2011-01-09 15:23 UTC by Kjell Ahlstedt
Modified: 2011-02-12 13:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch that solves items 1, 2, 3, 4. (10.42 KB, patch)
2011-01-19 12:20 UTC, Kjell Ahlstedt
committed Details | Review
gtkmm-documentation patch: custom_container and custom_widget (30.35 KB, patch)
2011-01-23 18:50 UTC, Kjell Ahlstedt
committed Details | Review
gtkmm patch: Change int* to int& in get_preferred_xxx_vfunc(). (12.21 KB, patch)
2011-01-30 15:34 UTC, Kjell Ahlstedt
none Details | Review
gtkmm-documentation patch: Change int* to int& in get_preferred_xxx_vfunc(). (12.55 KB, patch)
2011-01-30 15:36 UTC, Kjell Ahlstedt
none Details | Review
gtkmm patch: Widget, CellArea, CellRenderer: Modify and add some vfuncs. (32.37 KB, patch)
2011-02-03 08:49 UTC, Kjell Ahlstedt
none Details | Review

Description Kjell Ahlstedt 2011-01-09 15:23:37 UTC
Overview:
When I tried to run the example program
gtkmm-documentation/examples/book/custom/custom_widget with gtkmm 3 code,
I found a bunch of issues that affect that program, quite apart from the
fact that the program itself has not been completely adapted to gtkmm 3.

The program crashes with a segmentation fault, but that's not the only problem.

1. stylecontext.ccg
There are a number of functions that end with
  return Gdk::RGBA(&crgba, false);
or
  return Border(&cborder, false);
where &crgba and &cborder are pointers to local data on the stack.
This is one reason why the program can crash.

2. widget.hg
In addition to the new functions gtk_widget_get_request_mode,
gtk_widget_get_preferred_{width, height_for_width, height, width_for_height,
size} which are correctly wrapped with _WRAP_METHOD, there are also the new
virtual functions get_request_mode, get_preferred_{width, height_for_width,
height, width_for_height} (but no virtual get_preferred_size). These must be
wrapped with _WRAP_VFUNC. (get_request_mode is perhaps not that simple. See
a following comment.)

3. cssprovider.hg
get_default and get_named need 'refreturn'.

4. cssprovider.hg
Shouldn't Gtk::CssProvider have a 'create' method and a protected default
constructor?

5. gtkcssprovider.c
When gtk_css_provider_load_from_path parses the file with style properties,
it assumes that class names begin with an uppercase letter. Identifiers that
begin with a lowercase letter (e.g. gtkmm__CustomObject_mywidget) are assumed
to be names of regions within a widget. See functions parse_rule and
parse_selector in gtkcssprovider.c.

6. The custom_widget program
This program must be modified in some ways, but I'd like to defer a detailed
discussion until the other problems have been solved.

   Steps to reproduce:

   Actual results:

   Expected results:

   Build date and platform: Ubuntu 10.10, source code of gtkmm, gtk+, etc.
      built with jhbuild on 2011-01-04.

   Additional information:
Further discussion of these issues will follow right away in a comment.
Comment 1 Kjell Ahlstedt 2011-01-09 15:34:09 UTC
I stumbled upon all these issues just because I tried to check if the changes
in gtk+ 3 and gtkmm 3 have fixed bug 606903.

I can make a patch with corrections of some or all of the issues, but I don't
yet know what to do with some of them.

1. stylecontext.ccg
Quite obvious what to do. Set make_a_copy = true in the constructors.
Otherwise the functions return an object that contains a pointer to local data.

An alternative solution that avoids a copy (get_color used as an example):
  GdkRGBA* crgba = g_slice_new(GdkRGBA);
  gtk_style_context_get_color(....., crgba);
  return Gdk::RGBA(crgba, false); 

2. widget.hg
I don't know what's the best way to wrap the virtual function get_request_mode.
It's possible to let klass->get_request_mode = NULL, and I think that's what
most gtk+ widgets do. When the request mode of a widget is required,
gtk_widget_get_request_mode is called. It's defined in gtksizerequest.c:

   GtkSizeRequestMode
   gtk_widget_get_request_mode (GtkWidget *widget)
   {
     GtkWidgetClass *klass;
     g_return_val_if_fail (GTK_IS_WIDGET (widget), GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH);
     klass = GTK_WIDGET_GET_CLASS (widget);
     if (klass->get_request_mode)
       return klass->get_request_mode (widget);

     /* By default widgets are height-for-width. */
     return GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH;
   }

If get_request_mode is wrapped with _WRAP_VFUNC, then get_request_mode_vfunc
and get_request_mode_vfunc_callback end with

  typedef GtkSizeRequestMode RType; // or typedef SizeRequestMode RType;
  return RType();

Now this return value happens to be the same as the default return value from
gtk_widget_get_request_mode, but that's pure luck, I believe. It shouldn't be
relied on.

A better way would be to manually code get_request_mode_vfunc and
get_request_mode_vfunc_callback, and have them return
Gtk::SIZE_REQUEST_HEIGHT_FOR_WIDTH and GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH.

A still better way would be to call gtk_widget_get_request_mode to get its
default value. But then you must create a widget object of a class with
klass->get_request_mode == NULL. The input object (self in vfunc_callback,
gobject_ in vfunc) won't do, will it? But isn't it too much work to create a
new object just to get this default return value?
It would be nice if gtk+ contained a constant GTK_SIZE_REQUEST_DEFAULT or a
function gtk_widget_get_default_request_mode().

2.1 gtk_vfuncs.defs
When I added virtual functions to widget.hg, I became aware that I also had to
change gtk_vfuncs.defs. The tutorial "Programming with gtkmm" says that
gtk_vfuncs.defs is written manually. Is that still true, or is it generated by
a script?

3 and 4. cssprovider.hg
Obvious solution, I think. Or is there a reason not to create a
Gtk::CssProvider with a 'create' method?

5. gtkcssprovider.c
Does this mean that the prefix gtkmm__ must be changed? E.g. to Gtkmm__?

gtkcssprovider.c contains a long description of the syntax and semantics of
a css file. At the very end of that description it's mentioned how to set a
style property that has been registered with a call to
gtk_widget_class_install_style_property. But nowhere does it say that a class
name must begin with an uppercase letter. Only the source code shows that.

6. The custom_widget program
Some necessary changes:

Gtk::Widget::on_size_request does not exist any more. MyWidget::on_size_request
shall be replaced by some or all of the get_preferred_xxx_vfunc functions.

It's not correct to use Gtk::CssProvider::get_default(), and read the style
property file with that CssProvider. You must create a new CssProvider and add
it to the style context. The description of gtk_css_provider_load_from_path
says: "Loads the data contained in @path into @css_provider, making it clear
any previously loaded information." For a reason unknown to me, the program
later crashes with a segmentation fault, if load_from_path is called on the
default css provider.

The style property file must be changed, probably like this:
   * {
     -Gtkmm__CustomObject_mywidget-example-scale: 800;
   }

assuming the gtkmm__ prefix is changed to Gtkmm__ in glibmm/class.ccg and some
other files. I don't know if such a change can have bad side effects.


And what about bug 606903? No, it has not been fixed.
Comment 2 Kjell Ahlstedt 2011-01-11 10:13:55 UTC
The removal of Gtk::Widget::on_size_request() affects also the custom_container
example and the description in "Programming with gtkmm" chapter 26.
MyContainer::on_size_request() is never called.

ExampleWindow::m_Button_Two is a Gtk::Label in the custom_container example!
Comment 3 Murray Cumming 2011-01-14 22:39:44 UTC
(In reply to comment #1)
> I stumbled upon all these issues just because I tried to check if the changes
> in gtk+ 3 and gtkmm 3 have fixed bug 606903.
> 
> I can make a patch with corrections of some or all of the issues,

That would be very welcome. Actually, if you request git access then I'll gladly provide it.

> 2.1 gtk_vfuncs.defs
> When I added virtual functions to widget.hg, I became aware that I also had to
> change gtk_vfuncs.defs. The tutorial "Programming with gtkmm" says that
> gtk_vfuncs.defs is written manually. Is that still true, or is it generated by
> a script?

It's still manual. I don't know of any way to generate it.

> 3 and 4. cssprovider.hg
> Obvious solution, I think. Or is there a reason not to create a
> Gtk::CssProvider with a 'create' method?

When the C API has get_default() we generally do the same. create() suggests a new instance, while get_*() suggests returning the same instance.

If there is a *_new() function too then there should be a create() method in our API.

> 5. gtkcssprovider.c
> Does this mean that the prefix gtkmm__ must be changed? E.g. to Gtkmm__?

We have had a problem with this before with GtkRCStyle, but I think we fixed it. We can change it in gtkmm 3 if necessary, but I'd like to see some documentation saying that we need to. You could file a bug about that.
Comment 4 Kjell Ahlstedt 2011-01-18 20:01:34 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > I can make a patch with corrections of some or all of the issues,
> 
> That would be very welcome. Actually, if you request git access then I'll
> gladly provide it.

Thanks for your offer, but I won't request git access, at least not yet.
I'm a bit of a coward, and it's fine not to be able to mess up the git
repository, although I think the risk would be low.

I'm preparing a patch that solves items 1, 2, 3, and 4.

Concerning item 2 I have a question about constness of member functions.
The new functions gtk_widget_get_preferred_xxx and gtk_widget_get_request_mode,
and possibly also the corresponding virtual functions, have what I think Bjarne
Stroustrup calls physical but not logical constness. The compiler does not
complain if they are declared const (well, for the vfuncs it of course depends
upon the overriding functions in the subclass), but they can do a fair amount
of calculations, and save the results in the underlying C object.

Are there any rules for this kind of function? Shall they be declared const?

I'm inclined to remove const from get_preferred_xxx and get_request_mode.
Is it possible that anyone is already dependent on they being const?

I can imagine that it's next to impossible to get the constness right on all
wrapped functions. The easy way is to declare all get_xxx and is_xxx functions
const, except for some functions that return a pointer to a non-const object.
Comment 5 Murray Cumming 2011-01-18 20:30:56 UTC
(In reply to comment #4)
> I'm inclined to remove const from get_preferred_xxx and get_request_mode.
> Is it possible that anyone is already dependent on they being const?

No, I think the constness is appropriate. Whether it changes some bits in the memory is not really relevant. const means what we say it means. If it recalculates some cache values then that's not interesting to the user of the API. That's why C++ has the mutable keyword.
Comment 6 Kjell Ahlstedt 2011-01-19 12:20:22 UTC
Created attachment 178710 [details] [review]
Patch that solves items 1, 2, 3, 4.

1. make_a_copy = true in stylecontext.ccg.
2. Added vfuncs in widget.[hg|ccg].
3, 4. Added create() and some refreturn in cssprovider.hg.

As requested in comment 5 I have not removed any const declarations. The new
vfuncs are also const. The output parameters in get_preferred_xxx_vfunc are
int*, not int&. That's because these functions are sometimes called from the
C code with one of the pointers = NULL.

Item 5 has probably been solved by a change in gtk+. See bug 639754.

Item 6 remains, and the issues I mentioned in comment 2.
Comment 7 Kjell Ahlstedt 2011-01-23 18:50:00 UTC
Created attachment 179091 [details] [review]
gtkmm-documentation patch: custom_container and custom_widget

The attached patch modifies the custom_container and custom_widget examples
and the "Custom Widgets" chapter in "Programming with gtkmm".

The new versions of the example programs depend on the gtkmm patch in
comment 6.

If my English in the Custom Widgets chapter needs some polishing, please go
ahead and improve it. I tried to add at least a one-sentence description of
each virtual method mentioned in the chapter, but I gave up on on_map() and
on_unmap(). They have some relation to show() and hide(), but the exact
relationship beats me.

I have not updated custom_container.png and custom_widget.png. Am I right, if
I think that many figures need updating, and that they will all be regenerated
some time with a recommended theme, so they all get a good and consistent
look-and-feel?

If this patch and the one in comment 6 are both accepted, all issues in this
bug have been fixed.
Comment 8 Murray Cumming 2011-01-25 14:37:39 UTC
(In reply to comment #6)
> Created an attachment (id=178710) [details] [review]
> Patch that solves items 1, 2, 3, 4.

It looks good, but could you tell me more about this, please:
+// This vfunc is custom implemented, because the return value RType(),
+// generated by _WRAP_VFUNC may not be appropriate.
+SizeRequestMode Gtk::Widget::get_request_mode_vfunc() const
Comment 9 Murray Cumming 2011-01-25 14:39:48 UTC
(In reply to comment #7)
> Created an attachment (id=179091) [details] [review]
> gtkmm-documentation patch: custom_container and custom_widget

This look good. Thanks for taking the time to fix this.

However, this makes me notice that we are using int* where int& would probably be nicer:

+  virtual void get_preferred_width_vfunc(int* minimum_width, int* natural_width) const;
+  virtual void get_preferred_height_for_width_vfunc(int width, int* minimum_height, int* natural_height) const;
Comment 10 Kjell Ahlstedt 2011-01-25 20:20:12 UTC
(In reply to comment #8)
> It looks good, but could you tell me more about this, please:
> +// This vfunc is custom implemented, because the return value RType(),
> +// generated by _WRAP_VFUNC may not be appropriate.
> +SizeRequestMode Gtk::Widget::get_request_mode_vfunc() const

I discussed this issue in comment 1, item 2.

Consider this case:
gtk_widget_get_request_mode(GtkWidget*) is called for a C++ widget derived
from Gtk::Widget (actually of course for the underlying C widget). It will call
Gtk::Widget_Class::get_request_mode_vfunc_callback(). If it's a custom widget
which has not overridden get_request_mode_vfunc() or if it's a gtkmmproc-
generated class, the virtual get_request_mode() method of the parent C class is
called, if it's defined. If it's not defined (if base->get_request_mode == 0,
where base is a pointer to the parent C class), then
get_request_mode_vfunc_callback() returns a default value.

Now it seems that base->get_request_mode == 0 is a legitimate and perhaps even
common case in gtk+ widgets. For such widgets gtk_widget_get_request_mode()
returns GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH.
Gtk::Widget_Class::get_request_mode_vfunc_callback() and
Gtk::Widget::get_request_mode_vfunc() shall do the same.

The code generated by _WRAP_VFUNC ends with
  typedef GtkSizeRequestMode RType;
  return RType();
and
  typedef SizeRequestMode RType;
  return RType();

Now enum GtkSizeRequestMode is defined in such a way that RType() is actually
GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH, so the code generated by _WRAP_VFUNC
returns the same value as my slightly modified code. To me that seems just like
a coincidence that we should not rely on. Or am I wrong? Would _WRAP_VFUNC be
ok for get_request_mode_vfunc()? As discussed in comment 1, my version of
get_request_mode_vfunc() is not perfect either. It assumes that we know the
default value of gtk_widget_get_request_mode().

(In reply to comment #9)
> However, this makes me notice that we are using int* where int& would probably
> be nicer:

I agree that int& is usually nicer than int*. In comment 6 I mentioned very
briefly the reason for using int*.
Widget_Class::get_preferred_width_vfunc_callback() and the other
get_preferred_xxx_vfunc_callback() functions are called from gtk+. In some
calls the pointer to one of the output arguments is NULL. It's even less nice
to convert a NULL int* to an int& than it is to pass the int* directly to the
get_preferred_xxx_vfunc() C++ functions.
Comment 11 Murray Cumming 2011-01-26 10:57:13 UTC
Thanks. I commited both these patches, but I'm keeping this bug open until I have time to look at those issues in some more detail.
Comment 12 Kjell Ahlstedt 2011-01-27 12:06:02 UTC
Yet another comment! I'm afraid that my comments in this bug are becoming so
verbose that you, Murray, will never have time to read them. It's difficult to
be both clear and concise, and I'm not even sure my comments are clear.

(In reply to comment 8)
> It looks good, but could you tell me more about this, please:
> +// This vfunc is custom implemented, because the return value RType(),
> +// generated by _WRAP_VFUNC may not be appropriate.
> +SizeRequestMode Gtk::Widget::get_request_mode_vfunc() const

Why bother with the last return statement in get_request_mode_vfunc[_callback]?
In other vfuncs, no one cares much. The test
   if(base && base->gtk_virtual_func)
is just a safety measure. It should never fail, both base and
base->gtk_virtual_func should always be non-null. If they are null, something
is fundamentally wrong, and it doesn't matter much what value is returned.
Or am I wrong? Isn't this the normal case?

get_request_mode is different. Many gtk+ widgets leave
widget_class->get_request_mode == NULL, and accept the default value from
gtk_widget_get_request_mode(). Therefore I think it's important that
gtk_widget_get_request_mode() returns the same value when it's called on a
gtkmm-generated widget as it does when it's called on a gtk+ widget of the
corresponding type.

(In reply to comment 9)
> However, this makes me notice that we are using int* where int& would probably
> be nicer:

It's possible to avoid int* by modifying the code that _WRAP_VFUNC generates.
Take Widget_Class::get_preferred_width_vfunc_callback as an example.
Replace
   obj->get_preferred_width_vfunc(minimum_width, natural_width);
by
   int min_width = 0;
   int nat_width = 0;
   obj->get_preferred_width_vfunc(min_width, nat_width);
   if (minimum_width)
     *minimum_width = min_width;
   if (natural_width)
     *natural_width = nat_width;
or
   int min_width = 0;
   int nat_width = 0;
   obj->get_preferred_width_vfunc((minimum_width ? *minimum_width : min_width),
                                  (natural_width ? *natural_width : nat_width);

Here get_preferred_width_vfunc() would take int& arguments instead of int*.
(The initial values of min_width and nat_width are necessary only if there is
a compiler that would print a warning message if they are not initialized.)

If you think it's worth the trouble to do this in order to avoid int*, I can
make the two patches that are required (one for gtkmm, the other for
gtkmm-documentation).


I've noticed that GtkCellArea and GtkCellRenderer, which don't derive from
GtkWidget, also contain the virtual get_request_mode function and the 4
virtual get_preferred_xxx functions.
Comment 13 Murray Cumming 2011-01-27 14:07:44 UTC
(In reply to comment #12)
> The test
>    if(base && base->gtk_virtual_func)
> is just a safety measure. It should never fail, both base and
> base->gtk_virtual_func should always be non-null. If they are null, something
> is fundamentally wrong,

I don't think that's true. It's not unheard of for a vfunc to be NULL in a GObject. And it seems wise to be careful in case one is. What's the value in risking a NULL dereference?
 
> get_request_mode is different. Many gtk+ widgets leave
> widget_class->get_request_mode == NULL,

So there you go. That proves my point.

> and accept the default value from
> gtk_widget_get_request_mode(). Therefore I think it's important that
> gtk_widget_get_request_mode() returns the same value when it's called on a
> gtkmm-generated widget as it does when it's called on a gtk+ widget of the
> corresponding type.

It's unwise for a GObject to test for NULLness of its vfunc. A correct default vfunc implementation would be easier for us. CCing Tristan, who has dealt with a similar issue in GTK+ before.
Comment 14 Tristan Van Berkom 2011-01-27 15:01:16 UTC
A default implementation for get_request_mode() could certainly be added to
GTK+ with no harm done.

But please note that you should not call the ->get_preferred_width()
family of vfuncs directly except when you are calling it directly on
the object you are implementing from inside another implementation
of the ->get_preferred_height_for_width() vfunc family.

This is documented here:
   http://library.gnome.org/devel/gtk/2.91/GtkWidget.html
(around the text "Widget calling its own size request method")

For similar reasons why it was incorrect to call ->size_request() vfunc
directly on a widget in GTK+ 2.x

  o Direct calls to vfuncs of this family will not cache the results,
    which can be a bad performance hit.
  o Direct calls if made, are not complete requests, when requesting
    a child widget one would need to use GtkWidgetClass->adjust_size_request()
    afterwards.
  o When calling ->get_preferred_height_for_width() vfunc, the 'for_width'
    argument needs to be treated by GtkWidgetClass->adjust_size_allocation()
    before sending that for_width to the implementing class (this is to
    compensate for changes made by ->adjust_size_request() and to cap the
    for_width value at the natural width for the case that GtkWidget:xalign
    is not GTK_ALIGN_FILL).

All in all, it's very important to pass through the 
gtk_widget_get_preferred_height_for_width() APIs rather than calling
the vfuncs directly.
Comment 15 Tristan Van Berkom 2011-01-27 15:05:21 UTC
While it's important to pay attention to the fact that you 
*must* call the wrapper functions for those APIs, it's possible
I missed the point here.

If you are wrapping the widget's vfuncs and chaining up to the 
native C implementation I suppose the above text is irrelevant.

So long as when a caller calls widget.get_preferred_width(),
gtk_widget_get_preferred_width() is actually called, and
then the C++ wrapper vfunc invoked by virtue of it being
a derived class, and then possibly chaining up.

As I mentioned, if it's any help I can easily add a default
implementation for get_request_mode() instead of casing
it from the wrapper function.
Comment 16 Murray Cumming 2011-01-27 15:33:59 UTC
> As I mentioned, if it's any help I can easily add a default
> implementation for get_request_mode() instead of casing
> it from the wrapper function.

Yes, that seems safer than putting that code in a check for a NULL vfunc.

I don't think we are talking about calling the vfunc directly from an application, or even from outside the class itself.
Comment 17 Tristan Van Berkom 2011-01-28 06:52:16 UTC
I added a default implementation to GtkWidgetClass that returns the
default GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH instead of checking the
vfunc from gtk_widget_get_request_mode() today.

Hope this helps.
Comment 18 Murray Cumming 2011-01-28 09:00:12 UTC
Thanks, Tristan. (It was http://git.gnome.org/browse/gtk+/commit/?id=5a5854f6f64231be8df028dbcb87f75a8025609e )

So I have now simplified that code in widget.[hg|ccg] to just use _WRAP_VFUNC():
http://git.gnome.org/browse/gtkmm/commit/?id=3fadc846ec4fb1ce9736a96919a372380e006449
Comment 19 Murray Cumming 2011-01-28 09:26:57 UTC
(In reply to comment #12)
> If you think it's worth the trouble to do this in order to avoid int*, I can
> make the two patches that are required (one for gtkmm, the other for
> gtkmm-documentation).

That would be nice. Thanks.

I guess you'd need to hand-code the vfunc wrappers, because it would be hard to make gmmproc guess when to do this.

> I've noticed that GtkCellArea and GtkCellRenderer, which don't derive from
> GtkWidget, also contain the virtual get_request_mode function and the 4
> virtual get_preferred_xxx functions.

Ah, then we should wrap those vfuncs too, in the same way.
Comment 20 Kjell Ahlstedt 2011-01-30 15:34:03 UTC
Created attachment 179635 [details] [review]
gtkmm patch: Change int* to int& in get_preferred_xxx_vfunc().

(In reply to comment 19)
> I guess you'd need to hand-code the vfunc wrappers, because it would be hard to
> make gmmproc guess when to do this.

I've hand-coded the vfunc_callbacks, but not the vfuncs. The vfuncs can be
generated by _VFUNC_CC.

_WRAP_SIGNAL understands the optional arguments custom_default_handler,
no_default_handler and custom_c_callback. If _WRAP_VFUNC had understood
custom_vfunc_callback (and custom_vfunc, although not necessary in this case),
I could have used _WRAP_VFUNC instead of the more obscure m4 macros _VFUNC_PH
and VFUNC_CC. I could perhaps file a bug with a suggestion for a future
enhancement of gtkmm-proc. But I don't know where to file such a bug. In
Bugzilla glibmm does not contain a 'tools' component.
Comment 21 Kjell Ahlstedt 2011-01-30 15:36:57 UTC
Created attachment 179636 [details] [review]
gtkmm-documentation patch: Change int* to int& in get_preferred_xxx_vfunc().

I've included two changes in addition to the change from int* to int&.

1. Added '*~' in .gitignore, to make git ignore gedit's backup files.

2. Added 'mm-common-prepare --version' in autogen.sh, as suggested in
   bug 628713. If this patch is committed, bug 628713 can be closed.
Comment 22 Kjell Ahlstedt 2011-01-30 19:33:40 UTC
(In reply to comment 19)
> (In reply to comment 12)
>> I've noticed that GtkCellArea and GtkCellRenderer, which don't derive from
>> GtkWidget, also contain the virtual get_request_mode function and the 4
>> virtual get_preferred_xxx functions.

> Ah, then we should wrap those vfuncs too, in the same way.

-- Tristan, if you still read comments in this bug:

What you did in GtkWidgetClass helps gtkmm to make the C++ wrappers behave
just like the underlying C classes. I.e. if a virtual function (e.g. get_xyz)
is supposed to be called via gtk_baseclassname_get_xyz(), then put the default
action in gtk_baseclassname_real_get_xyz(), and store a pointer to it in the
virtual function table. In the cases where the virtual function returns void,
and there is no default action, it's ok to have a NULL pointer in the vfunc
table, but in other cases it simplifies for gtkmm if the default action is put
in a function reached from the vfunc table.

Now there are NULL vfunc pointers also in GtkCellArea and GtkCellRenderer.
At least these ones: GtkCellArea::get_request_mode, get_preferred_width,
get_preferred_height, GtkCellRenderer::get_request_mode, but there may be
others too. (get_preferred_[width|height] return void, but there is still a
default action in GtkCellArea, consisting of a warning message.)

And I think I've found an error in
gtk_cell_area_real_get_preferred_height_for_width(). It calls
get_preferred_width(), but I think it should be get_preferred_height().

-- The rest of this comment is probably of no interest to gtk+ developers.

Ok, I can add get_request_mode_vfunc() and get_preferred_xxx_vfunc() in
CellArea and CellRenderer, but after that I think it's time to close this bug,
although there are more functions in CellArea that have not been wrapped in C++
code.

Just a question before I supply the 5th patch in this bug:
The non-virtual CellArea::get_preferred_xxx() functions are non-const. Is that
correct? Shall the virtual CellArea::get_preferred_xxx_vfunc() also be
non-const?

And an observation: GtkCellRenderer, GtkTreeViewColumn and GtkWidget are
derived from GInitiallyUnowned in gtk+ 3. In gtk+ 2 they are derived from
GtkObject. Gtk::CellRenderer, Gtk::TreeViewColumn and Gtk::Widget are still
derived from Gtk::Object.
Comment 23 Kjell Ahlstedt 2011-01-31 07:20:27 UTC
The first part of comment 22 is incomplete. Let me correct myself:

From gtkmm's point of view it's ok to have a NULL pointer in the vfunc table if
- either the virtual function returns void, and there is no default action,
- or the default action consists of returning one of the "natural" default
  values 0, 0.0, FALSE, NULL (function's return value, not values set in output
  arguments).

In other cases it simplifies for gtkmm if the default action is put in a
function reached from the vfunc table.
Comment 24 Murray Cumming 2011-01-31 14:02:23 UTC
Tristan, it would be helpful if you could make the same fixes to these virtual functions.
Comment 25 Murray Cumming 2011-01-31 14:05:54 UTC
(In reply to comment #23)
> Shall the virtual CellArea::get_preferred_xxx_vfunc() also be
non-const?

Sure, that would be good.

> And an observation: GtkCellRenderer, GtkTreeViewColumn and GtkWidget are
> derived from GInitiallyUnowned in gtk+ 3. In gtk+ 2 they are derived from
> GtkObject. Gtk::CellRenderer, Gtk::TreeViewColumn and Gtk::Widget are still
> derived from Gtk::Object.

Yes, Gtk::Object in gtkmm 3 is a hacky class that doesn't correspond directly to any GtkObject type in GTK+. It lets us not force the use of RefPtr for these. I might change my mind about using it for CellRenderer.

>  I could perhaps file a bug with a suggestion for a future
> enhancement of gtkmm-proc. But I don't know where to file such a bug. In
> Bugzilla glibmm does not contain a 'tools' component.

So just use any component - that's not a big deal. But will this even be necessary if Tristan fixes the GTK+ vfuncs?
Comment 26 Murray Cumming 2011-01-31 14:18:19 UTC
(In reply to comment #21)
> 2. Added 'mm-common-prepare --version' in autogen.sh, as suggested in
>    bug 628713. If this patch is committed, bug 628713 can be closed.

I have committed that separately. Thanks. It was an unrelated bug so it should not be in the same commit.
Comment 27 Kjell Ahlstedt 2011-02-03 08:49:03 UTC
Created attachment 179976 [details] [review]
gtkmm patch: Widget, CellArea, CellRenderer: Modify and add some vfuncs.

This patch replaces the patch in comment 20.
- CellArea and CellRenderer: Add vfuncs get_request_mode,
  get_preferred_[width|height_for_width|height|width_for_height].
- Widget: Change int* to int& in
  get_preferred_[width|height_for_width|height|width_for_height]_vfunc.

This patch uses the _WRAP_VFUNC argument custom_vfunc_callback, implemented in
glibmm bug 641165.

This patch can be used even if GtkCellArea and GtkCellRenderer are not changed
as requested in comment 22 and comment 24. But then there would be slight
differences between Gtk::Cell[Area|Renderer] and GtkCell[Area|Renderer]:
- If anyone derives a custom CellArea from Gtk::CellArea and does not override
  get_request_mode, get_preferred_width and get_preferred_height, there will be
  no warning messages.
- If the default value of GtkCell[Area|Renderer]::get_request_mode() will ever
  be changed (very unlikely, I think), then the default value of
  Gtk::Cell[Area|Renderer]::get_request_mode() will not automatically change
  accordingly.

Shall I also make a replacement for the gtkmm-documentation patch in
comment 21? A patch that does not change autogen.sh?
Comment 28 Murray Cumming 2011-02-03 09:25:13 UTC
Thanks.

To avoid too much code churn, I'll wait until Tristan can fix GTK+. Hopefully he'll get to that soon.

> Shall I also make a replacement for the gtkmm-documentation patch in
> comment 21? A patch that does not change autogen.sh?

If you like, but it won't be a big problem to remove that later.
Comment 29 Tristan Van Berkom 2011-02-03 09:58:03 UTC
(In reply to comment #22)
> (In reply to comment 19)
> > (In reply to comment 12)
> >> I've noticed that GtkCellArea and GtkCellRenderer, which don't derive from
> >> GtkWidget, also contain the virtual get_request_mode function and the 4
> >> virtual get_preferred_xxx functions.
> 
> > Ah, then we should wrap those vfuncs too, in the same way.
> 
> -- Tristan, if you still read comments in this bug:
> 
> What you did in GtkWidgetClass helps gtkmm to make the C++ wrappers behave
> just like the underlying C classes. I.e. if a virtual function (e.g. get_xyz)
> is supposed to be called via gtk_baseclassname_get_xyz(), then put the default
> action in gtk_baseclassname_real_get_xyz(), and store a pointer to it in the
> virtual function table. In the cases where the virtual function returns void,
> and there is no default action, it's ok to have a NULL pointer in the vfunc
> table, but in other cases it simplifies for gtkmm if the default action is put
> in a function reached from the vfunc table.
> 
> Now there are NULL vfunc pointers also in GtkCellArea and GtkCellRenderer.
> At least these ones: GtkCellArea::get_request_mode, get_preferred_width,
> get_preferred_height, GtkCellRenderer::get_request_mode, but there may be
> others too. (get_preferred_[width|height] return void, but there is still a
> default action in GtkCellArea, consisting of a warning message.)
> 
> And I think I've found an error in
> gtk_cell_area_real_get_preferred_height_for_width(). It calls
> get_preferred_width(), but I think it should be get_preferred_height().
> 

Indeed, thanks for pointing that out, fixed.

I just committed something that should hopefully make binding GtkCellArea
a bit easier, As GtkCellArea and GtkCellRenderer are abstract classes they
leave a few things unimplemented, now instead of checking the vfunc I've
provided a default implementation for these vfuncs that simply prints
a warning (the same warning that would have been printed had the
vfunc not been there).

I've done this for the following cases:
   GtkCellRendererClass.get_request_mode

   GtkCellAreaClass.get_request_mode
   GtkCellAreaClass.create_context
   GtkCellAreaClass.copy_context
   GtkCellAreaClass.get_preferred_height
   GtkCellAreaClass.get_preferred_width
   GtkCellAreaClass.add
   GtkCellAreaClass.remove
   GtkCellAreaClass.foreach
   GtkCellAreaClass.foreach_alloc
   GtkCellAreaClass.focus

The special case of GtkCellRendererClass.get_size remains
unimplemented, this case is a little odd because it's a deprecated
api that we fall back on in the case that .get_preferred_xxx apis
are still unimplemented.

The relevant commits are:
    ce000db7f65a2066ea2c7e1806ec5830a37ee27f and
    b01fc35c189c8f934e0a63d09691ce1a6d7e01db

Hope this helps in the quest to wrap GtkCellArea classes in gtkmm.
Comment 30 Murray Cumming 2011-02-03 10:12:02 UTC
Thanks, Tristan.

> The special case of GtkCellRendererClass.get_size remains
> unimplemented, this case is a little odd because it's a deprecated
> api that we fall back on in the case that .get_preferred_xxx apis
> are still unimplemented.

Then we should not wrap that vfunc in gtkmm at all. We are trying to not wrap any deprecated API in gtkmm 3.0.0.
Comment 31 Kjell Ahlstedt 2011-02-10 12:25:36 UTC
Murray, are you waiting for me to do anything here?

I decided not to replace the gtkmm-documentation patch in comment 21, when you
said it's good enough.
The only possible problem I can think of with the gtkmm patch in comment 27 is
that it depends on the upgrade of _WRAP_VFUNC in bug 641165. That is, anyone
who wants to build gtkmm with the patch must first update glibmm.
Comment 32 Murray Cumming 2011-02-11 15:28:48 UTC
Thanks. Sorry. Yes, I've pushed this now.

And I've checked that we do not wrap the get_size() vfunc.

So this bug is all fixed, right? Many thanks.
Comment 33 Murray Cumming 2011-02-11 15:30:56 UTC
But does this now need some corresponding change in gtkmm-documentation's examples?
Comment 34 Kjell Ahlstedt 2011-02-11 19:10:17 UTC
(In reply to comment #33)
> But does this now need some corresponding change in gtkmm-documentation's
> examples?

Yes, that's done by the patch in comment 21, minus the change of autogen.sh that you've already committed (comment 26).
When the rest of the patch in comment 21 has been committed, this bug is really fixed. (The addition of *~ in .gitignore is optional, you can omit it if you don't like it.)
Comment 35 Murray Cumming 2011-02-12 13:33:05 UTC
Thanks. I have pushed that now.