GNOME Bugzilla – Bug 90148
TextBuffer::delete_text() & Co. have inconvinient, non-const iterator arguments
Last modified: 2004-12-22 21:47:04 UTC
This was requested by Erdi Gergo: On Tue, 2002-08-06 at 22:00, ERDI Gergo wrote: > Lots of TextBuffer operations have signatures like this: > > void delete_text(iterator& start, iterator& end); > > Do these really return something in start/end? It's very inconvenient that > I can't just write > buffer->delete_text (buffer->begin (), buffer->end ()); > If the signature was > > void delete_text (const iterator &begin, const iterator &end); > > it would work. Well, I found four member functions that unnecessarily take non-const iterators in the parameter list: delete_text, delete_interactive_text, insert_child_anchor, create_child_anchor, paste_clipboard. While the first two can be overloaded to remove this inconvinience, for the latter three the iterator in the parameter list can simply be changed to const without breaking anything because gtk doesn't change them (at least there is nothing in the gtk docs that suggests it).
Created attachment 10347 [details] [review] Patch removing these inconviniences...
Well done, but: 1. If you are making these iterator copies just so that the original can be const then please comment that, or just use const_cast<> so that it's obvious. 2. I would prefer overloaded methods to have the same return type, if that's possible.
Before using const_cast<> I have to dig in the C sources to be 100% sure that the iterators are not touched. Will do so later today. The returnal of an iterator from delete_text() was proposed by Carl Nygard to be more STL-like. I'm waiting for you to think that over before changing it to void :-)
No, I like that it returns an iterator. I think that the other overload should do that too. Yes, we do need to check the sources sometimes. C people don't use const very often unless it's a char*.
Thinking that iterator return thing over: the 'iterator delete_text(iterator&,iterator&)' overload seems completely useless, stupid and confusing now next to 'iterator delete_text(const iterator&, const iterator&)'! Shouldn't we just get rid of it? The same goes for delete_interactive_text() but here we have an additional problem: shall we really discard the boolean return value (which shows whether text was actually deleted)? Is there some kind of 'invalid' or 'eof' iterator that we could return in the case that no text was deleted?
Yes, if both the start and end would be equal after the method has finished then there is no need to return both as well as the return iterator. Maybe you could use end() to show that the deletion has not happened. I don't think that end() would ever be the real "location where text was deleted." That's what find() would use to show that something was not found. Or maybe you could add an overload that takes a bool& parameter.
Okay. A revised patch will be attached. The ChangeLog entry follows. Note that some API changes are involved now: Gtk::TextMark:get_iter() is added as a convinience function to help solving the TODO entry in textbuffer.hg and the delete_(interactive_)text() variants with non-const iterators in their parameter list are dismissed. * gtk/src/textiter.{ccg,hg}, gtk/src/textmark.{ccg,hg}: Avoid unnecessary includes. Add TextMark::get_iter() which works equivalent to mymark->get_buffer()->get_iter_at_mark(mymark). * gtk/src/textbuffer.{ccg,hg}: - Make TextBuffer::insert_(interactive_)at_cursor() return the iterator of the cursor position using get_insert()->get_iter() or end() if the insertion failed. - Make TextBuffer::delete_(interactive_)text() return the iterator of the position where the deletion occured or end() if no deletion occured. Make the iterators in the parameter list const. - Make the iterators in the parameter lists of TextBuffer::paste_clipboard(), TextBuffer::insert_child_anchor() and TextBuffer::create_child_anchor() const.
Created attachment 10386 [details] [review] The proposed API review of Gtk::TextBuffer and Gtk::TextIter.
Applied. Thankyou, though I would have preferred a few more comments in there. Hopefully people forgive us for not being really frozen.