elp-revive / origami.el

A folding minor mode for Emacs
MIT License
32 stars 6 forks source link

Fix build pair tree #17

Closed mosquito-magnet closed 3 years ago

mosquito-magnet commented 3 years ago

Description

I am writing an xml parser, but encountered 2 problems with origami-build-pair-tree along the way. This PR contains fixes for these (new parser will be a separate PR later), plus comments and cosmetic changes that I hope will make reading the code easier, as I had some trouble understanding what was going on :) Hope I got everything right, and you find this useful...

I also bundled a commit with a minor fix for non-interactive calls of the origami-open-node-recursively-till-depth, sorry.

Bug 1 - origami-build-pair-tree - wrong open match passed to fnc-offset

If the build function creates a node with children ("dig in" form, after coming back from recursion), it passes the wrong match to fnc-offset. Instead of the opening match that started that node in a prior loop, the current opening match is passed, but that belongs to the first child (which triggered the recursion).

The fallback logic, if no fnc-offset is passed, uses ml-open, but this also contains the wrong value.

Reproduction

Any parser that would evaluate the match length might produce errors, if the parent and child opening tag differ.

With the existing parsers I cannot produce an error, because they don't evaluate it. The ruby parser e.g. will always set its offset to the end of the line.

I could provide my xml parser early with a parsing sample for reproduction, if you like.

Fix

I refactored a bit, basically I keep the opening match in a variable now, just like it already works for the opening pos in beg. I named them beg-pos and beg-match, and the current node variables cur-pos and cur-match, and use these matches to pass to fnc-offset.

I removed ml-open and several match variables, as it seemed no longer necessary.

Bug 2 - origami-build-pair-tree - "else" match node starts one char after match position

To my understanding, else will end a previously started node, and start a new one with the "else" match as the new start. It adds +1 to beg though, making it start one char to the right of the else position.

Reproduction

Again no error producable with the current parsers, as they correct for that by offset function. E.g. the ruby parser will do so by passing a fnc-pos to origami-get-positions, that for ending and else positions will use line-beginning-position - 1, so that these nodes starting pos will be the end of the previous line.

Fix

Don't add +1 to the new opening pos, instead make the previous node end one char before our else match.

Then I could remove the "hacks" in the parsers fnc-pos that altered the else/end position by -1:

I changed the positions of the end nodes to the first non-whitespace in the line instead, so whitespace before a nested end will be folded too. Otherwise it is considered part of the parent node, and would stay visible.

Ruby sample:

    if x > 1
      if x > 2
        puts "x is greater than 2"
      end
    end
jcs090218 commented 3 years ago

To my understanding, else will end a previously started node, and start a new one with the "else" match as the new start. It adds +1 to beg though, making it start one char to the right of the else position.

I think I have intend to make it at the beginning of the newline. Hence, it should something like

if x > 1
  puts "x is greater than 2"
end

to


if x > 1[...]
end

I don't know which is better but that was designed by intention. Can you change those thing back? Thanks!

mosquito-magnet commented 3 years ago

Ah I see, sure, I'll take look at it later!

mosquito-magnet commented 3 years ago

Can you give it another try? Thanks :)

jcs090218 commented 3 years ago

Is this your automatic default elisp formatting? Do you have a tip what formatting variables set or functions to use, so I can adapt?

This is the default by my own design. I don't think original origami has this feature. Feel free to define a custom variable to adapt both style of formatting. :)

Can you give it another try? Thanks :)

I have tried with ruby and lua parsers, and both formatting work great! One problem is c-macro with the #endif keyword.

Expected formatting

#elif[...]
#endif 

Current formatting.

#elif[...]#endif 

Can you fix this also? Thank you! :)

mosquito-magnet commented 3 years ago

Sorry, I overlooked the different matching type!

jcs090218 commented 3 years ago

LGTM! Thank you very much! :)