Marwan Tanager wrote:
> On Thu, Mar 21, 2013 at 09:04:57AM +0100, Benoît Knecht wrote:
> > Marwan Tanager wrote:
> > > On Wed, Mar 20, 2013 at 11:03:54PM +0000, Benoît Knecht wrote:
> > > > This wasn't correct. Padding is already accounted for:
> > > >
> > > > scale = (width - (pages_per_row - 1) * padding) /
> > > > (pages_per_row * cell_width)
> > > >
> > > > If you add padding on the denominator, you end up with black margins on
> > > > the sides of the window, which isn't what padding is for (i.e. insert a
> > > > gap between rendered pages), and defeats the purpose of a "best-fit".
> > >
> > > Not accounting for the padding in the denominator is what will actually
> > > defeat
> > > the purpose of best-fit.
> > >
> > > To get a feel for this, try setting pages-per-row to e.g. 4, and
> > > page-padding
> > > to e.g. 7, while the show-scrollbars setting is set to true, then press
> > > 's' or
> > > 'a' to fit the collective width of the pages into the view. You will
> > > notice
> > > that the horizontal scrollbar is still showing up which shouldn't happen
> > > because the pages now should be fitted entirely inside the view.
> > Indeed, I see that too.
> > > This happens because not adding the paddings to the width of the cells
> > > will end
> > > you up with a scaling factor that makes the pages fit into the view but
> > > "only"
> > > if there is no paddings. To verify this, set page-padding to 0, and
> > > repeat the
> > > same procedure. Now, things will work as expected.
> > But your fix was in the wrong place, it should go within the
> > "if (show_scrollbars)" section, line 153, as everything works well when
> > show-scrollbars is false. And from what I can tell, the extra width
> > isn't proportional to padding (try setting it to something big, then
> > double it for instance); it seems to be just one pixel too wide, or
> > something like that.
> > > Since the paddings aren't part of the cells, consume some space of the
> > > view,
> > > and aren't subject to scaling, we should add them here to the total width
> > > of
> > > the cells in order to "best-fit" them together with the cells themselves.
> > No, padding is already accounted for in the numerator (it is substracted
> > from the window's width). So essentially, the scale is computed by
> > saying that the width of all pages on a row (the denominator) has to fit
> > in the width of the window minus the padding (the numerator). If you
> > scale it like that, and then add padding between the pages, they will
> > fit exactly in the window. The scrollbar issue is something completely
> > different.
> > > And regarding the borders on the edges, this only happens with multiple
> > > pages
> > > per row, which isn't the typical use case. And IMHO, as long as the pages
> > > are
> > > fitted entirely inside the view, the user wouldn't care. But having the
> > > scrollbar showing after best-fit is what would more likely be considered
> > > an
> > > issue.
> > Sure, the scrollbar issue needs to be fixed, but as I said, this isn't
> > the correct way to fix it, and it affects best-fit's behavior when
> > show-scrollbars is false, even though it was working fine before.
> It turns out you are right indeed. That was a misunderstanding of the formula
> on my side.
No problem, it's a tricky part; maybe I should add some comments
detailing what's going on...
> Also, I've just noticed something that confirms that the problem with the
> scrollbar is entirely unrelated: try experimenting with multiple subsequent
> values of page-padding and you will notice that the scrollbar sometimes
> and sometimes doesn't.
Yes, I actually suspect it's some rounding issue somewhere, I'll have to
look into this. I don't use scrollbars myself, so I didn't notice it
before. Thanks for pointing it out.
zathura mailing list