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.


Benoît Knecht
zathura mailing list

Reply via email to