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 511136 - TreeView::get_cursor() may return an invalid TreePath instance
TreeView::get_cursor() may return an invalid TreePath instance
Status: RESOLVED FIXED
Product: gtkmm
Classification: Bindings
Component: TreeView
2.12.x
Other All
: Normal minor
: ---
Assigned To: Daniel Elstner
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2008-01-21 21:46 UTC by Tom Jaeger
Modified: 2012-04-10 12:26 UTC
See Also:
GNOME target: ---
GNOME version: 2.19/2.20


Attachments
patch: gmmproc: Add macro _DEFAULT_COBJECT_FUNC in _CLASS_BOXEDTYPE (glibmm) (2.52 KB, patch)
2011-11-12 09:09 UTC, Kjell Ahlstedt
none Details | Review
patch: TreePath: All constructors guarantee that gobject_ != 0. (gtkmm) (9.00 KB, patch)
2011-11-12 09:12 UTC, Kjell Ahlstedt
none Details | Review
patch: gmmproc: Add macro _CUSTOM_CTOR_CAST in _CLASS_BOXEDTYPE. (glibmm) (2.28 KB, patch)
2012-04-05 16:22 UTC, Kjell Ahlstedt
none Details | Review
patch: TreePath: All constructors guarantee that gobject_ != 0. (gtkmm) (3.07 KB, patch)
2012-04-05 16:23 UTC, Kjell Ahlstedt
none Details | Review

Description Tom Jaeger 2008-01-21 21:46:27 UTC
TreeView::get_cursor "returns" a TreePath, which according to the C documentation, may be a NULL pointer (the gtkmm documenation is quiet about this).  The only way to check if the path is valid that I can see is checking if path.gobj() is valid, which is (i) ugly and (ii) undocumented.

My suggestion would be to implement
TreePath::operator bool () const {
    return gobj() != NULL;
}

or at least update the documentation to explain how the validity of a returned TreePath can be checked.


The returned TreeViewColumn is probably affected as well.

Other information:
Comment 1 Murray Cumming 2008-01-24 10:42:51 UTC
Isn't TreePath::empty() enough?

Comment 2 Tom Jaeger 2008-01-24 22:35:35 UTC
TreePath::empty() does work, but it will throw the following error message:

Gtk-CRITICAL **: gtk_tree_path_get_depth: assertion `path != NULL' failed

The issue here is that get_cursor doesn't return an empty TreePath, it returns a NULL-pointer TreePath, so all of the gtk_tree_path_* functions will produce the above error message.
Comment 3 Murray Cumming 2008-01-26 10:57:01 UTC
Ah. I'd happily accept a patch against svn trunk that did the extra check in empty() and also added an operator bool() that called empty().

As a workaround in the meantime, I guess you should indeed use if(treepath.gobj())


Comment 4 Murray Cumming 2008-02-20 13:31:42 UTC
I'd still like a patch.
Comment 5 Murray Cumming 2009-01-19 12:52:22 UTC
Fixed in svn trunk.
Comment 6 Daniel Elstner 2009-02-12 03:01:29 UTC
I think this change is broken and a bad idea. It makes no sense to make empty() work on a TreePath which is essentially in a state it should never have got into in the first place. The remainder of the TreePath methods would still fail anyway.

The correct fix is to change get_cursor() to treat the NULL case specially, just as we already do for many string return values.

On a side note, I don't think it is a good idea to add operator bool() to classes all over the place. It will make any instance implicitly convertible to integer type.
Comment 7 Murray Cumming 2009-02-12 10:00:20 UTC
(In reply to comment #6)
> I think this change is broken and a bad idea. It makes no sense to make empty()
> work on a TreePath which is essentially in a state it should never have got
> into in the first place.

It's debatable whether it should be in that state. 

> The remainder of the TreePath methods would still fail
> anyway.

But checking with operator bool can avoid that.

We use the same technique elsewhere already, at least in pangomm, I think.

> The correct fix is to change get_cursor() to treat the NULL case specially,
> just as we already do for many string return values.

But we can't be sure that we won't get a NULL GtkTreePath* from someplace else, so I'd like to keep this operator bool. I'm fine with you also adding a get_cursor() method overload with a bool& parameter, assuming that's what you mean.
Comment 8 Daniel Elstner 2009-02-12 11:29:24 UTC
(In reply to comment #7)

> > work on a TreePath which is essentially in a state it should never have got
> > into in the first place.
> 
> It's debatable whether it should be in that state.

TreePath does not, in any way, represent a pointer or pointer-like data structure. It isn't a handle. The interface is that of an STL container. Also, there is no need to make a distinction between an invalid TreePath and an empty one.

> 
> > The remainder of the TreePath methods would still fail
> > anyway.
> 
> But checking with operator bool can avoid that.

So we end up with two ways to represent an empty TreePath. One is actually empty but valid, the other isn't a valid TreePath at all. As far as empty() is concerned, they are now the same thing. But they suddenly aren't the same thing anymore if you use some other method on it.  Why make empty() work, but not size() == 0, or begin() == end(), or...

In other words: If the intent is to represent the state "no cursor position" as an empty TreePath, then why not actually make it an empty TreePath? Why invent an additional state "in limbo"?

The case with nstring in gtkmm 1.2 was much less clear-cut, and we still got rid of it.

> We use the same technique elsewhere already, at least in pangomm, I think.

I hope these usages actually introduce a distinct state, and not just an alternative internal representation for one and the same logical state.

> > The correct fix is to change get_cursor() to treat the NULL case specially,
> > just as we already do for many string return values.
> 
> But we can't be sure that we won't get a NULL GtkTreePath* from someplace else, so I'd like to keep this operator bool.

If operator bool() weren't identical to empty() but would actually indicate a distinct state, it would be acceptable.  That is, it would be acceptable if we actually needed to somehow represent a state distinct from an empty path. But we don't, as far as I'm aware.

> I'm fine with you also adding a
> get_cursor() method overload with a bool& parameter, assuming that's what you
> mean.

Eeek, no. Output parameters are a horrible last resort. And having such an overload *in addition* to some other means to represent that state is just ugly.
Comment 9 Murray Cumming 2009-02-12 16:02:43 UTC
> TreePath does not, in any way, represent a pointer or pointer-like data
> structure. It isn't a handle. The interface is that of an STL container. Also,
> there is no need to make a distinction between an invalid TreePath and an empty
> one.

We don't make a distinction. We do say that a (internally) invalid one is as useless as an empty one. As far as I know there is know, and empty TreePath has no meaning other than, "no path".

A std::vector can be empty too. It doesn't have operator bool() for that. Are you saying that you'd be happy with just making empty() check for an null gobj()?

> Eeek, no. Output parameters are a horrible last resort. And having such an
> overload *in addition* to some other means to represent that state is just
> ugly.

I'm not sure anymore what you actually want. 

Comment 10 Daniel Elstner 2009-02-12 16:48:15 UTC
(In reply to comment #9)
> We don't make a distinction. We do say that a (internally) invalid one is as
> useless as an empty one. As far as I know there is know, and empty TreePath has
> no meaning other than, "no path".

But it is still a valid container in one case but not in another.

So why have two internal states to say the same thing?

> A std::vector can be empty too. It doesn't have operator bool() for that.

But std::vector::empty() does not attempt to handle the situation where the vector object is corrupted and in an invalid state.

> Are
> you saying that you'd be happy with just making empty() check for an null
> gobj()?

Er, no. That's how it is right now.

    bool TreePath::empty() const
    {
      if(!gobject_)
        return false;

      return (gtk_tree_path_get_depth(gobject_) == 0);
    }

> I'm not sure anymore what you actually want.

Ditch the check for NULL and operator bool(), and custom-code TreeView::get_cursor() to return an empty but valid path if the GTK+ function returns NULL.
Comment 11 Daniel Elstner 2009-02-13 18:17:02 UTC
OK.  Murray and I discussed the problem on IRC.  We agreed on removing operator bool() again, and making sure that the object is never in an invalid state to begin with.  However, this should be done in a generic way and therefore needs to go into the constructor.

Unfortunately the constructor is auto-generated, and it is not possible to easily override the autogeneration. For this reason this won't be a quick fix, unfortunately, but will have to wait until someone finds the time to add another exception flag to gmmproc to suppress the autogeneration of the constructor.

However, the removal of operator bool() doesn't have to wait until then.  In fact, it needs to be removed before the API freeze, so I'll do it right now.
Comment 12 Hubert Figuiere (:hub) 2009-06-24 17:18:58 UTC
(In reply to comment #10)

> > Are
> > you saying that you'd be happy with just making empty() check for an null
> > gobj()?
> 
> Er, no. That's how it is right now.
> 
>     bool TreePath::empty() const
>     {
>       if(!gobject_)
>         return false;
> 
>       return (gtk_tree_path_get_depth(gobject_) == 0);
>     }
> 

Bug 586437 actually shows that this code is wrong. If gobjet_ is NULL TreePath::empty() should return 'true'.  Bug 586437 has been fixed in trunk.
Comment 13 Daniel Elstner 2009-06-24 18:57:46 UTC
Yes. It basically means that the questionable check didn't even work correctly.
Comment 14 Kjell Ahlstedt 2011-05-10 14:58:31 UTC
(In reply to comment #11)
> Unfortunately the constructor is auto-generated, and it is not possible to
> easily override the autogeneration. For this reason this won't be a quick fix,
> unfortunately, but will have to wait until someone finds the time to add
> another exception flag to gmmproc to suppress the autogeneration of the
> constructor.

I think it's easy to fix gmmproc. I just need to know more precisely what you
want it not to do.

TreePath is declared as
  _CLASS_BOXEDTYPE(TreePath, GtkTreePath, gtk_tree_path_new,
                   gtk_tree_path_copy, gtk_tree_path_free)

The autogenerated constructors are
  TreePath();
  explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true);
  TreePath(const TreePath& other);

The autogeneration of the default constructor can be suppressed by including
the m4 macro _CUSTOM_DEFAULT_CTOR in the .hg file.
I suppose you want to suppress the autogeneration of
  explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true);

It would be easy to add a macro for that. What about _CUSTOM_WRAP_CTOR? That
constructor is used by the 'wrap' function.

Unfortunately there is an inconsistency in the way _CUSTOM_xxx macros work in
_CLASS_BOXEDTYPE, _CLASS_BOXEDTYPE_STATIC, and _CLASS_OPAQUE_COPYABLE.
They suppress both the declaration in the .h file, and the implementation in
the .cc file.

In _CLASS_GOBJECT and _CLASS_GTKOBJECT, _CUSTOM_xxx means "generate the
declaration in the .h file, but suppress the implementation in the .cc file."
_NO_xxx means "suppress both declaration and implementation." The same goes for
optional arguments custom_xxx and no_xxx in _WRAP_SIGNAL and _WRAP_VFUNC.

What shall _CUSTOM_WRAP_CTOR (or whatever it shall be called) do? Shall it
suppress both declaration and implementation, or only implementation?

Btw, the hand-written constructor
  explicit TreePath(const Glib::ustring& path);
can also construct a TreePath with gobject_ == 0.

> However, the removal of operator bool() doesn't have to wait until then.  In
> fact, it needs to be removed before the API freeze, so I'll do it right now.

Was TreePath::operator bool() ever removed? It seems to have been replaced by
operator const void*().
Comment 15 Murray Cumming 2011-10-05 11:10:17 UTC
(In reply to comment #14)
> > However, the removal of operator bool() doesn't have to wait until then.  In
> > fact, it needs to be removed before the API freeze, so I'll do it right now.
> 
> Was TreePath::operator bool() ever removed? It seems to have been replaced by
> operator const void*().

Apparently, no, it wasn't removed. We did, however, change all operator bool()s to operator void*() to avoid unintended conversions. This is apparently correct.
Comment 16 Kjell Ahlstedt 2011-11-12 09:09:38 UTC
Created attachment 201281 [details] [review]
patch: gmmproc: Add macro _DEFAULT_COBJECT_FUNC in _CLASS_BOXEDTYPE (glibmm)

What about this solution? In a class of type _CLASS_BOXEDTYPE it's possible to
call the new m4 macro _DEFAULT_COBJECT_FUNC that specifies a function that
shall be used in constructor
  explicit CppObjectType(BaseObjectType* gobject, bool make_a_copy = true);
to create a new C object, if gobject == 0.

It can be called as
  _DEFAULT_COBJECT_FUNC(NEW) // Use the 3rd parameter of _CLASS_BOXEDTYPE
or
  _DEFAULT_COBJECT_FUNC(function_that_returns_a_BaseObjectType_pointer)

If someone can suggest a better name for the new macro, I'm willing to change
it. An alternative would be to add an extra optional parameter to the
_CLASS_BOXEDTYPE macro.
Comment 17 Kjell Ahlstedt 2011-11-12 09:12:14 UTC
Created attachment 201282 [details] [review]
patch: TreePath: All constructors guarantee that gobject_ != 0. (gtkmm)

Use the new m4 macro _DEFAULT_COBJECT_FUNC in treepath.hg.
Hand-coded constructors never construct an object with gobject_ == 0.

I've also updated .gitignore. Should that be in a separate commit?

(In reply to comment #0)
> The returned TreeViewColumn is probably affected as well.

No, it's not affected in the same way as TreePath. The prototype of
TreeView::get_cursor() is
  void get_cursor(TreeModel::Path& path, TreeViewColumn*& focus_column);
i.e. focus_column is a pointer. If no GtkTreeViewColumn is found, get_cursor()
returns a null-pointer, which is much better than an invalid object.
Comment 18 Kjell Ahlstedt 2011-11-24 15:02:38 UTC
Daniel, 2-3 years ago you had a definite opinion about how to modify TreePath.
What do you say about the solution I suggest in comment 16 and comment 17?

Of course I could add a possibility to suppress the autogeneration of the
constructor
  explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true);
and hand-code it, as you discuss in comment 11.
My proposed modification of gmmproc instead autogenerates a constructor with
the wanted functionality. Perhaps the same modification can be useful in other
classes in the future.
Comment 19 Kjell Ahlstedt 2012-03-14 15:20:27 UTC
(In reply to comment #17)
> I've also updated .gitignore. Should that be in a separate commit?

Was pushed separately 2012-02-16.
Comment 20 Kjell Ahlstedt 2012-04-05 16:22:07 UTC
Created attachment 211400 [details] [review]
patch: gmmproc: Add macro _CUSTOM_CTOR_CAST in _CLASS_BOXEDTYPE. (glibmm)

I've changed my mind. This patch adds _CUSTOM_CTOR_CAST instead of
_DEFAULT_COBJECT_FUNC, which I suggested in comment 16. It's more consistent,
since there is already a _CUSTOM_CTOR_CAST m4 macro in _CLASS_BOXEDTYPE_STATIC.
It's also what's asked for in comment 11.

When _CUSTOM_CTOR_CAST is used, gmmproc does not generate an
  explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true);
constructor.
Comment 21 Kjell Ahlstedt 2012-04-05 16:23:50 UTC
Created attachment 211401 [details] [review]
patch: TreePath: All constructors guarantee that gobject_ != 0. (gtkmm)

And here's the corresponding gtkmm patch.

Use the new m4 macro _CUSTOM_CTOR_CAST in treepath.hg. Hand-code
  explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true);
Hand-coded constructors never construct an object with gobject_ == 0.

I'll soon push these patches unless someone objects.
Comment 22 Murray Cumming 2012-04-10 07:25:12 UTC
Yes, please.
Comment 23 Murray Cumming 2012-04-10 12:26:01 UTC
Actually, I have pushed both. Thanks.