Non-handled optional argument of `kill-region`, `copy-region-as-kill` and `kill-ring-save`

[gusbrs]

Despite the comments to be found in whole-line-or-region-base-call:

;; region is defined, so just do what should normally be done ;; just call it, but make sure to pass all of the arguments....

not all arguments are passed to kill-region, copy-region-as-kill and kill-ring-save by they respective whole-line-or-region counterparts.

All three of them receive two mandatory arguments and an optional one (beg end &optional region). Whereas wlr calls them with whole-line-or-region-call-with-region which, in turn, passes nil to both pre-args and post-args in calling whole-line-or-region-base-call.

I’m not sure of other possible consequences of this, but I found one inconvenience that results from it. The rectangle facilities in rectangle-mark-mode ("C-x SPC") do not work when whole-line-or-region is active.

In practice, in both kill-region and copy-region-as-kill (kill-ring-save calls the latter), we find:

(if region
    (funcall region-extract-function 'delete)
  (filter-buffer-substring beg end 'delete))

So these functions call, when region is active region-extract-function. whole-line-or-region does not pass the region argument to those functions so that, when called by wlr they use filter-buffer-substring instead. It turns out rectangle-mark-mode performs its job with:

(add-function :around region-extract-function
              #'rectangle--extract-region)

(to be found in rect.el)

So that, with wlr disabled, if we enable rectangle-mark-mode and use "C-w" or "M-w" we will get the rectangle copied to the kill-ring. While, if wlr is enabled, the text from (point) to (mark) will be copied instead.

I’ve tried to find a proper solution for this, but it turns out to be above my league. So I can offer the above diagnosis.

I’m currently using a dirty workaround of disabling wlr when rectangle-mark-mode gets enabled, and reenabling it when the mark gets deactivated.

(add-hook 'rectangle-mark-mode-hook #'my/wlr-suspend)
(add-hook 'deactivate-mark-hook #'my/wlr-reenable)

(defvar my/wlr-global-p nil)
(defvar my/wlr-local-p nil)

(defun my/wlr-suspend ()
  (cond (whole-line-or-region-global-mode
         (setq my/wlr-global-p t)
         (whole-line-or-region-global-mode -1))
        (whole-line-or-region-local-mode
         (setq my/wlr-local-p t)
         (whole-line-or-region-local-mode -1))))

(defun my/wlr-reenable ()
  (cond (my/wlr-global-p
         (whole-line-or-region-global-mode +1)
         (setq my/wlr-global-p nil))
        (my/wlr-local-p
         (whole-line-or-region-local-mode +1)
         (setq my/wlr-local-p nil))))

But it would be nice if whole-line-or-region could handle this properly.

[purcell]

Sorry for the slow response.

I think the tricky thing here is that Emacs 24.3 (for example) has no "region" argument for the underlying functions. Hard, then, to plot a route through these changes without committing to dropping compatibility with certain not-super-old Emacs versions.

[gusbrs]

Thank for your response.

Uhm, I see. I didn't know that. I'm not (yet) an Emacs-oldie myself, my first version was 25.3. :-)

But wouldn't it be possible to condition on emacs-version to take the argument when appropriate without dropping compatibility with those older Emacs versions? Losing this rectangle-mark-mode functionality is significant, wouldn't you say so?

Anyway, just my 2c. But I do know the value of backward compatibility, so I'll be fine with what your experience says about the case. Feel free to close as you see fit, whatever your call is.

[purcell]

Losing this rectangle-mark-mode functionality is significant, wouldn't you say so?

I've never used that functionality myself. I tend to use the C-x r keymap commands with rectangle selections, and I think those still work. But I'm interested to learn that regular C-w and M-w (should!) work too. I'd like to fix this, so I'll leave it open and have a think.

[purcell]

Ugh, now that I've reproduced the problem locally, I can't unsee it. :D

[gusbrs]

Ugh, now that I've reproduced the problem locally, I can't unsee it. :D

Yeah, no coming back from the red pill. :D

If you feel like it, until you find the time and the spirit to tackle the issue, you can use the hack above. It does work for this particular functionality. Unfortunately, it is too hacky to be packageble and probably not general enough, as we don't know if that optional argument is used anywhere else in the code base.

If, and when, you have time for this, it will be appreciated. Thank you.

[purcell]

Haha, I have a small local rewrite of the entire package which handles this nicely. I'll have to think carefully about what to do with the new code... whole-line-or-region has a lot of handling of edge cases (e.g. paste whole line when point is in the middle of a line, or cutting a line with no newline at the end of a file) so I don't want to break behaviour for people. But at the same time there's definitely opportunity for modernisation. Turns out that under the covers, all of this is quite a lot easier to implement now.

[purcell]

And the same pre-existing hackery would still be needed to handle comment-dwim etc.

[gusbrs]

Wow! That's is very nice indeed. I'm glad to hear it.

All things considered, I guess I can only react to that with:

I'm looking forward to see this reach distribution, Neo!

And, of course, thank you very much.

[purcell]

I wrote a few unit tests for the old code, but haven't got around to working on plugging in my new code yet.

[gusbrs]

Hi @purcell , might I leave a respectful ping on this one?

[purcell]

I revisited my rewrite recently and felt it needed more work, which I haven't had time for yet. Still vaguely on my radar though.

[gusbrs]

Hi @purcell , as you've seen, I've been digging the code of whole-line-or-region these last days. Sorry if I've been pestering you too much.

But, in so doing, I've grown somewhat skeptical of the attempt of the package to be so general. In particular, I'm doubtful if "commenting" and "killing/yanking" are sufficiently alike to grant being handled by a single base function.

On the other hand, I really love the killing/yanking behavior of whole-line-or-region, or at least its intended behavior. As you said somewhere, it has become indispensable to me too. I got dependent on the clutch, if you like.

So I decided to attempt to rewrite the killing/yanking behavior of whole-line-or-region with specialised functions. And I think I got pretty nice results (and it took me less than trying to fix things ;-). Of course, still "alpha".

If you think it is appropriate, or are simply interested, I'd be glad to share it, and discuss the merits of this "specialised functions" approach, if that's the case.

[purcell]

Yeah, that's basically where I ended up with my rewrite. I've got the kill/yank behaviour covered, but not stuff like comment-dwim. Like you, I also suspect that it's a mistake to be too general about this. I wonder if I need to spin that out as a separate package and evolve it separately, starting from just that basic functionality.

[gusbrs]

Yes, that's also my view on the matter after banging my head for a while. And I'm pretty satisfied with the couple of functions I cooked, just for killing/yanking I was able to use the yank-handler facilities which simplified things a lot, and allowed me not to have to replace yank. rectangle-mark-mode works, kill-read-only-ok too, and even with org-yank out of the box.

From my side, feel free to do as you see fit with this issue, and the other one about kill-read-only-ok.

[purcell]

I was able to use the yank-handler facilities which simplified things a lot

Yeah, I ended up with something like

(advice-add 'insert-for-yank :around 'wlr2-insert-for-yank)
;; Ensure the wlr2 property is removed after yanking
(push 'wlr2 yank-excluded-properties)

to provide special handling of insert based on whether a flag property was set on the yanked text.

The best thing for (most) users would be to replace the guts of whole-line-or-region.el with my new code, but I'm wary of breaking the existing comment-dwim functionality and any overrides that users have defined using the library's facilities, so it feels risky.

[gusbrs]

I'm not sure I get the whole picture of what you did, but there is actually no need to advice insert-for-yank or add a new property to yank-excluded-properties. If you pass a function to the yank-handler property, this function is used instead of insert, and the yank-handler property is already dealt with by yank.

As to what to do, I understand your position. And I empathise with your concern for backwards compatibility, of course. I think you could either add the new code as an opt-in, or, as you mentioned previously, consider the case of just keeping this as it is and start afresh with a new name. If you allow me a ludic parenthesis, you could call it "region-or-whole-line": https://youtu.be/WboggjN_G-4.

[purcell]

Thanks. I'll look into the yank-handler thing but iirc I had tried that previously and found an issue.

I think my plan will be to replace whole-line-or-region in my local config with my hacked replacement, get comment-dwim working, and then just retrofit whole-line-or-region. That's probably the best outcome overall -- bold progress!

[gusbrs]

I guess then it is a matter of time until I find out where I have screwed up. ;-)

I wish well for the package, I've been using it for a number of years already, so I hope you find the occasion to do that eventually. And of course, when I do find out where my stuff breaks, I might well be back in the fold.

Thanks again!

[purcell]

I guess then it is a matter of time until I find out where I have screwed up. ;-)

I wonder if it's the rectangle case, since presumably yank-rectangle is normally set as the yank handler there.

What would also be really cool, if you happen to find the time, would be if you could add a couple more failing test cases in a PR. That's at least feasible now. In particular, I'd like to make sure the various rectangle cases work.

[purcell]

Thanks for sharing what you've figured out too, btw.

[gusbrs]

I wonder if it's the rectangle case, since presumably yank-rectangle is normally set as the yank handler there.

I'm not sure why it would. I've only really tested with transient-mark-mode, and with rectangle-mark-mode but it works fine, because when killing a rectangle there is a region, and in this case, there is no need to change the yank-handler for wlr, so those cases don't step in each others' toes. I offer again, if you'd like to see where I got, it's on the table. I'd be glad to share.

What would also be really cool, if you happen to find the time, would be if you could add a couple more failing test cases in a PR. That's at least feasible now. In particular, I'd like to make sure the various rectangle cases work.

I'm not really acquainted with the testing structure. I just manage my own init, and never dared a package, so that's something lacking in my toolbox. So I fear I'd rank somewhere between "not very useful" to "dangerous" in this task.

[andersjohansson]

I was also recently bitten by the failure to handle rectangle-mark-mode. If you would like to push a branch with your progress on the rewrite at any time, I could help with testing. I personally don’t care that much for backwards compatibility (I currently compile emacs from master) or things beyond the basic functionality though :slightly_smiling_face:.

[purcell]

Yeah, I think I just want to figure out the comment-dwim thing and then I'd be okay pushing quite aggressive internal updates in master. Locally I've been using my rewrite and it seems pretty okay.

[purcell]

Please see my rewrite in #13 -- would love it if you could both test and give feedback.