Code review comment for lp:~aauzi/midori/fix-1177553-3

Revision history for this message
André Auzi (aauzi) wrote :

> Could you provide some reasoning as to what the 2 new classes offer over
> standard cell renderers?

Sure.

The combobox uses the same tree store row for the folder and the top menuitem it presents, before the separator, in the subfolder list view.

That's why we get the same name and icons in the opened folder and the top menuitem of the subfolders list.

I had to figure out a way to distinguish between the two situations:
a/ when the rendered row corresponds to the folder
b/ when the rendered row corresponds to the top of subfolders list

Since the cell renderers are executed on the same row this could not be done by information stored in the row itself.

The cell renderers I've (rapidly) written examine the containing menu to determine if they're called on a the first widget of a menu, just before a separator.

This could had been avoided for pixbufs if the combobox managed the 'expanded' property of the cell renderer, as a treeview does, but, unless I've made a big mistake, it does not seem to do it. (I couldn't make it work :/)

The two new classes are actually tied to the combobox implementation and offer the capability to render different text and pixbufs for the folder when it's listed in a menu or presented at the top of it's containees.

Note: since the detection is the same for the two classes I believe they can be implemented with about half of the code I used here.

Honestly, if combobox on tree store is not used elsewhere, I would be reluctant to keep them.

I'd only provide them if negative user feedback was given on the previous, straight forward, implementation.

« Back to merge proposal