---- Reported by jim@yorba.org 2013-02-05 12:39:00 -0800 ----

Original Redmine bug id: 6327
Original URL: http://redmine.yorba.org/issues/6327
Searchable id: yorba-bug-6327
Original author: Jim Nelson
Original description:

A little whitespace between each account will give a nicer sense of grouping.

Related issues:
related to geary - 6397: Use text-only headers in folder list (Fixed)



---- Additional Comments From geary-maint@gnome.bugs 2013-09-04 16:47:00 -0700 ----

### History

####

#1

Updated by Jim Nelson 9 months ago

  * **Priority** changed from _Normal_ to _High_

####

#2

Updated by Jim Nelson 8 months ago

  * **Target version** changed from _0.3.0_ to _0.4.0_

####

#3

Updated by Avi Levy 5 months ago

  * **File** 6327v1.patch added
  * **Status** changed from _Open_ to _Review_

I added a bit of spacing around all headers, not just the account branches,
because this made more sense to me.

####

#4

Updated by Jim Nelson 5 months ago

  * **Status** changed from _Review_ to _Open_
  * **Assignee** set to _Avi Levy_

Yes, this is what we're looking for. Couple of things:

  * Let's try a HEADER_PADDING of 6 to make the separation stand out more. This is something we can easily tweak later if we don't like it.
  * text_renderer_function() should be private, not public. (Same for icon_renderer_function(); I should've caught that last time.)
  * Can you apply the padding only to the top of the header and not the bottom? I'd like it's children to "hug" the header line.

####

#5

Updated by Avi Levy 5 months ago

  * **File** 6327v1.patch added
  * **Status** changed from _Open_ to _Review_

The first two bullet points were a really simple fix. But the third bullet
point has been really challenging - which is surprising because of how simple
a fix it should be. First of all, CellRendererText ignores yalign. At first I
thought this was a bug, but now I see that it is not so cut-and-dried. And on
top of this, it is impossible (by default) to add the right amount of vertical
padding to the little expander arrows, so they would also render incorrectly.

So I sort of came to the conclusion that we don't have enough fine-grained
control over the rendering in Sidebar.Tree, and I started looking at ways to
get more control. Then things started getting involved. For instance, to gain
more control over how the expander arrows are rendered, I ended up removing
the default expander arrows and adding in a custom ExpanderRenderer that we
control. This is a similar approach as the one taken by Noise (see Eric's link
in #6397). Note that this has implications for #4985 - by using a custom
ExpanderRenderer, it is suddenly much easier to implement #4985. If you follow
the code in this patch, you'll see that the code for #4985 has been
reimplemented in a more natural fashion.

Overall, I am not pleased with this current patch. However, even though the
current patch may not be using the right approach, it seems to me like some
sort of underlying code changes in Sidebar.Tree will have to happen in order
to gain finer control over its rendering. Most likely, there will also need to
be some sort of refactoring to Sidebar.Tree. In fact, it seems like there are
3 main components to Sidebar.Tree:

  * How the Sidebar.Tree renders the underlying Gtk.Treeview
  * The wrapper layer that translates a Gtk.Treeview cell into a Sidebar.Entry
  * The actual logic in Sidebar.Tree

which can be somewhat decoupled from each other.

Of course, all of this might turn out to be unnecessary if we don't need finer
control over rendering in general. Also, I am not sure about the constraints
involved with keeping the Sidebar code synchronized with Shotwell. But these
are just my two cents.

####

#6

Updated by Eric Gregory 5 months ago

  * **Status** changed from _Review_ to _Open_

After playing with this, I'm not sure this is the right approach.

The way the patch is implemented, it adds a padding to the top each top-level
branch (with the exception of one named "Inboxes".) The trouble with this
approach is that clicking the "blank" space opens/closes the account branches.

My guess is that there's no way to resolve this by tweaking the cell padding.
What we need here is a cell _margin_, which afaik Gtk doesn't offer.

So my advice would be to see if there's a way to do this by inserting an empty
cell. That should solve the click-ability issue and reduce the amount of pixel
algebra going on here, but it will likely involve tweaking the sidebar code.

####

#7

Updated by Avi Levy 5 months ago

Thanks for the suggestion! My approach felt wrong, but I couldn't see another
way to add the margin. Adding empty rows should turn out much better.

The reason I had to hardcode the "Inboxes" branch was because checking for
path = 0 didn't work: the sidebar is populated in several stages, so different
branches are the "first" branch at different points in time.

Looking forward to implementing this properly.

####

#8

Updated by Avi Levy 5 months ago

  * **File** 6327v3.patch added
  * **Status** changed from _Open_ to _Review_

For now, the spacing is roughly the height of an "xx-small" line of text.
Should I have hardcoded a value (like 6 pixels) instead?

####

#9

Updated by Avi Levy 5 months ago

  * **File** 6327v4.patch added

Removed empty tooltips

####

#10

Updated by Eric Gregory 5 months ago

  * **Status** changed from _Review_ to _Open_

Code-wise, this looks fine. The big issue now is that the blank lines appear
to be full lines rather than half lines. I tried setting the span from xx-
small to 6px myself and didn't see any difference (note that you have to do
font="6px" rather than size="6px").

Could you look into reducing the line height?

####

#11

Updated by Avi Levy 5 months ago

  * **File** 6327v5.patch added
  * **Status** changed from _Open_ to _Review_

Let me know if this is any better. There's some extra padding that I can't
seem to control, and it looks like this has been a long-standing issue with
Gtk.TreeView (https://mail.gnome.org/archives/gtk-devel-
list/2006-April/msg00218.html)

####

#12

Updated by Jim Nelson 5 months ago

  * **Status** changed from _Review_ to _Open_
  * **Assignee** deleted (<strike>_Avi Levy_</strike>)

Thanks for the patch, Avi. Unfortunately, the spacing is still too wide for
what we're looking for here. I can tell this is a deep, perhaps intractable
problems with GtkTreeView. Let's let this one simmer for a while and move on
to other things. Thanks for all the effort you've put in to this one.

####

#13

Updated by Jim Nelson 4 months ago

  * **Category** changed from _client_ to _ux_

####

#14

Updated by Jim Nelson 4 months ago

  * **Priority** changed from _High_ to _Normal_

####

#15

Updated by Jim Nelson 3 months ago

  * **Target version** changed from _0.4.0_ to _0.5.0_



--- Bug imported by chaz@yorba.org 2013-11-21 20:29 UTC  ---

This bug was previously known as _bug_ 6327 at http://redmine.yorba.org/show_bug.cgi?id=6327
Imported an attachment (id=261090)
Imported an attachment (id=261091)
Imported an attachment (id=261092)
Imported an attachment (id=261093)
Imported an attachment (id=261094)

Unknown milestone "unknown in product geary. 
   Setting to default milestone for this product, "---".
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.
Resolution set on an open status.
   Dropping resolution