Created attachment 58457 [details]
Implementation of the ListBox wrapper for TreeView

I actually kind of like the idea of a convenience wrapper in this case.  Thanks for starting the discussion.  A couple of initial thoughts on the implementation:

- At first glance, the name ListBox made me think that it was related to ComboBox -- this should probably be renamed to ListView or something like that if we include it in gtkmm

- It should probably inherit from TreeView instead of creating a new class that contains a TreeView.  That way we would retain the benefits of polymorphism.  As it is now, it's not a member of the Gtk::Widget heirarchy, so it couldn't be added directly to a container widget.  You'd have to use an ugly hack like myContainer.add(*(myListBox.get_treeView()));

- Another impact of not using inheritance is that there's no easy way to access the signals / events of the widget without calling get_treeView() and then using the TreeView interface.  I assume that even if people wanted a simpler interface for simple use-cases, they'd still need to attach to signals, so obscuring this interface doesn't seem like a good idea (yes, this is directly related to the previous point, but I thought I'd mention it explicitly).

- There is already a specialization like this for ComboBox called ComboBoxText  (http://www.gtkmm.org/docs/gtkmm-2.4/docs/reference/html/classGtk_1_1ComboBoxText.html).  It would be nice if the interface for this specialization could match that as closely as possible (The only real difference between them seems to be that with the ListView/ListBox you can have a multiple-selection widget whereas the ComboBox can only have single-selection).

- put_row() seems to be misnamed.  To me it looks more like it should be called set_row().  Both put_row() and get_row() could be implemented with operator[]() as well.  Also note that ComboBoxText does not provide either of these convenience functions.

- the function get_rows_number() should probably be called something like size() instead to make it more consistent with similar existing interfaces in gtkmm and the standard library

(In reply to comment #2)
> I actually kind of like the idea of a convenience wrapper in this case.  Thanks
> for starting the discussion

I think I have addressed all of them with the new attachment.

> - put_row() seems to be misnamed.  To me it looks more like it should be called
> set_row().  Both put_row() and get_row() could be implemented with operator[]()
> as well.  Also note that ComboBoxText does not provide either of these
> convenience functions.

Well, the problem is that get_value doesn't return a reference, AFAIK, or maybe I am missing something.

Created attachment 58504 [details]
Implementation of ListViewText

* New name for this class
* Now inherits from TreeView, instead of wrapping it
* Renaming, from append to append_text, for instance
* Added some members, just to make it as similar as possible to ComboBoxText

So, is this meant to be for a single column? Surely you'd want a ComboBox for _most_ of the places that you'd use that? I'd like a wrapper like this that allowed the number of columns to be specified in the constructor. Otherwise it wouldn't be very useful. You are halfway there.

I'd also like it to allow children (by using TreeStore). We'd then call it TreeViewText.

/// Returns a vector with the selected rows
/// @return a SelectionList with the selected results
const SelectionList &get_selected();

Needs to be documented as "vector of the indexes of the selected rows." And there's no significant performance reason to return a const &.

get_treeViewModel() should_use_underscores. And there's no reason to provide an accessor method for this anyway.

Much better.  Murray makes a good point (IMO) that most of the time a single column of text selections would probably be implemented as a ComboBox.  Adding columns would complicate the interface of this class somewhat, but it would probably be worth experimenting to see if we can come up with something slightly more flexible but still sufficiently simple.

Regarding get_text() and set_text(), you use std::advance (which, BTW is defined in <iterator>, not <utility>) to advance the iterator by i.  Then you check to make sure it's not equal to end().  But what if you pass in i=12 and there are only 6 rows.  Then you'd end up 6 past end() and have an invalid iterator but wouldn't catch it by the simple check to make sure that it wasn't equal to end() (At least that's what I think would happen -- correct me if I'm wrong).  One alternative is to use the operator[] function of TreeModel::Children, although I don't know how that function behaves when passed an index that's too large either.

I'm not sure that we want to have 'using std::xyz' declarations.  The actual names are used fairly rarely so it's not a big inconvenience, and IMO it's generally not a good idea for a library to import things into the global namespace.

type SelectionList is defined as a vector<int>.  I'd prefer consistency with other interfaces by using a Glib::ListHandle type.  This way the application programmer isn't tied to using std::vector, but can use any of the standard container types to store the return value.

in get_selected(): does get_depth() really do what we want it to do here?  I assume we want to build a list of row indexes.  My assumption is that get_depth() returns 1 (or 0) for all top-level items and 2 for all children of toplevel items, and 3 for all children of children of top-level items, etc.  In our case, we're just building a flat list (i.e. no children of rows) so I would think this would return the same value for all rows, but I could be wrong.  The documentation is very sparse on this function and I don't have the ability to test it now.

(In reply to comment #6)
> Much better.  Murray makes a good point (IMO) that most of the time a single
> column of text selections would probably be implemented as a ComboBox.  Adding
> columns would complicate the interface of this class somewhat, but it would
> probably be worth experimenting to see if we can come up with something
> slightly more flexible but still sufficiently simple.

Think of a dialog allowing you to insert e.mail addresses in your TO: and CC: fields. Would you create a ComboBox for inserting those names ? Actually not. You would put something similar to ListViewText and probably you would like it to allow multiple selection.

Something like ListViewText is needed. Maybe it is not used all of the times, but is definitely needed.

If you want something similar to ListViewText but allowing more than one column, then you are talking about a StringGrid or something similar, i.e., ANOTHER class/widget. So you would have ListViewText for list boxes and StringGrid (GridViewText?) for grids of text.

If you also want to allow children for each row, then you are talking about TreeViewText, i.e., something making TreeView more easy for strings.

The problem here is that TreeView is excellent from an Gtkmm implementer point of view, but catastrophic for the programmer who is going to use it in his applications, because there are too many abstraction levels in the documentation to follow and is easy to get lost inside it.

Do you know the SYS UNIX function io_ctrl() ? (I don't remember exactly the name of the function). With TreeView you have io_ctrl(). You have to know exactly the parameters it accepts of you want to know the size of a file, Or you can implement a function called getFileSize() which internally calls io_ctrl(). Implementers are happy because they still have io_ctrl(),  a single entry point for system calls, and because getFileSize() does not really add overload. Programmers are happy because they have a simple, intuitive function to use and can get rid of the complex syntax of the io_ctrl.

> 
> Regarding get_text() and set_text(), you use std::advance (which, BTW is
> defined in <iterator>, not <utility>)

My fault. However, it compiles :-)

 to advance the iterator by i.  Then you
> check to make sure it's not equal to end().  But what if you pass in i=12 and
> there are only 6 rows.  Then you'd end up 6 past end() and have an invalid
> iterator but wouldn't catch it by the simple check to make sure that it wasn't
> equal to end() (At least that's what I think would happen -- correct me if I'm
> wrong).  One alternative is to use the operator[] function of
> TreeModel::Children, although I don't know how that function behaves when
> passed an index that's too large either.

If you are not sure, the think about me (again what I said about io_ctrl() ...). :-)
I expected that kind of functions to send an exception, but haven't found anything about it.

About the other comments, I will take them in account to create a final version, provided it is accepted, but I don't think they are not vital at this stage.

So, to summarize, my proposal here would be to have ListViewText, and also GridViewText and TreeViewText.

Each class would be specialized in a) list boxes of strings, b) grids of strings, and c) trees of strings.

> So, to summarize, my proposal here would be to have ListViewText, and also
GridViewText and TreeViewText.

No, I see no reason no to add optional column parameters to the methods, so there's no need for "GridViewText". I suppose it would complicate the interface more to have children, because it requires an abstract iterator instead of a simple row index, so forget about the TreeStore for now.

Please don't rant.

(In reply to comment #9)
> > So, to summarize, my proposal here would be to have ListViewText, and also
> GridViewText and TreeViewText.
> 
> No, I see no reason no to add optional column parameters to the methods, so
> there's no need for "GridViewText". I suppose it would complicate the interface
> more to have children, because it requires an abstract iterator instead of a
> simple row index, so forget about the TreeStore for now.

So ... ? What now ?
Do you mean you don't agree with GridViewText and TreeViewText ? Or you don't agree with ListViewText as well ?

> Please don't rant.

I'm not completely sure about the meaning of "rant" (I'm spaniard). I just tried to make my point. Sorry if I sounded too enthusiastic or even I seemed to be trying to teach you anything. That was not my objective.

I am happy to have a TreeViewText class, like the patch, but with multiple columns.

(In reply to comment #11)
> I am happy to have a TreeViewText class, like the patch, but with multiple
> columns.
> 

Can you be more specific ? The patch doesn't allow children for each row, so it would be misguiding to call it TreeViewText.

Do you want me to just add the possibility of more columns to the ListViewText ?

If you really want a TreeViewText, then multiple columns must be supported, as well as child rows.

Or maybe I am missing something.

I still think that a few convenience classes deriving from TreeView, At least ListViewText (more or less like the patch, maybe with more than one column), and TreeViewText (the whole functionality of TreeView for text only). This is only my opinion, not an imposition.

> The patch doesn't allow children for each row, so it would be misguiding to call it TreeViewText.

Yes, OK, so let's call it ListViewText, at least for now.

> Do you want me to just add the possibility of more columns to the ListViewText

Yes, please.

Created attachment 58764 [details]
New implementation oif ListView allowing multiple columns

Now the new class allows multiple columns
* Various comments addressed
* Now you can't name the column(s) in the constructor. Use set_column_title( 0, "Column_name" ) for the simplest case.
* It is still possible to manage comofortably single columns list boxes:
listbox->append_text( "Item1" );
* Or multiple columns:
stringgrid->append_text();
stringgrid->set_text( 0, 0, "Item11" );
stringgrid->set_text( 0, 1, "Item12" );

It looks good. It needs the following small changes:
1. gtkmm coding style:
  - 2 spaces for indents.
  - public, protected, private, instead of private at the top.
  - string&, not string &
  - /**
     *
     */
    instead of 
    ///
    ///
  - = Glib::ustring() instead of "".
  - m_something or something_ for member variables.
  - No // ------------------ lines in the .cpp file.

2. In append_text() and friends, the row parameter should probably be called column_one_value.

3. get_text(unsigned int i, unsigned int j): The i and j parameters should be explicitly named as row and column. column can default to 0.

4. Add a set_text(unsigned int row, const Glib::ustring& column_one_value); overload.

5. get_selected() should return by value, not by reference. It should be const. There's no need for a member variable for it.


Could you patch the build files and make a small example, in the style of the existing tree examples. Ideally it would show how to respond to a user edit.

Created attachment 59331 [details]
Implementation of ListViewText (multiple columns)

I think I have addressed most of the coding style issues, though I need more information (no examples available yet).

Currently, the files have an indentation of 4 spaces: do you really want that to be of only two spaces?

Say you have something like:

data = new(std::nothrow) DataClass;

if ( data != NULL ) {
// more stuff ...
}
else throw ?

Does Gtk have any exception classes ? Should I just put:

data = new DataClass;

... and let C++ throw the bad_alloc exception ?


Another example:

void ListViewText::set_column_title(unsigned int i, const Glib::ustring& title)
{
	if ( get_columns().size() > i ) {
		get_column( i )->set_title( title );
	}
}

I would feel more comfortable, if 'i' is incorrect, throwing an exception (or returning an error value in the worst case). But I don't know what style you use in these cases.I don't like the "do nothing on error" approach, but obviously I will follow the Gtkmm style lines. I ask just because I haven't found anything about this in docs.

I have removed the comment separators in the cpp files. Do you use something else to separate member functions or just a blank space, as it is now ?

get_selected() still returns a vector<int>, as I simply don't know to deal with list_handle<>. Murray ? Did you receive my email ?

Created attachment 59917 [details]
Implementation of the listviewtext component

After diving into gtkmm source, the following changes have applied:
- only two characters for indentation (all format changes applied now)
- columns are editable if the user indicates so in the constructor

Created attachment 60993 [details]
Example of use of ListViewText

The attached files are a very tiny example of use of listviewtext. It is a window divided in three parts:
- A simple listbox
- A simple stringrid showing some values
- A simple stringrid showing values that are editable

listviewtext.cpp and .h are included as well

Good work Baltasar,
I'm not sure I'm completely happy with the API yet, though.  I think it works well for a one-column TreeView, but it feels clumsy for the multi-column case.  For example, when you've got multiple columns, you add a new row by specifying the content of the first column with append_text().  Then you have to specify the content of the remaining columns one-by-one, but using a different function, set_text().  It just doesn't feel right to me.  I'd like to do everything at once, preferably by specifying all column values as arguments to append_text() (though I admit this would be difficult to achieve).  If this isn't possible, it seems that they should all use the same interface for all column values instead of having one function for the first column and a different function for the others.  For example, you could change 
void append_text(string) 
to something like
int append(void)
which would return the row index that you could then use to set all column values with set_text(...).  Admittedly, this makes the single-column case more clumsy, so it's probably not a great solution either [By the way, this is all just off the top of my head -- I haven't thought through this API much either, just tossing out ideas].  

A few other thoughts:
- Several functions check the size of the TreeView before performing operations.  They just do nothing if the argument is too large.  Do we want to use some sort of assert instead? something like g_return_if_fail() or g_assert()?  I'm not sure about this one, just thought I'd pose the question.
- gtkmm uses a file extension .cc instead of .cpp

> For example, you could change 
> void append_text(string) 
> to something like
> int append(void)
> which would return the row index that you could then use to set all column
> values with set_text(...).

The parameter to append_text() is already optional. But I agree that append_text() should return the new row number.

I don't see any way other than varargs (...), or a clumsy vector to specify additional column values.

Yes, it would probably be a good idea to use the g_ macros.

I've been meaning to give this a last look over and commit it to HEAD. I haven't been rushed because HEAD won't have a release very soon.

(In reply to comment #19)
> I'm not sure I'm completely happy with the API yet, though.  I think it works
> well for a one-column TreeView, but it feels clumsy for the multi-column case.

Actually, I was thinking the same while writing the example, but can't think of any other method than passing a vector<string>. Or maybe this possibility could be added to the already present ones.
 
> For example, when you've got multiple columns, you add a new row by specifying
> the content of the first column with append_text().  

Yes, but this is not mandatorily needed to be carried out this way: you can call append() with no arguments and then fill all cells with set_text().

> Then you have to specify
> the content of the remaining columns one-by-one, but using a different
> function, set_text().  It just doesn't feel right to me.

Yeah, I understand. :-)

>  I'd like to do
> everything at once, preferably by specifying all column values as arguments to
> append_text() (though I admit this would be difficult to achieve).

Mmmm ... a variable argument list ?

>  If this
> isn't possible, it seems that they should all use the same interface for all
> column values instead of having one function for the first column and a
> different function for the others.  For example, you could change 
> void append_text(string) 
> to something like
> int append(void)
> which would return the row index that you could then use to set all column
> values with set_text(...).

It is possible to return the column in any case. I think it is a good idea.

>  Admittedly, this makes the single-column case more
> clumsy, so it's probably not a great solution either [By the way, this is all
> just off the top of my head -- I haven't thought through this API much either,
> just tossing out ideas].  

Don't worry, that's great, maybe we can find a better solution this way !

> A few other thoughts:
> - Several functions check the size of the TreeView before performing
> operations.  They just do nothing if the argument is too large.  Do we want to
> use some sort of assert instead? something like g_return_if_fail() or
> g_assert()?  I'm not sure about this one, just thought I'd pose the question.

I asked about this before (see comments #16 and #17) but received no answer (I'm not giving you a hard time). So I decided to look into the gtkmm source code and saw (to my onw surprise) that at least the components I look into (comboboxtext, I think) didn't do anything in that case (or maybe g_* was there but I didn't realise). I don't know about g_return_if_fail(), but I would be glad to include these error checks if any you can put me an example or address me to any source code.

> - gtkmm uses a file extension .cc instead of .cpp

okaaaaaaaaay

Salud !

Baltasar

(In reply to comment #20)

> The parameter to append_text() is already optional. But I agree that
> append_text() should return the new row number.

True, the parameter is optional, but It feels weird to call a function named append_text() without specifying any text.  Maybe it wouldn't feel as odd to me if the function were named append() or append_row() or something like that.  But the advantage of append_text() is that it remains consistent with the ComboBoxText class.

> I don't see any way other than varargs (...), or a clumsy vector to specify
> additional column values.

Yeah, I don't particularly like either of those options, especially varargs since it's not typesafe.  and if you use a vector, you've got to decide what to do in the case where the vector contains too many items or not enough items.  Maybe the existing API is the best we can do for now.  I suppose there's nothing stopping us from adding an alternate set_text API if we come up with a better way in the future.

(In reply to comment #21)
> 
> I asked about this before (see comments #16 and #17) but received no answer
> (I'm not giving you a hard time). So I decided to look into the gtkmm source
> code and saw (to my onw surprise) that at least the components I look into
> (comboboxtext, I think) didn't do anything in that case (or maybe g_* was there
> but I didn't realise). I don't know about g_return_if_fail(), but I would be
> glad to include these error checks if any you can put me an example or address
> me to any source code.

These are just simple macros provided by Glib [1].

So, for example, you could change the set_column_title function from:

void ListViewText::set_column_title(unsigned int column, const Glib::ustring& title)
{
  if ( get_columns().size() > column ) {
    get_column( column )->set_title( title );
  }
}

to:

void ListViewText::set_column_title(unsigned int column, const Glib::ustring& title)
{
  g_return_if_fail( get_columns().size() > column );
  get_column( column )->set_title( title );
}

For non-void functions, you'd need to use g_return_val_if_fail() instead.

[1] http://developer.gnome.org/doc/API/2.0/glib/glib-Warnings-and-Assertions.html

> Maybe the existing API is the best we can do for now.  I suppose there's
> nothing stopping us from adding an alternate set_text API if we come up with a
> better way in the future.

I'm unsure of whether to say this again or not, but perhaps the problem with the API comes from the fact that there are actually two widgets in one, and that we could better create a ListViewText and a GridText

All right, all right, I will never do that again.

Salud !

Baltasar

Created attachment 61029 [details]
Modifications to the listviewtext example

Modifications to the example, including the new ListViewText component with the g_return_if_fail macros.
* Now using just append() for the StringGrid instead of naming the first column (more homogenous).
* append() now returns an int with the number of row.

Nice to know about developers.gnome.org/doc, everything is there, including style guides. It would have been interesting to know before. I would have used the Gnome's style guidelines from the beginning.

The last point is that there are various places in which operator new is called. This means that an standard exception (bad_alloc) is thrown ... should I change this in order to capture the NULL pointer returned with g_return..., using new(std::nothrow), or should I leave it the way it is now ?

Salud !

Baltasar

I'll defer to Murray on the 'new' issue (although I don't think std::nothrow is currently used in gtkmm).  

One more minor style issue I just noticed:  There's just a couple places in the code that still use tabs instead of spaces for indenting.

And here's another proposal to consider:  There's currently a single ListViewText constructor where the user can specify a number of columns.  After construction, the application must go through and set all of the column titles one-by-one or else they'll be set to column#X.  What if we add an overloaded constructor like this:

ListViewText::ListViewText(std::vector<Glib::ustring> columnNames,
  bool isEditable,  const Gtk::SelectionMode& mode);

Then the column titles would all be set immediately upon construction and we wouldn't need to set them to a temporary string first and then reset them.  I think a vector<string> works better in this case than it does in the set_text() case (see comment #22) because here we don't have to worry about matching the size of the vector to the number of columns -- we just set the number of columns to the size of the vector.  

And (hopefully) my final note: I think I'd rather have the data (i.e. the model and columns members) be protected instead of private so that subclasses can access them if they need to.

Overall It's coming along nicely.

I'd rather avoid using vector<>, or any specific API in the constructor. We don't do it elsewhere. Let's leave the constructor as it is for now.

I agree about the protected column members.

Don't worry about making the last few changes. I'll go over it all when I commit it.

Created attachment 61047 [details]
ListViewText example of use

I have extended  the example so no a [big] button can be pressed in order to report about the selected rows.

* Now the private part of ListViewText is protected
* Removed all tabs I found from files

Includes the example itself and the ListViewText class.

Hi !

> I agree about the protected column members.
 Done.

> Don't worry about making the last few changes. I'll go over it all when I
> commit it.

  Well, I hope I dealt with all of them. It's probably easier for me than for you to change anything in these files. 

  Salud !

  Baltasar

What is your real name, for the ChangeLog?

(In reply to comment #30)
> What is your real name, for the ChangeLog?
> 

J. Baltasar García Perez-Schofield

This is now in cvs HEAD for gtkmm, with style/syntax corrections and some other minor changes. I created an example based on treeview/list.

Please submit changes as a patch, if necessary.

Thanks and well done.