search

allefeld / atom-pdfjs-viewer

Themed, fully featured PDF viewer for the Atom editor
MIT License
12 stars 3 forks source link

pdfjs does not copy text (key binding resolver reports nothing) #9

Closed zardini123 closed 2 years ago

zardini123 commented 3 years ago

Hi! This pdfjs viewer is certainly a fantastic package! Though I am having issues.

According to the readme, there is copy text support. On my macOS Mojave Atom, highlighting text and copying it with command-c does not do anything. None of my text that I highlighted is transferred to the clipboard. Opening "Key Binding Resolver" shows that when in pdf viewer, almost every command-related key bind is "eaten." Key binding resolver repots nothing when I do command-c, or command-v, or any of the undo's. Of course there is no reason for undos (unless there is annotation support).

I'd attempt to copy via the context menu, but right-click is overridden by this SyncTex feature.

Key binding resolver does notice command-c, command-v keybindings outside of pdfjs viewer, so its not a direct Atom issue. There are no errors reported through the developer console.

I'm interested to hear your thoughts on this. Thank you!

allefeld commented 3 years ago

Hi, great that you like the package!

My Atom package is just a wrapper for the PDF.js javascript PDF viewer application, which runs in an iframe within the viewer panel. It is therefore outside of the standard Atom logic, and Atom keybindings do not apply. The viewer itself has a number of hard-coded keyboard shortcuts listed here. It does not include "Copy" though.

If the viewer simply runs in a browser, the context menu contains a "Copy" entry, but my impression is that that is not provided by the viewer application, but by the browser itself. I don't remember whether there was a functional context menu before I implemented the SyncTex feature.

I agree that being able to copy text is an important feature. I'm guessing so far I didn't notice it might be missing because I'm on Linux, where simply selecting text makes it accessible in the clipboard.

I'll see whether I can make this work – might be a bit though, because right now I'm very busy.

zardini123 commented 3 years ago

I don't remember whether there was a functional context menu before I implemented the SyncTex feature.

Is there a quick and dirty way I can disable the SyncTex feature? I'm curious to see if there was a context menu or not. I'd imagine there is.

It is therefore outside of the standard Atom logic, and Atom keybindings do not apply

Would there somehow be a way to forward and receive the key-related events of PDF.js into the Keybinder? I'd imagine doing so could help with forward compatibility of this package and all Atom-supported operating systems.

zardini123 commented 3 years ago

Is there a quick and dirty way I can disable the SyncTex feature? I'm curious to see if there was a context menu or not. I'd imagine there is.

I was pretty curious so I commented out this line that attaches the context menu listener in the viewer code. Reloading the window to reload the package then trying to load the pdf resulted in issue https://github.com/allefeld/atom-pdfjs-viewer/issues/7 appearing. The only way I could get around it was by restarting Atom. After that, loading a pdf and right clicking selected text for a context menu results in nothing. So that solves that curiosity.

Though, I do appreciate the disabled SyncTex simply as I don't use it. Possibly a future feature addition (config addition)?

allefeld commented 3 years ago

My preferred solution would be to have a proper context menu, with both "Copy" and "SyncTeX" as entries. Apart from the right-mouse-click binding, SyncTeX doesn't really get in the way if it isn't used.

Btw., since you're already poking at the code, in case you feel so inclined: PR welcome!

Would there somehow be a way to forward and receive the key-related events of PDF.js into the Keybinder?

I think that would be hard to achieve and quite error-prone, because both Atom and PDF.js are big JavaScript applications with their own event loops. What might be possible is to have Atom keybindings for the package, detect when they are changed, and transfer the new bindings into the PDF.js viewer.

zardini123 commented 3 years ago

Btw., since you're already poking at the code, in case you feel so inclined: PR welcome!

Awesome, I'd love to if I get the time! I am also rather busy currently.

What might be possible is to have Atom keybindings for the package, detect when they are changed, and transfer the new bindings into the PDF.js viewer.

Would you happen to know where in your codebase does PDF.js handle its keyboard input? It'd be nice to look around that area and see what kind of ideas I can gather from it.

allefeld commented 3 years ago

At least some of it seems to be in this function

https://github.com/allefeld/atom-pdfjs-viewer/blob/ba4ce37b1d39a95ebf7f452365708e5674399a18/pdfjs/web/viewer.js#L2384

allefeld commented 2 years ago

This seems to have been easier than I thought: I blocked Ctrl-C from being sent to the viewer and let Atom handle it instead, and this seems to enable copying from a PDF. I couldn't completely test it though because as mentioned in Linux simply selecting text copies it to the clipboard.

allefeld commented 2 years ago

published with v1.3.0