Hi Marwan, Marwan Tanager wrote: > Sorry for being late on this.
No worries, and thanks again for taking the time to review 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. Thanks :) > > 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. I meant that it already happened on the current develop branch, with no patch applied. That's why I didn't think it was introduced by my patch, but perhaps yours was actually fixing this as well. > 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. Yes you're right, I see that too. I think I have a fix for that, I'll submit it in a different patch. > Apart from this, I think the patch is now ready for getting merged. I think so too, if no one else spotted major issues with it. I'm going to submit it for inclusion now. Cheers, -- Benoît Knecht _______________________________________________ zathura mailing list [email protected] http://lists.pwmt.org/mailman/listinfo/zathura
