Hi Benoît,
Sorry for being late on this.
On Fri, Mar 22, 2013 at 11:02:57AM +0100, Benoît Knecht wrote:
> Hi Marwan,
>
> Marwan Tanager wrote:
> > This patch eliminates the aforementioned problems by setting the adjustment
> > ratio dynamically from the 'value-changed' callback only if the vertical
> > adjustment upper bound hasn't changed. If it's found to be changed, this
> > means
> > that the document scale have changed (e.g due to zooming or resizing the
> > window) so the adjustment ratio shouldn't be updated. Otherwise, we assume
> > that
> > the adjustment value has been changed as a result of scrolling or jumping
> > to a
> > specific page.
> >
> > Of course it's just a draft, but you can generalize it to account for the
> > horizontal adjustment as well as the adjustments lower bounds. IMO, the
> > adjustment_ratio struct could now be called something like 'struct
> > adjustments'
> > with possibly two substructures for the horizontal and vertical adjustments
> > each containing the ratio and the upper/lower bounds, but of course it's
> > your
> > patch in the first place so you are free to arrange things as you see fit.
>
> Great idea, checking the bounds in the "value-changed" callback! Your
> comments and proof of concept were very helpful. I came up with a
> slightly different implementation, but it should essentially be the same
> concept as you just described. The only difference is, instead of
> keeping track of the ratio and bounds inside the document struct, I
> created two GtkAdjustments in zathura->ui, each keeping track of the
> horizontal/vertical adjustments, respectively, through signals/handlers.
>
> I'll send a patch for review right away, so you'll see what I mean.
I've just reviewed your patch and I should say thanks for the great work.
Actually, your approach is more intuitive and the documentation is very clear.
> The only remaining issue I can see (but maybe you'll find others :) ) is
> that the page number is not updated if you (say) zoom out from page 1,
> and then zoom in (to whichever page is at the center of the screen). But
> that's not really an issue introduced by this patch, and there's a
> trivial fix for that.
Yeah, I see that too. However, I'm afraid to say that it's caused by your patch
because I've tested it with the draft version and it doesn't happen there. But
I think, like you said, that it's a minor issue somewhere.
Also, there is another small issue which isn't specific to your implementation,
as it happens with mine, too. When you try jumping to a page from the input
bar, it appears slightly scrolled up. You won't see this unless you have
page-padding set to some non-zero value. You will notice the padding showing up
at the top of the view, which isn't used to happen before. If you pay
attention, you will see that the displacement happens as a result of the
input bar being hidden after you press Enter to jump to the page, because if
you activated it again after the jump you would see the page getting back to
it's correct position by displacing it by the same amount in the opposite
direction.
Apart from this, I think the patch is now ready for getting merged.
> Anyway, thanks again for your precious feedback and advice, it was very
> much appreciated!
And thank you for your great work. Now we have zooming functionality that's
worth using (It was very broken before).
Marwan
_______________________________________________
zathura mailing list
zathura@lists.pwmt.org
http://lists.pwmt.org/mailman/listinfo/zathura
