Two-Up View Redux #2660

waddlesplash · 2013-02-02T16:02:20Z

Building on what @Moistly started in #2395 (to fix #1475). Note that the following things might still be an issue:

To-Do (before merging)

timvandermeij · 2013-02-03T21:51:15Z

Fantastic work, thank you! Nice to see that this feature is still being worked on :)

waddlesplash · 2013-02-03T22:09:30Z

You're welcome! Now only if someone would make a decision about the possible problems above...

gigaherz · 2013-02-04T07:32:44Z

/botio-windows preview

pdfjsbot · 2013-02-04T07:32:45Z

From: Bot.io (Windows)

Received

Command cmd_preview from @gigaherz received. Current queue size: 0

Live output at: http://107.22.172.223:8877/34114f1bb3fd149/output.txt

pdfjsbot · 2013-02-04T07:32:58Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/34114f1bb3fd149/output.txt

Total script time: 0.21 mins

Published

Viewer: http://107.22.172.223:8877/34114f1bb3fd149/web/viewer.html
Extension: http://107.22.172.223:8877/34114f1bb3fd149/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.22.172.223:8877/34114f1bb3fd149/extensions/firefox/pdf.js.amo.xpi

gigaherz · 2013-02-04T07:49:29Z

I can't make a decision, but my oppinions/suggestions are as follows:

I'd say that if the number of menu items is growing too large, it should be discussed in a separate issue. Feel free to open an issue about it, I guess -- it doesn't bother me yet. You could see if adding a separator above the Page Display sub-menu looks fine, but adding any other menu stuffs should be done separately since it's not part of this feature.

Menuitems looking like radio-buttons may be a Firefox limitation, since the standard Windows menu system is perfectly able to draw them.

For bookmarks, you may want to add more args to the url, something like #page=7&zoom=auto,0,792&viewmode=2up,cover.

timvandermeij · 2013-02-04T18:45:09Z

The updated viewer looks really nice. I only spot one problem: if I change the view on Windows 7, Firefox 18.0.1, on http://107.22.172.223:8877/34114f1bb3fd149/web/viewer.html, I need to scroll down just a tiny bit to actually see the page. I.e.: if I change the view, I keep seeing the AJAX loader thingy on the first page and it only disappears if I scroll down only a little bit. Strange...

jviereck · 2013-02-05T14:14:09Z

/botio-linux preview

pdfjsbot · 2013-02-05T14:14:11Z

From: Bot.io (Linux)

Received

Command cmd_preview from @jviereck received. Current queue size: 0

Live output at: http://107.21.233.14:8877/bb8aab1c411e694/output.txt

pdfjsbot · 2013-02-05T14:14:25Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/bb8aab1c411e694/output.txt

Total script time: 0.22 mins

Published

Viewer: http://107.21.233.14:8877/bb8aab1c411e694/web/viewer.html
Extension: http://107.21.233.14:8877/bb8aab1c411e694/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/bb8aab1c411e694/extensions/firefox/pdf.js.amo.xpi

jviereck · 2013-02-05T14:19:55Z

Personally, I would like to have a toolbar button to switch between the different view modes. But that can be done as a followup.

Some notes:

I don't like the shadow between the two pages. Can we avoid it?
Does this PR store in the settings, that the page was viewed in 2up mode and restores the mode the next time the same PDF is viewed (also store there if the first page was used as cover page or not)?

Otherwise from playing with the preview, looks good :)

Snuffleupagus · 2013-02-06T22:26:58Z

@waddlesplash This is a great effort so far, but I have made a couple of observations:

If you activate two-up and then set zoom to page-width, each row will contain three (!) pages. Sometimes this doesn't always show right away, but if you toggle between automatic zoom and page-width a few times, it will show up.
In the preview, the full screen mode is completely broken if two-up is active.
When two-up is active and the zoom is set to 'page-fit', the vertical alignment of the pages becomes wrong. In this configuration, the uppermost pixels of the next pages become visible in the bottom of the screen. Note that I can only reproduce this if "Show Cover Page in Two Page View" is toggled on.
In two-up view the margin to the left of the pages is always bigger than the right one, i.e. the pages gets placed of-centre horizontally in the viewerContainer.
Edit: From looking at the HTML/CSS with the inspector in Firebug, it's clear that the margins are set totally wrong. Perhaps it would be good to instead define a special CSS class that gets invoked when switching to two-up. (This might also fix full screen mode.)
There seems to be some issue regarding the rendering of pages when toggling "Show Cover Page in Two Page View". Until you actually scroll, or change page, the pages won't render.
The above also sometimes happen when switching to/from two-up, but less frequently.
Edit2: It seems that the right hand page in two-up mode doesn't render as it should when scrolling. Observing how PDFView.getVisiblePages().views changes while scrolling down in a document probably explains a lot. Currently PDFView.getVisiblePages().views doesn't always correspond to what you actually see on the screen.
Edit: When zooming in, the horizontal scrollbar don't get big enough, so some of the pages gets clipped of particular on the left side. This might be related to the margins, see above.

I really hope that the above doesn't sound negative. I'm excited about a working two-up mode in pdf.js and wanted to help out by adding my thoughts.

waddlesplash · 2013-02-07T02:06:02Z

Wow, I got a lot more attention on this than any of my other PRs! O_o Sadly, I am busy and won't get back to this until the end of next week at the earliest. I'll address all of your comments then.

gigaherz · 2013-02-07T07:36:17Z

It's all right. No one had worked on this for months, people can wait 2 weeks more ;P

timvandermeij · 2013-02-07T16:00:50Z

@waddlesplash: the issue that I reported earlier is also mentioned by @Snuffleupagus, so you can ignore my comment from earlier. Please, do take your time. No need to hurry, I'm just glad that you are willing to work on this nice feature! Thank you in advance for your work.

Snuffleupagus · 2013-02-18T15:12:57Z

@waddlesplash the updated to-do list looks good!

gigaherz · 2013-02-18T16:24:10Z

Woah, it's actually clickable and functional?

waddlesplash · 2013-02-18T16:29:30Z

You mean the TODO list? That's a new GitHub feature. You just add [ ]s for checks, and [x] for filled checks. And if you go to a place this issue is referenced (when you #-refence an issue) it'll have a number of tasks, finished, and unfinished. Read the GitHub blog from about 3months ago. https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments

timvandermeij · 2013-02-20T19:03:42Z

Wouldn't #2747 also fix point 3 on the to-do list?

waddlesplash · 2013-02-20T19:45:43Z

It would -- if it gets merged.

waddlesplash · 2013-02-21T00:49:47Z

OK, I have a prototype of this that fixes a lot of the problems. However, it relies on having the browser NOT word-wrap the two divs when they don't fit side-by-side, instead adding a scrollbar. Adding overflow-x: scroll; does not help. Anyone?

waddlesplash · 2013-02-21T00:57:37Z

Whoops! nevermind, figured it out. I needed to use white-space: nowrap; instead.

waddlesplash · 2013-02-21T01:14:43Z

OK, just pushed a MAJOR change. This should fix three-up, margins, and fullscreen mode (at least). It creates <br>s inbetween every page always (display: inline-block; is always enabled), and then assigns class first (for first page) two (for even pages) one (for odd pages). So now when you click on "two up", all it does is add twoUp to #viewer (and .noCover if you have cover page disabled).

Snuffleupagus · 2013-02-21T01:20:13Z

@waddlesplash Just looking at the code, this seems to be a much simpler solution. Nice work so far!

gigaherz · 2013-02-21T08:34:17Z

@waddlesplash CSS3 has :first-child, :nth-child(odd), :nth-child(even), would those work instead of assigning the classes manually?

jviereck · 2013-03-06T10:42:56Z

web/viewer.js

+    }
+  },
+
+  nextPage: function pdfViewNextPage() {


I know what you mean by nextPage, but in twoUp mode you don't always go to the next page but instead to the next container that might hold a single or multiple pages. Same goes for prevPage.

So what should I call it? advancePage? nextPageContainer?

@waddlesplash Your suggestion nextPageContainer sound good to me.

jviereck · 2013-03-06T10:43:41Z

Overall, this patch looks good so far, but I'd like to do another round of review once the comments above are addressed.

jviereck · 2013-03-07T16:55:09Z

What are the opinions about: https://github.com/mozilla/pdf.js/pull/2660/files#r3259033 ?

brendandahl · 2013-03-07T17:30:58Z

web/viewer.js

@@ -1506,6 +1659,15 @@ var PDFView = {

    if (hash.indexOf('=') >= 0) {
      var params = PDFView.parseQueryString(hash);
+      if ('twoup' in params) {


Has anyone looked into the spec to see if there are standard parameters to setup this view mode?

Where is that in the PDF reference? I can't find it.

Not in http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/pdf_open_parameters_v9.pdf...

waddlesplash · 2013-03-07T17:35:14Z

What are the opinions about:
https://github.com/mozilla/pdf.js/pull/2660/files#r3259033 ?

@Snuffleupagus?

brendandahl · 2013-03-07T17:36:38Z

web/viewer.js

@@ -3061,6 +3261,26 @@ document.addEventListener('DOMContentLoaded', function webViewerLoad(evt) {
      PDFView.rotatePages(90);
    });

+  document.getElementById('one_page_view').addEventListener('click',


Please use camelCase id's since all id's in the viewer are camel case.

But the previous event listener (document.getElementById('page_rotate_cw').addEventListener('click',) is like that...

Snuffleupagus · 2013-03-07T17:39:38Z

What are the opinions about: https://github.com/mozilla/pdf.js/pull/2660/files#r3259033 ?

I'm fine with it, just wanted to make sure that I understood the reasoning.

waddlesplash · 2013-03-07T17:40:11Z

OK well, once I fix @brendandahl's comments you can fix that.

Snuffleupagus · 2013-03-07T17:47:53Z

@waddlesplash I'm currently fixing the review comments for the code I wrote.
I also found another issue with getVisiblePages(), but I think I solved it. I'll wait for your changes before uploading my fixes.

waddlesplash · 2013-03-07T18:22:44Z

@Snuffleupagus done. Do your stuff and tell me when you're done :)

timvandermeij · 2013-03-07T18:46:01Z

Some great work seems to be going on here. Can't wait to test a new preview :)

Snuffleupagus · 2013-03-07T19:34:00Z

web/viewer.js

+  } else {
+    PDFView.showCover(PDFView.showCoverPage);
+  }
+


Is this code block really necessary here? Isn't it enough to just have this logic in the setHash() method.

OK, I'll try without it

jviereck · 2013-03-07T20:44:02Z

I have to concentrate on my studies and fight a pretty long TODO queue. Therefore I won't make it to spend time on reviewing this patch in the near future. Can someone else please make sure this gets reviewed and lands?

Snuffleupagus · 2013-03-08T13:06:54Z

@brendandahl I've now got a working prototype for a more generic getVisibleElements(), but there is some clean-up necessary in the code. I intend to get this done during the weekend.
I've got one question though: is it really a good idea to make the changes in this PR, or if it should be done in it's own PR to make sure that it gets properly reviewed?

brendandahl · 2013-03-08T17:45:32Z

Splitting that out into another PR would be good. We could land that first then bring those changes into this PR.

waddlesplash · 2013-03-08T17:55:18Z

OK, so what's the course of action for this PR?
And FYI I will be out the rest of today and the whole weekend.

Snuffleupagus · 2013-03-08T21:05:44Z

@waddlesplash Unless something unforeseen happens during the weekend, I'll submit a PR with a reworked getVisibleElements(). Once that is merged, we should find someone willing to review this PR.
I'm looking forward to getting done with this PR, so that I get to finish my page wise scrolling PR (#2709).

timvandermeij · 2013-03-19T19:05:19Z

getVisibleElements() just got merged. Now let's hope we can find someone to review this... Would be really cool to be able to use this!

Snuffleupagus · 2013-03-19T19:13:13Z

@timvandermeij This PR needs to be rebased, since it currently isn't in merge-able condition. Also there is some clean-up needed in the code, because of the rewrite of getVisibleElements.

timvandermeij · 2013-03-19T19:14:55Z

Oh, indeed. I also see some comments on the code above, so these need to be addressed too.

Snuffleupagus · 2013-09-08T09:54:49Z

@waddlesplash Are you still working on this? If not, I would be willing to take it over and finish the PR!

Snuffleupagus · 2013-09-23T11:54:53Z

Superseded by PR #3723.

waddlesplash mentioned this pull request Feb 2, 2013

Add 'Two Up View' #2395

Closed

Moistly and others added 2 commits February 4, 2013 09:50

Moistly's two-up work.

d73c467

Merge branch 'master' of https://github.com/mozilla/pdf.js

b28092d

jviereck mentioned this pull request Feb 7, 2013

Implements whole page scrolling when zoom is set to 'page-fit' #2682

Closed

Snuffleupagus mentioned this pull request Feb 20, 2013

PDF Open Parameters should be implemented #2757

Open

jviereck reviewed Mar 6, 2013
View reviewed changes

brendandahl reviewed Mar 7, 2013
View reviewed changes

Address @Snuffleupagus, @mduan, @jviereck, @brendandahl comments.

aa77501

Snuffleupagus reviewed Mar 7, 2013
View reviewed changes

Snuffleupagus mentioned this pull request Mar 9, 2013

Rewrite and refactor getVisibleElements() to make it more generic #2916

Merged

Snuffleupagus mentioned this pull request Apr 7, 2013

PDF browsing history - using window.history #2921

Merged

Snuffleupagus mentioned this pull request May 11, 2013

Secondary toolbar #2792

Merged

Snuffleupagus mentioned this pull request Sep 23, 2013

Implement a Two Page View Mode #3723

Closed

3 tasks

Snuffleupagus closed this Sep 23, 2013

prohtex mentioned this pull request Mar 6, 2018

Add scrolling and spread modes to web viewer #9208

Merged

Two-Up View Redux #2660

Two-Up View Redux #2660

Conversation

waddlesplash commented Feb 2, 2013

To-Do (before merging)

timvandermeij commented Feb 3, 2013

waddlesplash commented Feb 3, 2013

gigaherz commented Feb 4, 2013

pdfjsbot commented Feb 4, 2013

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 4, 2013

From: Bot.io (Windows)

Success

Published

gigaherz commented Feb 4, 2013

timvandermeij commented Feb 4, 2013

jviereck commented Feb 5, 2013

pdfjsbot commented Feb 5, 2013

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 5, 2013

From: Bot.io (Linux)

Success

Published

jviereck commented Feb 5, 2013

Snuffleupagus commented Feb 6, 2013

waddlesplash commented Feb 7, 2013

gigaherz commented Feb 7, 2013

timvandermeij commented Feb 7, 2013

Snuffleupagus commented Feb 18, 2013

gigaherz commented Feb 18, 2013

waddlesplash commented Feb 18, 2013

timvandermeij commented Feb 20, 2013

waddlesplash commented Feb 20, 2013

waddlesplash commented Feb 21, 2013

waddlesplash commented Feb 21, 2013

waddlesplash commented Feb 21, 2013

Snuffleupagus commented Feb 21, 2013

gigaherz commented Feb 21, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviereck commented Mar 6, 2013

jviereck commented Mar 7, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waddlesplash commented Mar 7, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Mar 7, 2013

waddlesplash commented Mar 7, 2013

Snuffleupagus commented Mar 7, 2013

waddlesplash commented Mar 7, 2013

timvandermeij commented Mar 7, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviereck commented Mar 7, 2013

Snuffleupagus commented Mar 8, 2013

brendandahl commented Mar 8, 2013

waddlesplash commented Mar 8, 2013

Snuffleupagus commented Mar 8, 2013

timvandermeij commented Mar 19, 2013

Snuffleupagus commented Mar 19, 2013

timvandermeij commented Mar 19, 2013

Snuffleupagus commented Sep 8, 2013

Snuffleupagus commented Sep 23, 2013