After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 169676 - A "Fit" function that shows only the text on a page
A "Fit" function that shows only the text on a page
Status: RESOLVED OBSOLETE
Product: evince
Classification: Core
Component: general
2.22.x
Other Linux
: Normal enhancement
: ---
Assigned To: Evince Maintainers
Evince Maintainers
: 169904 170077 171066 308945 313831 316459 317597 335183 410632 511595 567626 600439 603820 614749 617434 620773 682190 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2005-03-09 00:56 UTC by dann
Modified: 2018-05-22 12:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Acroread with geometry specification to show only the bounding box of a pdf file. (5.37 KB, text/plain)
2005-08-18 18:59 UTC, Ashvin Goel
  Details
adding the the fit to text function (23.38 KB, patch)
2014-04-14 15:28 UTC, Amine kabab
none Details | Review
I fixed some problems in the first patch, and i have test it with a bunch of pdf files, i think it's the right fix for this issue (17.71 KB, patch)
2014-04-17 00:03 UTC, Amine kabab
none Details | Review
Add fit to text function (15.95 KB, patch)
2014-05-01 20:46 UTC, Amine kabab
needs-work Details | Review
Add fit to text function (15.55 KB, patch)
2014-06-21 12:55 UTC, Amine kabab
none Details | Review
Add fit to text feature (15.96 KB, patch)
2014-07-25 03:14 UTC, Amine kabab
none Details | Review
Fit to text function (15.92 KB, patch)
2014-07-25 18:09 UTC, Amine kabab
reviewed Details | Review
Add fit te text feature (15.43 KB, patch)
2014-07-25 20:27 UTC, Amine kabab
needs-work Details | Review
Patch for the "trim margins" feature (24.20 KB, patch)
2015-02-11 17:00 UTC, Lauri Kasanen
needs-work Details | Review
Patch for the "fit content" feature (25.31 KB, patch)
2015-02-16 12:50 UTC, Lauri Kasanen
none Details | Review

Description dann 2005-03-09 00:56:06 UTC
Documents that are meant to be printed (like books, conference papers, etc.)
always have large margins that are empty (typically 1" at the top and bottom and
0.75" on the sides).

It would be great if Evince had a "Fit to text" function that would zoom in such
that only the text would be visible in the document window. The whitespace on
the margings does not matter at all when one wants to just read the content.
Even more this would allow for a higher zoom factor, making the text easier to
read.
Comment 1 Bryan W Clark 2005-03-11 06:49:09 UTC
*** Bug 169904 has been marked as a duplicate of this bug. ***
Comment 2 Martin Kretzschmar 2005-03-12 16:19:22 UTC
*** Bug 170077 has been marked as a duplicate of this bug. ***
Comment 3 Bryan W Clark 2005-03-24 00:45:47 UTC
*** Bug 171066 has been marked as a duplicate of this bug. ***
Comment 4 Alan Horkan 2005-03-24 13:11:09 UTC
Relevant comments paraphrased from bug 171066 (when closing duplicates including
relevant comments might help prevent more duplicates)

Adobe Acrobat reader has a feature called "Fit Visible" which will zoom to page
Width excluding margins.  This is what was expecting when I chose "Best Fit" but
for some reason 'best' means fit to height, which isn't something I think I'd
ever want to use (because if I wanted an overview I'd use the thumbnails).  

This is not a feature I have seen in any of the other open source PDF readers
yet so it would be a big incentive for me to use Evince if this feature was
implemented.  
Comment 5 Bryan W Clark 2005-07-11 14:59:24 UTC
Yeah, I'd like to see this done and possibly replace Best Fit.  I haven't been
pushing for it because there's other functionality I'd rather see done first.
Comment 6 Marco Pesenti Gritti 2005-07-11 15:51:01 UTC
*** Bug 308945 has been marked as a duplicate of this bug. ***
Comment 7 Nickolay V. Shmyrev 2005-08-18 18:37:03 UTC
*** Bug 313831 has been marked as a duplicate of this bug. ***
Comment 8 Ashvin Goel 2005-08-18 18:59:38 UTC
Created attachment 50943 [details]
Acroread with geometry specification to show only the bounding box of a pdf file.
Comment 9 Ashvin Goel 2005-08-18 19:01:11 UTC
Comment on attachment 50943 [details]
Acroread with geometry specification to show only the bounding box of a pdf file.

I have implemented zoom to bounding box externally for acroread. I use pdfinfo
to determine the page size of a pdf file. Then I use ghostscript to figure out
the bounding box of the document. Since the bounding box varies across pages, I
heuristically find the maximal bounding box until it does not change for 5-7
pages. Then based on the screen resolution and the bounding box, I calculate
the geometry for acroread so that *only* the bounding box is visible on screen
and then invoke acroread with this geometry. I can't do this with evince since
it doesn't provide the geometry argument. Please add the X geometry argument to
evince or if possible implement the "zoom to bounding box" or "fit to text"
since that is what I do manually otherwise while reading documents. Thanks!

I have attached my short perl-based bbacroread program.
Comment 10 Nickolay V. Shmyrev 2005-08-18 19:36:16 UTC
Thanks, Ashvin.

Now we are in freeze, but we'll certainly look at it later.
Comment 11 Nickolay V. Shmyrev 2005-09-16 23:01:20 UTC
*** Bug 316459 has been marked as a duplicate of this bug. ***
Comment 12 Nickolay V. Shmyrev 2005-09-30 21:41:20 UTC
*** Bug 317597 has been marked as a duplicate of this bug. ***
Comment 13 Zack Weinberg 2006-01-29 17:59:01 UTC
I too would value this functionality.
Comment 14 Martin Kretzschmar 2006-03-20 09:27:04 UTC
*** Bug 335183 has been marked as a duplicate of this bug. ***
Comment 15 Daniel Holbach 2006-04-06 17:32:19 UTC
Mentioned in https://launchpad.net/distros/ubuntu/+source/evince/+bug/38297 as well.
Comment 16 Carlos Garcia Campos 2007-02-22 09:55:14 UTC
*** Bug 410632 has been marked as a duplicate of this bug. ***
Comment 17 gwern 2008-07-31 20:55:27 UTC
I hope no one minds me chiming in with a Me Too! I agree with Zack and the rest; I was actually coming here to file exactly this request, so I'm glad to see so much progress already.
Comment 18 Carlos Garcia Campos 2009-11-02 16:57:39 UTC
*** Bug 600439 has been marked as a duplicate of this bug. ***
Comment 19 Carlos Garcia Campos 2009-11-02 16:59:32 UTC
I'm working on this, but it requires some changes in poppler first (and cairo <= 1.9.4 will be needed too)
Comment 20 Carlos Garcia Campos 2009-12-05 08:46:52 UTC
*** Bug 603820 has been marked as a duplicate of this bug. ***
Comment 21 Carlos Garcia Campos 2010-04-04 10:42:15 UTC
*** Bug 614749 has been marked as a duplicate of this bug. ***
Comment 22 Carlos Garcia Campos 2010-05-02 11:48:35 UTC
*** Bug 617434 has been marked as a duplicate of this bug. ***
Comment 23 Carlos Garcia Campos 2010-06-07 08:59:09 UTC
*** Bug 620773 has been marked as a duplicate of this bug. ***
Comment 24 oliver 2010-06-08 08:39:52 UTC
Would it be possible to increase the priority of this feature?  I keep coming back to this page to see if there was any progress, and it is a bit disheartening to see that nothing is changing for years...

I would like to add a few related thoughts which may be easier to implement.  It's clear that the use case is "on-screen reading of material typeset for printing on letter or A4-size paper".  The current user interaction model is not very well suited for this, although (at least for me) this is the most common situation in which I need to read PDFs or similar.

For this use case, "fit to visible" is probably an easy (on the user) sensible default behavior, but there are other possibilities which, independently, could lead to a similar or even better user experience.

I see two problems with "fit to visible": typically, not all pages have the same visible area.  Thus, should the zoom factor change from page to page, or possibly be the smallest zoom factor compatible with all cached pages?  Also, some documents have large headers or footers which I often would like to move off the visible area as they contain not information of importance.   So, the following would be nice:

- Almost continuous zoom levels (especially when zooming with CTRL-mousewheel)

- Moving the page up or down by mousewheel should be more continuous

- Very important: The default behavior of "Page Up" or "Page Down" should keep the view point, so that one does not need to reposition the page when scrolling through a document (most pages are similar, so once the document is well-positioned, I do not want to lose that).  If there is a good reason for not making this the default (I cannot see any, but possibly is), then there should be at least a mode which enables this behavior.

- Presentation mode currently disables "Dual" mode.  However, Presentation mode is the only way of maximizing vertical space (which is the bottleneck of modern 16:9 screens), so it's really annoying that it does not support "Dual"

- Actually, "Dual" mode could really be enhanced as follows: A 16:9 screen is usually capable of displaying three pages next to each other with maximal vertical visible area.  This could be exploited in several ways: A "Triple" mode which displays three pages ("fit to visible" or better manually positioned for maximal zoom level at which continuous reading is possible), page up/down would only go one physical page up or down so that any three consecutive physical pages could be positioned on-screen for continuous reading.  What would be really cool is a horizontal scroll mode where one could continuously move, with the mouse, a horizontal band of vertically maximized pages, left or right.  (I think this would be much more useful than the vertical scroll mode ("Continuous") which I almost always switch off because the physical distance between the last relevant line of one page and the first relevant line, excluding footers and headers, of the next page is usually far to big for documents which have been laid out for printing.)

Excuse my adding feature requests which do not strictly speaking fall under this bug's heading, but I believe that they are very much part of the underlying problem that should be solved.
Comment 25 Carlos Garcia Campos 2010-07-15 10:03:49 UTC
(In reply to comment #24)
> Would it be possible to increase the priority of this feature?  I keep coming
> back to this page to see if there was any progress, and it is a bit
> disheartening to see that nothing is changing for years...
> 
> I would like to add a few related thoughts which may be easier to implement. 
> It's clear that the use case is "on-screen reading of material typeset for
> printing on letter or A4-size paper".  The current user interaction model is
> not very well suited for this, although (at least for me) this is the most
> common situation in which I need to read PDFs or similar.
> 
> For this use case, "fit to visible" is probably an easy (on the user) sensible
> default behavior, but there are other possibilities which, independently, could
> lead to a similar or even better user experience.
> 
> I see two problems with "fit to visible": typically, not all pages have the
> same visible area.  Thus, should the zoom factor change from page to page, or
> possibly be the smallest zoom factor compatible with all cached pages?

Good point, well, I guess we should do the same we are currently doing with fit-width in continuous mode and use the smallest zoom factor. That's a problem because getting the page bounding box is a heavy task. So maybe we could just use a different scale factor for every page. Another problem is the page positioning, in single page mode we can just move the scrollbars on every page change, but it's not possible in continuous mode. Dual page makes the whole thing even harder, I'm afraid. 

Another solution might be what okular does, instead of a new zoom mode they use an option called "trim margins", that changes the page size removing the margings. Trim margins and fit-width it's probably what most people want.

>  Also,
> some documents have large headers or footers which I often would like to move
> off the visible area as they contain not information of importance.   So, the
> following would be nice:
> 
> - Almost continuous zoom levels (especially when zooming with CTRL-mousewheel)

what do you mean by continuous zoom levels?

> - Moving the page up or down by mousewheel should be more continuous
> 
> - Very important: The default behavior of "Page Up" or "Page Down" should keep
> the view point, so that one does not need to reposition the page when scrolling
> through a document (most pages are similar, so once the document is
> well-positioned, I do not want to lose that).  If there is a good reason for
> not making this the default (I cannot see any, but possibly is), then there
> should be at least a mode which enables this behavior.

We currently do that, see bug #170871

> - Presentation mode currently disables "Dual" mode.  However, Presentation mode
> is the only way of maximizing vertical space (which is the bottleneck of modern
> 16:9 screens), so it's really annoying that it does not support "Dual"

Presentation mode is for presentation, we have fullscreen mode for that. 

> - Actually, "Dual" mode could really be enhanced as follows: A 16:9 screen is
> usually capable of displaying three pages next to each other with maximal
> vertical visible area.  This could be exploited in several ways: A "Triple"
> mode which displays three pages ("fit to visible" or better manually positioned
> for maximal zoom level at which continuous reading is possible), page up/down
> would only go one physical page up or down so that any three consecutive
> physical pages could be positioned on-screen for continuous reading.  What
> would be really cool is a horizontal scroll mode where one could continuously
> move, with the mouse, a horizontal band of vertically maximized pages, left or
> right.  (I think this would be much more useful than the vertical scroll mode
> ("Continuous") which I almost always switch off because the physical distance
> between the last relevant line of one page and the first relevant line,
> excluding footers and headers, of the next page is usually far to big for
> documents which have been laid out for printing.)

Please, file this as a new bug report. 

> Excuse my adding feature requests which do not strictly speaking fall under
> this bug's heading, but I believe that they are very much part of the
> underlying problem that should be solved.

Thanks!, the feedback is always very appreciated
Comment 26 oliver 2010-07-15 12:16:06 UTC
(In reply to comment #25)
> (In reply to comment #24)
> > I see two problems with "fit to visible": typically, not all pages have the
> > same visible area.  Thus, should the zoom factor change from page to page, or
> > possibly be the smallest zoom factor compatible with all cached pages?
> 
> Good point, well, I guess we should do the same we are currently doing with
> fit-width in continuous mode and use the smallest zoom factor. That's a problem
> because getting the page bounding box is a heavy task. So maybe we could just
> use a different scale factor for every page. Another problem is the page
> positioning, in single page mode we can just move the scrollbars on every page
> change, but it's not possible in continuous mode. Dual page makes the whole
> thing even harder, I'm afraid. 
> 
> Another solution might be what okular does, instead of a new zoom mode they use
> an option called "trim margins", that changes the page size removing the
> margings. Trim margins and fit-width it's probably what most people want.

Thinking about it again, what might work best is a "trim margins" button/keyboard shortcut which does what it advertises AND keeps the same magnification and viewpoint for subsequent Page Up/Page Down operations.  That will suffice for typical print material.  If necessary, the user can always readjust be hitting "trim margins" again.

> >  Also,
> > some documents have large headers or footers which I often would like to move
> > off the visible area as they contain not information of importance.   So, the
> > following would be nice:
> > 
> > - Almost continuous zoom levels (especially when zooming with CTRL-mousewheel)
> 
> what do you mean by continuous zoom levels?

When I do CTRL-mousewheel, the steps in which I can zoom is 85%, 100%, 125%, 175%, etc.  Really rather coarse, I'd like to have more steps in between.

> > - Moving the page up or down by mousewheel should be more continuous
> > 
> > - Very important: The default behavior of "Page Up" or "Page Down" should keep
> > the view point, so that one does not need to reposition the page when scrolling
> > through a document (most pages are similar, so once the document is
> > well-positioned, I do not want to lose that).  If there is a good reason for
> > not making this the default (I cannot see any, but possibly is), then there
> > should be at least a mode which enables this behavior.
> 
> We currently do that, see bug #170871

I read through this bug, but I am not sure it does what I was describing.  Running evince-2.30.3 right now, I cannot find anything close.  Maybe I am just brain-dead?  In any case, I am thinking of the equivalent of the "k" keyboard shortcut in xdvi.  (They got it right more than 20 years ago... it is the main reason I still fire up xdvi for dvi files rather than using evince - speed is NOT the issue, on modern hardware, the perceived difference is negligible.)

> > - Presentation mode currently disables "Dual" mode.  However, Presentation mode
> > is the only way of maximizing vertical space (which is the bottleneck of modern
> > 16:9 screens), so it's really annoying that it does not support "Dual"
> 
> Presentation mode is for presentation, we have fullscreen mode for that. 

Yes, but I cannot find how to remove the title bar in fullscreen mode, which is eating up my vertical screen real estate.  

> > - Actually, "Dual" mode could really be enhanced as follows: A 16:9 screen is
> > usually capable of displaying three pages next to each other with maximal
> > vertical visible area.  This could be exploited in several ways: A "Triple"
> > mode which displays three pages ("fit to visible" or better manually positioned
> > for maximal zoom level at which continuous reading is possible), page up/down
> > would only go one physical page up or down so that any three consecutive
> > physical pages could be positioned on-screen for continuous reading.  What
> > would be really cool is a horizontal scroll mode where one could continuously
> > move, with the mouse, a horizontal band of vertically maximized pages, left or
> > right.  (I think this would be much more useful than the vertical scroll mode
> > ("Continuous") which I almost always switch off because the physical distance
> > between the last relevant line of one page and the first relevant line,
> > excluding footers and headers, of the next page is usually far to big for
> > documents which have been laid out for printing.)
> 
> Please, file this as a new bug report. 

Done, see https://bugzilla.gnome.org/show_bug.cgi?id=624447
Comment 27 Philip Ganchev 2010-12-01 02:18:53 UTC
I do not get a title bar in fullscreen mode, either with or without dual mode. All I see is two of the pages of the document, side-by-side, and a vertical scroll bar at the right edge. This is Evince 2.28.1.
Comment 28 oliver 2010-12-20 11:31:28 UTC
(In reply to comment #27)
> I do not get a title bar in fullscreen mode, either with or without dual mode.
> All I see is two of the pages of the document, side-by-side, and a vertical
> scroll bar at the right edge. This is Evince 2.28.1.

Indeed, I misspoke: It's the toolbar, not the title bar (evince 2.30.3 here).  But my point stands: on a 16:9 screen, the critical dimension is the vertical.  In the horizontal, there is enough space to conveniently display two (sometimes even three) A4/letter sized pages.  So what I was asking for is a mode which maximizes the display height for the document (as in presentation mode) while still allowing to display two pages side by side.
Comment 29 José Aliste 2010-12-20 13:26:09 UTC
(In reply to comment #28)
> (In reply to comment #27)
> > I do not get a title bar in fullscreen mode, either with or without dual mode.
> > All I see is two of the pages of the document, side-by-side, and a vertical
> > scroll bar at the right edge. This is Evince 2.28.1.
> 
> Indeed, I misspoke: It's the toolbar, not the title bar (evince 2.30.3 here). 
> But my point stands: on a 16:9 screen, the critical dimension is the vertical. 
> In the horizontal, there is enough space to conveniently display two (sometimes
> even three) A4/letter sized pages.  So what I was asking for is a mode which
> maximizes the display height for the document (as in presentation mode) while
> still allowing to display two pages side by side.

Oliver, indeed, this is the exact behaviour you get if you disable the toolbar, set dual mode + best fit mode and fullscreen mode. If  you feel that something is wrong/missing about this please file a new bug describing what you want/need, as this bug is about trimming the margins to get rid of the blank space :)
Comment 30 oliver 2010-12-20 14:34:57 UTC
(In reply to comment #29)
> Oliver, indeed, this is the exact behaviour you get if you disable the toolbar,
> set dual mode + best fit mode and fullscreen mode. If  you feel that something
> is wrong/missing about this please file a new bug describing what you
> want/need, as this bug is about trimming the margins to get rid of the blank
> space :)

Indeed, this works.  I could have thought of this myself...

I think this is the case where the View Options are not perfectly aligned with the typical usage scenario.  I have always thought of "Full Screen" mode as the instruction "give me quickly as much screen real estate as possible".  So if this is what I want, I need to uncheck "Toolbar", check "Fullscreen", after exit, check "Toolbar" again (as the Toolbar is generally a bit click saver...).

I am not sure if I am making this up against everybody else's typically working habit, or if others feel that this is a bit awkward, too.  

In any case, this is a very minor wart.  The bigger issue is the fit visible/retain view point functionality (what this bugzilla entry is nominally about), which year after year remains my number one complaint with evince, so progress in that respect would be great if progress could be made.
Comment 31 eclecticos 2011-06-26 05:02:49 UTC
(In reply to comment #26)
> Thinking about it again, what might work best is a "trim margins"
> button/keyboard shortcut which does what it advertises AND keeps the same
> magnification and viewpoint for subsequent Page Up/Page Down operations.  That
> will suffice for typical print material.  If necessary, the user can always
> readjust be hitting "trim margins" again.

I would also like to vote strongly for this design. Like Oliver, I have wanted a "trim margins" feature for years, so that I can increase text size and avoid overlapping windows.

Use case #1 (Best Fit mode): I am writing a paper in TeX and experimenting with ways to edit and reformat it so as to get an 8-page PDF.  To test the effect of different edits on the layout, I prefer to see a whole page of the PDF at once.  I would like to trim top and bottom margins so that I can use the full height of my laptop monitor, and to trim left and right margins so that I have room to edit the TeX file in an editor window to the right to the evince window.

Use case #2 (Fullscreen mode): Two students and I are studying a technical paper together on the same monitor.  The monitor is rotated to portrait orientation and is presenting the paper in fullscreen mode.  Because there are 3 of us, we are not sitting very close to the monitor.  So we would like to trim margins in order to maximize text size.
Comment 32 eclecticos 2011-06-26 05:17:26 UTC
Some further remarks:

* Trimming exactly to the bounding box may be too much.  One may need to keep a small margin for readability, so that pixels of ink are not right up against the edge of the frame.

* The requested feature is NOT really about trimming in the sense of altering the page.  It is only about how to set the viewpoint for trimmed versions of Best Fit and Fullscreen mode.  For example, in a wide window, this viewpoint would zoom in to the point where the top and bottom margins were no longer visible, but the left and right margins would still be visible.

* However, some users might also want a true trimming feature that (mostly) removes the top and bottom margins during Continuous mode, and the left and right margins during Dual mode.  This avoids wasting space in the middle of the window between adjacent pages.

* The bounding box sometimes includes a page number, copyright notice, background graphics, and other things that are not important to view.  So it would be nice to be able to trim to something other than the bounding box.  But the choice may be tricky to adjust automatically or manually.  So I wouldn't start with this.
Comment 33 Josh Triplett 2011-10-28 07:07:26 UTC
This bug seems like a perfect fit for Google Code In.  If anyone involved with this bug wanted to consider serving as a mentor for it, please add a task to https://live.gnome.org/action/login/GoogleCodeIn/Tasks .
Comment 34 Giampiero Salvi 2013-05-03 14:10:16 UTC
Hi,
I agree this feature will improve the user experience immensely. I also suggest an additional very related feature. I do it here because I believe large part of the code would be shared with the one discussed in this thread.

I often need to print long documents such as PhD/Master theses for review. In order to save paper and kilos to carry around, I print two pages per side. It would be great if the same "fit to text" functionality was available when printing in this mode to use the paper area in the most efficient way.

So far I have been using pdfshissors (http://www.pdfscissors.com/) to create a trimmed pdf and then Evince to print it. Works fine, but it would be great if I could do the same with a click in Evince.

Thank you!
Comment 35 Germán Poo-Caamaño 2013-06-15 06:35:03 UTC
*** Bug 567626 has been marked as a duplicate of this bug. ***
Comment 36 Germán Poo-Caamaño 2013-06-15 06:36:08 UTC
*** Bug 511595 has been marked as a duplicate of this bug. ***
Comment 37 Arian@sanusi.de 2013-12-06 11:49:48 UTC
here is an example especially painful to read in pdf readers without trim margins feature [0]
This feature is the sole reason I have okular and hence half of KDE installed on my Laptop, where screen space is scarce. I pledge 50$ for this to be fixed.

[0] http://www.pqcrypto.org/www.springer.com/cda/content/document/cda_downloaddocument/9783540887010-c1.pdf
Comment 38 Adam Dingle 2013-12-06 13:50:23 UTC
Arian, thanks for your interest in seeing this fixed.  If you'd like to pledge $50, please add it to the existing bounty for this bug at Bountysource:

https://www.bountysource.com/issues/1327922-a-fit-function-that-shows-only-the-text-on-a-page
Comment 39 Germán Poo-Caamaño 2014-02-23 20:08:36 UTC
*** Bug 682190 has been marked as a duplicate of this bug. ***
Comment 40 Amine kabab 2014-04-14 15:25:42 UTC
I have found a solution, I tried to find all the text layouts in the current page using ev_document_text_get_text_layout, and I looked for the min margin left and the max margin right, to get the new page width, and I added a new scroll type I called scroll to text, so when the page scale changed I put SCOLL_TO_TEXT in pending_scoll field. I have attached my patch in the bug, please can someone review it, thanks.
Comment 41 Amine kabab 2014-04-14 15:28:14 UTC
Created attachment 274284 [details] [review]
adding the the fit to text function

I tried to find all the text layouts in the current
page using ev_document_text_get_text_layout, and I looked for the min margin
left and the max margin right, to get the new page width, and I added a new
scroll type I called scroll to text, so when the page scale changed I put
SCOLL_TO_TEXT in pending_scoll field.
Comment 42 Snark 2014-04-14 16:44:52 UTC
Is the "test-driver" shell script really related to this bug report?
Comment 43 Amine kabab 2014-04-14 18:05:49 UTC
no, I don't know how it got there ?!
Comment 44 Snark 2014-04-14 18:44:27 UTC
Eh, just remove it... 

Did you test it with the large-margins pdf in comment 37?

It would be good if I could lay my hand on a more interesting example, like a scanned document where left and right margins have various sizes depending on the page (well, the page number) : do you think your patch would handle such a case nicely?
Comment 45 Amine kabab 2014-04-14 19:06:10 UTC
of course, I don't estimate the large margins but I calculate them, I get the current page number, after that I extract all the coordinates of the text layouts in the page, andIi think it will work with all the text documents.
Comment 46 Snark 2014-04-14 19:37:57 UTC
Are there "text layouts" in scanned documents? [I'm thinking about something from http://www.numdam.org or http://gdz.sub.uni-goettingen.de/gdz/ ]
Comment 47 Amine kabab 2014-04-14 20:24:32 UTC
a scanned document doesn't have text layouts, you should first apply OCR to the document. here s a way to do it : http://www.adobe.com/products/acrobat/convert-jpeg-scan-ocr-to-pdf.html
Comment 48 Amine kabab 2014-04-17 00:03:32 UTC
Created attachment 274537 [details] [review]
I fixed some problems in the first patch, and i have test it with a bunch of pdf files, i think it's the right fix for this issue
Comment 49 Amine kabab 2014-04-18 19:09:48 UTC
Can someone please review the last patch ?
Comment 50 Snark 2014-04-19 06:24:16 UTC
I had a deeper look, and noticed the following:
1. the patch sometimes adds empty lines which are not really empty (there are spaces): see the chunk on ev_view_set_adjustment_values and on ev_view_zoom_for_size_continuous_and_dual_page
2. the patch removes an empty line separating a local variable declaration from a g_return_* guard: see the chunk on ev_view_zoom.

I'll be trying your patch when I find more time... for now I just had five minutes to read the code. (Notice I'm not an evince developer, just an interested user)
Comment 51 Snark 2014-04-19 12:36:07 UTC
I compiled it and tested it on the pdf from comment #37, and it worked quite well. It's indeed not able to detect where the text is if it comes from a scan.
Comment 52 eclecticos 2014-04-19 15:48:44 UTC
Amine, thanks.  I am one of the users who submitted design ideas above.  I am willing to try out the patch and give feedback if you can point me to instructions for how to build a patched version.

(I have never had to compile any part of GNOME myself and don't have the source on my system.  In fact, what I am running is MATE, the fork of Gnome 2 that is distributed with some editions of Linux Mint.  Under MATE, evince was renamed to atril.)
Comment 53 Amine kabab 2014-04-19 18:05:02 UTC
(In reply to comment #52)
> Amine, thanks.  I am one of the users who submitted design ideas above.  I am
> willing to try out the patch and give feedback if you can point me to
> instructions for how to build a patched version.
> 
> (I have never had to compile any part of GNOME myself and don't have the source
> on my system.  In fact, what I am running is MATE, the fork of Gnome 2 that is
> distributed with some editions of Linux Mint.  Under MATE, evince was renamed
> to atril.)

Visit this link https://wiki.gnome.org/Apps/Evince/GettingEvince and follow the instructions for installing evince on debian and ubuntu, after installing evince, download the patch to this folder 
~/jhbuild/evince/checkout/evince/
After that you have to apply the patch to the code, using this command
$ git apply path_file_name
Before applying the patch you have to be on the folder above, now you have to build the code
$ cd ~/jhbuild
$ jhbuild -f jhbuildrc-evince build evince
To run evince
$ jhbuild -f jhbuildrc-evince run evince
good luck
Comment 54 Snark 2014-04-27 12:55:40 UTC
Ok, I gave the patch another, longer try, with a pdf I wanted to read ; the impression I have is that the zoom is computed correctly, but that the horizontal position is left as-is. That is doubly annoying: first, on the current page, one has to scroll precisely so the text is entirely in view ; and second, when scrolling to each next page, one has to scroll again for the same reason (because odd and even pages have a tendency to alternate).

The pdf I used was: http://www.math.polytechnique.fr/~golse/MAT431-10/POLY431.pdf
Comment 55 Amine kabab 2014-05-01 20:45:37 UTC
(In reply to comment #54)
> Ok, I gave the patch another, longer try, with a pdf I wanted to read ; the
> impression I have is that the zoom is computed correctly, but that the
> horizontal position is left as-is. That is doubly annoying: first, on the
> current page, one has to scroll precisely so the text is entirely in view ; and
> second, when scrolling to each next page, one has to scroll again for the same
> reason (because odd and even pages have a tendency to alternate).
> 
> The pdf I used was:
> http://www.math.polytechnique.fr/~golse/MAT431-10/POLY431.pdf

Well, I fixed the problem for the horizontal scroll, that when you 'fit to text',it focus on the text, but the feature that when you scroll down the functionality apply on each page, I don't think it a good idea, because on the continuous mode, you can see two pages at once, so the question is which one we will focus on ? I checked Acrobat Reader and I found that when you go to the next page the horizontal scroll position doesn't change.
Comment 56 Amine kabab 2014-05-01 20:46:44 UTC
Created attachment 275572 [details] [review]
Add fit to text function
Comment 57 eclecticos 2014-05-01 20:56:10 UTC
> because on the continuous mode, you can see two pages at once, 
> so the question is which one we will focus on ?

Seems to be an obvious answer: Both.  If two pages are visible, then you should set the zoom and scroll so that you can see all of the text on both pages.
Comment 58 Amine kabab 2014-05-08 11:35:35 UTC
(In reply to comment #57)
> Seems to be an obvious answer: Both.  If two pages are visible, then you should
> set the zoom and scroll so that you can see all of the text on both pages.

Yeah, but don't you think if we change the zoom and the scroll each time the user changes the current page, it s going to be annoying for the UX ? otherwise I watched the behavior of fit to width function, and I found out that evince calculates the max width for all pages, it doesn't change the width for each page, I recommend you to try this on a file that contains pages with different width.
Comment 59 Snark 2014-05-09 09:24:48 UTC
The pages should be cropped, rescaled, etc so reading just involves scolling vertically... that bug is precisely about fitting the view and the text so this happens, doesn't it?
Comment 60 Amine kabab 2014-05-24 20:33:56 UTC
If we rescale the page size the font size will change too, I think it will be quit good if implement the same behavior of the fit visible function in acrobat reader, what do you think ?
Comment 61 Carlos Garcia Campos 2014-05-30 08:26:16 UTC
Review of attachment 275572 [details] [review]:

I've finally found some time to try this, thanks for the patch and sorry for the long delay. By trying it I've noticed a couple of things:

 - In some documents, like hig2.0, odd pages look good, but even pages are scrolled horizontally, so you always have to scroll horizontally to manually fit the text in the view.
 - It doesn't work when the window is resized, again the horizontal scrollbar is reset to the start/end position
 - In documents with no text a huge scale factor is used.
 - It doesn't work at all in documents with different page sizes/orientations, try with FinalLinkTest.pdf (from acroeng.adobe.com), for example

I have some comments also about the code, it seems there are many indentation issues and coding style inconsistencies.

::: libdocument/ev-document-text.c
@@ +93,3 @@
 }
+
+EvRectangle

Either return a pointer or make this boolean and return the rectangle as an out parameter.

@@ +94,3 @@
+
+EvRectangle
+ev_document_text_get_text_region(EvDocumentText *document_text, EvPage *page)

This is confusing, we already have a function to return the text region, this actually returns the bounding box, right? I would call this get_text_bounding_box. I'm not sure we want this in ev_document, though, I think this could be a private function in ev-view.c since it's only used there.

@@ +101,3 @@
+	int i;
+	gboolean success;
+	success = ev_document_text_get_text_layout (document_text, page, &rect, &n_rect);

Leave an empty line between the var declarations block and the function body.

@@ +102,3 @@
+	gboolean success;
+	success = ev_document_text_get_text_layout (document_text, page, &rect, &n_rect);
+    if(!success || n_rect <= 0){

This is not correctly indented, see the previous line.

@@ +111,3 @@
+	rect_res.y2 = rect[0].y2;
+
+	for(i=1;i<n_rect;i++){

Please, use a space before (, around operators like = and < and after the ). This should look something like:

for (i = 1; i < n_rect; i++) {

::: libview/ev-document-model.h
@@ +43,3 @@
  * @EV_SIZING_FIT_WIDTH:
  * @EV_SIZING_FREE:
+ * @EV_SIZING_FIT_TEXT:

This should be after the AUTOMATIC one and include a Since: 3.14 tag

@@ +51,3 @@
 	EV_SIZING_FIT_WIDTH,
 	EV_SIZING_FREE,
+	EV_SIZING_FIT_TEXT,

This should be the last one

::: libview/ev-view.c
@@ +650,3 @@
 			factor = (value + page_size * 0.5) / upper;
 			break;
+				case SCROLL_TO_TEXT:

This is not correctly indented.

@@ +652,3 @@
+				case SCROLL_TO_TEXT:
+			current_page = ev_document_model_get_page (view->model);
+			page = ev_document_get_page(view->document, current_page);

ev_document_get_page(view->document -> ev_document_get_page (view->document

@@ +661,3 @@
+				factor = (value + ((rect.x2 -rect.x1) / 2 + rect.x1 ) * page_size / width) / upper;
+			} else {
+				factor = (value + page_size * 0.5) / upper;

So, for vertical orientation you are doing a scroll to center? in this case you could check the orientation before to avoid getting the text bbox for nothing.

@@ +693,3 @@
 			gtk_adjustment_set_value (adjustment, (int)new_value);
 			break;
+				case SCROLL_TO_TEXT:

This is not correctly indented either, which makes the patch more difficult to follow.

@@ +696,3 @@
+			new_value = CLAMP (upper * factor - page_size * 0.5 + 0.5,
+					   0, upper - page_size);
+			gtk_adjustment_set_value (adjustment, (int)new_value);

This code is exactly the same as the scroll to center. Instead of duplicating it, add case SCROLL_TO_TEXT: right after the case SCROLL_TO_CENTER: to use the same code in both cases.

@@ +3596,2 @@
 			break;
+			case EV_SIZING_FIT_TEXT:

Bad indented.

@@ +3627,2 @@
 			break;
+			case EV_SIZING_FIT_TEXT:

Ditto.

@@ +7397,3 @@
+{
+
+	return (double)target_width / doc_width;

What's the point of this function if it's exactly the same as zoom_for_size_fit_width?

@@ +7484,3 @@
+	current_page = ev_document_model_get_page (view->model);
+	page = ev_document_get_page(view->document, current_page);
+	rect = ev_document_text_get_text_region ( EV_DOCUMENT_TEXT(view->document ), page );

Don't use space after the ( and before the )

@@ +7485,3 @@
+	page = ev_document_get_page(view->document, current_page);
+	rect = ev_document_text_get_text_region ( EV_DOCUMENT_TEXT(view->document ), page );
+	text_width = rect.x2 - rect.x1;

Don't do this before the switch since it's only needed if sizing mode is fit to text.

@@ +7519,3 @@
 	gint sb_size;
+	gint current_page;
+    EvPage *page;

Bad indented.

@@ +7542,3 @@
+	page = ev_document_get_page(view->document, current_page);
+	rect = ev_document_text_get_text_region ( EV_DOCUMENT_TEXT(view->document ), page );
+	text_width = rect.x2 - rect.x1;

This code is duplicated in several places, you could add a helper function ev_view_get_page_text_width() or something like that. Probably zoom_for_size_fit_text should actually do this

::: shell/ev-window.c
@@ +6270,3 @@
 	  N_("Make the current document fill the window width"),
 	  G_CALLBACK (ev_window_cmd_view_fit_width) },
+	{ "ViewFitText", EV_STOCK_ZOOM_WIDTH, N_("Fit Text"), NULL,

Don't use the same icon than fit width action
Comment 62 Amine kabab 2014-06-21 12:55:47 UTC
Created attachment 278890 [details] [review]
Add fit to text function 

Thank you for reviewing the patch,  and sorry for the bad coding style, I tried to fix some problems in the code, that you are already mention.

- I fixed the resizing and scrolling problem for normal documents
- I added a condition to check if there s no text in the page,in case of no text, the function apply fit automatic feature.
- I fixed the code indentation issues

But I didn't add a solution, for document with different orientations.
Comment 63 Amine kabab 2014-07-25 03:14:21 UTC
Created attachment 281653 [details] [review]
Add fit to text feature

Update for the previous patch
Comment 64 Germán Poo-Caamaño 2014-07-25 15:11:50 UTC
Please, may you rebase the patch against master?

In addition, please keep in mind some changes too:


if(...)    --> if (...)
if (...){  --> if (...) {
}else      --> } else

etc.

Thanks for working on this.
Comment 65 Amine kabab 2014-07-25 18:09:15 UTC
Created attachment 281716 [details] [review]
Fit to text function

Thanks for reviewing the patch :)
Comment 66 Germán Poo-Caamaño 2014-07-25 19:03:55 UTC
Review of attachment 281716 [details] [review]:

More styles issues commented.

::: libview/ev-view.c
@@ +294,3 @@
+static gboolean   get_text_bounding_box                      (EvDocumentText *document_text,
+							      EvPage *page,
+							      EvRectangle *bbox);

The variables should be aligned in the function definition.
See the other groups of declarations as an example.
e.g. ev_view_primary_get_cb ()

@@ +669,3 @@
+				}
+			} else
+				return;

Here you can use an early return instead.

if (orientation != GTK_ORIENTATION_HORIZONTAL)
    return;

And have everything saving one level of indentation.

@@ +3675,3 @@
+		case EV_SIZING_FIT_TEXT:
+		case EV_SIZING_FREE:
+			{

This bracket is unnecessary. See the style of the 'case',
e.g. in _ev_view_transform_doc_rect_to_view_rect ()

@@ +3714,3 @@
+				compute_border (view, &border);
+				requisition->width = max_width + (view->spacing * 2) + border.left + border.right;
+			}

Same here.

@@ +7771,3 @@
+			}
+
+		}

Same here.

@@ +7834,1 @@
 	case EV_SIZING_AUTOMATIC:

Same here

@@ +7897,3 @@
+				break;
+			}
+		}

Same here.

@@ +7952,3 @@
+				break;
+			}
+		}

Same here
Comment 67 Amine kabab 2014-07-25 20:02:42 UTC
(In reply to comment #66)
> Review of attachment 281716 [details] [review]:
> 
> More styles issues commented.
> 
> ::: libview/ev-view.c
> This bracket is unnecessary. See the style of the 'case',
> e.g. in _ev_view_transform_doc_rect_to_view_rect ()
> 
> @@ +3714,3 @@
> +                compute_border (view, &border);
> +                requisition->width = max_width + (view->spacing * 2) +
> border.left + border.right;
> +            }
> 
> Same here.
> 
> @@ +7771,3 @@
> +            }
> +
> +        }
> 
> Same here.
> 
> @@ +7834,1 @@
>      case EV_SIZING_AUTOMATIC:
> 
> Same here
> 
> @@ +7897,3 @@
> +                break;
> +            }
> +        }
> 
> Same here.
> 
> @@ +7952,3 @@
> +                break;
> +            }
> +        }
> 
> Same here

( As in understand from you comment ), I think the brackets are necessary cause I m declaring variables inside the brackets scope ?
Comment 68 Germán Poo-Caamaño 2014-07-25 20:08:58 UTC
In such case, use them as in the rest of the code.

  case ....: {


  }
      break;
  ...
Comment 69 Amine kabab 2014-07-25 20:27:19 UTC
Created attachment 281727 [details] [review]
Add fit te text feature
Comment 70 Carlos Garcia Campos 2014-08-03 10:16:17 UTC
Review of attachment 281727 [details] [review]:

I've tried it and it still doesn't seem to work, I have to scroll horizontally after setting zoom to fit to text to see the text, and then everytime I move to a different page.

::: libview/ev-view.c
@@ +294,3 @@
+static gboolean   get_text_bounding_box                      (EvDocumentText     *document_text,
+							      EvPage             *page,
+							      EvRectangle        *bbox);

You could define these before they are used and you don't need to add more prototypes.

@@ +652,3 @@
+			gboolean success;
+			gint current_page;
+			EvPage *page;

Move the declaration block before the if

@@ +655,3 @@
+
+			current_page = ev_document_model_get_page (view->model);
+			page = ev_document_get_page (view->document, current_page);

This returns a new EvPage that you are leaking. You should call g_object_unref() when you no longer need it.

@@ +667,3 @@
+				ratio = ((rect.x2 - rect.x1) / 2 + rect.x1) / width;
+				factor = (value + ratio * page_size) / upper;
+				break;

I don't think this is correct, or do you really want to fallback to SCROLL_TO_CENTER? If that's the case I would add a comment, because it's very confusing.

@@ +3684,3 @@
+		break;
+	default:
+		g_assert_not_reached ();

Are you changing anything here? or only the indentation?

@@ -3760,3 @@
 	     view->sizing_mode == EV_SIZING_AUTOMATIC)) {
 		GtkAllocation allocation;
-

Do not remove this line, please, we use an empty line between the declarations block and the body.

@@ +7765,3 @@
+							 height);
+			view->pending_scroll = SCROLL_TO_TEXT;
+			break;

And here again... do you want to fallback to AUTOMATIC when there's not text?

@@ +7824,3 @@
+							 height);
+			view->pending_scroll = SCROLL_TO_TEXT;
+			break;

Ditto.

@@ +7869,3 @@
 	height -= (border.top + border.bottom + 2 * view->spacing);
 
+

Do not add this extra line here, please.

@@ +7890,3 @@
+							 height);
+			view->pending_scroll = SCROLL_TO_TEXT;
+			break;

Ditto.

@@ +8813,3 @@
+	success = get_text_bounding_box (EV_DOCUMENT_TEXT (view->document),
+					 page,
+					 &rect);

This code is duplicated in ev_view_set_adjustment_values(). Here you are leaking the EvPage as well.

@@ +8830,3 @@
+	gboolean success;
+
+	success = ev_document_text_get_text_layout (document_text,

This is calling the backend directly without taking a lock. Actually, you should use the page cache for this, ev_page_cache_get_text_layout(). I wonder if you could use the text mapping instead and call cairo_region_get_extents to get the bbox.

@@ +8847,3 @@
+	}
+
+	ev_rectangle_free (rect);

This is not correct, rect is an array of rectangles, not a EvRectangle, it should be freed with g_free
Comment 71 Stefan Jeremic 2014-11-01 17:36:33 UTC
Does anybody works on this right now? I would try to solve this problem, it's looks like an interesting thing to do :) I would maybe need couple of pointers, to get around the code if main developers don't mind... I saw bounty on bounty source so for me it is one bonus motive to start working on it, but I don't want to be in position of "stealing" somebody place if he is working on this already
Comment 72 Amine kabab 2014-11-03 11:10:26 UTC
Hi, I was also working on this for the bounty in the summer, I already submitted some patches, but it still not totally completed, I don't have a problem if you completed the work or you restart the work, I m not working on it for the time being due to school and exams, so feel free to work on it :).
Comment 73 Lauri Kasanen 2015-02-11 17:00:00 UTC
Created attachment 296615 [details] [review]
Patch for the "trim margins" feature

Hi all,

Attaching a patch against current git. It works by reading a thumbnail, meaning scanned/image-only PDFs are also handled fine (same way as Okular). Currently only left and right margins are handled.

All tested files work fine, though extreme zoom changes like with the FinalLinkTest.pdf can be disorienting. The crypto pdf linked became quite nice.

Everything's ok in single page modes, in dual mode selecting text does not work currently. Perhaps disabling this for dual mode is the easiest solution, as the scroll_x var cannot be used -> many, many places would need to be modified to handle the offsets in dual mode.
Comment 74 Carlos Garcia Campos 2015-02-15 11:28:16 UTC
Review of attachment 296615 [details] [review]:

Thanks for the patch. I think this patch is mixing two different ways of implementing this, trim margins and zoom to contents. It trims the margins, but implemented as a zoom mode. I think they are different:

 a) Trim margins: this is not a zoom mode, but a feature. Margins are simply not rendered, but the zoom is not affected. It would be a view option like inverted colors, not a zoom mode, since the margins could be trimmed at any zoom level. In this case, to save memory, we should clip when rendering the page instead of when drawing the surface in the screen. The main problem of this approach is that it would break all page size calculations made for continuous mode, jump to links, caret mode, etc. Because we have a surface size that is different to the page size.

 b) Zoom to contents: this is a zoom mode, similar to zoom width, but using the contents bounding box instead of the page size. In this mode, margins aren't trimmed, but the page is scaled to fit the contents and scrolled to the contents bounding box. In this mode, you can scroll to the margin if you want.

I think this patch is somehow mixing both things. I've noticed some unexpected page jumps in continuous mode, and UI freezes with some documents while trying out this patch. I think we should do one or the other, I think I prefer b), but I wouldn't be opposed to a) either.

::: libview/ev-document-model.h
@@ +52,3 @@
 	EV_SIZING_FREE,
+        EV_SIZING_AUTOMATIC,
+        EV_SIZING_TRIM_MARGINS

If we end up implementing this as a zoom mode, I prefer something like EV_SIZING_FIT_CONTENTS, for consistency with the other fit modes.

::: libview/ev-margin-cache.c
@@ +33,3 @@
+	unsigned num_pages;
+
+	g_return_val_if_fail (EV_IS_DOCUMENT (document), NULL);

Use g_assert instead og g_return macros in private API.

@@ +36,3 @@
+
+	num_pages = ev_document_get_n_pages (document);
+	rects = calloc (num_pages, sizeof (GdkRectangle));

use g_new0() instead of calloc

@@ +47,3 @@
+	g_return_val_if_fail (cache != NULL, FALSE);
+
+	return cache[page].width > 1;

This could be used internally by ev_margin_cache_compute_margins() to return early, then the caller doesn't need to call this.

@@ +64,3 @@
+
+static void
+find_bounds (cairo_surface_t * const surface,

const cairo_surface_t *?

@@ +70,3 @@
+	     int * const             maxx,
+	     int * const             miny,
+	     int * const             maxy)

I don't understand these consts for out parameters

@@ +141,3 @@
+	int minx, miny, maxx, maxy;
+	EvRenderContext *rc;
+	cairo_surface_t *surface = NULL;

You don't need to initialize this, since it's unconditionally assigned.

@@ +143,3 @@
+	cairo_surface_t *surface = NULL;
+	EvPage *evpage;
+	const unsigned int FACTOR = 3;

Use a macro for this

@@ +144,3 @@
+	EvPage *evpage;
+	const unsigned int FACTOR = 3;
+	const int border = 18;

Ditto, but in upper case like FACTOR.

@@ +156,3 @@
+	// as the API can only give us the text bbox, which would fail
+	// for image-heavy pages. Synchronous, as we're called when it's
+	// needed.

This not acceptable, for some documents that are bit slower to render, it freezes evince for a while, even if it's rendering just the thumbnails, because if a page is currently rendered, we block here waiting for the doc mutex. This should be done asynchronously to not block the UI thread.

@@ +159,3 @@
+	//
+	// While Poppler may be extended to do this faster in the future,
+	// other backends may still require rendering.

I think it's preferred that backend implement this, and we simply don't support it for backends that don't implement it (or we could try to fall back to this implementation for those backends).

@@ +166,3 @@
+	ev_render_context_set_target_size (rc, width, height);
+	g_object_unref (evpage);
+	surface = ev_document_get_thumbnail_surface (view->document, rc);

I think we should always render at the desired size instead of using ev_document_get_thumbnail_surface(), because some PDF documents have prerendered thumbs that might not match exactly the page (they are very rare, but they exist as it's part of the PDF spec).

::: libview/ev-margin-cache.h
@@ +20,3 @@
+#if !defined (__EV_EVINCE_VIEW_H_INSIDE__) && !defined (EVINCE_COMPILATION)
+#error "Only <evince-view.h> can be included directly."
+#endif

You don't need this, since this is not a public header.

::: libview/ev-view.c
@@ +1172,3 @@
+	    is_dual_page (view, NULL)) {
+		if (!ev_margin_cache_is_page_cached (view->margin_cache, page))
+			ev_margin_cache_compute_margins (view, page);

This should never be called in the UI thread.

@@ +1252,3 @@
+			// Not ideal, but we can't compute all margins
+			if (view->sizing_mode == EV_SIZING_TRIM_MARGINS)
+				max_width = width * 1.05f;

Could you explain this?

@@ +4417,3 @@
+		if (view->sizing_mode == EV_SIZING_TRIM_MARGINS) {
+			if (!ev_margin_cache_is_page_cached (view->margin_cache, i))
+				ev_margin_cache_compute_margins (view, i);

This should never be called in the UI thread.

@@ +7456,3 @@
+
+	if (view->margin_cache) {
+		free (view->margin_cache);

Use g_free(). I would add ev_margin_cache_free() though.

@@ +7964,3 @@
+}
+
+static int get_largest_margin_width_around (EvView       *view,

function name and parameters should be in the next line.

@@ +7976,3 @@
+	for (i = first_visible; i <= last_visible; i++) {
+		if (!ev_margin_cache_is_page_cached (view->margin_cache, i))
+			ev_margin_cache_compute_margins (view, i);

This should never be called in the UI thread, even less inside a loop

@@ +7999,3 @@
+	if (view->sizing_mode == EV_SIZING_TRIM_MARGINS) {
+		doc_width = get_largest_margin_width_around (view, view->current_page);
+	}

Don't use { } for single line ifs

@@ +8052,3 @@
+	if (view->sizing_mode == EV_SIZING_TRIM_MARGINS) {
+		doc_width = get_largest_margin_width_around (view, view->current_page);
+	}

Ditto.
Comment 75 Lauri Kasanen 2015-02-15 21:48:52 UTC
Hi,

Thanks for the review.

> b) Zoom to contents: this is a zoom mode, similar to zoom width, but using the > contents bounding box instead of the page size. In this mode, margins aren't
> trimmed, but the page is scaled to fit the contents and scrolled to the contents 
> bounding box. In this mode, you can scroll to the margin if you want.

This is the target really. But in my view, scrolling to margins is never done, and is prevented so it doesn't even happen accidentally; because when using this mode, I imagine I'm reading, and don't want anything like that to interrupt the flow.

If we were to allow scrolling outside the box, then what would happen in continuous mode when another page becomes visible? I think both ways would confuse users:
a) the existing scroll is kept - now how do I get the ideal zoom/scroll back?
b) the automatic scroll position is applied, keeping both pages fully visible. I explicitly scrolled, why did the program work against me?

The ideal solution to me is to not allow scrolling outside the box, as in the current solution. Allowing it would annoy users either way.

> +	     int * const             maxx,
> +	     int * const             miny,
> +	     int * const             maxy)
>
> I don't understand these consts for out parameters

By telling the compiler the pointers do not change, it can optimize a tiny bit better. Just a matter of "const everywhere when possible". I would also have added restrict, but I wasn't sure if it was ok in this project.

> This not acceptable, for some documents that are bit slower to render, it
> freezes evince for a while, even if it's rendering just the thumbnails, 
> because if a page is currently rendered, we block here waiting for the doc 
> mutex. This should be done asynchronously to not block the UI thread.

Nothing can be rendered before the bounding box is known? Do you mean just the difference between an empty responsive window and an empty nonresponsive window?

If so, should the job system be used, or?

> +			// Not ideal, but we can't compute all margins
> +			if (view->sizing_mode == EV_SIZING_TRIM_MARGINS)
> +				max_width = width * 1.05f;
>
> Could you explain this?

Imagine how long it would take to render all 300 pages of a big document - the user won't wait for minutes. Without that we can't know the maximum content bounding box. 5% is simply a guess that should cover usual documents with only small variance between pages.

But anyway, this part would be moot if it were disabled in dual mode. Do you think it's ok to only support this zoom mode in single-page modes, given the extra complexity in supporting it in dual?
Comment 76 Lauri Kasanen 2015-02-16 09:33:13 UTC
> +find_bounds (cairo_surface_t * const surface,
> 
> const cairo_surface_t *?

This one is not possible, as all the cairo functions there take a non-const surface.
Comment 77 Lauri Kasanen 2015-02-16 12:50:56 UTC
Created attachment 296920 [details] [review]
Patch for the "fit content" feature

Hi,

Here's an updated patch. It incorporates the style/etc fixes requested above, and does the rendering in a background thread using the job system, making for a fluid UI.

The open questions are still unanswered, and dual mode in particular is still not working, pending the question above.
Comment 78 baltazar.bz 2016-04-28 11:29:13 UTC
Guys, what's the status here? I really miss this feature. I use it all the time on my mobile phone PDF reader and it's very convenient. Wish I could use on desktop as well.
Comment 79 Arian@sanusi.de 2016-04-28 11:43:54 UTC
baltazar.bz@gmail.com: Switch to qpdfview. Has trim margins (it's in the settings), tabs. Does not pull in half of kde like okular. Although no column view like some mobile readers.
Comment 80 Lauri Kasanen 2016-04-28 11:46:47 UTC
Nobody reviewed my last patch, and the open questions are still open.

A bit later, I wrote a proof-of-concept PDF reader that rendered everything on startup, to see if that approach would be viable. Indeed it is, at least on multi-core processors, and I released it as FlaxPDF. http://flaxpdf.sf.net

Flax uses all cores and caches everything in memory, and for most documents it's both faster and uses less RAM than Evince, which is rather surprising. Of course it's more limited in features, and supports only PDF, so it's not a full replacement to many Evince users.

Anyway, I'd be willing to develop my patch further if reviewed, but I'm rather busy nowadays, and rebasing to a year newer evince would take time, so I can't tell when I have enough free time.
Comment 81 baltazar.bz 2016-04-28 12:05:29 UTC
Arian@sanusi.de: Wow! Thank you a lot for the tip! qpdfview is great! I'm a little sad as I really liked sexy GNOME 3 interface but I think I'll get by. Thank you again!

Lauri Kasanen: Thanks! I've tried it and it looks promising. It does its job. Is this a valid link to flaxpdf? https://github.com/clbr/flaxpdf
Comment 82 Lauri Kasanen 2016-04-28 13:24:18 UTC
Yeah, I use SF for releases + website and github for development.
Comment 83 GNOME Infrastructure Team 2018-05-22 12:54:30 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/3.