Drag'N'Drop #9772

lalagias · 2018-06-02T20:37:29Z

Drag'n'Drop functionality. Drag'n'Drop a PDF file in the pdf viewer to render it.

Snuffleupagus · 2018-06-03T09:36:56Z

A quick drive-by comment, since I'm not an official reviewer.

I don't understand why the proposed solution is so complicated, since for one it shouldn't be necessary to edit web/viewer.html (adding inline JavaScript code in the HTML isn't really an acceptable pattern anyway)!?
Just using the viewerContainer as a drop target ought to be sufficient, and I don't understand why the code needs to live in a separate file since it could otherwise easily piggy-back on the existing fileInput code-path (the drag-drop functionality should probably only be enabled in GENERIC builds anyway).

This is untested, but why can't the entirety of the patch be limited to only e.g. the code below, placed just after this line?


// Enable draging-and-dropping a new PDF file onto the viewerContainer.
appConfig.mainContainer.addEventListener('dragover', function(evt) {
  evt.preventDefault();

  evt.dataTransfer.dropEffect = 'move';
});
appConfig.mainContainer.addEventListener('drop', function(evt) {
  evt.preventDefault();

  const files = evt.dataTransfer.files;
  if (!files || files.length === 0) {
    return;
  }
  PDFViewerApplication.eventBus.dispatch('fileinputchange', {
    fileInput: evt.dataTransfer,
  });
});

lalagias · 2018-06-03T12:54:03Z

@Snuffleupagus I like your approach and I think is it a more viable option.

I thought about the inline JavaScript Pattern that it might don't be acceptable, although it was the only solution I could think of at that time.

In communication with a maintainer, I asked to use the open() function and he pointed me to app.js, also I mentioned that I have created a new js file and I didn't get a comment on that.

Your solution and the thought process of yours are much more solid.

Thank you for your feedback, I should try your solution and make a new PR!

Snuffleupagus · 2018-06-03T14:13:35Z

In communication with a maintainer, I asked to use the open() function and he pointed me to app.js,

Well, the suggestion in #9772 (comment) does indirectly rely on PDFViewerApplication.open too. Please see the webViewerFileInputChange function, which also feature tests e.g. URL.createObjectURL properly:

pdf.js/web/app.js

Lines 1926 to 1953 in 2921cc0

    
           let webViewerFileInputChange; 
        
           if (typeof PDFJSDev === 'undefined' || PDFJSDev.test('GENERIC')) { 
        
             webViewerFileInputChange = function webViewerFileInputChange(evt) { 
        
               let file = evt.fileInput.files[0]; 
        
               if (URL.createObjectURL && !AppOptions.get('disableCreateObjectURL')) { 
        
                 PDFViewerApplication.open(URL.createObjectURL(file)); 
        
               } else { 
        
                 // Read the local file into a Uint8Array. 
        
                 let fileReader = new FileReader(); 
        
                 fileReader.onload = function webViewerChangeFileReaderOnload(evt) { 
        
                   let buffer = evt.target.result; 
        
                   PDFViewerApplication.open(new Uint8Array(buffer)); 
        
                 }; 
        
                 fileReader.readAsArrayBuffer(file); 
        
               } 
        
               PDFViewerApplication.setTitleUsingUrl(file.name); 
        
               // URL does not reflect proper document location - hiding some icons. 
        
               let appConfig = PDFViewerApplication.appConfig; 
        
               appConfig.toolbar.viewBookmark.setAttribute('hidden', 'true'); 
        
               appConfig.secondaryToolbar.viewBookmarkButton.setAttribute('hidden', 
        
                                                                          'true'); 
        
               appConfig.toolbar.download.setAttribute('hidden', 'true'); 
        
               appConfig.secondaryToolbar.downloadButton.setAttribute('hidden', 'true'); 
        
             }; 
        
           }

also I mentioned that I have created a new js file and I didn't get a comment on that.

Without having seen the actual solution that someone proposes, please keep in mind that it's not always easy (or meaningful) to try and comment on every little detail beforehand :-)

[...] I should try your solution and make a new PR!

You should be able to just update the current one, by removing the existing commits (e.g. with git rebase -i) and then force push the changes.

timvandermeij

In general this looks good (and much simpler this way). To make the review process easy, we usually ask contributors to not introduce changes that are unrelated to the patch itself, such as whitespace changes. I have indicated here which ones can be reverted. The goal is to make the diff as small as possible so it's easier to understand and to review.

When you made these changes, you can squash all commits into one commit, which is better because it keeps the commit history clean. Here we have written how to do that easily: https://github.com/mozilla/pdf.js/wiki/Squashing-Commits

Thank you!

timvandermeij · 2018-06-03T14:30:26Z

web/app.js

@@ -698,6 +698,7 @@ let PDFViewerApplication = {
   * @returns {Promise} - Returns the promise, which is resolved when document
   *                      is opened.
   */
+


Undo this change.

timvandermeij · 2018-06-03T14:30:31Z

web/app.js

@@ -790,7 +791,7 @@ let PDFViewerApplication = {
        throw new Error(msg);
      });
    });
-  },
+  }, // open closes


Undo this change.

timvandermeij · 2018-06-03T14:30:36Z

web/viewer.html

@@ -60,10 +60,8 @@
 <!--#endif-->

  </head>
-


Undo this change.

timvandermeij · 2018-06-03T14:30:40Z

web/viewer.html

  <body tabindex="1" class="loadingInProgress">
    <div id="outerContainer">
-


Undo this change.

timvandermeij · 2018-06-03T14:31:49Z

web/viewer.html

-            </div>
-            <div class="buttonRow">
-              <button id="printCancel" class="overlayButton"><span data-l10n-id="print_progress_close">Cancel</span></button>
+  <!--#if !(FIREFOX || MOZCENTRAL)-->


I don't think we need to change anything in this file, correct? The diff is hard to read, but by appending ?w=1 to the URL I can see that only some div element moved. Why is that required?

That's correct, this file shouldn't appear anywhere in the diff.

Snuffleupagus · 2018-06-03T14:42:20Z

web/app.js

@@ -1641,6 +1642,23 @@ function webViewerInitialized() {
        fileInput: evt.target,
      });
    });


Nit: Let's add a new line here, to improve readability, since the code above has nothing to do with drag and drop.

Edit: Also please make sure that the commit message makes sense, since the current one doesn't really make sense for the final version of the patch.

lalagias · 2018-06-03T15:22:04Z

First of all, both of you were a huge help and made this contribution seems really easy!

So, as I was trying to make the changes you asked, I messed up something on Git.

The -git status showed me this:
On branch dragNdrop
Your branch and 'upstream/master' have diverged,
and have 5 and 7 different commits each, respectively.
(use "git pull" to merge the remote branch into yours)
All conflicts fixed but you are still merging.
(use "git commit" to conclude merge)

Changes to be committed:

modified:   gulpfile.js
modified:   src/core/function.js
modified:   src/core/jpg.js
new file:   test/pdfs/issue9679.pdf.link
modified:   test/test_manifest.json
modified:   web/app.js
modified:   web/viewer.html

What should I do? To make the commit with the changes you asked for!

Edit: I managed to work around it, I will squash my commits and push it.

timvandermeij · 2018-06-03T17:03:55Z

Really good. Don't worry too much about messing up with Git because there are always ways to restore your work. First of all, it's already pushed to GitHub, so if something breaks locally you can always fetch the code from GitHub again. On your local system you can use git reflog to, for example, undo merges/squashes that went wrong. git reflog is basically the undo button for any action in Git.

Commands like git rebase -i and git reflog do take a bit of time to master, but are very useful once you get to know them well.

lalagias · 2018-06-03T17:19:04Z

Guys you are really helpful and your guidance was essential.

I think I have squashed the commits. (not sure though)

Mention me for any other mistakes I have done, or any other changes I need to make!

timvandermeij · 2018-06-03T17:28:24Z

It looks like it did not complete successfully. You can tell by the fact that this pull request contains 12 commits instead of one (you can see this at the top of the page or on the commit overview page at https://github.com/mozilla/pdf.js/pull/9772/commits). I cloned your branch and tried it out to make sure that the following suggestions work:

Create a new commit, on top of of the already existing ones, that contains the changes from the review comments.
After that is committed, you will have 13 commits. Then run git rebase -i HEAD~13 (since you're fixing up 13 commits) and change all the pick values to fixup except for the first one, so you'll end up with:

1 pick aa37d094 Set Drag'n'Drop zone,dragNdrop.js file to handle the drag func    tions
2 fixup b59f6fe8 Added functionality to handle dragged'n'dropped pdf files
3 fixup e9c04cf8 Removed some <p> elements and comments from viewer.html, and f    ixed some linting errors
4 fixup aaa72175 Fixed the script tag to importhe dragNdrop.js
5 fixup f278186d Inline Javascript Pattern and dragNdrop.js file are ditched, a    dded the same functionality with some minor changes to app.js
6 fixup f7f955f7 Fixed some whitespace changes, added drag-n-drop functionality     inside app.js
7 fixup 69b05158 Removed some trailing spaces
8 fixup e414bb56 Set Drag'n'Drop zone,dragNdrop.js file to handle the drag func    tions
9 fixup 83135c67 Added functionality to handle dragged'n'dropped pdf files
10 fixup f9f0c0f4 Removed some <p> elements and comments from viewer.html, and f    ixed some linting errors
11 fixup 1fe41945 Fixed the script tag to importhe dragNdrop.js
12 fixup 61a261d0 Fixed some commits issues
13 fixup [hash of new commit] [message of new commit]

This means that all commits will be squashed into the first one, so you'll end up with one commit.

Finally, give the one commit you have left a good commit message. Run git commit --amend and change the commit message to e.g., Implement drag-and-drop support in the viewer for local files.
Use git push -f origin dragNdrop to push the new commit (assuming origin is the remote of your fork).

Feel free to comment if something is unclear.

Implement drag-and-drop support in the viewer for local files

lalagias · 2018-06-05T16:49:41Z

Finally, I made it to 1 commit, I hope that I didn't mess anything up with the force push!

Fellows, once more you are incredibly helpful and thank you a lot for this collaboration!

I learned a lot of new things.

timvandermeij · 2018-06-10T14:07:10Z

It looks like there were still some problems during the rebase. It's indeed one commit, but there are some unrelated changes in it.

Since the solution is good now and works properly, I merged the patch for you in the pull request above (you're still the author of the commit). I'm really happy that this functionality is in place now as it's really useful, so thank you for working on this!

timvandermeij added viewer ux labels Jun 3, 2018

timvandermeij reviewed Jun 3, 2018

View reviewed changes

Snuffleupagus reviewed Jun 3, 2018

View reviewed changes

lalagias force-pushed the dragNdrop branch from a803489 to 61a261d Compare June 3, 2018 17:08

Remove the gulp extension build target

99e36a1

Implement drag-and-drop support in the viewer for local files

lalagias force-pushed the dragNdrop branch from 61a261d to 99e36a1 Compare June 5, 2018 16:45

timvandermeij mentioned this pull request Jun 10, 2018

Implement drag-and-drop support in the viewer for local files #9794

Merged

timvandermeij closed this Jun 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drag'N'Drop #9772

Drag'N'Drop #9772

lalagias commented Jun 2, 2018 •

edited by timvandermeij

Loading

Snuffleupagus commented Jun 3, 2018

lalagias commented Jun 3, 2018

Snuffleupagus commented Jun 3, 2018 •

edited

Loading

timvandermeij left a comment •

edited

Loading

timvandermeij Jun 3, 2018

timvandermeij Jun 3, 2018

timvandermeij Jun 3, 2018

timvandermeij Jun 3, 2018

timvandermeij Jun 3, 2018

Snuffleupagus Jun 4, 2018

Snuffleupagus Jun 3, 2018 •

edited

Loading

lalagias commented Jun 3, 2018 •

edited

Loading

timvandermeij commented Jun 3, 2018 •

edited

Loading

lalagias commented Jun 3, 2018

timvandermeij commented Jun 3, 2018 •

edited

Loading

lalagias commented Jun 5, 2018

timvandermeij commented Jun 10, 2018

		<body tabindex="1" class="loadingInProgress">
		<div id="outerContainer">

Drag'N'Drop #9772

Drag'N'Drop #9772

Conversation

lalagias commented Jun 2, 2018 • edited by timvandermeij Loading

Snuffleupagus commented Jun 3, 2018

lalagias commented Jun 3, 2018

Snuffleupagus commented Jun 3, 2018 • edited Loading

timvandermeij left a comment • edited Loading

Choose a reason for hiding this comment

timvandermeij Jun 3, 2018

Choose a reason for hiding this comment

timvandermeij Jun 3, 2018

Choose a reason for hiding this comment

timvandermeij Jun 3, 2018

Choose a reason for hiding this comment

timvandermeij Jun 3, 2018

Choose a reason for hiding this comment

timvandermeij Jun 3, 2018

Choose a reason for hiding this comment

Snuffleupagus Jun 4, 2018

Choose a reason for hiding this comment

Snuffleupagus Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

lalagias commented Jun 3, 2018 • edited Loading

timvandermeij commented Jun 3, 2018 • edited Loading

lalagias commented Jun 3, 2018

timvandermeij commented Jun 3, 2018 • edited Loading

lalagias commented Jun 5, 2018

timvandermeij commented Jun 10, 2018

lalagias commented Jun 2, 2018 •

edited by timvandermeij

Loading

Snuffleupagus commented Jun 3, 2018 •

edited

Loading

timvandermeij left a comment •

edited

Loading

Snuffleupagus Jun 3, 2018 •

edited

Loading

lalagias commented Jun 3, 2018 •

edited

Loading

timvandermeij commented Jun 3, 2018 •

edited

Loading

timvandermeij commented Jun 3, 2018 •

edited

Loading