On Thu, Mar 21, 2013 at 05:18:15PM +0100, Benoît Knecht wrote:
> Hi Marwan,
> 
> Thanks a lot for your review and feedback.
> 
> Marwan Tanager wrote:
> > On Wed, Mar 20, 2013 at 10:17:36PM +0000, Benoît Knecht wrote:
> > > Instead of guesstimating the values of the scrollbars adjustments after
> > > a change in zoom level, connect callbacks to the "changed" GtkAdjustment
> > > event (which is emitted when the bounds or page_size of the adjustment
> > > change, e.g. when the zoom level changes), and compute the new values
> > > from there.
> > > 
> > > cb_view_hadjustment_changed() centers the page horizontally if a
> > > "best-fit" or "width" zoom is being performed, or if "zoom-center" is
> > > true; otherwise, it keeps the view horizontally centered around the same
> > > area of the page.
> > > 
> > > cb_view_vadjustment_changed() always keeps the view vertically centered
> > > around the same area of the page.
> > > ---
> > > This is in preparation for addressing <http://bugs.pwmt.org/issue99>. A 
> > > patch
> > > will follow shortly.
> > 
> > Just a couple of notes based on my experimentation with the patch:
> > 
> >     - Jumping to a page using a numeric command doesn't work anymore. The 
> >       page number is updated on the status bar, but the view is not 
> >       adjusted to the target page. This will not happen unless you zoom at 
> >       least one time.
> 
> I wasn't able to reproduce this one, unfortunately.

That's ironic because it's fairly predictable on my system. Here is the steps I 
follow:

        - Open some document.

        - Jump to a page by typing a number in the input bar. This works 
          normally.

        - Zoom in/out, or press 'a' or 's'.

        - Jump again to an arbitrary page. This time it doesn't work and the 
          view stays in it's current position.

Note that you must change the scale of the document at least one time before 
you would be able to reproduce it.

> 
> >     - Activating the input bar using ':' will cause the view to be adjusted 
> >       to the same position it was in during the most recent zoom. Again, 
> >       this happens only after at least one zoom.
> 
> This one I was able to reproduce, though.
> 
> > I think this happens because in both cases the act of showing the input bar 
> > causes the upper bound of the vertical adjustment to be changed which 
> > triggers 
> > cb_view_vadjustment_changed which in turn will call 
> > set_adjustment_from_ratio.  
> > Since you don't update the adjustment ratios during the above operations, 
> > set_adjustment_from_ratio will get us back to where we were during the last 
> > zoom.
> 
> Yes, I think you're right. I hadn't thought of that.
> 
> > Also, your patch makes it necessary to update the adjustment ratios in the 
> > adjustment_ratio struct during scrolling in the document to reflect the 
> > changed 
> > adjustments.
> > 
> > One could argue that we should then call 
> > zathura_readjust_view_after_zooming[1]
> > from cb_view_vadjstment_value_changed in order to update the ratio whenever 
> > the 
> > vertical adjustment changes, and indeed when I did that, the above problems 
> > disappeared, but now zooming doesn't work as it should, because when we 
> > zoom,
> > "both" the adjustment upper bound and value change, and apparently, the 
> > 'value-changed' callback is called "before" the 'changed' callback. So, the 
> > 'value-changed' callback will update the ratio based on the new upper bound 
> > before the 'changed' callback has a chance to update the adjustment value 
> > from 
> > the ratio. This sort of chicken-and-egg problem will cause zooming to 
> > effectively scroll the document backward, because the 'changed' callback 
> > will 
> > then set the adjustment value using the wrong ratio computed from the 
> > 'value-changed' callback.
> 
> You're absolutely right, nice analysis!
> 
> > I hope I haven't lost you somewhere!
> 
> Not at all, it was very clear, thanks.
> 
> > Similar arguments could be made for the horizontal adjustment.
> > 
> > [1] I think this function should now be called something more generic 
> > because 
> > all that it does now is updating the adjustment ratios, which isn't 
> > specific to 
> > zooming and should be done whenever the adjustments change (e.g. scrolling).
> 
> Indeed, but that's the easy part ;)
> 
> I think one way to solve the issue you brought up without having to
> remember saving the ratio every time we move around in the document,
> would be to do it in set_adjustment() from utils.c. But that would mean
> accessing zathura->document from there, so it isnt' as straightforward
> as I'd like it to be.
> 
> What do you think? Do you see possible issues there? Do you have a
> better idea?

Well, changing the ratios in set_adjustment will only be useful in cases where 
we explicitly change the adjustments like when we jump to a specific page, but 
we would still need a way to update the ratios for implicit cases like 
scrolling through the document, and the only way for doing this AFAICS is to do 
it from the 'value-changed' and 'changed' callbacks.

I will send you a proof-of-concept patch based on yours that you can build on 
it if you choose to go that direction.


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

Reply via email to