Created an attachment (id=1322) [details]
The trivial patch.

Created an attachment (id=1323) [details]
screenshot of test page at "fit width" zoom before applying the patch

Created an attachment (id=1324) [details]
and after

Confirming, and many thanks for the patch!  This may also go some way to
satisfying bug 761.

The only issue is that osso-pdf-viewer now displays a (useless) horizontal
scrollbar when at "fit width" zoom (see attached screenshots).  Not this
patch's fault though.

> The only issue is that osso-pdf-viewer now displays a (useless) horizontal
> scrollbar when at "fit width" zoom (see attached screenshots).  Not this
> patch's fault though.
> 

Thanks for screenshots and pointing this out. I'll see if I can fix it. The
patch might finally not be so trivial.

Created an attachment (id=1325) [details]
combined fit-width & scrollbar patch

The scrollbar problem seems to be calc_size_dpi() subtracting SCROLLBAR_SIZE
from width/height.  I don't think that's needed.

The scrollbar calculation worked before your patch only because
get_custom_zoom_level() was subtracting an even larger value.

The attached patch seems to work fine, but verification welcome (it is rather
late here...)

Created an attachment (id=1326) [details]
combined fit_width & get_custom_zoom_level patch

> The scrollbar problem seems to be calc_size_dpi() subtracting SCROLLBAR_SIZE
> from width/height.  I don't think that's needed.
No, it IS needed. With your patch there will be sometimes where a needed
scrollbar will not be showed.

> The scrollbar calculation worked before your patch only because
> get_custom_zoom_level() was subtracting an even larger value.
Yes, indeed.

The real problem seems to come from that (in get_custom_zoom_level):
    double screen_width =
        (double) priv->app_ui_data->scrolled_window->allocation.width;
    double screen_height =
        (double) priv->app_ui_data->scrolled_window->allocation.height;
In my tests, [...]allocation.width is 696 wich is bigger than
VIEWPORT_WIDTH(=672).
Then the SCROLLBAR_SIZE's got removed from that, giving 674 and due to the int
rounding of dpi, it works (the layout is then 671).
I don't know what exactly that scrolled_window->allocation.width represents nor
why does it has that value. But we don't really need it, indeed screen_width in
get_custom_level represents the available drawing space. So it is either
FULLSCREEN_WIDTH or VIEWPORT_WIDTH depending on the case we're in fullscreen
mode or not.
Maybe the use of scrolled_window->allocation.width was thought a shortcut to
avoid having to check the PDF_FLAGS_FULLSCREEN flag.

The attached patch should be okay.

(From update of attachment 1325 [details])
Thanks again :-)

One problem now is that it still wastes horizontal pixels (though fewer than
the original) when in windowed mode.  I think it's because the VIEWPORT_WIDTH
value already has the scrollbar width subtracted.

To my calculations, the only wasted pixels are coming from the rounding of the
calculated dpi value in the (guint) cast. But I didn't verify that on the new
patch, I don't have much time for that today, it will be for tomorrow/tonight.
This should be fixed if we modify the code to use (double) dpi everywhere or
only on the path of dpi's values used by the custom_zoom_level mechanism.
>the original) when in windowed mode.  I think it's because the VIEWPORT_WIDTH
>value already has the scrollbar width subtracted.
Will have to verify that too, but I think it is correct since, if I remember
well, after the initial trivial patch the layout width used was 674, so 2
pixels higher than VIEWPORT_WIDTH (explaining the scrollbar) and it seemed to
use full viewport width.

> Will have to verify that too, but I think it is correct since, if I remember
> well, after the initial trivial patch the layout width used was 674, so 2
> pixels higher than VIEWPORT_WIDTH (explaining the scrollbar) and it seemed to
> use full viewport width.
Hum, I'm sorry, I was totally wrong and you were right Lucas. Indeed it shows
in fact that, yes, the VIEWPORT_WIDTH value already has the scrollbar width
substracted. Since the layout size would have to be VIEWPORT_WIDTH -
SCROLLBAL_SIZE.
I have doubts about the real VIEWPORT_WIDTH, is it 694(=672+22) or
676(=priv->app_ui_data->scrolled_window->allocation.width) ?
So 2 pixels would have to be substracted somewhere ?

(In reply to comment #10)
> I have doubts about the real VIEWPORT_WIDTH, is it 694(=672+22) or
> 676(=priv->app_ui_data->scrolled_window->allocation.width) ?
> So 2 pixels would have to be substracted somewhere ?

It should be 696 pixels according to
<http://maemo.org/development/training/maemo_technology_overview_content/plain_html/node5/#SECTION00532000000000000000>.
 I don't know where those 2 extra pixels go :-(

Created an attachment (id=1329) [details]
dpi rounding consequence

> It should be 696 pixels according to [...]
Right, I have measured the viewport size on screenshots, with the scrollbar it
is 674 and without it, it is
696(=priv->app_ui_data->scrolled_window->allocation.width) -- excuse the typos
my previous post.
So Lucas was definitely right about the fact that SCROLLBAR_SIZE was already
substracted from VIEWPORT_WIDTH.
Updating the VIEWPORT_WIDTH value to the correct one works, but there is still
the problem of the rounding of the dpi value. That value is sometimes used as
double, and sometimes used as integer.
The rendering actually use the dpi value as double, the width of the displayed
document when using fit width (with vertical scrollbar) is indeed 674 as
expected. But at some places, the dpi value is used as integer, e.g the width
value in calc_size_dpi. That width value is then 671 and that causes a little
problem in the centering of the document. Indeed, the layout's width is viewed
as being 671 in a 674 viewport (although it really is 674) so it is centered
one pixel right and then you loose 1 pixel at right of the document.
Can we change the casts to double everywhere foo the dpi value, would'nt it
cause any other bug ?
And also, since SCROLLBAR_SIZE was already substracted from the VIEWPORT_WIDTH
value, isn't there any code that makes the assumption that it is done so ? Does
someone knows that or will we have to check the code everywhere that value is
used ?

Created an attachment (id=1330) [details]
attachment 1326 [details] + correct VIEWPORT_WIDTH/VIEWPORT_HEIGHT + floating point DPI
calculations

For reference:

- According to the documentation the normal view application area with a single
toolbar should be 696 x 360.

- src/constant.h defines:
#define SCROLLBAR_SIZE       22
#define VIEWPORT_WIDTH       672
#define VIEWPORT_HEIGHT      362

SCROLLBAR_SIZE seems correct (measured with gimp it's 20 pixels wide plus one
pixel padding on either side).

VIEWPORT_WIDTH has (SCROLLBAR_SIZE + 2) pixels subtracted relative to the
documented dimensions, while VIEWPORT_HEIGHT has 2 pixels *added*.  Fun...

(In reply to comment #12)
> Updating the VIEWPORT_WIDTH value to the correct one works, but there is still
> the problem of the rounding of the dpi value. That value is sometimes used as
> double, and sometimes used as integer.
> [...]
> But at some places, the dpi value is used as integer, e.g the width
> value in calc_size_dpi. That width value is then 671 and that causes a little
> problem in the centering of the document.

Indeed, good catch!  It also causes a few lines at the bottom to be off-screen
in fit-width or fit-page zoom on some documents.

> Can we change the casts to double everywhere foo the dpi value, would'nt it
> cause any other bug ?

Let's find out :-) This change touches several places, and I'm not sure I got
all of them but it compiles without (additional) warnings and it seems to work.
 Review welcome of course.

> And also, since SCROLLBAR_SIZE was already substracted from the VIEWPORT_WIDTH
> value, isn't there any code that makes the assumption that it is done so ?

It doesn't seem so, in fact calc_size_dpi() assumes the opposite and subtracts
another SCROLLBAR_SIZE pixels if it thinks it's necessary.

> someone knows that or will we have to check the code everywhere that value is
> used ?

Thankfully these #defines are not used outside of pdfviewer.cc so it should be
safe.

Created an attachment (id=1331) [details]
Incremental corrections to attachment 1330 [details]

(In reply to comment #13)
Thanks for that patch.

> - According to the documentation the normal view application area with a single
> toolbar should be 696 x 360.
It is strange that app_ui_data->scrolled_window->allocation.height is 362. And
in fact measuring the viewport height in gimp gives 362 too. So I would trust
gimp and not the documentation about that viewport dimensions :-D
So here is my incremental patch to attachment 1330 [details].

That may sound somewhat out of topic (it would rather go with some code
cleaning) but couldn't we use floats instead of doubles for dpi ?
We don't need a high precision on dpi since the layout dimensions are of 1
pixel per SCREEN_DPI/page_dimension dpi, that is 72/612 in our common case for
width. That is 1 pixel per ~0.11764 dpi. So we could save some bits without any
cost (i think).

(In reply to comment #14)
> It is strange that app_ui_data->scrolled_window->allocation.height is 362. And
> in fact measuring the viewport height in gimp gives 362 too. So I would trust
> gimp and not the documentation about that viewport dimensions :-D

You are right, and 362 does give 2 extra pixels at "fit page" zoom.  I guess
there's no point filing a bug against Diablo docs anymore, but hopefully this
one should now contain enough google fodder for others who are wondering to
find.

> So here is my incremental patch to attachment 1330 [details].

You missed VIEWPORT_HEIGHT ;-) (VIEWPORT_BUFFER_HEIGHT is dead code at the
moment, I just changed it to be consistent).

> That may sound somewhat out of topic (it would rather go with some code
> cleaning) but couldn't we use floats instead of doubles for dpi ?

Sure, if you feel like it, although the rest of the code uses doubles (with one
trivial exception) so it hardly seems worth the effort. But maybe
(stylistically speaking, it doesn't make any difference otherwise) some should
be gdoubles/gfloats instead?

Created an attachment (id=1335) [details]
combined previous patchs with corrections

Here is the full patch against 1.5.35
(In reply to comment #15)
> You missed VIEWPORT_HEIGHT ;-) (VIEWPORT_BUFFER_HEIGHT is dead code at the
Oops. Thanks.
There was another equality test on newly double instead of int in
ui/interface.c.
I have removed the test for the PDF_FLAGS_FULLSCREEN that I have introduced
before, since the initial code using
app_ui_data->scrolled_window->allocation.width (and height) is finally correct.

Thanks again for your help Lucas.
I hope there's no more missing corrections to make against those changes.

(From update of attachment 1330 [details])
(In reply to comment #16)
> Here is the full patch against 1.5.35

Looks fine to me.

> Thanks again for your help Lucas.

No, thank you :-)

Comment 18 Andre Klapper 2009-09-28 20:11:17 UTC

As Diablo/Maemo4 will not receive any such fixes anymore, it would be more than
welcome if you could update the patch for the Maemo5 codebase once this part is
public. There should not have been many changes in this code (probably none).
Thanks in advance!

(In reply to comment #18)
> As Diablo/Maemo4 will not receive any such fixes anymore, it would be more than
> welcome if you could update the patch for the Maemo5 codebase once this part is
> public. There should not have been many changes in this code (probably none).

Source or binaries aren't available yet (at least not in
<http://repository.maemo.org/pool/maemo5.0/free/o/> or anywhere apt-get can
find them), but based on the on-device pre-installed binary there's at least
one significant change: "fit width" zoom has been removed from the Fremantle
version.  Otherwise the app hasn't changed much so it shouldn't be too hard to
add it back.

> Source or binaries aren't available yet (at least not in
> <http://repository.maemo.org/pool/maemo5.0/free/o/> or anywhere apt-get can
> find them),
I'm patiently waiting for them.

Comment 21 Andre Klapper 2009-10-13 14:13:18 UTC

Zoom is only available from the toolbar, so I wonder how to quickly go to Fit
width mode? :-/

Currently this becomes enhancement because Fid width is simply not available
and not planned. Which is a pity of course.

Marking patches of interest to Diablo (Maemo4) community updates, please excuse
the noise.

Comment 23 Andre Klapper 2012-03-24 11:44:12 UTC

The Maemo 5 User Interface and Maemo 5 platform components (e.g. libraries)
used for the N900 are considered stable by Nokia and it seems that there are no
plans for official updates currently, hence nobody plans to work on this
enhancement/wishlist request. 
(And in case you feel like discussing this situation: Nokia Customer Care or
http://talk.maemo.org would be the place to do so as you will not reach Nokia
officials in this community bugtracker - though all of this is really no news.)

Reflecting this status by setting RESOLVED WONTFIX for this
enhancement/wishlist request (see
https://bugs.maemo.org/page.cgi?id=fields.html#status for status explanations).

There is a small chance for issues in those Maemo components that are open
source: Contributed patches could be included and made available in the Maemo 5
Community CSSU updates. 
The Maemo CSSU project is run by a small team of volunteers; see
http://wiki.maemo.org/CSSU for more information.
So in case that you can provide a patch that fixes the reported problem, please
feel encouraged to file a request under
https://bugs.maemo.org/enter_bug.cgi?product=Maemo%205%20Community%20SSU .
Please note: The Maemo CSSU project is not related in any way to Nokia.


( Tag for mass-deleting bugmail: [cleanup20120324] )