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

match block constructs in single pass #95

Merged

Conversation

cosmicexplorer
Copy link
Contributor

Improve on #91 by increasing speed of matching block constructs, and
allow them to be nested within each other, by matching all of them in a
single pass.

  • Ensure font locking of block constructs is predicated solely on text
    properties.
  • Add "language" field to tilde fence opening regexp.
  • Make test-markdown-font-lock/pandoc-yaml-metadata pass.

It's a lot of code, sorry. I tried to cut it down as much as possible, since what I'm trying to do isn't actually too hard. If you can see ways to slim it, I'll spend some time cutting it down.

@jrblevin
Copy link
Owner

Thanks! I will take a closer look.

I skimmed the patch and saw the new test called test-markdown-parsing/propertize-fenced-in-between. The multi-pass issue makes me wonder if this patch might also address #85, where some fenced code blocks weren't being detected.

@cosmicexplorer
Copy link
Contributor Author

That's totally possible. I was never able to reproduce that one but it definitely makes sense.

@jrblevin
Copy link
Owner

I wasn't able to reproduce that one either. "Static" font lock has always been pretty reliable, but sometimes I've noticed that "dynamic" font lock (updating on the fly while editing) occasionally does get confused about GFM code blocks. My sense is that these dynamic inconsistencies are the underlying source for that issue.

I've been testing this patch and so far it seems very solid!

@jrblevin
Copy link
Owner

I ran into an error with tilde fenced code blocks:

  1. emacs --no-init
  2. load markdown-mode
  3. create a new buffer and start markdown-mode
  4. Start typing a tilde-fenced code block: ~~~ then RET

Upon RET, I get the following errors:

Error during redisplay: (internal--syntax-propertize 5) signaled (wrong-type-argument integer-or-marker-p nil)
back-to-indentation: parse-sexp-propertize-function did not move syntax-propertize--done
Error during redisplay: (jit-lock-function 1) signaled (wrong-type-argument integer-or-marker-p nil)
Error during redisplay: (jit-lock-function 1) signaled (wrong-type-argument integer-or-marker-p nil)

My best guess is that one of the matching functions isn't returning values according to the specs:

When function is called, it receives one argument, the limit of the search; it should begin searching at point, and not search beyond the limit. It should return non-nil if it succeeds, and set the match data to describe the match that was found. Returning nil indicates failure of the search.

Fontification will call function repeatedly with the same limit, and with point where the previous invocation left it, until function fails. On failure, function need not reset point in any particular way.

@cosmicexplorer
Copy link
Contributor Author

I made an assumption in markdown-get-fenced-block-from-start that if the start of a block construct (the ~~~ or ---) was propertized, that the middle or end would be propertized too. As shown in the other test I wrote, that's not always the case; we start off by propertizing the start, and then we propertize the middle or end when they're completed. This means that the start of a block construct might be propertized (like you described), without it being completed at all. I've added a test case which should make sure this doesn't happen.

@cosmicexplorer
Copy link
Contributor Author

Ok, this is a little bit more complete; I hadn't thought about the case where there was no newline in between fences:

~~ ~
~~ ~

And was also getting wonky behavior when there was a single newline in between fences.

@jrblevin
Copy link
Owner

Great, that's fixed now. Thanks.

One more thing related to typing in code blocks. Suppose I type in the following:

   print "Hello, world!"

The face for the beginning and ending backquotes is set, but not for the contents (which should have markdown-pre-face).

@cosmicexplorer
Copy link
Contributor Author

I'm not sure I see what you're saying -- it seems to work on my end, on both my config and a bare emacs. Do you mean markdown-pre-face isn't being applied at the boundaries? Could you write a test case or something to show what you mean?

@jrblevin
Copy link
Owner

Sorry--it was late and my Markdown wasn't rendered correctly. The text to type should have been this (the backquotes were missing):

```
print "Hello, world!"
```

It matters that this is typed in, as opposed to using the GFM code block insertion command.

After I type the third opening backquote, the opening is propertized and the correct face is applied.
After I type the third closing backquote, the entire block is propertized correctly and the match data is stored, however, only the opening and closing backquotes are fontified correctly. The code portion--the "middle" part--only has the default face and not markdown-pre. The issue seems to be that Emacs only refontifies the closing and not the entire block.

We have an -extend-region function for syntactification that extends the region to include the entire code block. I thought that would carry over font-lock as well (it seemed to work this way before), but perhaps not.

I've tried to write test cases for such dynamic issues before, but I haven't managed to get them to work. If you open the file and run markdown-mode it would work fine, but as you're typing there is an issue.

@cosmicexplorer
Copy link
Contributor Author

Sorry about that, I didn't understand the intricacy with "typing" it in. You're right that what you're describing currently does work on master. I think this is because the text properties for the end of code blocks don't extend to the newline after a code block. I will investigate and try to write a test case which measures this behavior.

@cosmicexplorer
Copy link
Contributor Author

I'll squash these commits once you give the ok on this. I thought it would be easy to make a test case demonstrating that dynamic behavior, but it turns out font-lock is still halfway a mystery to me. In any case, what was happening was that although syntax propertizing was happening correctly using markdown-syntax-propertize-extend-region, font locking wasn't happening, and font locking of block constructs with this commit is done based upon text properties. I added a function markdown-font-lock-extend-region-function and added it to the jit-lock-after-change-extend-region-functions for the buffer. It seems to work now; I'll try to look through some docs and see if I can figure out a way to test it. Until then I'll just remember to test that every time I play with font locking.

@jrblevin
Copy link
Owner

This works for me now. Thanks for sorting this out.

If you could squash and also rebase on the latest master, that would be perfect.

@cosmicexplorer cosmicexplorer force-pushed the fix/yaml-metadata-inside-code-blocks branch from 20c3359 to 453055d Compare February 16, 2016 03:36
Improve on jrblevin#91 by increasing speed of matching block constructs, and
allow them to be nested within each other, by matching all of them in a
single pass.

- Ensure font locking of block constructs is predicated solely on text
properties.
- Add "language" field to tilde fence opening regexp.
- Make `test-markdown-font-lock/pandoc-yaml-metadata` pass.
- Add `markdown-font-lock-extend-region-function` and rework
`markdown-syntax-propertize-extend-region` to stretch
repropertization/refontification to all of a code block.
- Add test cases for fenced block conditions.
@cosmicexplorer cosmicexplorer force-pushed the fix/yaml-metadata-inside-code-blocks branch from 453055d to e72bd3f Compare February 16, 2016 03:44
@cosmicexplorer
Copy link
Contributor Author

Done.

@jrblevin jrblevin merged commit e72bd3f into jrblevin:master Feb 16, 2016
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