Re: [zathura] [PATCH] Revert commit ce6e735
Sebastian Ramacher wrote: > Maybe I'm missing something from the thread, but what is the conclusion? > Should I consider the patch for applying or do you want to solve the > remaining issues first? I think it should be applied as is. It's not the correct fix, and makes things a bit worse when show-scrollbars is false. Hopefully we can fix the actual issue soon, but I think we should first undo this. -- Benoît Knecht ___ zathura mailing list zathura@lists.pwmt.org http://lists.pwmt.org/mailman/listinfo/zathura
Re: [zathura] [PATCH] Revert commit ce6e735
Maybe I'm missing something from the thread, but what is the conclusion? Should I consider the patch for applying or do you want to solve the remaining issues first? Regards -- Sebastian Ramacher ___ zathura mailing list zathura@lists.pwmt.org http://lists.pwmt.org/mailman/listinfo/zathura
Re: [zathura] [PATCH] Revert commit ce6e735
Marwan Tanager wrote: > On Thu, Mar 21, 2013 at 09:04:57AM +0100, Benoît Knecht wrote: > > Marwan Tanager wrote: > > > On Wed, Mar 20, 2013 at 11:03:54PM +, Benoît Knecht wrote: > > > > This wasn't correct. Padding is already accounted for: > > > > > > > > scale = (width - (pages_per_row - 1) * padding) / > > > > (pages_per_row * cell_width) > > > > > > > > If you add padding on the denominator, you end up with black margins on > > > > the sides of the window, which isn't what padding is for (i.e. insert a > > > > gap between rendered pages), and defeats the purpose of a "best-fit". > > > > > > Not accounting for the padding in the denominator is what will actually > > > defeat > > > the purpose of best-fit. > > > > > > To get a feel for this, try setting pages-per-row to e.g. 4, and > > > page-padding > > > to e.g. 7, while the show-scrollbars setting is set to true, then press > > > 's' or > > > 'a' to fit the collective width of the pages into the view. You will > > > notice > > > that the horizontal scrollbar is still showing up which shouldn't happen > > > because the pages now should be fitted entirely inside the view. > > > > Indeed, I see that too. > > > > > This happens because not adding the paddings to the width of the cells > > > will end > > > you up with a scaling factor that makes the pages fit into the view but > > > "only" > > > if there is no paddings. To verify this, set page-padding to 0, and > > > repeat the > > > same procedure. Now, things will work as expected. > > > > But your fix was in the wrong place, it should go within the > > "if (show_scrollbars)" section, line 153, as everything works well when > > show-scrollbars is false. And from what I can tell, the extra width > > isn't proportional to padding (try setting it to something big, then > > double it for instance); it seems to be just one pixel too wide, or > > something like that. > > > > > Since the paddings aren't part of the cells, consume some space of the > > > view, > > > and aren't subject to scaling, we should add them here to the total width > > > of > > > the cells in order to "best-fit" them together with the cells themselves. > > > > No, padding is already accounted for in the numerator (it is substracted > > from the window's width). So essentially, the scale is computed by > > saying that the width of all pages on a row (the denominator) has to fit > > in the width of the window minus the padding (the numerator). If you > > scale it like that, and then add padding between the pages, they will > > fit exactly in the window. The scrollbar issue is something completely > > different. > > > > > And regarding the borders on the edges, this only happens with multiple > > > pages > > > per row, which isn't the typical use case. And IMHO, as long as the pages > > > are > > > fitted entirely inside the view, the user wouldn't care. But having the > > > scrollbar showing after best-fit is what would more likely be considered > > > an > > > issue. > > > > Sure, the scrollbar issue needs to be fixed, but as I said, this isn't > > the correct way to fix it, and it affects best-fit's behavior when > > show-scrollbars is false, even though it was working fine before. > > It turns out you are right indeed. That was a misunderstanding of the formula > > on my side. No problem, it's a tricky part; maybe I should add some comments detailing what's going on... > Also, I've just noticed something that confirms that the problem with the > scrollbar is entirely unrelated: try experimenting with multiple subsequent > values of page-padding and you will notice that the scrollbar sometimes > appears > and sometimes doesn't. Yes, I actually suspect it's some rounding issue somewhere, I'll have to look into this. I don't use scrollbars myself, so I didn't notice it before. Thanks for pointing it out. Cheers, -- Benoît Knecht ___ zathura mailing list zathura@lists.pwmt.org http://lists.pwmt.org/mailman/listinfo/zathura
Re: [zathura] [PATCH] Revert commit ce6e735
On Thu, Mar 21, 2013 at 09:04:57AM +0100, Benoît Knecht wrote: > Marwan Tanager wrote: > > On Wed, Mar 20, 2013 at 11:03:54PM +, Benoît Knecht wrote: > > > This wasn't correct. Padding is already accounted for: > > > > > > scale = (width - (pages_per_row - 1) * padding) / > > > (pages_per_row * cell_width) > > > > > > If you add padding on the denominator, you end up with black margins on > > > the sides of the window, which isn't what padding is for (i.e. insert a > > > gap between rendered pages), and defeats the purpose of a "best-fit". > > > > Not accounting for the padding in the denominator is what will actually > > defeat > > the purpose of best-fit. > > > > To get a feel for this, try setting pages-per-row to e.g. 4, and > > page-padding > > to e.g. 7, while the show-scrollbars setting is set to true, then press 's' > > or > > 'a' to fit the collective width of the pages into the view. You will notice > > that the horizontal scrollbar is still showing up which shouldn't happen > > because the pages now should be fitted entirely inside the view. > > Indeed, I see that too. > > > This happens because not adding the paddings to the width of the cells will > > end > > you up with a scaling factor that makes the pages fit into the view but > > "only" > > if there is no paddings. To verify this, set page-padding to 0, and repeat > > the > > same procedure. Now, things will work as expected. > > But your fix was in the wrong place, it should go within the > "if (show_scrollbars)" section, line 153, as everything works well when > show-scrollbars is false. And from what I can tell, the extra width > isn't proportional to padding (try setting it to something big, then > double it for instance); it seems to be just one pixel too wide, or > something like that. > > > Since the paddings aren't part of the cells, consume some space of the > > view, > > and aren't subject to scaling, we should add them here to the total width > > of > > the cells in order to "best-fit" them together with the cells themselves. > > No, padding is already accounted for in the numerator (it is substracted > from the window's width). So essentially, the scale is computed by > saying that the width of all pages on a row (the denominator) has to fit > in the width of the window minus the padding (the numerator). If you > scale it like that, and then add padding between the pages, they will > fit exactly in the window. The scrollbar issue is something completely > different. > > > And regarding the borders on the edges, this only happens with multiple > > pages > > per row, which isn't the typical use case. And IMHO, as long as the pages > > are > > fitted entirely inside the view, the user wouldn't care. But having the > > scrollbar showing after best-fit is what would more likely be considered an > > issue. > > Sure, the scrollbar issue needs to be fixed, but as I said, this isn't > the correct way to fix it, and it affects best-fit's behavior when > show-scrollbars is false, even though it was working fine before. It turns out you are right indeed. That was a misunderstanding of the formula on my side. Also, I've just noticed something that confirms that the problem with the scrollbar is entirely unrelated: try experimenting with multiple subsequent values of page-padding and you will notice that the scrollbar sometimes appears and sometimes doesn't. Thanks for spotting this. Marwan ___ zathura mailing list zathura@lists.pwmt.org http://lists.pwmt.org/mailman/listinfo/zathura
Re: [zathura] [PATCH] Revert commit ce6e735
Marwan Tanager wrote: > On Wed, Mar 20, 2013 at 11:03:54PM +, Benoît Knecht wrote: > > This wasn't correct. Padding is already accounted for: > > > > scale = (width - (pages_per_row - 1) * padding) / > > (pages_per_row * cell_width) > > > > If you add padding on the denominator, you end up with black margins on > > the sides of the window, which isn't what padding is for (i.e. insert a > > gap between rendered pages), and defeats the purpose of a "best-fit". > > Not accounting for the padding in the denominator is what will actually > defeat > the purpose of best-fit. > > To get a feel for this, try setting pages-per-row to e.g. 4, and page-padding > to e.g. 7, while the show-scrollbars setting is set to true, then press 's' > or > 'a' to fit the collective width of the pages into the view. You will notice > that the horizontal scrollbar is still showing up which shouldn't happen > because the pages now should be fitted entirely inside the view. Indeed, I see that too. > This happens because not adding the paddings to the width of the cells will > end > you up with a scaling factor that makes the pages fit into the view but > "only" > if there is no paddings. To verify this, set page-padding to 0, and repeat > the > same procedure. Now, things will work as expected. But your fix was in the wrong place, it should go within the "if (show_scrollbars)" section, line 153, as everything works well when show-scrollbars is false. And from what I can tell, the extra width isn't proportional to padding (try setting it to something big, then double it for instance); it seems to be just one pixel too wide, or something like that. > Since the paddings aren't part of the cells, consume some space of the view, > and aren't subject to scaling, we should add them here to the total width of > the cells in order to "best-fit" them together with the cells themselves. No, padding is already accounted for in the numerator (it is substracted from the window's width). So essentially, the scale is computed by saying that the width of all pages on a row (the denominator) has to fit in the width of the window minus the padding (the numerator). If you scale it like that, and then add padding between the pages, they will fit exactly in the window. The scrollbar issue is something completely different. > And regarding the borders on the edges, this only happens with multiple pages > per row, which isn't the typical use case. And IMHO, as long as the pages are > fitted entirely inside the view, the user wouldn't care. But having the > scrollbar showing after best-fit is what would more likely be considered an > issue. Sure, the scrollbar issue needs to be fixed, but as I said, this isn't the correct way to fix it, and it affects best-fit's behavior when show-scrollbars is false, even though it was working fine before. Cheers, -- Benoît Knecht ___ zathura mailing list zathura@lists.pwmt.org http://lists.pwmt.org/mailman/listinfo/zathura
Re: [zathura] [PATCH] Revert commit ce6e735
On Wed, Mar 20, 2013 at 11:03:54PM +, Benoît Knecht wrote: > This wasn't correct. Padding is already accounted for: > > scale = (width - (pages_per_row - 1) * padding) / > (pages_per_row * cell_width) > > If you add padding on the denominator, you end up with black margins on > the sides of the window, which isn't what padding is for (i.e. insert a > gap between rendered pages), and defeats the purpose of a "best-fit". Not accounting for the padding in the denominator is what will actually defeat the purpose of best-fit. To get a feel for this, try setting pages-per-row to e.g. 4, and page-padding to e.g. 7, while the show-scrollbars setting is set to true, then press 's' or 'a' to fit the collective width of the pages into the view. You will notice that the horizontal scrollbar is still showing up which shouldn't happen because the pages now should be fitted entirely inside the view. This happens because not adding the paddings to the width of the cells will end you up with a scaling factor that makes the pages fit into the view but "only" if there is no paddings. To verify this, set page-padding to 0, and repeat the same procedure. Now, things will work as expected. Since the paddings aren't part of the cells, consume some space of the view, and aren't subject to scaling, we should add them here to the total width of the cells in order to "best-fit" them together with the cells themselves. And regarding the borders on the edges, this only happens with multiple pages per row, which isn't the typical use case. And IMHO, as long as the pages are fitted entirely inside the view, the user wouldn't care. But having the scrollbar showing after best-fit is what would more likely be considered an issue. Marwan ___ zathura mailing list zathura@lists.pwmt.org http://lists.pwmt.org/mailman/listinfo/zathura
