The `mark` in `pulsar-pulse-region` is problematic

protesilaos commented 4 days ago

Hello @shipmints. I am using the new user option pulsar-pulse-region that you introduced. I think we need to refine some aspects of its behaviour. Specifically, the mark is giving us the wrong thing in some cases. For example, start a new buffer and use a command that sets the mark like end-of-buffer. Now type some words, separated by new lines. Then do backward-kill-word and notice how the whole region pulses instead of the part where the word was deleted.

On the point of deleted regions, we cannot pulse in them because the beginning and end will be identical, so I wonder if we should do something else instead like pulsing the line when (eq beg end).

shipmints commented 4 days ago

In an empty buffer, setting mark to eob sets mark at character position 1 and it won't move until it is reset so highlighting the whole region is "correct." It may not be intuitively or visibly obvious since marks are invisible. I whipped up a little minor mode to show the mark position in the mode line. It'll make it easier to see when the mark changes and what it changes to.

  (defun my/mode-line-mark-lighter ()
    "mode-line lighter for `my/mode-line-mark-mode'."
    (let ((mark (mark)))
      (propertize (format " 🔖%d/%d" (point) (if mark mark -1))
                  'face (list
                         :weight (if (region-active-p) 'bold 'normal)
                         :height 0.75))))

  (define-minor-mode my/mode-line-mark-mode
    "Display mark position."
    :global t
    :lighter (:eval (my/mode-line-mark-lighter)))
  (my/mode-line-mark-mode)

akreisher commented 3 days ago

I believe this issue is due to https://github.com/protesilaos/pulsar/pull/19 in combination with the new region pulsing functions in #20.

Currently, we check if the region is active, and use the appropriate region-beginning/end functions to highlight it. However, with #19, we also highlight the region if it is inactive as long as the mark is set. I believe this causes the unexpected behavior here (since the mark is still at the eob and not reset by backward-kill-word). I see similar issues using kill-line.

Reverting #19 fixes the issue for me, but it also causes the region not to pulse when yanking/undoing, since the region is not active in those cases.

shipmints commented 3 days ago

@akreisher can you describe the precise behavior you'd prefer?

abougouffa commented 3 days ago

The behavior can also be observed with kill-backward-line, this command uses kill-region internally, which triggers pulsar-pulse-region with what ever the (mark) points at!

akreisher commented 3 days ago

can you describe the precise behavior you'd prefer?

I would expect calling kill-line to not highlight anything (or if anything, the current line) if the region is not active. Currently, it highlights the inactive region, which can be anything based on wherever the mark happened to be set last.

The reason we are highlighting the region in this case is that kill-line calls kill-region with a manually determined region (not the region based on the current mark). kill-region sets this-command to itself, so this triggers pulsar-pulse-region, which highlights the region based on the mark, even though transient-mark-mode is not active.

protesilaos commented 3 days ago

From: Alex Kreisher @.***> Date: Mon, 18 Nov 2024 15:16:18 -0800

can you describe the precise behavior you'd prefer?

I would expect calling kill-line to not highlight anything (or if anything, the current line) if the region is not active. Currently, it highlights the inactive region, which can be anything based on wherever the mark happened to be set last.

I keep experimenting with this on my end and I do keep getting these false positives. Basically, after any motion that sets the mark (e.g. 'scroll-down-command') you have an inactive region that gets highlighted even though you may be operating on a small part with, say, 'kill-word'.

What we need in this scenario is either to do nothing or to pulse the current line. To me, doing nothing when the region is no longer there feels more natural. It also does not interfere with writing because when you are doing it you normally follow the cursor.

But I am happy to consider alternatives. I think the idea is good in general.

-- Protesilaos Stavrou https://protesilaos.com

shipmints commented 2 days ago

I've been using region pulsing and, I guess it's a matter of taste, but I don't have an issue with pulsing the active region since that's precisely what Emacs defines as the region. I enable a different color from line pulsing so it's clear what's being pulsed.

What it seems we need to improve the aesthetics of region pulsing is a different, practical definition of region (practical being that we can trivially observe region boundaries without shenanigans like advising lots of Emacs functions and examining their arguments). I've read through portions of simple.el and nothing obvious stands out.

Perhaps one thing that could be done is (optionally?) pulse the line point is on, in the region face, and ignore the extent of the active/implied region.

Perhaps this:

(defcustom pulsar-pulse-region-mark-line-only nil
  "When non-nil, `pulsar-pulse-region' pulses the current line.
This is in effect when mark is active and on the line containing point.
When nil, `pulsar-pulse-region' will pulse the entire region from mark
to point."
  :type 'boolean
  :package-version '(pulsar . "1.2.0")
  :group 'pulsar)

(defun pulsar-pulse-region ()
  "Temporarily highlight the active region if any."
  (interactive)
  (if (region-active-p)
      (let ((beg (region-beginning))
            (end (region-end)))
        ;; snip
        (pulsar--pulse nil pulsar-region-face beg end))
    (when (mark)
      (if pulsar-pulse-region-mark-line-only
          (pulsar--pulse nil pulsar-region-face)
        (pulsar--pulse nil pulsar-region-face (mark) (point))))))

shipmints commented 2 days ago

can you describe the precise behavior you'd prefer?

I would expect calling kill-line to not highlight anything (or if anything, the current line) if the region is not active. Currently, it highlights the inactive region, which can be anything based on wherever the mark happened to be set last.

The reason we are highlighting the region in this case is that kill-line calls kill-region with a manually determined region (not the region based on the current mark). kill-region sets this-command to itself, so this triggers pulsar-pulse-region, which highlights the region based on the mark, even though transient-mark-mode is not active.

@akreisher In this case, you could remove kill-region from pulsar-pulse-region-functions and add the specific higher-level commands over which you want more control?

shipmints commented 2 days ago

@protesilaos @akreisher @abougouffa, check out this branch and see if these refinements are acceptable:

https://github.com/shipmints/pulsar/tree/region-window-change-redux

Do (setq pulsar-pulse-region-mark-line-only t) to enable the point-line only region pulse.

abougouffa commented 1 day ago

The pulsar-pulse-region-mark-line-only is working as expected.

I wonder why you are keeping the other implementation, especially as the default one? It is not generic enough. For example, when undoing. If we undo once, it can be useful (since we probably were at the same place as the mark), however, after one or two undos, the cursor can move but the mark stay at the previous place. This causes big regions being highlighted! Which is very distracting!

Take a look at this scenario:

Wayfarer_2024-11-20_221709.webm

shipmints commented 1 day ago

Per https://github.com/protesilaos/pulsar/issues/21#issuecomment-2489608495, I don't experience the same flashing. I'm keeping the other implementation, so far, because I like it.

As far as the excess flashing is concerned, perhaps it's down to differences in packages we use. Happy to help get to the bottom of this and help make pulsar work for various use cases. Perhaps we can consolidate the conversation in this thread.

akreisher commented 1 day ago

In this case, you could remove kill-region from pulsar-pulse-region-functions and add the specific higher-level commands over which you want more control?

That works (and in fact is what I've already done). However, what is the reason for highlighting the region after kill-region in the first place? As @protesilaos pointed out, if the region is killed from the buffer, we end up pulsing an empty region. I'm not sure in what cases we expect calling kill-region to pulse a meaningful region of the given buffer. It seems to me like all it does right now is cause other functions using kill-region (e.g. kill-line) to pulse the inactive region, even though the command is not actually acting on that.

As far as the excess flashing is concerned, perhaps it's down to differences in packages we use.

The issue here is not because of any additional packages. Minimum reproduction steps are:

Run emacs -Q
M-x load-file RET /path/to/pulsar.el
M-x pulsar-global-mode
In a buffer with multiple lines, set the mark somewhere, but do not activate the region (e.g. with C-SPC C-SPC)
Go to another line in the buffer and call kill-line (or kill-word, or any function which calls kill-region) and the area between where the mark was previously set and the current point is flashed (even though this is in no way related to the region that was just killed).

Happy to help get to the bottom of this and help make pulsar work for various use cases.

The problem is that kill-region only kills the region defined as the region between the mark and point when called interactively. Otherwise, it kills whatever region is passed in to it per its BEG and END arguments. Currently, pulsar highlights the correct region in the interactive case, but not in the non-interactive case when called from other functions.

protesilaos commented 1 day ago

From: shipmints @.***> Date: Wed, 20 Nov 2024 14:14:03 -0800

Per https://github.com/protesilaos/pulsar/issues/21#issuecomment-2489608495, I don't experience the same flashing. I'm keeping the other implementation, so far, because I like it.

I mentioned in another thread one option about this "liking" part. I think it is fine for everyone to have their preferences. What we then need is to make those various styles easy to opt in or out of.

I think we should have this:

Keep the original approach as the default one by setting 'pulsar-pulse-on-window-change' to nil.
Make the region pulsing a separate minor mode: 'pulsar-region-mode', 'pulsar-global-region-mode'.

All these are consistent with the principle of least surprise: existing users get what they were used to and any user will be opting in to something they are informed about.

-- Protesilaos Stavrou https://protesilaos.com

protesilaos commented 1 day ago

From: Alex Kreisher @.***> Date: Wed, 20 Nov 2024 14:44:15 -0800

In this case, you could remove kill-region from pulsar-pulse-region-functions and add the specific higher-level commands over which you want more control?

That works (and in fact is what I've already done). However, what is the reason for highlighting the region after kill-region in the first place? As @protesilaos pointed out, if the region is killed from the buffer, we end up pulsing an empty region. I'm not sure in what cases we expect calling kill-region to pulse a meaningful region of the given buffer. It seems to me like all it does right now is cause other functions using kill-region (e.g. kill-line) to pulse the inactive region, even though the command is not actually acting on that.

To me, the killed region is a case that should either not be included or should resolve in a line pulsing. I think line pulsing is closer to the spirit of this feature as, for example, 'M-x backward-kill-sexp' may move the cursor far back so it is helpful to reorient yourself.

As far as the excess flashing is concerned, perhaps it's down to differences in packages we use.

The issue here is not because of any additional packages. Minimum reproduction steps are:

Run emacs -Q

M-x load-file RET /path/to/pulsar.el

M-x pulsar-global-mode

In a buffer with multiple lines, set the mark somewhere, but do not activate the region (e.g. with C-SPC C-SPC)

Go to another line in the buffer and call kill-line (or kill-word, or any function which calls kill-region) and the area between where the mark was previously set and the current point is flashed (even though this is in no way related to the region that was just killed).

Same here. I get this false positive frequently, but especially when editing prose. For example, as I am typing this know, if I do 'scroll-up-command' and then 'kill-word' I get a huge region highlight. I do not think this is helpful.

-- Protesilaos Stavrou https://protesilaos.com

shipmints commented 7 hours ago

@akreisher @protesilaos To accommodate kill-region when it is invoked under kill-line, for example, to my branch, I've added the following test for interactive invocation. Let me know if that seems sufficient to help mitigate visual noise.

(defun pulsar-pulse-region ()
...
    (when (mark)
      (if (or (not (called-interactively-p 'interactive)) pulsar-pulse-region-mark-line-only)
          (pulsar--pulse nil pulsar-region-face)
        (pulsar--pulse nil pulsar-region-face (mark) (point))))))

abougouffa commented 5 hours ago

Hi @shipmints

I'm testing this one, it seems more acceptable. But TBH, I don't see much benefit from the pulse-region for now. The only consistent case is when we do kill-ring-save and maybe append-next-kill.

I see what you are trying to achieve by this but I think the current approach is not the right one.

The current pulsar-pulse-region is used as a generic function for undo, kill-region, kill-ring-save, ... However, each of these commands has its own semantics if we want to do it correctly. For example, if we want to flash the region changed by undo, we are not supposed to do it with region-beginning/end in post-command-hook as these values don't tell us much about the command we just run.

I think the right approach is to hook a function to after-change-functions, which receives (beginning end length) for each change. So, from these values we can get the accurate region in which the change happened, and if it is an addition (like yank), a deletion (like kill-region)... and so on.

Based on these info, we can for example flash the pasted region in yank, flash the undo region in undo, ... etc.

You can take a look at the implementation of goggle-mode, it does exactly that, and it works perfectly for all editing operations.

abougouffa commented 5 hours ago

@protesilaos @shipmints I've tried to implement this behavior. I will push my changes in a few minutes.

abougouffa commented 5 hours ago

@shipmints Please check my fork here, it is synced on top of your last changes:

https://github.com/abougouffa/pulsar/tree/refined-pulse-region

EDIT:

You need enable pulsar-pulse-on-region for it to work.

(setq pulsar-pulse-on-region t)

The logic is inspired by goggle-mode. With these changes, we will have the following behavior:

When we delete a region ➡️ flash the character under the cursor
When we copy a region ➡️ flash the copied region
When we insert (paste) a region ➡️ flash the pasted region
When we undo ➡️ flash the changed region

@protesilaos What do you think?

shipmints commented 4 hours ago

May I suggest that if goggles does what you want and you also want current pulsar behavior, just use both packages. I stayed away from reimplementing goggles and also from after-change-functions because it can be very noisy. My approach is to take a different path for these reasons. Not saying taking goggles behavior on board is in and of itself completely undesirable but why is an operative question when both exist and solve slightly different problems.

abougouffa commented 4 hours ago

May I suggest that if goggles does what you want and you also want current pulsar behavior, just use both packages.

I can live with it, but in this case, whats the point of having pulsar-pulse-region-functions? As its behavior doesn't have a real benefit if all it does is flashing a region which is inconsistent with the edits we did! I'm honestly asking because I can't see a useful use case for it!

I stayed away from reimplementing goggles and also from after-change-functions because it can be very noisy.

Yes it can, in goggles-mode, this is implemented by advising individual commands (like kill-region, yank, ...) and recording the changes while running the command. I tried to not do the same to avoid advising all the commands in pulsar-pulse-region-functions and to allow changing its dynamically much easier. In the after-change-function hook, we do record the changes only when we are in the context of one of the supported commands in pulsar-pulse-region-functions. And then after the command, we pulse the region if needed.

~The current implementation can be modified to merge pulsar--post-command-pulse-changes and pulsar--post-command-pulse (easy to do).~ (EDIT: Done in the last commit)

I don't think this adds a lot of overhead!

Also, why should I use two separate packages to do basically the same thing when just an extra hook can solve the issue in pulsar !!

shipmints commented 37 minutes ago

I'll experiment with your changes. If it's okay with you, I may adapt them into my branch.

protesilaos / pulsar

The `mark` in `pulsar-pulse-region` is problematic #22

EDIT: