Nested folds #44

Monkeypac · 2017-12-02T22:04:04Z

Hi, as I told you earlier, I needed the nested folds for my usage.

Review welcome if you find something not good.
I added the nested folds as an option but I changed a few other things so don't hesitate on the review :)

mrkkrp

Thank you for this contribution!

I tried the new feature and it looks like it works, but fold deletion behaves a bit unexpectedly: it deletes all enclosing folds instead of just the one I place cursor at.

I'm not sure what is the correct behavior here, as I mentioned before, nested folds look confusing to me :-)

mrkkrp · 2017-12-03T03:11:30Z

vimish-fold.el

@@ -131,6 +131,15 @@ overhead is undesirable."
  :type 'boolean
  :package-version '(vimish-fold . "0.2.3"))

+(defcustom vimish-fold-allow-nested nil
+  "Whether to allow nested folds


The first line should end with a full stop. In emacs-lisp-mode Emacs should show warnings for these things I think...

Oh I missed it. Done.

I don't have warning though, do you have something special for this in your configuration ?
(I don't have flycheck enabled right now though)

Yeah, I think it's Flycheck.

mrkkrp · 2017-12-03T03:13:13Z

vimish-fold.el

+and will not be created."
+  :tag "Allow nested folds."
+  :type 'boolean
+  :package-version '(vimish-fold . "0.2.3"))


Let's have 0.2.4 here, because I'll release a new version with this feature.

mrkkrp · 2017-12-03T03:14:28Z

vimish-fold.el

@@ -415,6 +451,23 @@ This feature needs `avy' package."
           folds-before-point)))
      (message "No more folds before point"))))

+(defun vimish-fold-previous-existing-fold ()
+  "Jump to previous vimish region in current buffer."


How this is different from vimish-fold-previous-fold? It'd help if you mention the differences in the docs string too.

Not sure about this one.

The difference is that if you have the following:

# {{{ Fold 1 # }}} # {{{ Fold 2 # {{{ Fold 2.1 # }}} # }}}

When you are on Fold 2.1, the old function will make you go back to Fold 1 because Fold 2 is in the region of Fold 2.1. The new function makes you go back to Fold 2.

I thought that a new function was better since it does not alter the behavior but works fine without nested folds. It is however just a little bit slower since we process a little bit the list of folds (vimish-fold--folds-beginning-in).

Done, tell me if you prefer another solution for this :)

mrkkrp · 2017-12-03T03:19:50Z

vimish-fold.el

@@ -224,6 +233,21 @@ This includes fringe bitmaps and faces."
        '(vimish-fold--folded
          vimish-fold--unfolded)))

+(defun vimish-fold--vimish-overlay-at (pos)
+  (let ((overlays (overlays-at pos)) res-ov tmp-ov (min-space (max-char)))


Emacs says:

All variables and subroutines might as well have a documentation string

I understand that this is nitpicking, but since we're already following the whims of Emacs Lisp official style guide, there is no reason to stop following them now :-)

Done.

I'm not used enough to elisp dev I guess ^^"

mrkkrp · 2017-12-03T03:20:07Z

vimish-fold.el

@@ -311,6 +338,15 @@ If OVERLAY does not represent a fold, it's ignored."
   #'vimish-fold--vimish-overlay-p
   (overlays-in beg end)))

+(defun vimish-fold--folds-beginning-in (beg end)
+  (let ((list (vimish-fold--folds-in beg end)) tmp-ov)


The same stuff about missing docstring.

mrkkrp · 2017-12-03T03:21:16Z

vimish-fold.el

+  (let ((list (vimish-fold--folds-in beg end)) tmp-ov)
+    (while (list)
+      (setq tmp-ov (car list))
+      (unless (and (>= pos (overlay-start tmp-ov))


I get the following warning here:

reference to free variable pos

You should first declare the variable using let and then use it.

Woops it should have been beg and end, how did this even work ? ><

mrkkrp · 2017-12-03T03:23:48Z

vimish-fold.el

@@ -261,7 +286,8 @@ This includes fringe bitmaps and faces."
 (defun vimish-fold-unfold ()
  "Delete all `vimish-fold--folded' overlays at point."
  (interactive)
-  (mapc #'vimish-fold--unfold (overlays-at (point))))
+  (save-excursion


Just to make sure: does save-excursion make any difference is this case?

It seems like no ... I wonder why I did it. Removed.

Monkeypac · 2017-12-03T13:04:30Z

Oh and I forgot to check the fold deletion, I'll do it.

mrkkrp · 2017-12-04T12:15:02Z

This looks mostly good. However, when you have nested folds, say B inside A, and you expand both A and B, there is no visual clue left that B exists. To fold it back one has to find it blindly or from memory. This is not acceptable. We should do something about it. Maybe the color of fringe should change somehow for levels of nesting greater than one.

Monkeypac · 2017-12-04T13:35:23Z

Oh I don't use fringes so I didn't see that :D
I'll check that

mrkkrp · 2017-12-13T03:43:31Z

Any update on this? In principle, I can merge it in the current state if you find it usable.

Monkeypac · 2017-12-13T09:24:33Z

Don't merge it yet, I have a few changes to make but I didn't have many time last week, sorry for that ^^"

Monkeypac · 2018-02-08T13:20:54Z

Little update here, forget this, I don't have much time recently so I'll make it work on my fork when I have time and restart a pull request then.

Sorry for taking your time there, still working on it but when I have time ...

This patch only handles the fold / unfold functions.

If a fold is nested inside another one, set a different fringe color.

This patch does not handle the reclosing of the folds when terminating the search. This would need some more development to change the way vimish refolds / unfolds its folds.

Monkeypac · 2018-02-18T17:59:17Z

Okay so finally forced myself to take the time for this, waiting for your review but I hope it's going to be ok :D

Monkeypac · 2018-02-22T08:43:46Z

@mrkkrp , up ?

mrkkrp · 2018-02-22T08:44:51Z

Yes I remember about this. This weekend.

Monkeypac · 2018-02-22T08:45:23Z

Thanks :D

mrkkrp

This makes the whole thing too hairy. I'm unsure if I'll be able to maintain it because instead of relatively simple and elegant solution we get a monstrosity. I must admit that I just did not think about nested folds when I wrote the original code.

Not sure I should merge even if you address the comments, sorry. Maybe you could fork?

mrkkrp · 2018-02-24T11:33:24Z

vimish-fold.el

@@ -518,6 +662,7 @@ This minor mode sets hooks so when you `find-file' it calls

 For globalized version of this mode see `vimish-gold-global-mode'."
  :global nil
+  :keymap (make-sparse-keymap)


Why this is necessary?

mrkkrp · 2018-02-24T11:36:33Z

vimish-fold.el

@@ -205,7 +218,10 @@ If PREFIX is not NIL, setup fringe for every line."
                 (propertize "…" 'display
                             (list vimish-fold-indication-mode
                                   'empty-line
-                                   'vimish-fold-fringe)))))
+                                   (if (vimish-fold--nested-vimish-overlay-p


This does not work for me. Nested folds use the same fringe as non-nested folds: vimish-fold-fringe.

mrkkrp · 2018-02-24T11:44:25Z

vimish-fold.el

-(defun vimish-fold (beg end)
-  "Fold active region staring at BEG, ending at END."
+(defun vimish-fold (beg end &optional nested)
+  "Fold active region starting at BEG, ending at END.


As a user, why should I care about manually telling vimish-fold whether my new fold happens to be nested in some other fold or not? As I understand it, the fact whether the new fold is nested or not is determined entirely by position of the new fold and existing folds in the buffer.

mrkkrp · 2018-02-24T11:51:11Z

Or alternatively, this needs a total re-thinking of the design and a good test suite because of the added complexity. Unfortunately the package was intended as a very lightweight thing originally and if I merge there will be too many moving parts in it, if you understand what I mean.

Test suite would be really good at this point, but I don't have any spare time at all these days :-(

Opened #48, maybe I'll get to this some day...

Monkeypac · 2018-02-24T14:25:35Z

Yeah, I feared this but I understand.

This might be a little bit off-topic but:
For the time being, could you explain me how I could use my fork locally with use package ?
For the moment, I load the original package and then force load my local vimish-fold.el.

If I find time, I'll try to participate in #48.

mrkkrp · 2018-02-24T14:28:57Z

I don't use use-package, maybe you could ask on its issue tracker? Also, I think there is a project that allows to pull packages from git directly, but I forgot its name.

Monkeypac · 2018-02-24T14:30:03Z

Okay thx :D

mrkkrp requested changes Dec 3, 2017

View reviewed changes

Monkeypac force-pushed the nested_folds branch from 0edec10 to 500f06c Compare December 3, 2017 11:21

alexmurray mentioned this pull request Jan 8, 2018

How to create a nested fold? alexmurray/evil-vimish-fold#15

Closed

Monkeypac and others added 7 commits February 14, 2018 22:15

Add minor mode keymap.

29733a6

Introduce nested fold option.

7a05963

This patch only handles the fold / unfold functions.

Nested folds: handle previous and next

579d21e

Nested folds: handle refold

bad4439

Nested folds: handle delete

ce524a3

Nested folds: change fringe color if nested

0e69343

If a fold is nested inside another one, set a different fringe color.

Open folds on isearch.

7d20350

This patch does not handle the reclosing of the folds when terminating the search. This would need some more development to change the way vimish refolds / unfolds its folds.

Monkeypac force-pushed the nested_folds branch from 500f06c to 7d20350 Compare February 18, 2018 17:57

mrkkrp requested changes Feb 24, 2018

View reviewed changes

mrkkrp closed this Jun 22, 2018

Monkeypac mentioned this pull request Feb 22, 2020

Fold over folds #17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested folds #44

Nested folds #44

Monkeypac commented Dec 2, 2017

mrkkrp left a comment •

edited

Loading

mrkkrp Dec 3, 2017

Monkeypac Dec 3, 2017

mrkkrp Dec 4, 2017

mrkkrp Dec 3, 2017

Monkeypac Dec 3, 2017

mrkkrp Dec 3, 2017

Monkeypac Dec 3, 2017

mrkkrp Dec 3, 2017

Monkeypac Dec 3, 2017

mrkkrp Dec 3, 2017

Monkeypac Dec 3, 2017

mrkkrp Dec 3, 2017

Monkeypac Dec 3, 2017

mrkkrp Dec 3, 2017

Monkeypac Dec 3, 2017

Monkeypac commented Dec 3, 2017

mrkkrp commented Dec 4, 2017

Monkeypac commented Dec 4, 2017

mrkkrp commented Dec 13, 2017

Monkeypac commented Dec 13, 2017

Monkeypac commented Feb 8, 2018

Monkeypac commented Feb 18, 2018 •

edited

Loading

Monkeypac commented Feb 22, 2018

mrkkrp commented Feb 22, 2018

Monkeypac commented Feb 22, 2018

mrkkrp left a comment

mrkkrp Feb 24, 2018

mrkkrp Feb 24, 2018

mrkkrp Feb 24, 2018

mrkkrp commented Feb 24, 2018 •

edited

Loading

Monkeypac commented Feb 24, 2018

mrkkrp commented Feb 24, 2018

Monkeypac commented Feb 24, 2018

Nested folds #44

Nested folds #44

Conversation

Monkeypac commented Dec 2, 2017

mrkkrp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeypac commented Dec 3, 2017

mrkkrp commented Dec 4, 2017

Monkeypac commented Dec 4, 2017

mrkkrp commented Dec 13, 2017

Monkeypac commented Dec 13, 2017

Monkeypac commented Feb 8, 2018

Monkeypac commented Feb 18, 2018 • edited Loading

Monkeypac commented Feb 22, 2018

mrkkrp commented Feb 22, 2018

Monkeypac commented Feb 22, 2018

mrkkrp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrkkrp commented Feb 24, 2018 • edited Loading

Monkeypac commented Feb 24, 2018

mrkkrp commented Feb 24, 2018

Monkeypac commented Feb 24, 2018

mrkkrp left a comment •

edited

Loading

Monkeypac commented Feb 18, 2018 •

edited

Loading

mrkkrp commented Feb 24, 2018 •

edited

Loading