elp-revive / origami.el

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

Fix offsetting to start of buffer on empty offset function #43

Closed mosquito-magnet closed 2 years ago

mosquito-magnet commented 2 years ago

Hi,

I had a problem with xml folding, it would start every fold at the start of buffer, while the end of the fold looked reasonable. Looking at the code, I think this is the case for every caller to origami-build-pair-tree that leaves fnc-offset empty.

I wrote a quick fix to origami-util-function-offset, but it might require some discussion and/or refactoring:

Fix

If arg fnc is nil, the function would return pos 0 - beg, which is the start of the buffer.

If arg fnc is empty, we want some sensible default behavior. beg is the start of the beginning match. Usually we want it to remain visible, and start folding right after it, so offset length(match) sounds sensible.

Discussion

Looking at the call sites of origami-util-function-offset, this used to be done by or at the call site, where origami-util-function-offset used to return nil when the offset function argument fnc was nil in prior versions (using ignore-errors). E.g. here in origami-build-pair-tree:

(or (origami-util-function-offset fnc-offset beg-pos beg-match)
    (length beg-match))

The call site in origami-build-pair-tree-2 works the same way I think, not sure though.

Now origami-util-function-offset will never return nil, we either pass a fnc-pos, or we get some default behaviour (which cannot be determined by call site anymore), seems fine to me.

If that's the intent, I think the call-site fallback code can be safely removed (no more or). What do you think?

jcs090218 commented 2 years ago

LGTM. Thanks for the changes! 👍