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.

Cheers,

-- 
Benoît Knecht
_______________________________________________
zathura mailing list
zathura@lists.pwmt.org
http://lists.pwmt.org/mailman/listinfo/zathura

Reply via email to