meow icon indicating copy to clipboard operation
meow copied to clipboard

WIP: Add command meow-expand-beacon

Open fbergroth opened this issue 2 years ago • 27 comments

This command does the following:

  • if we're not already in beacon mode, activate it by creating a secondary selection from region
  • expand secondary selection by last selection command, so it will include one additional cursor (or # of numeric arg)
  • with a negative arg, shrink secondary selection so that one less cursor is included (not yet implemented)

I think it's more conventient in some cases, since you don't have to first create a region and loose cursor position.

I also made meow-expand use it in beacon-mode. So e + 3 would create cursor for 5 words.

What do you think?

fbergroth avatar Jun 30 '22 20:06 fbergroth

This is a really neat idea, i love it! Solves a problem i didn't even know I had.

I would like it if the same number overlay used in standard expand commands also appeared in the beacon expansions. I think this will probably require a small refactor of how it's done elsewhere (edit: it didn't). I'll take a look at implementing this.

eshrh avatar Jul 01 '22 03:07 eshrh

I am not immediately so sure how to implement the negative argument idea. I think a cleaner and more intuitive solution is to maintain another stack like meow--selection-history. Whenever we expand the secondary-selection, push the state onto the stack, and then in the beacon map we can define a key that shadows meow--pop-selection and instead calls meow--pop-beacon-state. I played around with this, and I think it's doable. Let me know what you think.

eshrh avatar Jul 01 '22 04:07 eshrh

I am ambivalent on the choice to make the default expansion line. I might say char is better, or even maybe no expansion at all. I'll let somebody else weigh in on this.

eshrh avatar Jul 01 '22 04:07 eshrh

This is a really neat idea, i love it! Solves a problem i didn't even know I had.

Great :-)

I would like it if the same number overlay used in standard expand commands also appeared in the beacon expansions. I think this will probably require a small refactor of how it's done elsewhere (edit: it didn't). I'll take a look at implementing this.

There is an edge case, when you are in beacon mode, and have cursors before and after point. Then the overlays are displayed relative to point, rather than relative to boundary of secondary selection.

I am not immediately so sure how to implement the negative argument idea. I think a cleaner and more intuitive solution is to maintain another stack like meow--selection-history. Whenever we expand the secondary-selection, push the state onto the stack, and then in the beacon map we can define a key that shadows meow--pop-selection and instead calls meow--pop-beacon-state. I played around with this, and I think it's doable. Let me know what you think.

I think there's an easy way to do this. Just index into meow--beacon-overlays to get an overlay and shrink secondary selection just before it starts. If the index is outside the bounds, we drop back to normal mode (i.e. if you have 10 cursors and give an arg of -15, all of them are removed). I can add this when I get some free time.

I am ambivalent on the choice to make the default expansion line. I might say char is better, or even maybe no expansion at all. I'll let somebody else weigh in on this.

I made the choice to be similar to vims rectangular selection and mc/mark-next-like-this from multiple cursors does this if there is no selection. For chars, why not use find/till?

fbergroth avatar Jul 01 '22 08:07 fbergroth

@eshrh I pushed a version that removes cursors with a negative arg. You can try it with, say, w + + + + +, and then remove one using - + or multiple with C-- 2 +. For some reason region and beacon overlays are removed when I press - for other types of selections. I haven't dug into this yet.

I think the stack-based idea does not allow a scenario where you first run meow-grab, create some beacons, and then remove cursors from either end? Or was there some other benefit you had in mind?

I also see that since I'm activating the region in meow-expand-region, meow-grab is recreating the secondary selection rather than destroying it. This needs to be fixed as well.

fbergroth avatar Jul 01 '22 19:07 fbergroth

Alright, I'll check it out. By the way, it looks like going backward is kind of broken. Try going back a word and then calling beacon-expand, it expands forward for me. The reason is that (secondary-selection-from-region) somehow cancels the region or something, making the subsequent call to meow--direction-backward-p always return nil. Do you think you could figure something out for that? (it'll be some time before I can look at this again)

eshrh avatar Jul 01 '22 19:07 eshrh

There is an edge case, when you are in beacon mode, and have cursors before and after point. Then the overlays are displayed relative to point, rather than relative to boundary of secondary selection.

Just fixed this, but it won't work going backwards until the issue i mentioned is resolved somehow.

I'm with you on the other points.

As for the stack undo, I still think it's maybe worth implementing. Consider if i forward word, and expand the 2selection by 5 words, when I really wanted 3. Currently you'd have to do some subtraction in your head to get to the result you want. I think it's much easier to just undo your last action and then expand 3 again. This is the same approach taken with normal selections. I do see how negative args are more precise and flexible though, and they definitely should be supported.

eshrh avatar Jul 01 '22 20:07 eshrh

By the way, it looks like going backward is kind of broken. Try going back a word and then calling beacon-expand, it expands forward for me.

I think I fixed the case you described, but switching direction during beacon mode via meow-reverse does not work. Probably the same issue as with negative argument, since it works to do w + + ; + +, but not for other selection types.

Perhaps stack undo is warranted, I'm not exactly sure how this will be used. At first I wanted a possibility to skip certain cursors, but that will be a lot more complex to implement, and maybe not even useful.

fbergroth avatar Jul 01 '22 20:07 fbergroth

Sounds very cool. Want a screencast for this feature?

DogLooksGood avatar Jul 01 '22 20:07 DogLooksGood

I think the mentioned bugs should be fixed now.

@DogLooksGood Sure, do you have an example in mind? Perhaps we can screencast the example we put in the tutorial.

fbergroth avatar Jul 02 '22 19:07 fbergroth

I think backward motion is still not quite working. Only the first backward expansion works, the direction isn't preserved after that.

eshrh avatar Jul 02 '22 19:07 eshrh

I think backward motion is still not quite working. Only the first backward expansion works, the direction isn't preserved after that.

Can you give me a reproducible example, things seems to work on my end? I think I'll write some tests for this, since it seems so brittle.

fbergroth avatar Jul 02 '22 19:07 fbergroth

Sure,

text: abc abc abc abc| abc where | is the point commands: meow-back-word meow-beacon-expand (no expansion) meow-beacon-expand expected: abc ^abc abc abc$ abc where ^ and $ are the beg and end of the overlay actual: abc abc ^abc abc abc$

Actually, for some time now I've been working on writing tests for most important functions in meow (#230), but I haven't pushed them. Feel free to ad-hoc it for now, we can merge the tests later.

eshrh avatar Jul 02 '22 20:07 eshrh

Unfortunately I get the expected behavior when trying it. Could if be something in your configuration? Does this work (if you change the path to meow)?

emacs -Q -L ~/code/meow --eval '(progn (insert "abc abc abc abc abc") (backward-word 2) (forward-word) (meow-global-mode) (meow-back-word 1) (meow-beacon-expand 2))'

Actually, for some time now I've been working on writing tests for most important functions in meow (#230), but I haven't pushed them. Feel free to ad-hoc it for now, we can merge the tests later.

I'm interested in that. Would it be possible to get a couple of tests merged, along with CI updates as a start? I've done some similar work like that before.

fbergroth avatar Jul 02 '22 20:07 fbergroth

Huh, that's weird. Could you try calling meow-beacon-expand twice instead of passing it the argument? I just tried it on emacs -Q.

eshrh avatar Jul 02 '22 21:07 eshrh

Strange. This works,

emacs -Q -L ~/code/meow --eval '(progn (insert "abc abc abc abc abc") (backward-word 2) (forward-word) (meow-global-mode) (meow-back-word 1) (meow-beacon-expand 1) (meow-beacon-expand 1))'

but when typing the commands via M-x, it does trigger the bug.

Do you have meow-beacon-expand bound to a key, or do you call it from M-x?

fbergroth avatar Jul 02 '22 21:07 fbergroth

I have it bound to a key, that's probably the issue then

eshrh avatar Jul 02 '22 21:07 eshrh

Ah, I have meow-use-cursor-position-hack set to t. That seems to be the culprit. Without it, I can reproduce.

fbergroth avatar Jul 02 '22 21:07 fbergroth

The last commit seems to fix it. The reason emacs ... --eval ... didn't work was because post-command-hook didn't run between commands.

Edit: seems this caused an issue with insert/append being offset in other cursors.

fbergroth avatar Jul 03 '22 10:07 fbergroth

Can you please clarify what you mean by insert/append being offset? I'd like to see this merged soon, and i have some time to work on it now.

eshrh avatar Jul 13 '22 17:07 eshrh

I haven't had much free time finishing this off. Please give it a go! Current issues from the top of my head:

  • I think it worked pretty fine with meow-use-cursor-position-hack=t. With meow-use-cursor-position-hack=nil, not so much. In a buffer with aa bb cc, C-a e + + i X <esc> I get aaX bbX cXc. C-e b + + a X <esc> gives me aXa XXbb XXcc. So a2a9c038fbc03de3384cc5f68203107758d7611f should be reverted, it only partly enables the cursor hack.
  • If you have an existing secondary selection, pressing meow-left followed by meow-beacon-expand should keep current column selected and expand one row. Now it first moves point to end of selection, and then expands a row.
  • Ideally in the previous scenario, one should be able to press meow-reverse and expand rows above secondary selection.
  • If you have a secondary selection and press meow-right-expand, followed by meow-beacon-expand, the selected columns should remain and we just extend another row.

I remember I didn't find a simple way to make all of these work last time.

fbergroth avatar Jul 13 '22 20:07 fbergroth

I personally don't like meow-use-cursor-position-hack, It's just something for Vim-ish. But then I realized it's point-less, it doesn't solve any problem, except some pseudo display.

DogLooksGood avatar Jul 15 '22 17:07 DogLooksGood

I rebased on master and added 4da64992b4e916ee0e1509e43696136358ee26e5 which hopefully fixes the direction issues / cursor hacking. Now it's possible to meow-reverse in beacon even if there is no active selection, and thus the command no longer supports meow-selection-command-fallback. I hope that's ok.

Just so you are following, now you can do e + + ; + + to select beacons on the following two words and the preceding two words. It should also behave like you would expect for the other selection types, fixing the rest of the outlined issues should be easy.

Would you like to write documentation @eshrh ?

fbergroth avatar Aug 06 '22 20:08 fbergroth

I think this is good. I also have another idea:

  • When you do grab, you create cursors by previous selection type
  • When you pop grab, you restore the selection type

So, you can do: next-word, expand, expand, grab Or you can do: arbitrary selection, grab, next-word, pop-grab, expand, grab

Some random thoughts. Using existing commands, with very few modifications.


Review: Just need some docs, and remove TODOs

DogLooksGood avatar Aug 07 '22 16:08 DogLooksGood

Yeah, i'll write the documentation. Sorry i couldn't get it to work, i gave it a shot but couldn't figure out how to get it right without some big refactors

eshrh avatar Aug 07 '22 21:08 eshrh

I also have another idea: ...

Great idea, that'll make things more consistent.

Yeah, i'll write the documentation. Sorry i couldn't get it to work, i gave it a shot but couldn't figure out how to get it right without some big refactors

Thanks! I struggled with it as well.

I'll clean up this branch and try to fix any remaining issues in a couple of days hopefully.

fbergroth avatar Aug 07 '22 21:08 fbergroth

Are you going to change the original meow-expand? Well, meow-expand should work as it is, even though in beacon state.

DogLooksGood avatar Aug 08 '22 00:08 DogLooksGood

Sorry, couldn't find time for this until now.

@DogLooksGood does 2ff655972dc94ee578984703c29e84c0eead0163 do what you had in mind? I'm not sure about pop-grab, I rarely use it. The other commits are just the previous stuff squashed.

fbergroth avatar Aug 14 '22 21:08 fbergroth

Manipulate the grab region with meow-expand is a bad idea, because if you want edit each 5 lines in grab, it should be grab, line, expand 4.

This update will break the functionalities. I didn't implement all the multiplied selection in beacon state, but that's the idea of the design.

So we should use a independent command for expand in beacon state. Or we make it easier to back to normal mode, to continue the region manipulation.

DogLooksGood avatar Aug 15 '22 09:08 DogLooksGood

Manipulate the grab region with meow-expand is a bad idea, because if you want edit each 5 lines in grab, it should be grab, line, expand 4.

I did miss that, however... I think it's much more intuitive to have expand 4 expand primary selection in normal state, and secondary selection in beacon state. Editing every fifth line is a much less common operation, and isn't really related to "expanding". I think it would be better to send that as a numeric argument to grab instead of overloading expand. So C-5 grab instead of grab, expand 4.

fbergroth avatar Aug 15 '22 11:08 fbergroth