After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 560918 - Time colum header is not right aligned.
Time colum header is not right aligned.
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: User Interface
1.4.1
Other Linux
: Normal normal
: 1.x
Assigned To: Banshee Maintainers
Banshee Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-11-15 13:33 UTC by Andreas Nilsson
Modified: 2009-02-11 19:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
this is how it looks now (21.43 KB, image/png)
2008-11-15 13:33 UTC, Andreas Nilsson
  Details
this is how it would look if the header was right aligned as well (25.40 KB, image/png)
2008-11-15 13:34 UTC, Andreas Nilsson
  Details
If cells in a column are aligned right, also align the column header right. (8.61 KB, patch)
2009-02-01 23:35 UTC, John Millikin
reviewed Details | Review
If cells in a column are aligned right, also align the column header right (v2) (2.24 KB, patch)
2009-02-02 21:55 UTC, John Millikin
needs-work Details | Review
If cells in a column are aligned right, also align the column header right (v3) (1.70 KB, patch)
2009-02-05 22:53 UTC, John Millikin
needs-work Details | Review
If cells in a column are aligned right, also align the column header right (v4) (1.89 KB, patch)
2009-02-09 21:52 UTC, John Millikin
committed Details | Review

Description Andreas Nilsson 2008-11-15 13:33:18 UTC
the duration column of each song is right aligned, but the Time header is not.
Comment 1 Andreas Nilsson 2008-11-15 13:33:43 UTC
Created attachment 122724 [details]
this is how it looks now
Comment 2 Andreas Nilsson 2008-11-15 13:34:32 UTC
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.
Comment 3 John Millikin 2009-02-01 23:35:29 UTC
Created attachment 127736 [details] [review]
If cells in a column are aligned right, also align the column header right.
Comment 4 Bertrand Lorentz 2009-02-02 21:35:14 UTC
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 ?
Comment 5 John Millikin 2009-02-02 21:55:40 UTC
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.
Comment 6 Gabriel Burt 2009-02-03 23:04:19 UTC
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.
Comment 7 John Millikin 2009-02-05 22:53:18 UTC
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.
Comment 8 Gabriel Burt 2009-02-09 02:21:13 UTC
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.
Comment 9 John Millikin 2009-02-09 02:27:37 UTC
> 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
Comment 10 Gabriel Burt 2009-02-09 15:43:31 UTC
True - so let's change the first ctor to call the latter with new ColumnHeaderCellText (HeaderCellDataHandler); instead of null for the header argument.
Comment 11 John Millikin 2009-02-09 21:52:50 UTC
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..; }
Comment 12 Gabriel Burt 2009-02-11 19:58:10 UTC
Thanks John, committed (w/ a few slight style changes).