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.

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.

Thanks for spotting this.


                
        Marwan
_______________________________________________
zathura mailing list
zathura@lists.pwmt.org
http://lists.pwmt.org/mailman/listinfo/zathura

Reply via email to