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. > - 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 > 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. > >  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? In any case, thanks again for your insight. Cheers, -- Benoît Knecht _______________________________________________ zathura mailing list firstname.lastname@example.org http://lists.pwmt.org/mailman/listinfo/zathura