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

Add support for blocks in list items. #10

Closed

Conversation

kristijanhusak
Copy link
Contributor

@kristijanhusak kristijanhusak commented Nov 20, 2021

This PR adds support for src and dynamic blocks in list items. I got a report here about this. I tested on emacs orgmode, and it properly supports these blocks in list items.
I created an anonymous node for this for the sake of DRY, but if you think it needs to be done differently, let me know. I set it up to go only after new line.

@milisims
Copy link
Owner

Is there any reason why not to use _element here?

@kristijanhusak
Copy link
Contributor Author

Not that I know of. I just wasn't aware I could use it. I'll update.

@kristijanhusak
Copy link
Contributor Author

Well actually, seems I can't use it because of conflicts:

Trying this:

    itemtext: $ => seq(
      repeat1($._textelement),
      repeat(seq(
        $._nl,
        optional($._nl),
        choice(repeat1($._element), $.list)
      )),
    ),

And getting this:

Unresolved conflict for symbol sequence:

  _liststart  bullet  paragraph_repeat1  '
'  list  •  '
'  …

Possible interpretations:

  1:  _liststart  bullet  paragraph_repeat1  '
'  (_element  list)  •  '
'  …
  2:  _liststart  bullet  paragraph_repeat1  (itemtext_repeat2  '
'  list)  •  '
'  …

Possible resolutions:

  1:  Specify a higher precedence in `_element` than in the other rules.
  2:  Specify a higher precedence in `itemtext_repeat2` than in the other rules.
  3:  Add a conflict for these rules: `_element`, `itemtext`

@milisims
Copy link
Owner

milisims commented Nov 21, 2021

Okay. Some rearranging is necessary, the nodes in _element have newlines associated already, and lines of text (repeat1($._textelement)) do not. I'd test a few things out but I'm not at a computer.

What we want here is very similar overall to the body node, basically. Without the standalone directives, which simplifies parsing. Should we just use the paragraph node for separate bodies of text within the list item? Since we're adding the rest of the elements. I.e we might see (itemtext (paragraph)) for a simple list item, (itemtext (paragraph) (paragraph) (dynamic_block (directive)) (paragraph)) for something that looks like:

-  paragraph 1

   Paragraph 2
   #+NAME: A
   #+begin: a
   b
   #+end:

    Paragraph 3

This was too much phone effort. :S

What do you think?

@kristijanhusak
Copy link
Contributor Author

kristijanhusak commented Nov 21, 2021

This was too much phone effort. :S

Don't do this to yourself :D

Yeah, I think we could easily have a paragraph in there. From my tests, list item can contain anything that a headline body can, so I think that should be the way to go.

@milisims
Copy link
Owner

Just wanted to update on why this isn't done yet:
So, the _listitemend node that's matched by the external scanner expects to be just before a newline, which means that matching the element nodes as written (which include the newline chars) won't work right. For example, your PR here will match the following as a single list instead of two lists:

- A
  #+begin: a
  b
  #+end:


- B

There are at least three ways to resolve this.

My first attempt was to rewrite the scanner to start looking at column=0, which complicates things because the extras includes whitespace and is matched before the external scanner, so I'm not sure how to resolve it with this strategy. I can't use token.immediate with an external scanner, so I don't think this is easily resolved, but there might be a few other tricks we can pull with zero-width nodes that I would like to try before throwing out this idea.

The next way to resolve it is change from using 'listitemend' to move newline matching logic that's currently in the external scanner into the grammar, and only scan for indentation changes. This might be the route to settle on, but I'm not excited about it.

The next way to resolve it is not add the elements to the list parser at all, and inject an org parser into the itemtext. This seems like a bit extra to me, and it would be simple but a last resort.

For the short term, it might be simpler to solve why the parser is throwing an error in your example and resolve that, so I can sink into solving this problem when I have a day to invest into it.

@kristijanhusak
Copy link
Contributor Author

I hoped this issue was fixed with the latest changes, but it seems it's still broken:

This:

* TODO Test
  - Test
    #+BEGIN_SRC
    Content
    #+END_SRC

Gives this:

(document [0, 0] - [5, 0]
  (ERROR [0, 0] - [5, 0]
    (headline [0, 0] - [1, 0]
      stars: (stars [0, 0] - [0, 1])
      item: (item [0, 2] - [0, 11]
        (expr [0, 2] - [0, 6])
        (expr [0, 7] - [0, 11])))
    (bullet [1, 2] - [1, 3])
    (expr [1, 4] - [1, 8])
    (ERROR [2, 4] - [2, 15])
    (expr [3, 4] - [3, 11])))

Is it still complicated to add a support for this? I know previous version caused some issues.

@milisims
Copy link
Owner

milisims commented Feb 9, 2022

Is it still complicated to add a support for this? I know previous version caused some issues.

Unfortunately yes. I think the best solution until I can invest some time on that problem will be to inject an org parser across (itemtext), which is a bit sloppy feeling but I don't have a better solution

That said, it really shouldn't be an error, so I'll see if there's a solution to that.

@milisims
Copy link
Owner

milisims commented Mar 28, 2022

Should be resolved in v0.3.0, commit 9a595e5. I removed (itemtext) to do so breaking previous compatibility, and parsing descriptions was a nightmare, so I'm moving that to queries. But it seems to be working for me. Checkboxes are trivial via queries and were added to the highlights.scm

@milisims milisims closed this Mar 28, 2022
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