Created attachment 24442 [details] [review]
gtkmm_treemodel_constiter.patch

So, I found that:
1. I was wrong about the conversion - you _can_ convert an iterator to
a const_iterator.
2. But things like stl::list::insert() use iterator instead of
const_itearator as parameters, even though they can't be changed. So
it's not so important to do that in Gtk::TreeModel anyway. This seems
to be a bug in the C++ standard - see the Effective STL book, as
discussed on list.

But:
TreeRow currently inherits from TreeIter so that we can cast a TreeRow
to a TreeIter, so we can return a TreeRow pointer or reference from
TreeIter::operator* and TreeIter::operator->, without complicated
memory management. But TreeRow can only have one base class (iterator)
and that will not allow us to cast to a TreeRow from a
const_itearator. TreeNodeChildren does the same inheritance trick.

Possible solutions for this are:
1. Make TreeRow and TreeNodeChildren templates as well? But what
should the template types be? The half-finished patch
(gtkmm_treemodel_constiter2.patch) tries this, but
  template<_Tp, _Ref, _Ptr>
  class TreeRow : public TreeIter<_Tp, _Ref, _Ptr>
is problematic when _Tp is itself TreeRow. This seems like a recursive
declaration.

2.
Have a TreeRow (and maybe a TreeNodeChildren) instance in every
TreeIter instance? But that would be slow, I think.

At the moment, it doesn't seem worth the bother.

Created attachment 24939 [details] [review]
/gtkmm_treemodel_constiter2.patch (an unfinished experiment)

That fact that std::list<>::insert() demands an iterator as argument is not a
bug but a feature.  const_iterator means "the data pointed to is not
modifiable".  If you insert() a new element, the container is modified.  OK,
strictly speaking the data to which the iterator points is not modified by the
insertion.  But you still need the container to be modifiable -- that is,
non-const -- and thus you would have access to non-const begin() and end()
anyway.  Apart from that, for reference-counted types like std::string the
non-const begin() and end() methods force a copy of the string data, which is
mandatory for insert() to work.

About the conversion: Yes, you can convert an iterator to a const_iterator (but
not the other way around), just like you can convert char* to const char*
(remember, iterators are just like pointers, and const_iterator is like a
pointer to a constant object).

*** Bug 162693 has been marked as a duplicate of this bug. ***

There appears to be a const_iterator now [1].  Shouldn't this be marked fixed?

[1] http://www.gtkmm.org/docs/gtkmm-2.4/docs/reference/html/classGtk_1_1TreeModel.html#b25e01638bef26f1e93617e36b212292

Dave, I don't think it's a real const iterator.

Created attachment 335443 [details]
patch: Gtk::TreeIter: Make a real const_iterator

It's a tar.gz file with these files:

- 0001-Gtk-TreeIter-Make-a-real-const_iterator.patch
  gtkmm patch:
  Gtk::TreeIter: Make a real const_iterator

- 0001-Update-to-suit-the-restructured-Gtk-TreeIter-class-h.patch
  gtkmm-documentation patch:
  Update to suit the restructured Gtk::TreeIter class hierarchy

- treeiter.ccg
- treeiter.hg
  Updated versions of these files. They are included in the patch. These files
  are thoroughly modified. It's probably difficult to understand the patch, and
  the patch will probably not apply to these files once they are changed because
  of other updates.

This is an updated suggestion for a new TreeIter. It breaks API and ABI.
Perhaps it's worth considering for gtkmm4? There are probably details that need
refinement.

Now we are working towards gtkmm4. It's time to fix this bug.
Is my patch in comment 8 ok? It restructures the TreeIter class hierarchy.

Present class hierarchy:
  class TreeIterBase
  class TreeIter : public TreeIterBase
  class TreeRow : public TreeIter
  class TreeNodeChildren : public TreeIter

Proposed new class hierarchy:
  class TreeIterBase
  class TreeIterBase2 : public TreeIterBase
  class TreeIterBase3 : public TreeIterBase2
  template <typename T>
  class TreeIter : public TreeIterBase3
  class TreeRow : public TreeIterBase2
  class TreeNodeChildren : public TreeIterBase2

The template parameter T in TreeIter<T> must be TreeRow or const TreeRow.

With this new class hierarchy, a TreeRow and a TreeNodeChildren are no longer a
TreeIter. Which is a good thing, I think.

I have pushed patches similar to the ones in comment 8.
https://git.gnome.org/browse/gtkmm/commit/?id=5e420c96f1ce3517295eb15e5425c918174c6730
https://git.gnome.org/browse/gtkmm-documentation/commit/?id=5fbdf18e8634a1914cfbc316e3816d1328860b52

A const-correct TreeIter is not enough. We also need const versions of TreeRow
and TreeNodeChildren. They are actually disguised iterators.
A const TreeRow can easily and implicitly be copied to a non-const TreeRow,
which can be used for changing the contents of the underlying TreeModel.
We need TreeConstRow and TreeNodeConstChildren, which can't implicitly be
converted or copied to a TreeRow and TreeNodeChildren.
https://git.gnome.org/browse/gtkmm/commit/?id=acd6dcfb65c61e8517ed84a51721cdd2bcdeeeb5
https://git.gnome.org/browse/gtkmm-documentation/commit/?id=1de607e4e494e468d3a5faccf8e01c3f81b2f35a

It seems that we cannot compare (with = or !=) a TreeModel::iterator and const TreeModel::const_iterator (Gtk::TreeIter<Gtk::TreeRow> and Gtk::TreeIter<Gtk::TreeConstRow>, I think). Apparently that should be generally possible:

https://stackoverflow.com/questions/35390835/is-comparison-of-const-iterator-with-iterator-well-defined/35392987#35392987

Created attachment 347926 [details] [review]
patch: Gtk::TreeIter<>: Fix operator==() and operator!=()

I think this patch will fix it. Have you got a test case? Or do I have to make
one? Or is it obvious enough, so we don't need a test case?

Thanks. I have pushed that, and a very simple test case.

Created attachment 347990 [details] [review]
0001-Tree-ComboBox-Change-some-parameters-to-const_iterat.patch

Do you agree that we should changes these method parameters to const_iterator? I noticed some of these while building Glom. I guess there might be more to change.

We also seem to have lost implicit conversions from TreeRow to TreeIterator. For instance, in gtkmm-2.4, we can pass a TreeRow to a method that takes a TreeIterator. Is this change intentional?

Comment on attachment 347990 [details] [review]
0001-Tree-ComboBox-Change-some-parameters-to-const_iterat.patch

(In reply to Murray Cumming from comment #15)
> Do you agree that we should changes these method parameters to
> const_iterator?

I have pushed this, and another commit to change some vfunc parameters to use const_iterator:
https://git.gnome.org/browse/gtkmm/commit/?id=2a2e78a47cf05dbe7357eac1a7274d510b39536a

Please feel free to change any of it back if you disagree.

Your changes of method parameters to const_iterator are fine. I expected that
some uses of Treemodel::iterator could be changed to const_iterator. I only
changed some of the most obvious occurrences.

> We also seem to have lost implicit conversions from TreeRow to TreeIterator.
> Is this change intentional?

More or less. From comment 9:
> With this new class hierarchy, a TreeRow and a TreeNodeChildren are no longer
> a TreeIter. Which is a good thing, I think.

A TreeRow can be regarded as an element in a TreeNodeChildren container.
An implicit conversion from an element to an iterator is not what you usually
expect from a container.

It's easy to get an iterator from a TreeRow with TreeRow::get_iter().
It would probably be possible to have implicit conversion by adding
  TreeRow::operator TreeIter<TreeRow>();
  TreeRow::operator TreeIter<TreeConstRow>() const;
but I don't recommend it.

The implicit conversion from TreeRow to iterator was often convenient, though I'm open to the idea that it's a symptom of something else that should be fixed instead.

I seem to have used it almost always when using the result of TreeModel::children(), which returns a "virtual container" returning TreeRows rather than iterators.
  https://developer.gnome.org/gtkmm/stable/classGtk_1_1TreeModel.html#a62142e8a5beffb04a2b643d7f62c890f

so you can do this, for instance, though it's not pretty:

for (auto row : model->get_children())
{
  if(row[m_columns_alignment.m_col_alignment] == alignment)
  {
    m_combo->set_active(row);
    // Now: m_combo->set_active(row.get_iter());
    break;
  }
}

Maybe TreeModel::children should instead return iterators because that's more likely to be what you want.

(In reply to Murray Cumming from comment #19)
> Maybe TreeModel::children should instead return iterators because that's
> more likely to be what you want.

That would be awkward, as the TreeModelChildren's own iterator is already a TreeModel::iterator. And I guess I should just not use a range-based for if I want the iter, just as with a std container.

Thanks.