Rewrite internals

[purcell]

• Allows rectangle-mark-mode to work as expected (fixes #9)
• Uses yank-handler, so no need to replace the yank command
• Improved preservation of point, generally to improve correctness. However, after yanking full lines, the point is now preserved, not placed at the line beginning, so feedback would be appreciated regarding how this feels.
• Default override for comment-dwim is changed, and now uses prefix to denote the number of lines to comment/uncomment, rather than comment level
• Removed the obsolete whole-line-or-region-mode alias: use either the -local-mode or the -global-mode now.
• Removed the old-fashioned "turn-on" function
• Greatly improved tests

Plan would be to release this as v2.0

[gusbrs]

Hi @purcell , thank you very much for this!

As requested, I took the branch for a spin, and I really liked what I saw. I particularly liked you went with the yank-handler, as it avoid a number of pitfalls, such that it is definitely the right tool for the task. That given, I would also not refrain from helping polish things. But I'll be commenting the kill/yank part, as I was never a user of the other functionalities of the package, and I'm not really acquainted with them.

I can gladly report rectangle-mark-mode does work as expected alongside whole-line-or-region. :-)

A number of things which called my attention are:

1 - The rewrite passes the no-exclude argument to the yank-handler text properties, as I believe it should. But that means whole-line-or-region-yank-handler should be dealing with cleaning up yank-excluded-properties, but I don't see it there. I inspected the text properties in the buffer after yanking, and didn't find anything strange. So I suppose the rewrite is dealing with it somehow, and I do not grasp it. So this comment is a "just checking" one.

2 - I don't know if it was meant to, but the rewrite still does not handle read-only buffers and does not honor kill-read-only-ok, as reported in #12. It does indeed complicate the situation by adding the * interactive specification to all the other wlr-kill functions, making it harder to kill text in read-only buffers. This is quite at odds with the behavior of the corresponding standard commands, which always place the read-only string in the kill-ring (regardless of kill-read-only-ok, which changes only the message).

3 - I've noticed you changed the test for a region from:

(if (and mark-active
         (/= (point) (mark)))
    ...)

to:

(if (use-region-p)
    ....)

I'm also well aware of the comment "This package will behave unexpectedly (and indeed is nearly useless) if transient-mark-mode is off, as there is then always a region defined."

That statement was true with the previous test, but no longer. Now, if transient-mark-mode is off, a region is never defined. This means wlr will really misbehave if transient-mark-mode is off. I did ponder this issue in my own attempts, and eventually came back to (and mark-active (/= (point) (mark)), but I can't say I'm really satisfied. One of the incantations I played with was:

(if (or
     ;; If transient-mark-mode is not enabled just proceed as the regular
     ;; kill-func would.
     (not transient-mark-mode)
     ;; If transient-mark-mode is enabled, still proceed as the regular
     ;; kill-func if region is active and not empty.
     (and (region-active-p)
          (> (region-end) (region-beginning))))
    ...)

This would render wlr kill functionality useless, but harmless, in the absence of transient-mark-mode. More aligned to the original comment regarding the interaction between the two. WDYT?

Well, those are my observations after testing. I hope they are useful. And thanks again!

[purcell]

Thanks @gusbrs, this is excellent feedback. I think my conclusion here is that I need to write a bunch of failing tests for these scenarios and then work through them.

[purcell]

Okay, that's a couple of fixes. Still going...

[purcell]

Alright, now I think the only outstanding issue should be the behaviour when transient-mark-mode is off.

[purcell]

Probably the safest default would be to skip using use-region-p and stick with the old code's test: (and mark-active (/= (point) (mark)))...

[gusbrs]

It looks good!

As to

now I think the only outstanding issue should be the behaviour when transient-mark-mode is off.

Probably the safest default would be to skip using use-region-p and stick with the old code's test: (and mark-active (/= (point) (mark)))...

Well, that's a decision to make. I think testing for transient-mark-mode and then proceeding with (region-active-p) seems more "proper" to me. But indeed, wrt backward compatibility (and mark-active (/= (point) (mark))) is probably "safer". Do you have any idea if folks actually use wlr without transient-mark-mode?

[gusbrs]

Ah, I recalled one small polishing detail. The docstring for yank says:

With just C-u as argument, put point at beginning, and mark at end.

whole-line-or-region-yank-handler could easily honor one such "Emacs treat" by pushing the mark in the appropriate place.

[purcell]

Well, that's a decision to make. I think testing for transient-mark-mode and then proceeding with (region-active-p) seems more "proper" to me. But indeed, wrt backward compatibility (and mark-active (/= (point) (mark))) is probably "safer".

I ended up doing this: 6b019b2

Do you have any idea if folks actually use wlr without transient-mark-mode?

I very much doubt they do. I can't see what the point would be. But it would still be nice if the behaviour degrades fairly smoothly.

[purcell]

* However, after yanking full lines, the point is now preserved, not placed at the line beginning, so feedback would be appreciated regarding how this feels.

Any thoughts on this point, @gusbrs? It would actually be pretty easy to make this behaviour customisable.

[purcell]

Ah, I recalled one small polishing detail.

Oh cool

[gusbrs]

But it would still be nice if the behaviour degrades fairly smoothly.

Good point.

I ended up doing this: 6b019b2

Why honor use-empty-active-region there? (I'm not sure it should do otherwise, just suggesting you think this carefully).

[purcell]

Why honor use-empty-active-region there? (I'm not sure it should do otherwise, just suggesting you think this carefully).

It's what region-active-p does. /shrug

[gusbrs]

It's what region-active-p does. /shrug

I know, but does it make sense in this case? The difference will be if we have the current-kill as a whole line, and set the mark and then yank the yank will not be done as a whole line. Do you see a use case where we would want that? (Again, I'm not sure of the answer, just thinking this through with you).

[gusbrs]

Regarding

* However, after yanking full lines, the point is now preserved, not placed at the line beginning, so feedback would be appreciated regarding how this feels.

Any thoughts on this point, @gusbrs? It would actually be pretty easy to make this behaviour customisable.

I was confused when I read the PR description, and I still am. Isn't that what already happened? (I just tested it again on master, and that's what I think is going on).

Could you elaborate what the difference from previous behavior is?

[gusbrs]

The difference will be if we have the current-kill as a whole line, and set the mark and then yank the yank will not be done as a whole line.

This is actually not correct. This will happen regardless of whole-line-or-region-use-region-p as it is not used in whole-line-or-region-yank-handler. And I could not notice a difference in behavior when killing. I don't understand well why, but if this is so, it looks good then.

[purcell]

I was confused when I read the PR description, and I still am. Isn't that what already happened? (I just tested it again on master, and that's what I think is going on).

Oh, you're right -- my mistake. Something felt a bit different to me.

I've just made what I think is a further useful improvement: supporting yank-pop properly. So while the full line gets inserted before the current line, subsequently yank-pop-ing a partial line will correctly remove that full line and insert the partial line at point.

[purcell]

I'm happy that all this has prompted a big increase in test coverage.

[gusbrs]

Something felt a bit different to me.

Also to me, but it is not yanking. I think the difference is in killing, particularly whole-line-or-region-kill-region. Now when calling it, the point does go to BOL, while I think it didn't before. But I'm not sure this is in my mind. Can you confirm if that's what feels different to you?

[gusbrs]

I'm happy that all this has prompted a big increase in test coverage.

I'm happy for that, and also that functionality is much improved. :-)

[gusbrs]

I've just made what I think is a further useful improvement: supporting yank-pop properly. So while the full line gets inserted before the current line, subsequently yank-pop-ing a partial line will correctly remove that full line and insert the partial line at point.

Seems appropriate. I had missed this, as I use counsel-yank-pop. Anyway, that's a good point.

[purcell]

Also to me, but it is not yanking. I think the difference is in killing, particularly whole-line-or-region-kill-region. Now when calling it, the point does go to BOL, while I think it didn't before. But I'm not sure this is in my mind. Can you confirm if that's what feels different to you?

Yes, I think that is what's different. There was previously some attempt to preserve the column, so if you did C-w at the end of a line, point would be left at the end of the following line. Now it's unconditionally left at BOL. Arguably the new behaviour is more predictable, but I don't know what's preferable really.

[gusbrs]

Yes, I think that is what's different.

Ah! It's hard to analyse muscle memory. ;-)

Now it's unconditionally left at BOL. Arguably the new behaviour is more predictable, but I don't know what's preferable really.

I think did prefer the previous behavior. With it whole-line-or-region-kill-region and whole-line-or-region-kill-ring-save seem more alike. But, true, debatable. And, also true, maybe it is just "what I'm used to" speaking.

[purcell]

I think did prefer the previous behavior. With it whole-line-or-region-kill-region and whole-line-or-region-kill-ring-save seem more alike.

I'm inclined to agree, except that the kill-whole-line builtin also leaves point at BOL. It's easy to implement though...

[gusbrs]

except that the kill-whole-line builtin also leaves point at BOL.

Because it does not handle the yanking as a whole line either, so you have to be at BOL to yank.

But true, debatable. I'm fine with either, and also with an option for it. As you see fit.

[purcell]

Another think I'd love to get working is that C-w C-w C-y would insert both copied lines. It does that now, but in reverse order. :-/ That one looks fiddly.

[gusbrs]

A small nitpick, to reduce one layer of complexity. You are doing:

(remove-yank-excluded-properties (point) (- (point) (length string)))

But, since you are let-binding beg and end and have their values at hand, why use (point) and (- (point) (length string))?

[purcell]

A small nitpick, to reduce one layer of complexity.

Good catch, thanks - done.

[gusbrs]

Another think I'd love to get working is that C-w C-w C-y would insert both copied lines. It does that now, but in reverse order. :-/ That one looks fiddly.

Uhm, indeed. Missed that too. My stuff here is working with that, but I cannot figure what's the difference that's causing the diverging results... Shouldn't kill-region be appending?

[purcell]

Shouldn't kill-region be appending?

Yes, and it does. If I hit C-w C-w then M-: (car kill-ring) it shows a single string appended as expected.

[gusbrs]

Yes, and it does. If I hit C-w C-w then M-: (car kill-ring) it shows a single string appended as expected.

Indeed, simultaneous discovery. :-)

In this case, we can assume the problem is likely in yanking. Right?

[gusbrs]

In this case, we can assume the problem is likely in yanking. Right?

Indeed... Remove the save-excursion from whole-line-or-region-yank-handler and try. I don't really understand what's going on though.

[purcell]

Remove the save-excursion from whole-line-or-region-yank-handler and try. I don't really understand what's going on though.

Yeah, that kinda breaks everything else though. 😂

[gusbrs]

Yeah, that kinda breaks everything else though. :joy:

Sure... I just meant that's where the problem is. :shrug:

[gusbrs]

Still trying to pin this down, and still being eluded. But two additional pieces of information.

If you do C-w C-w C-f C-y it works. That is, it only fails if (bolp).

Using insert-before-markers instead of insert in whole-line-or-region-insert-clean seems to work. I'm not "suggesting" it, as I still don't understand what's wrong. It just might help you understand.

[purcell]

Ooh, good find! Pushed a fix to use that, and more tests.

[purcell]

(I had circled round to the same conclusion about bol, but was shockingly unaware of insert-before-markers.)

[gusbrs]

Great that it works, but I still don't get it. Could you explain why insert didn't work in the first place?

[purcell]

Great that it works, but I still don't get it. Could you explain why insert didn't work in the first place?

Not quite sure. At BOL you can produce the same effect with M-: (save-excursion (move-beginning-of-line 1) (insert "foo\n")) -- the point moves to the beginning of "foo". I haven't looked at save-excursion, but presumably it preserves point using a marker.

Same effect with (let ((pos (point-marker))) (move-beginning-of-line 1) (insert "foo\n") (goto-char pos)) and (let ((pos (point-marker))) (insert "foo\n") (goto-char pos)) at BOL. So it's a quirk of insert alone. Here are the C functions that are used to parameterise the difference between the two: https://github.com/emacs-mirror/emacs/blob/78ec68e18f07a90a9ad400683b973ff51baa80e1/src/insdel.c#L950-L983

[gusbrs]

At BOL you can produce the same effect with M-: (save-excursion (move-beginning-of-line 1) (insert "foo\n")) -- the point moves to the beginning of "foo". I haven't looked at save-excursion, but presumably it preserves point using a marker.

Same effect with (let ((pos (point-marker))) (move-beginning-of-line 1) (insert "foo\n") (goto-char pos)) and (let ((pos (point-marker))) (insert "foo\n") (goto-char pos)) at BOL. So it's a quirk of insert alone.

A tricky one, eh? But, if you are confident insert-before-markers is the proper solution, I do trust your judgement.

Another curiosity: why use (move-beginning-of-line 1) instead of (beginning-of-line)?

[purcell]

But, if you are confident insert-before-markers is the proper solution, I do trust your judgement.

I can't know if there's an outright best solution, but I feel like we only need to be confident that the tests pass and that the behaviour is not hugely divergent from the old version of the package.

Another curiosity: why use (move-beginning-of-line 1) instead of (beginning-of-line)?

No good reason: it looks like the low-level (forward-line 0) would even be fine here.

The one remaining point I may address here is the fact that columns are not preserved after killing the current line.

[gusbrs]

No good reason: it looks like the low-level (forward-line 0) would even be fine here.

Thanks!

I can't know if there's an outright best solution, but I feel like we only need to be confident that the tests pass and that the behaviour is not hugely divergent from the old version of the package.

Agreed.

The one remaining point I may address here is the fact that columns are not preserved after killing the current line.

That would be nice. But I've already shown myself a partisan in this one.

As far as I can tell, you might have forgotten about:

Ah, I recalled one small polishing detail. The docstring for yank says: With just C-u as argument, put point at beginning, and mark at end. whole-line-or-region-yank-handler could easily honor one such "Emacs treat" by pushing the mark in the appropriate place.

But either way, it is shining overall. And I'm looking forward to the v2.0 release! Thank you very much!

[purcell]

The other changes are pushed now

Ah, I recalled one small polishing detail. The docstring for yank says: With just C-u as argument, put point at beginning, and mark at end. whole-line-or-region-yank-handler could easily honor one such "Emacs treat" by pushing the mark in the appropriate place.

I'm not sure what's best to do about this: when a full line is yanked prior to the current one, is it actually helpful to set mark when we are preserving point unchanged?

[purcell]

But either way, it is shining overall. And I'm looking forward to the v2.0 release! Thank you very much!

Your advice has been extraordinarily helpful. So thank you!

[gusbrs]

I'm not sure what's best to do about this: when a full line is yanked prior to the current one, is it actually helpful to set mark when we are preserving point unchanged?

In my settings, I just (push-mark) after (move-beginning-of-line 1). With that, calling yank with an argument will leave point at the beginning of the yanked text, as normally happens with yank. Even without argument, we will be able to exchange point and mark to the beginning of the yanked text. True, in this case, exchanging point and mark will no longer select the yanked region precisely, but potentially a little more than that. But it seems something sensible to do.

[purcell]

But it seems something sensible to do.

Yes! Done.

[purcell]

The one remaining nit is that after a mix of killing whole lines and partial lines, if you do a bunch of yank-pop then it doesn't always undo cleanly. I don't think it's a blocker for release, but we've done a good job here of making everything work well, and it's a shame for it to have known failure modes. :-)

[gusbrs]

Yes! Done.

You've went with removing save-excursion. That's possibly a good idea, and it might release us from insert-before-markers. I understand what you did there, I cannot find where it goes astray, but it does not work as expected. Try one of the following sequences somewhere: C-w C-w C-u C-y (point should be at the start of the inserted string, and it's not); C-w C-w C-y C-x C-x (point should be at the start of the inserted string, and mark active at the original point position, and they are not).

The one remaining nit is that after a mix of killing whole lines and partial lines, if you do a bunch of yank-pop then it doesn't always undo cleanly.

FWIW, I cannot reproduce this.

we've done a good job here of making everything work well

Oh, yes. Indeed a good job, things are looking very good. And it's been fun too! :wink:

[gusbrs]

I found another corner case for the use of save-excursion (I know you removed it in the last commit, but as it is not working as intended, you might include this case in your considerations about what to do with it).

So, checkout to commit f74e1e1, where save-excursion is still used. Kill a whole line in a buffer such that the corresponding yank will be handled by whole-line-or-region-yank-handler. Now go to another completely empty buffer. Try to yank. It doesn't yank. Hit enter. Yank. Now it does.

Edit: That was not precise. The point is not "empty buffer", it appears to be being in the first line of the buffer, which makes the case "less cornerish".

[purcell]

Try one of the following sequences somewhere

Do these work with your own code? It seems to be a result of the separately-propertised stretches of the string being successively yanked, so that the mark set by the last yank-handler invocation is the one that wins. So after C-w C-w C-u C-y, a further pop-mark-command takes you back to the first-yanked line. I'm not sure tht this case will be easy to fix.

I found another corner case for the use of save-excursion

I guess I'll add a test for this.

[gusbrs]

Do these work with your own code?

They do. But unfortunately, I don't know the reason for the difference, and I read your code and it looks perfectly correct. But, then again, we've been bitten by the interaction of save-excursion, mark and insert haven't we?

It seems to be a result of the separately-propertised stretches of the string being successively yanked, so that the mark set by the last yank-handler invocation is the one that wins.

If that was the case, I'd be worried. But I don't think it is. The code is properly stripping the properties it should strip and, as far as I know, markers are not text properties. And that's a marker issue, as far as I can tell.

I'm not sure that this case will be easy to fix.

You know what? I'll make a suggestion, for your pondering. I played a lot with my code yesterday too. In particular I experimented with save-excursion. It's definitely more elegant, and apparently the right tool for the job. But I eventually stepped back from it, because this interaction with the markers was not paying off. So I returned to something similar to what the previous yank wlr function did, which is: save column position, move to the beginning of the line, insert the text, restore column position (of course, only when we had moved to bol to start with, that is, also conditioned on (and (not (and delete-selection-mode mark-active)) whole-line-or-region-local-mode)).

It sounds crude, but it is sound, because we ensure the text insertion is always of whole lines (insertion starts at bol, text ends with newline), so after inserting the text we know for sure we are exactly at the same line we started, so not only the stored column always exists, but it is the correct one. Bonus, no markers unintendedly tampered with or relied upon.

WDYT?