Re: GtkTreeView changes

[Kristian Rietveld <kris loopnest org>]

Hello Benjamin,

On Nov 24, 2011, at 2:57 AM, Benjamin Otte wrote:
> In case you didn't notice, I did some refactoring in the treeview code
> and pushed it to master. It's mainly meant for the accessibility
> implementation. I'll continue doing that for a bit, until the treeview
> a11y implementation is not a disaster anymore. If you don't agree with
> any of it, poke me.

I did notice, thanks for your mail.

> Highlevel overview of what I've done:
> 
> 1) Make GtkRBTree track the node number
> Instead of parity, we now have a total_count, which is basically like
> parity, just without modulo 2. It is used via gtk_rbtree_find_index()
> by the a11y code to be able to index rows by number.

I would have said, look at _gtk_tree_view_find_path(), until I realized that you need to index rows by number *including* all expanded rows and not excluding like is done for paths.  So I can see this change making sense.

This is very likely a non-issue, but I will mention it anyway for completeness: the only thing I am slightly concerned about, but really only for 32-bit limited-memory platforms is the increase in memory usage now that the parity bitfield (before nicely padded with the 14-bit flags) is a full int. For our current 64-bit platforms I am not concerned, since the 4 pointers in the node structure have doubled since 32-bit.

> 2) Make GtkRBNode pointers not reference different nodes over time
> The reordering and node removal code used to swap the data from one
> node to another. I fixed that. Now a Tree/Node tuple can be used
> instead of a GtkTreeRowReference. I'm using these instead of
> GtkTreeRowReferences in the accessibilty code to have a stable mapping
> from accessibles to rows.

Makes sense, though it *could* be that there's some specific GtkTreeView code that depends on this behavior. For reorderings, the GtkRBNodes used to stay at the some (x,y) location and only have their data swapped, now they would move location. From a quick look, we already unset the prelight and expand_collapsed nodes (which care about location not content) before reodering, so we are likely fine.  I think we might still want to explicitly stop rubberbanding and unset the button_pressed node if we are going to reorder the rows in the middle of rubberbanding or a button press and release (similar for node removal).

> 3) Add a bunch of convenience wrappers/refactors to make code more
> understandable
> Basically, I moved GtkRBTree closer to something that could be exposed
> as a public API, or at least as a semi-public API internally. The
> rbtree code is actually quite powerful (we could use it to flatten
> trees to lists in various places, also it might be interesting to
> implement TreeModelFilter with it, I might want to use it to replace
> the textview's btree implementation) and might allow a bunch of cool
> things. I almost convinced Ryan to pull it into glib already. ;)

Fine with me, but my plan was to not touch TreeModelFilter any time soon, since it seems to be finally working quite nicely since the refactorings over the Summer :)

> 4) Move treeview a11y from signals to direct function calls
> Instead of signal handlers that have to work with what is available, I
> added direct function calls into the a11y code. This allows better
> control about what information we send to the treeview. And we can
> actually send private data types without having to expose it to the
> public API.
> The treeview is the first widget where I'm doing that excessively, the
> other a11y widgets should ideally follow.

Totally makes sense, I think such integration is definitely necessary if we want to be a first-class citizen on the a11y front.


regards,

-kris.