Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug that fails to display link in eldoc when url-hidding mode is enabled #674

Merged
merged 1 commit into from
Jan 10, 2022
Merged

fix bug that fails to display link in eldoc when url-hidding mode is enabled #674

merged 1 commit into from
Jan 10, 2022

Conversation

taquangtrung
Copy link
Contributor

@taquangtrung taquangtrung commented Jan 10, 2022

Hi,

Description

Currently, when the URL hiding feature is enabled, markdown eldoc always throws the error eldoc error: (wrong-type-argument stringp nil) when a cursor is put under the hidden URL.

I investigated the code and see that the problem is due to the fact that the regular expression matching state performed by (thing-at-point-looking-at markdown-regex-link-inline), which is used to get the URL, is cleared by the next call to (markdown--substitute-command-keys (if imagep "\\[markdown-insert-image]" "\\[markdown-insert-link]")).

So, this PR will fix this bug by getting the URL information early, before the call to markdown--substitute-command-keys.

Can anyone take a look and merge it if possible?

Thank you!

Related Issue

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves an existing feature)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have updated the documentation in the README.md file if necessary.
  • I have added an entry to CHANGES.md.
  • I have added tests to cover my changes.
  • All new and existing tests passed (using make test).

@syohex
Copy link
Collaborator

syohex commented Jan 10, 2022

Could you show us the code which reproduces your issue or add unit tests ?

@taquangtrung
Copy link
Contributor Author

The code that creates the issue is inside markdown-eldoc-function, in particular this part:

   ((and (or (thing-at-point-looking-at markdown-regex-link-inline)
             (thing-at-point-looking-at markdown-regex-link-reference))
         (or markdown-hide-urls markdown-hide-markup))
    (let* ((imagep (string-equal (match-string 1) "!"))
           (edit-keys (markdown--substitute-command-keys
                       (if imagep
                           "\\[markdown-insert-image]"
                         "\\[markdown-insert-link]")))
           (edit-str (propertize edit-keys 'face 'font-lock-constant-face))
           (referencep (string-equal (match-string 5) "["))
           (object (if referencep "reference" "URL")))
      (format "Hidden %s (%s to edit): %s" object edit-str
              (if referencep
                  (concat
                   (propertize "[" 'face 'markdown-markup-face)
                   (propertize (match-string-no-properties 6)
                               'face 'markdown-reference-face)
                   (propertize "]" 'face 'markdown-markup-face))
                (propertize (match-string-no-properties 6)
                            'face 'markdown-url-face)))))

The function call (thing-at-point-looking-at markdown-regex-link-inline) is supposed to be used to fetch URL information later by (match-string-no-properties 6).

But there is another call by markdown--substitute-command-keys at

 (edit-keys (markdown--substitute-command-keys
                       (if imagep
                           "\\[markdown-insert-image]"
                         "\\[markdown-insert-link]")))

which also performs regular expression matching. This call will clear the state produced by (thing-at-point-looking-at markdown-regex-link-inline).

Therefore the latter call to (match-string-no-properties 6) always produces nil.

In my patch, I put the call to (match-string-no-properties 6) to be earlier than markdown--substitute-command-keys

@syohex
Copy link
Collaborator

syohex commented Jan 10, 2022

Sorry I would like you to show us sample markdown text and minimal configuration to reproduce this issue. And could you tell us how to reproduce in detail, cursor position, commands etc ?

@taquangtrung
Copy link
Contributor Author

taquangtrung commented Jan 10, 2022

Ah I see. For example, I created and open a markdown file foo.md and put the following content into it: [This is a hidden link](http://hidden.link).

Then run M-x markdown-toggle-url-hiding, turn on eldoc-mode, and put the cursor under the link's text. There will be an error message like the below (the keyboard cursor is under the link text)

markdown

@taquangtrung
Copy link
Contributor Author

Here is what obtained after applying my patch:

markdown-new

syohex added a commit that referenced this pull request Jan 10, 2022
@syohex syohex mentioned this pull request Jan 10, 2022
7 tasks
syohex added a commit that referenced this pull request Jan 10, 2022
@syohex syohex merged commit f3eb784 into jrblevin:master Jan 10, 2022
@syohex
Copy link
Collaborator

syohex commented Jan 10, 2022

@taquangtrung Thanks for good explanation. I have merged your patch and added a unit test #675

@taquangtrung
Copy link
Contributor Author

@syohex: thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants