[PATCH] diff view: improve appearance of line numbers

Wed May 6 10:35:45 EDT 2015

Hello,

On Wed, 06 May 2015 01:52:16 +0200
Mads Kiilerich <mads at kiilerich.com> wrote:

> >   * display the ellipsis just once and centered

> "instead of showing ellipsis in both the - and the + column" (I had
> to test to figure out what this meant)

Sure, I'll add this to the commit message.

> >   * expand column width as needed to accomodate longer numbers

> Does that fix an issue? In which case? Related to the other changes?

This is actually a side effect of the change. I needed a link to fill
the table cell so I set width: 100%, which automagically made the
columns flexible.

> >   * enable box-sizing: border-box for the number links to
> >     make it easier to set paddings

> I have no idea about this. Is it related to the other changes? Which 
> paddings were hard in which cases? How is it easier than before? Is
> it a trick we also could/should use elsewhere?

In short, border-box is what previously used to be known as IE quirks
mode box model. The difference is that width does include paddings and
borders (in the traditional W3C model it didn't). This simplified
calculations for some cases, but used to be non-standard for some time
until it was accepted by W3C. These days some popular CSS frameworks,
like Bootstrap, use box-sizing: border-box exclusively to implement
responsive design (it would require complicated CSS hacks otherwise).

Without setting this thing here I couldn't figure out how to properly
set internal spacing in the line number column which collapsed after I
used width: 100%.

> > -                    _html.append('''\t<td %(a_id)s
> > class="%(nlc)s">''' % {
> > -                        'a_id': anchor_new_id,
> > -                        'nlc': new_lineno_class
> > -                    })
> > +                    if not no_lineno:
> > +                        _html.append('''\t<td %(a_id)s
> > class="%(nlc)s">''' % {
> > +                            'a_id': anchor_new_id,
> > +                            'nlc': new_lineno_class
> > +                        })
> >   
> > -                    _html.append('''%(link)s''' % {
> > -                        'link': _link_to_if(True,
> > change['new_lineno'],
> > -                                            '#%s' % anchor_new)
> > -                    })
> > -                    _html.append('''</td>\n''')
> > +                        _html.append('''%(link)s''' % {
> > +                            'link': _link_to_if(True,
> > change['new_lineno'],
> > +                                                '#%s' % anchor_new)
> > +                        })
> > +                        _html.append('''</td>\n''')
> 
> It seems like it would be cleaner to have separate cases for with vs 
> without line number.

Possibly.

> Slightly related thought:
> we need a way to link to and comment on binary files such - such as
> on
> https://kallithea-scm.org/repos/kallithea/changeset/4857d8f170d937383b8bde700494320fff80a88f .
> I guess these not-even-ellipses lines also need some treatment ... or 
> perhaps just that the first one should have line number 0.

Agree.