vedang / pdf-tools

Emacs support library for PDF files.
https://pdftools.wiki
GNU General Public License v3.0
625 stars 89 forks source link

Evil Mode: Can't select the text on a PDF sometimes (fixed in Spacemacs, waiting for upstream merge) #137

Open sasinhe opened 2 years ago

sasinhe commented 2 years ago

Sometimes when I try to select some text on the PDF it works fine (with some delay, but no problem). But right after that when I try again no text is selected, this happens randomly and on the same file and text section. I've already removed my config file to see if something was conflicting, but with no success. I'm using doom Emacs standard config.

orgtre commented 2 years ago

I cannot reproduce this.

Can you maybe try to find out more about when this occurs? Maybe it's an issue for your specific OS, Emacs, or pdf-tools version, or their combination? Or is it just for some pdf files? A reproducible example would help us to address this.

In the cases where no text is selected, if you then add a highlight (pdf-annot-add-highlight-markup-annotation), does it still highlight the section of text you think should have been selected?

workcomplete commented 2 years ago

I also experience this issue on Doom emacs. I believe the issue is related to evil, or at least I am able to resolve this by disabling evil mode in the PDF buffer.

sasinhe commented 2 years ago

In the cases where no text is selected, if you then add a highlight (pdf-annot-add-highlight-markup-annotation), does it still highlight the section of text you think should have been selected?

In this case, It also doesn't highlight (gives the "region is not active" error).

Can you maybe try to find out more about when this occurs? Maybe it's an issue for your specific OS, Emacs, or pdf-tools version, or their combination? Or is it just for some pdf files? A reproducible example would help us to address this.

It happens on every PDF that I try, I also think it's related to evil (or evil settings on DOOM Emacs), because after deactivating the module the highlights works again.

sasinhe commented 2 years ago

I just discovered something when debugging which functions are being called when selecting the text. As it seems, when the text is not selected, the function call show evil-mouse-drag-region (but not when selecting correctly the text). So maybe it's a conflict with this function?

sasinhe commented 2 years ago

Ok, I just solved it!!! I just needed to set evil-mouse-drag-region to nil inside the pdf-view-mode. Basically, I just added this into my config.

(add-hook 'pdf-view-mode-hook (define-key evil-motion-state-map [down-mouse-1] nil))

How can we insert this into the source code?

orgtre commented 2 years ago

Since Doom Emacs is used a lot and users are likely to be annoyed by this but unlikely to investigate it, I think it makes sense to add an automatic fix in the pdf-tools source code (most likely under (define-derived-mode pdf-view-mode...).

I'm not sure how to properly fix this though: To apply the fix only for Evil users (if (boundp 'evil-motion-state-map)... works, but using define-key like above also maps down-mouse-1 to nil in all other buffers where evil-motion-state-map is active. This is a common problem and a solution is suggested on stackoverflow here, but I'm unable to get it to work.

sasinhe commented 2 years ago

Ok, so I tried to adapt the stack overflow suggestion in our case, and it looks like it's working.

(add-hook 'pdf-view-mode
  (lambda ()
    (let ((oldmap (cdr (assoc 'evil-mode minor-mode-map-alist)))
          (newmap (make-sparse-keymap)))
      (set-keymap-parent newmap oldmap)
      (define-key newmap [down-mouse-1] nil)
      (make-local-variable 'minor-mode-overriding-map-alist)
      (push `(evil-mode . ,newmap) minor-mode-overriding-map-alist))))

Can I create a pull request? (yay! My first one!)

orgtre commented 2 years ago

Hm, are you sure the above works? When I enable evil-mode (without Doom) I can reproduce the issue, but by default (describe-key [down-mouse-1]) isn't bound to anything evil-related in pdf-view-mode buffers, while this is the case in other buffers. Also, in both pdf-view-mode and other buffers, minor-mode-map-alist contains no mention of evil, so the above code shouldn't override anything. For me this issue happens too infrequently to tell for sure with the limited testing I've done.

Furthermore, the remapping should be done only for evil users and (I think) preferably in the pdf-view-mode definition (see above and here) instead of a the mode hook.

sasinhe commented 2 years ago

I don't know if it's a doom only issue, but when remapping through the code above, it seems to work. In my case, sometimes the [down-mouse-1] gave rise to the correct function to select text, and sometimes (normally in the second attempt after opening the PDF) it called the evil-mouse-drag-region. About the describe key not showing anything evil-related, I think that's related to the random nature of the issue. Maybe describe-key shows the relevant keybinding of the major-mode first, but because evil-mode has a special way of overriding keybindings, it doesn't show on describe-key. Can you check if the code works on your setup?

sasinhe commented 2 years ago

If it works, I think I can modify it in order to correct it to obey the necessities you told.

tcluri commented 2 years ago

Have the same issue as your first comment, double clicking on the page solves this for me and selection works again. Ran the snippet posted above but it broke things, so had to doom doctor it

sasinhe commented 2 years ago

Ok, I really don't know what happened. But it stopped raising this error for a while (but now it's back). I just discovered (looking at this https://github.com/emacs-evil/evil/issues/1671) that when selecting text, evil mode enters visual mode. So by preventing this (maybe through disabling this feature in pdf-mode) we solve this error (probably).

vedang commented 2 years ago

Linking this to the issue already filed in evil-mode: emacs-evil/evil#1671

It looks like the workaround merged into Spacemacs by @dalanicolai works -- or at least significantly improves the user experience. Linking it here as a workaround for impacted users: https://github.com/syl20bnr/spacemacs/blob/9cdf97f87e89b3f5045da84e181057e82bfc13aa/layers/%2Breaders/pdf/packages.el#L35-L39

dalanicolai commented 2 years ago

To prevent confusion, the fix only starts to work after the first time entering visual state (it does not yet work for the first time making a selection). Therefore, I would recommend using the evil-visual-state-exit-hook (instead of the entry hook), see https://github.com/syl20bnr/spacemacs/pull/15736#issuecomment-1249224117.

dalanicolai commented 2 years ago

A straightforward fix has been implemented in https://github.com/syl20bnr/spacemacs/pull/15762 :tada:

vedang commented 2 years ago

@dalanicolai : It may be worthwhile to contribute this into Doom Emacs as well, which also has the same problem.

Thank you for the fix! I don't understand it, but it looks elegant!

dalanicolai commented 2 years ago

@vedang Well, you are probably no evil user then, because the fix simply adds the right keybindings... that's it! I noticed that you've added 'waiting for upstream merge' to the subject line, but there is no upstream fix in the make. The evil and pdf-tools packages are fine, users just should add the right keybindings (of course pdf-tools, could offer a pdf-evil.el, to provide the evil keybindings, but I find that unneccessary).

I'll create an issue at Doom to tip them about this fix/configuration. I guess you can close the issue here, as there seems there is no anything that has to be fixed in pdf-tools...

dalanicolai commented 2 years ago

I have notified the Doom developers via their Discord 'development' channel. I guess they'll pick it up at some point.

And I've notified the evil-collection guys now also...