Iter to stl #1751 redux

[christopherlam]

Initially discussed in #1745, attempted further work in #1751 now rescinded. Main branch reverted with bd2d318.

More iter changes. This now works with 80f4e10 merged in.

• to fully be compatible with std::find_if and friends, the GncTreeIter seems to need iterator traits inherited from std::iterator. See the class definition
• GncTreeContainer adds an append() methods which adds a new GtkTreeIter and returns its GncTreeIter.
• GncTreeIter adds a get_gtk_tree_iter to return the GtkTreeIter to be useful when find_if returns the GncTreeIter. This creates another method to access the GtkTreeIter; one from the GncTreeIter and another from GncTreeData. The benefit is this allows:

 auto iter = std::find_if (...);
 gtk_list_store_set (store, &iter.get_gtk_tree_iter(), ...)

instead of the uglier

 auto iter = std::find_if (...);
 gtk_list_store_set (store, &(*iter).get_iter(), ...)

• Adds an example std::find_if use in import-match-picker.cpp.

[christopherlam]

With the operator-> able to access a GncTreeData, there's no need for a second accessor for the GtkIter.

[jralls]

Unit tests? Or do you think this is boilerplate pass-through that doesn't need them?

[jralls]

@christopherlam I need to study up on iterator class creation. This looks very strange but I imagine it's because of the way std::forward_iterator is implemented.

[christopherlam]

Unit tests

here you go.

[christopherlam]

EXPECT_EQ (iter, container.end()); looks like it should work, but doesn't ;)

[christopherlam]

latest iteration uses template type deduction and std::string overloading in set_column.

[christopherlam]

The only remaining concern is that gtk4 will deprecate some of these getters/setters however I think the replacement should be straightforward

[Bob-IT]

By using separate iter->set_column, does that mean multiple row-changed signals. It may not matter here but maybe some thing to watch out for if so.

[jralls]

It probably does, and setting a single column at a time instead of batching them into a single gtk_list_store_set will incur a bunch of overhead just like doing a bunch of xaccTransFoo without wrapping them in xaccTransBeginEdit/xaccTransCommitEdit.

[christopherlam]

It probably does, and setting a single column at a time instead of batching them into a single gtk_list_store_set will incur a bunch of overhead just like doing a bunch of xaccTransFoo without wrapping them in xaccTransBeginEdit/xaccTransCommitEdit.

It's just a convenience interface. More (IMHO) elegant than gtk_list_store_set.

[Bob-IT]

It is interesting what you have done and left a couple of comments.
Obviously John is in a better position to comment on the c++ code, one day I may try and get into it.

[christopherlam]

Thx for comments, a few from me

1. it seems gtk is moving towards https://docs.gtk.org/gio/class.ListStore.html so maybe this should be renamed gnc-list-container.hpp
2. thanks to template type deduction we need only 1 set_column which overloads std::string; however there's still a need for get_string and get_int in addition to get
3. get() should be renamed get_list_column to help disambiguate from other gets?
4. set() should be renamed set_list_column too
5. append() should be renamed append_row()?
6. pros of this change- use stl more elegantly. more concise (and more readable?) code. cons - the underlying gtk_list_store_get are less visible, so, unless one knows to look for the GncTreeContainer constructor, it's not very obvious that we're manipulating gtk objects. therefore it could make code less readable.

[jralls]

I thought about this strategy-- constructing the vector in the container ctor; however it means the container becomes a readonly copy of GtkListModel.

You can't use a vector--or any other monotype container--because the columns contain different types. It doesn't make it read-only, you can as easily write it back as read it out. You can use a struct/class if you think it will make for a cleaner interface to access the internals. Regardless, construction/destruction of the contained object whether it's a class or a tuple is not part of the container's job.

STL-type thin wrapper rather than take over the GtkList/TreeStore contents.

That makes it seem that you don't understand STL iterators. The whole point is to provide a consistent way to access the contained objects similar to a pointer into a basic array. Your attempt to combine iterator access with access to the object's internals breaks the model and is the source of your trouble.

I'm a bit pressed for time right now, more later.

[jralls]

I'm a bit pressed for time right now, more later.

So for the lightweight GncTreeData, GncTreeIter::operator* and GncTreeIter::operator-> have to return an object that either contains a GtkTreeModel* and a GtkTreeIter* to pass to the various GtkTreeModel functions along with whatever ever args those function takes. For proper separation of responsibility GncTreeIter increment/decrement operators should call GncTreeData increment/decrement operators or functions that wrap gnc_tree_model_next_iter/gnc_tree_model_prev_iter and return the boolean result. GncTreeIter can use the boolean to set its end and begin sentinels.

You can use a std::optional<GncTreeData>'s nullopt for the sentinels on forward and reverse iterators, but it won't work for a bidirectional or random one because it also loses the reference to the model; once you hit it you pretty much have to destroy and reconstruct the GncTreeIter. A bool m_valid or maybe m_begin and m_end flags will work with bidirectional and random iterators.

That lightweight version of GncTreeData is nicely universal, meaning that you don't need to specialize it for different GtkListModels, but it offers no C++ API to the GtkListModel. You can't call iter->foo() because there's no C++ object that implements foo. operator-> instead gives you access to m_model and m_iter so you call gtk_tree_model_foo(iter->m_model, &iter->m_iter, /* other args */).

The heavyweight version of GncTreeData will get very heavy and very complicated.

[christopherlam]

You're modeling the container wrong. Think of a GtkTreeModel as a std::vector<std::tuple</*…*/>> where the tuple contains all of the columns in a tree entry. An iterator points to, and its get and set methods read and write, the tuple. The tuple can itself contain vector-of-tuple elements unless its a GtkListModel and unless you're willing to spend some time (maybe a lot of time) poring over the STL implementations looking for how to iterate over trees you probably want to restrict this to GtkListModel with only POD in the tuple.

I'm very aware that a GtkListModel or GtkTreeModel describes a 2D spreadsheet, whereby columns may be of different types (bool/int/void*). (And a GtkTreeModel has subrows cf GtkListModel doesn't). So a vector<tuple<bool,int,gpointer>> can contain all of the GtkListMode Data.

Are you describing a GtkListContainer whose ctor will pull out all of the data into the vector<tuple>? What's the benefit of this?

This branch in itself uses the same iterator as the underlying gtk one, and the GncTreeIter's getters/setters simply call the gtk ones. If we use the vector then we'd need to find the iter's path, find the right GtkPath in the GtkListModel, then get/set in the GtkListModel.

This branch IMHO presents an already lightweight version. Nothing needs to be added to support other GtkListModels.

[jralls]

You mean vector<variant<bool, int, void*>>, but no, that would not be a good way to represent a row. You'd have no way to extract the real type of whatever had been stuck into the void*.

Are you describing a GtkListContainer whose ctor will pull out all of the data into the vector? What's the benefit of this?

Absolutely not. In a heavyweight design the GncTreeData's get() method would convert the row's contents into something easily digested by C++ code and the set() method would take that representation and write it back into the GtkTreeModel row.The GncListModelContainer's sole job is to provide the begin and end functions; in fact there doesn't even need to be a GtkListModelContainer class, those could more easily be free functions that take a GtkListModel* as a parameter.

The benefit is a representation that fits nicely into C++ and exposes the members to the compiler for type assurance. I don't think that the time required to develop it nor the runtime cost is worth that benefit, which is why I laid out the lightweight design where get returns a tuple or a struct with the GtkTreeModel* and GtkTreeIter* to use with the GtkTreeModel API.

This branch in itself uses the same iterator as the underlying gtk one, and the GncTreeIter's getters/setters simply call the gtk ones.

Completely missing the point: The GncTreeIter shouldn't have getters or setters. That's not an iterator's job. The thing that's the iterator's value might have getters and setters. That's the GtkListModel*.

[jralls]

in fact there doesn't even need to be a GtkListModelContainer class, those could more easily be free functions that take a GtkListModel* as a parameter.

Note that that's true for standard algorithms that take begin and end parameters but not true for range-based for: That does require it to be a class.

[christopherlam]

In this case I completely don't get it... It would be best revert completely the gnc-tree-container.hpp and retain the previous gtk iterators.

This branch restores the GncTreeContainer deleted in bd2d318, renamed as GncListModelContainer.

[christopherlam]

Instead of us describing the characteristics of the class, how about we decide the usage pattern of such class instead? So far my approach had been:

  auto container = GncListModelContainer (GTK_LIST_STORE (model));
  for (auto iter : container)
  {
    auto acc = iter.get_column<Account*> (COL_ACCOUNT);
    iter.set_column_string (COL_ACCOUNT_NAME, xaccAccountGetName (acc));
  }

[jralls]

Consider

std::vector<int> foo{0, 1, 2};
for (bar : foo)
   std::cout << typeid(bar).name() << std::endl;

What do you think that will print?

[christopherlam]

No contest:

i
i
i

[jralls]

Good. How about typeid(foo.begin()).name()?
If instead of using range-based for, suppose you wrote the loop

for (auto iter = foo.begin(); iter != foo.end(), ++foo)
    std::cout << typeid(XXX).name() << std::endl;

What would you put into XXX to get the same result as the range-based loop? (This is what the range-based loop does internally.)

[christopherlam]

Easy: *iter

This is exactly the approach originally

GncTreeIter iter = container.begin();
if (iter != container.end())
{
  GncTreeData data = *iter:
  std::string s = data.get_string(COL_NAME);
}

[jralls]

And that is correct. GncTreeData has the get method, not GncTreeIter.

It's not a great interface design though: You have to go find the declaration of the underlying GtkModel to discover COL_NAME and that it stores a string. There's also the potential efficiency issue that @Bob-IT brought up about calling gtk_tree_model_get/set once per column instead of just one with all the columns you need. On the other hand it does consolidate and hide all of fluff with passing output parameters and cleaning up the result.

Which returns us to the original question, which now that I understand better what's going on I can phrase more coherently: Having to keep copies of model at every level (container needs it to generate iterators, GncTreeIter needs it to call gtk_tree_model_iter_next and friends, and GncTreeData needs it to get and set data) is ugly, as is having to keep two copies of the GtkTreeIter, worsened by having to keep those two in sync. The GncTreeIter/GncTreeData duplication can be resolved by having GncTreeIter delegate operator++ and operator== to GncTreeData, e.g. (just a sketch, much detail left out)

class GncTreeData
{
    GtkTreeModel *m_model;
    GtkTreeIter m_iter;
public:
    GncTreeData(GtkTreeModel* model, bool is_end) : m_model{model}, m_iter{}
   {
        if (!is_end)
            gnc_tree_model_get_iter_first(m_model, &m_iter);
   }
   bool iter_equal(const GncTreeData& other);
   bool next_iter()
   {
        if (m_iter.stamp)
            return gtk_tree_model_iter_next(m_model, &m_iter);
        return false;
   }
   std::pair<GtkTreeModel*, GtkTreeIter*> get_iter()
   {
        // parameters needed for calling arbitrary GtkTreeModel functions
        return {m_model, &m_iter};
   }
};

class GtkTreeIter
{
    GncTreeData m_data;
    bool m_end{false};
protected:
    bool is_end()
    {
        return m_end;
    }
    const GncTreeData& get_model() const
    {
         return m_model;
    }
public:
    GtkTreeIter(GtkTreeModel* model, bool end = false) : m_data{model, end}, m_end{end} {}
    operator==(const GncTreeIter& other)
    {
       if (m_end && other.is_end())
            return true;
        if (m_end || other.is_end())
            return false;
        return m_data.iter_equal(other.get_model());
    }
    operator++()
    {
        m_end = !m_model.next_iter();
     }
};

class GncTreeModelContainer
{
    GtkTreeModel* m_model;
public:
    GncTreeModelContainer(GtkTreeModel*) : m_model{model} {}
    GncTreeIter begin()
    {
        return GncTreeIter(m_model);
    }
    GncTreeIter end()
    {
        return GncTreeIter(m_model, true);
    }
};

BTW, I haven't yet looked at your latest push, I'm just trying to show a minimal design. If you've already worked this out I'll find out tomorrow.

[christopherlam]

This would mean that the data struct now has next() duties which IMHO belongs to the iter struct.
Update: ok the iter ultimately does the ++. Still, if iter has a copy of the model, it doesn't need to call data.get_model().

[christopherlam]

Update 2: if the main concern is the m_model being passed around, then we could solve by making Iter a child class within Container.

[jralls]

This would mean that the data struct now has next() duties which IMHO belongs to the iter struct.
Update: ok the iter ultimately does the ++. Still, if iter has a copy of the model, it doesn't need to call data.get_model().
Update 2: if the main concern is the m_model being passed around, then we could solve by making Iter a child class within Container.

You mean get_iter(), and it doesn't: It calls next_iter which calls gtk_tree_model_next to modify the GtkTreeIter. GncTreeIter doesn't need either the GtkTreeModel* or GtkTreeIter, but GncTreeData does. This how-to on custom iterators says that the iterator class should in fact be declared inside the container class, e.g.

class GncTreeModelContainer
{
      class GncTreeIter { /*…*/};
     /*…*/
};

But that doesn't confer any access to GncTreeModelContainer's private members, it just changes the namespace of the type from GtkTreeIter to GtkTreeModelContainer::GtkTreeIter.

[christopherlam]

I'll leave this parked for now. I'm not sure how to make the iter more lightweight than it is. The container should ideally be capable of all the capabilities demonstrated in the test suite.