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 
> appears
> 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.

Cheers,

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

Reply via email to