GNOME Bugzilla – Bug 560918
Time colum header is not right aligned.
Last modified: 2009-02-11 19:58:10 UTC
the duration column of each song is right aligned, but the Time header is not.
Created attachment 122724 [details] this is how it looks now
Created attachment 122725 [details] this is how it would look if the header was right aligned as well Less lines for the eye to follow.
Created attachment 127736 [details] [review] If cells in a column are aligned right, also align the column header right.
Thanks for the patch, it works for me. I'm not sure, but maybe it would be better to have the column header pick the alignment from the type of ColumnCell ? For example, a ColumnCellPositiveInt is always right aligned, so the header should also be. And btw, does anyone know how this all looks in a RTL language ?
Created attachment 127796 [details] [review] If cells in a column are aligned right, also align the column header right (v2) > I'm not sure, but maybe it would be better to have the column header pick the alignment from the type of ColumnCell ? Good idea. ColumnCell doesn't have an Alignment property, that's only in ColumnCellText. This patch will create an overloaded `DefaultColumnController.Create()` method for text columns, to pick up their alignment.
Please always puts {} around if/etc statements, although for the getter it would probably be cleaner as: return header_text != null ? header_text.Alignment : Pango.Alignment.Left; Not really happy with this though - we have columns declared in other controllers that wouldn't use the Create override - src/Core/Banshee.ThickClient/Banshee.Collection.Gui/XmlColumnController.cs Instead, how about we override the Column controller (line 82 src/Libraries/Hyena.Gui/Hyena.Data.Gui/Column.cs) to do the header_cell.Align = cell.Align thing? Then no matter how they're created it'll get set to match.
Created attachment 128061 [details] [review] If cells in a column are aligned right, also align the column header right (v3) Adds the ``Column.AlignHeader()`` method, and calls it after constructing a Column to align text-based headers with text-based content cells.
The first ctor you modified calls the latter one via this (..) - so might as well just put the AlignHeader method's code into the latter ctor. Your if and { have a newline between that they shouldn't.
> The first ctor you modified calls the latter one via this (..) - so might as > well just put the AlignHeader method's code into the latter ctor. `AlignHeader` will only work properly when `this.header_cell` != null. The order of operations for the first constructor looks something like: Column (string title, ColumnCell cell, double width) this (null, title, cell, width, visible) this.header_cell = null AlignHeader if (header_cell != null) ... this.header_cell = new ColumnHeaderCellText (... AlignHeader <---- call to AlignHeader in first constructor if (header_cell != null) set alignment
True - so let's change the first ctor to call the latter with new ColumnHeaderCellText (HeaderCellDataHandler); instead of null for the header argument.
Created attachment 128337 [details] [review] If cells in a column are aligned right, also align the column header right (v4) > True - so let's change the first ctor to call the latter with new > ColumnHeaderCellText (HeaderCellDataHandler); instead of null for the header > argument. This caused a compilation error, since `HeaderCellDataHandler` is not available in the static constructor. New patch to implement this suggestion from IRC: > <gabaug> hrm, it changes API a bit, but maybe can move that header ctor into the last Column ctor, if (hdr == null) { hdr = new ColumnHeaderCell..; }
Thanks John, committed (w/ a few slight style changes).