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 655416 - append_column_numeric_editable doesn't compile with short or unsigned short
append_column_numeric_editable doesn't compile with short or unsigned short
Status: RESOLVED FIXED
Product: gtkmm
Classification: Bindings
Component: TreeView
2.24.x
Other Linux
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2011-07-27 12:32 UTC by Morpheus
Modified: 2011-09-07 14:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Basic test case (891 bytes, application/x-download)
2011-07-27 12:32 UTC, Morpheus
  Details
patch: Make TreeView::append_column_numeric_editable work for [unsigned] short (12.39 KB, patch)
2011-08-05 15:09 UTC, Kjell Ahlstedt
none Details | Review
patch: Make TreeView::append_column_numeric_editable work for [unsigned] short (14.89 KB, patch)
2011-09-07 14:35 UTC, Kjell Ahlstedt
none Details | Review

Description Morpheus 2011-07-27 12:32:38 UTC
Created attachment 192750 [details]
Basic test case

With a TreeModel column short or unsigned short, use of append_column_numeric_editable doesn't compile; if it isn't editable, it compile and works.

I've tested it with gtkmm 2.4 (apt: 1:2.24.0-1) and 3.0 (3.0.1-1), on Debian
testing.

Message thrown by compiler is:

In file included from /usr/include/gtkmm-2.4/gtkmm/combobox.h:34:0,
                 from /usr/include/gtkmm-2.4/gtkmm.h:123,
                 from main.cc:3:
/usr/include/gtkmm-2.4/gtkmm/treeview.h: In function ‘void
Gtk::TreeView_Private::_auto_store_on_cellrenderer_text_edited_string(const
Glib::ustring&, const Glib::ustring&, int, const Glib::RefPtr<Gtk::TreeModel>&)
[with ColumnType = short int]’:
/usr/include/gtkmm-2.4/gtkmm/treeview.h:2463:82:   instantiated from ‘void
Gtk::TreeView_Private::_connect_auto_store_editable_signal_handler(Gtk::TreeView*,
Gtk::CellRenderer*, const Gtk::TreeModelColumn<ColumnType>&) [with ColumnType =
short int]’
/usr/include/gtkmm-2.4/gtkmm/treeview.h:2181:5:   instantiated from ‘int
Gtk::TreeView::append_column_numeric_editable(const Glib::ustring&, const
Gtk::TreeModelColumn<ColumnType>&, const Glib::ustring&) [with ColumnType =
short int]’
main.cc:63:57:   instantiated from here
/usr/include/gtkmm-2.4/gtkmm/treeview.h:2492:9: error: invalid cast from type
‘const Glib::ustring’ to type ‘short int’

And, for 3.0:

In file included from /usr/include/gtkmm-3.0/gtkmm/combobox.h:36:0,
                 from /usr/include/gtkmm-3.0/gtkmm/appchooserbutton.h:27,
                 from /usr/include/gtkmm-3.0/gtkmm.h:101,
                 from main.cc:3:
/usr/include/gtkmm-3.0/gtkmm/treeview.h: In function ‘void
Gtk::TreeView_Private::_auto_store_on_cellrenderer_text_edited_string(const
Glib::ustring&, const Glib::ustring&, int, const Glib::RefPtr<Gtk::TreeModel>&)
[with ColumnType = short int]’:
/usr/include/gtkmm-3.0/gtkmm/treeview.h:2242:82:   instantiated from ‘void
Gtk::TreeView_Private::_connect_auto_store_editable_signal_handler(Gtk::TreeView*,
Gtk::CellRenderer*, const Gtk::TreeModelColumn<ColumnType>&) [with ColumnType =
short int]’
/usr/include/gtkmm-3.0/gtkmm/treeview.h:1960:5:   instantiated from ‘int
Gtk::TreeView::append_column_numeric_editable(const Glib::ustring&, const
Gtk::TreeModelColumn<ColumnType>&, const Glib::ustring&) [with ColumnType =
short int]’
main.cc:63:57:   instantiated from here
/usr/include/gtkmm-3.0/gtkmm/treeview.h:2271:9: error: invalid cast from type
‘const Glib::ustring’ to type ‘short int’

This bug it's (maybe) related with https://bugzilla.gnome.org/show_bug.cgi?id=118922
Comment 1 Kjell Ahlstedt 2011-08-01 17:09:25 UTC
The test case contains the comment

  /* If not editable, it works */
  m_TreeView.append_column_numeric_editable("index", model_columns.col, "%d");

That's part of the truth. There are 4 member functions to choose from. The
results when they are used for a TreeModelColumn<short> or
TreeModelColumn<unsigned short> are:

  append_column_numeric_editable(): Compilation error as specified in the bug
                                    description.
  append_column_editable(): Compilation error as specified in the bug
                            description.
  append_column_numeric(): Works correctly.
  append_column(): Compiles without errors or warnings. Runtime warnings:
    GLib-GObject-WARNING **: unable to set property `text' of type `gchararray'
    from value of type `glibmm__CustomBoxed_s'
    Nothing written in the TreeView cell.

append_column_numeric_editable() and append_column_editable() can be made to
work for [unsigned] short by adding template function specializations of
_connect_auto_store_editable_signal_handler<>() for types short and unsigned
short, but that won't fix append_column().

As described in bug 118922 comment 32, the conversion of numeric data to a
string in a column added with append_column() (or, I guess, insert_column())
depends on conversion functions registered by Glib with
g_value_register_transform_func(). No conversion functions from
[unsigned] short to string are registered, probably because [unsigned] short
are not fundamental types in the GType system. See
http://developer.gnome.org/gobject/unstable/gobject-Type-Information.html

The chapter on the TreeView widget in the gtkmm tutorial,
http://developer.gnome.org/gtkmm-tutorial/unstable/sec-treeview.html.en
says about TreeView::append_column():

  Note that (unsigned) short is not supported by default - You could use
  (unsigned) int or (unsigned) long as the column type instead. 

The reference documentation of TreeView::append_column(),
http://developer.gnome.org/gtkmm/unstable/classGtk_1_1TreeView.html
does not mention [unsigned] short, but gives examples of allowed data types,
and mentions the warning "GLib-GObject-WARNING **: unable to set property
`text' ..."

Some alternative ways to proceed:

1. Improve the reference documentation of TreeView::append_column(),
   append_column_editable(), append_column_numeric_editable(),
   insert_column(), insert_column_editable(),
   mentioning that [unsigned] short are unsupported types.

2. (can be combined with 1) Add declarations, but not definitions, of template
   function specializations of _connect_auto_store_editable_signal_handler<>()
   for types short and unsigned short. That will make the compilation errors
   from ..._editable() slightly more understandable.

   /opt/gnome/include/gtkmm-3.0/gtkmm/treeview.h:2077:6: warning: inline function ‘void Gtk::TreeView_Private::_connect_auto_store_editable_signal_handler(Gtk::TreeView*, Gtk::CellRenderer*, const Gtk::TreeModelColumn<ColumnType>&) [with ColumnType = short unsigned int]’ used but never defined
   /tmp/ccqn5gRK.o: In function `int Gtk::TreeView::append_column_numeric_editable<unsigned short>(Glib::ustring const&, Gtk::TreeModelColumn<unsigned short> const&, Glib::ustring const&)':
   /opt/gnome/include/gtkmm-3.0/gtkmm/treeview.h:1962: undefined reference to `void Gtk::TreeView_Private::_connect_auto_store_editable_signal_handler<unsigned short>(Gtk::TreeView*, Gtk::CellRenderer*, Gtk::TreeModelColumn<unsigned short> const&)'
   
3. Add definitions of template function specializations of
   _connect_auto_store_editable_signal_handler<>() for types short and
   unsigned short. That will make ..._editable() work, but it's inconsistent
   to make append_column_editable() work, but not append_column().

4. (shall be combined with 3) Register suitable conversion functions with
   g_value_register_transform_func(). 
   Is it really wise to do that? g_value_transform() is used by
   g_object_set_property(), so some new registered conversion functions can
   affect just about any GObject class.
Comment 2 Kjell Ahlstedt 2011-08-05 08:06:10 UTC
(In reply to comment #1)
> 3. Add definitions of template function specializations of
>    _connect_auto_store_editable_signal_handler<>() for types short and
>    unsigned short. That will make ..._editable() work, but it's inconsistent
>    to make append_column_editable() work, but not append_column().

Wrong! Shall read:

3. Add definitions of template function specializations of
   _connect_auto_store_editable_signal_handler<>() for types short and
   unsigned short. That will make append_column_numeric_editable() work.
   append_column_editable() will behave as append_column(), i.e. it compiles
   without errors or warnings, but it prints run-time warnings. It writes
   nothing in the TreeView cell.

The behaviour of append_column_editable() actually becomes worse with this
modification. A compile-time error is removed, but a run-time error remains.

I think it's possible to make a reasonably simple modification that affects
only append_column_numeric_editable(). Then append_column_numeric() and
append_column_numeric_editable() can be used for [unsigned] short data, but
append_column() and append_column_editable() cannot.

Hopefully I can attach a patch later today. If not, it will take a few weeks.
After today I will make a break from all work with gtkmm for 2-3 weeks.
Comment 3 Kjell Ahlstedt 2011-08-05 15:09:09 UTC
Created attachment 193314 [details] [review]
patch: Make TreeView::append_column_numeric_editable work for [unsigned] short

Here's a patch that makes TreeView::append_column_numeric_editable() work for
[unsigned] short columns. A few questions:

-- I've added a function template, and modified other function templates in
treeview.hg. Can that be done without breaking ABI? If any of the supported
compilers save information about function templates in an object code library,
I suppose my changes might break ABI. Is that possible, or is all information
about function templates always fetched from the header files when an
application file is compiled?

-- Why aren't there any insert_column_numeric() and
insert_column_numeric_editable() functions?
Comment 4 Murray Cumming 2011-09-06 08:04:59 UTC
(In reply to comment #3)
> Created an attachment (id=193314) [details] [review]
> patch: Make TreeView::append_column_numeric_editable work for [unsigned] short
> 
> Here's a patch that makes TreeView::append_column_numeric_editable() work for
> [unsigned] short columns. A few questions:

I'm a little surprised that
a) We had all those template specializations that seem to have the same copy/pasted implementations. Are they really all the same?
b) You now just call the new function from them. Are they even needed?
 
> -- I've added a function template, and modified other function templates in
> treeview.hg. Can that be done without breaking ABI? If any of the supported
> compilers save information about function templates in an object code library,
> I suppose my changes might break ABI. Is that possible, or is all information
> about function templates always fetched from the header files when an
> application file is compiled?

I don't think that's a problem.

> -- Why aren't there any insert_column_numeric() and
> insert_column_numeric_editable() functions?

Laziness?
Comment 5 Kjell Ahlstedt 2011-09-07 07:36:47 UTC
(In reply to comment #4)
> I'm a little surprised that
> a) We had all those template specializations that seem to have the same
> copy/pasted implementations. Are they really all the same?

Yes, the 6 template specializations for int, unsigned int, long, unsigned long,
float, double are really the same.

> b) You now just call the new function from them. Are they even needed?

I hope they are not needed, because I don't like them. This kind of repetition
of identical code for different data types is exactly what you can often
replace by a C++ template function. But I don't know how we can do without
them.

The problem is explained in a comment in the patch in comment 3. I'll try
another explanation here:

append_column_editable<>() and insert_column_editable<>() (without the patch
also append_column_numeric_editable<>()) call template function
_connect_auto_store_editable_signal_handler<ColumnType>().
There are 3 versions of _connect_auto_store_editable_signal_handler<>():

1. The general version for string data. It connects
   CellRendererText::signal_edited() to template function
   _auto_store_on_cellrenderer_text_edited_string<ColumnType>().

2. A bool specialization. It connects CellRendererToggle::signal_toggled()
   to function _auto_store_on_cellrenderer_toggle_edited_with_model().

3. Six identical (except for the type name) specializations for numeric data.
   They connect CellRendererText::signal_edited() to template function
   _auto_store_on_cellrenderer_text_edited_numerical<ColumnType>().

Perhaps the version for numeric data could be the general one? Then there could
be specializations for all possible string types. Are there other acceptable
string types than Glib::ustring and std::string?
Comment 6 Murray Cumming 2011-09-07 07:54:29 UTC
Thanks for the explanation. I guess the general string one is also for char*, const char*, char[], etc, so we'd end up adding those specializations instead.

It's a pity that we can't just say some_specialization<Numeric Types>, but nevermind.

Please commit, but with a comment saying that we need all those specializations, with identical implementations, because we chose to avoid multiple specializations for the string types by dealing with string types in the default implementation.
Comment 7 Kjell Ahlstedt 2011-09-07 14:35:49 UTC
Created attachment 195879 [details] [review]
patch: Make TreeView::append_column_numeric_editable work for [unsigned] short

Second version of this patch, with a long comment in treeview.hg explaining
why we need all those template specializations.

I have pushed the patch, and thus this bug has been fixed.