Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

selecting sentence & paragraph; marking target as region for certain commands #347

Closed
jdtsmith opened this issue Aug 15, 2021 · 65 comments
Closed

Comments

@jdtsmith
Copy link

In text modes, it would be wonderful if repeated embark-act application toggled word/sentence/paragraph selection (and/or if u increased the scope in that manner).

@oantolin
Copy link
Owner

Are there some good actions for sentences and paragraphs? I guess beyond saving on the kill-ring, which is always useful, we could have killing and transposing. For paragraphs, filling, indent-rigidly.

By the way, I just noticed that the defun target in markdown mode and org mode is a little wonky. It seems to want to select an entire section including it's heading, but not always. And it's odd, because C-M-h seems to mark a paragraph...

Maybe the defun target finder should only be active in modes that derive from prog-mode.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

Maybe the defun target finder should only be active in modes that derive from prog-mode.

I think we had it like this originally?

Probably it makes sense to have special text mode target finders (sentence, paragraph) and the defun finder should only be active in prog-modes or code blocks (org, markdown, etc).

@jdtsmith
Copy link
Author

Fill, count words, cleanup blanks, all the region stuff seems interesting. If it's a short sentence you may want to consult grep for it.

BTW I'm often wishing some commands would treat the target as a (temporarily) active region, since it is/will be so easy to "select" something interesting with embark. For example, there is no "mark sentence" that I know of. Maybe pushing it too far?

@hmelman
Copy link

hmelman commented Aug 15, 2021

Maybe the defun target finder should only be active in modes that derive from prog-mode.

FWIW, the meaning of a defun in markdown-mode is deliberate.

My first thought was that if marking a defun is broken in some mode, that's the fault of the mode and embark shouldn't be altering it's functionality because of that. Then I thought that having two different configs for prog and text modes was pretty reasonable since they'd break along symbol/expression/block/defun and word/sentence/paragraph/page lines. But FWIW, expand-region defines different expansion lists for lots of modes, (see the bottom of expand-region.el). This probably isn't needed for embark (yet), but having some means for a user to do so for a mode when they've written a custom target would be good.

BTW I'm often wishing some commands would treat the target as a (temporarily) active region, since it is/will be so easy to "select" something interesting with embark. For example, there is no "mark sentence" that I know of. Maybe pushing it too far?

I was going to ask if something like this was possible. I know that some target maps bind SPC to the relevant mark command. If there was a presumed region or perhaps a generic embark-mark-target command, that would make it easy to apply various region commands to a target. This would enable an ispell-sentence when there's only an ispell-region command. Perhaps a user option to have embark-mark-target also change the current embark target to region automatically to give quick access to the region commands.

@oantolin
Copy link
Owner

For any commands you would like to treat the target as region you can add a pre-action hook that marks the target:

(cl-defun embark--mark-target (&key bounds &allow-other-keys)
  (when bounds
    (set-mark (car bounds))
    (goto-char (cdr bounds))))

(push #'embark--mark-target (alist-get 'kill-region embark-pre-action-hooks))

I'm thinking of adding embark--mark-target to Embark itself, because it seems generally useful. In the code above I configured kill-region to use it, so that you can kill any at-point target with the standard C-w keybinding. (I think I might like this even better than k for kill-sexp in the action map for s-expressions.)

@jdtsmith
Copy link
Author

Generally re-using key commands that you already know seems better than learning a new embark one for use only when embarking. For example, I usually use my binding for consult-line in preference to the embark one, and am glad that "it just works".

@oantolin
Copy link
Owner

oantolin commented Aug 15, 2021

Yeah, I started out doing that too with consult-line, but I think I do use the C l binding now. My binding for embark-act is C-; and my binding for consult-line is M-g l. I think it's just easier for me to go from C-; to no modifiers than to a M- binding. Probably if the modifiers in my key bindings for embark-act and consult-line agreed I wouldn't use the modifierless C l binding.

For example, there is no "mark sentence" that I know of.

Oh, by the way, @jdtsmith, there is a built-in mark-end-of-sentence command, which marks from point to the end of the sentence. (That's also the behavior of mark-word, I wonder why the naming scheme differs.) It has no binding by default. I use M-E (with an uppercase E). Other commands useful for prose that have no default bindings (together with the bindings I use personally): transpose-sentences (M-T), kill-paragraph (M-K), transpose-paragraphs (C-x M-t).

@oantolin
Copy link
Owner

oantolin commented Aug 15, 2021

So, which region commands is one likely to want to use on an embark target that isn't already a region? I don't want to preconfigure every possible region command to run embark--mark-target as a pre-action hook, only the most useful ones. (And of course people can do this for any other region commands they want to run on non-region targets.)

I was thinking:

  • We definitely want: kill-region, kill-ring-save, indent-region, narrow-to-region.
  • Maybe: whitespace-cleanup-region, apply-macro-to-region-lines, indent-rigidly.
  • Even less sure: write-region, append-to-file.
  • When we have sentence and paragraph targets: ispell-region, count-words-region, fill-region.

Is there any other region command you are likely to want to run on a non-region target?

@oantolin
Copy link
Owner

I decided to add all of those (except the prose ones, which I'll add later). 0de5038

@jdtsmith
Copy link
Author

Thanks! Consult-line gets a lot of use for me so I also bind to super:

   ("M-s l" . consult-line)
   ("s-l"   . consult-line)
   ("s-L" . (lambda () (interactive) ; same mode buffers line
		(consult-line-multi `(:mode ,major-mode))))
   ("M-s-L"   . consult-line-multi) ; all buffers line

We've talked about a transient frontend package that lets you consult-line(-multi) with various configured parameters.

@jdtsmith
Copy link
Author

jdtsmith commented Aug 15, 2021

Is there any other region command you are likely to want to run on a non-region target?

Will it work for other commands which eventually call the specified embark-able region commands? I have unfill-region and unfill-paragraph that get plenty of use.

@oantolin
Copy link
Owner

No, you'd directly need to (push #'embark--mark-target (alist-get 'unfill-region embark-pre-action-hooks)) and same for unfill-paragraph and any other command you want to recieve the treatment.

Which hooks get run are looked up by action name, so the command needs to directly appear as a key in the alist embark-pre-action-hooks.

@jdtsmith
Copy link
Author

Understood. Perhaps delete-backward-char and delete-backward-char-untabify should be added as they delete region if transient-mark-mode is active.

@jdtsmith
Copy link
Author

Also perhaps case conversion:

C-x C-l Convert region to lower case (downcase-region).
C-x C-u Convert region to upper case (upcase-region).

@oantolin
Copy link
Owner

We've talked about a transient frontend package that lets you consult-line(-multi) with various configured parameters.

I'm hoping to see more transient UIs pop up now that it is in core Emacs. I suspect people will abuse transient for my taste, and use it for things you don't really need it, but for some cases it is really great. The consult-*grep commands could definitely use it, as could consult-line-multi for buffer selection. I don't think I see much point to consult-line having a transient interface, but I could be missing something.

@oantolin
Copy link
Owner

Also perhaps case conversion

I thought of those, but do you really want to put an entire string or defun in all caps that often? Although maybe that's the wrong question, maybe I should be thinking instead: "if an embark user uses a case command as an action, what do they expect to happen?" And I should configure these so they just work if someone decides to try them. OK, I think I talked myself into it. :D

@oantolin
Copy link
Owner

Understood. Perhaps delete-backward-char and delete-backward-char-untabify should be added as they delete region if transient-mark-mode is active.

Oh, and those just delete without saving on the kill-ring right? That's good to have.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

The mark and kill generalization for targets with bounds is a great idea. Didn't think of this when we added highlighting.

Are some specific mark/kill commands made obsolete by the generic commands?

@jdtsmith
Copy link
Author

We've talked about a transient frontend package that lets you consult-line(-multi) with various configured parameters.

I'm hoping to see more transient UIs pop up now that it is in core Emacs. I suspect people will abuse transient for my taste, and use it for things you don't really need it, but for some cases it is really great. The consult-*grep commands could definitely use it, as could consult-line-multi for buffer selection. I don't think I see much point to consult-line having a transient interface, but I could be missing something.

Exactly, for all those buffer-selecting optional arguments. A good interface I think is "map any prefix argument into a transient", which has good mnemonics. I do hope the transient docs get improved and some rich examples start appearing; the current docs are detailed but hard to put into actual practice.

@jdtsmith jdtsmith changed the title selecting sentence & paragraph selecting sentence & paragraph; marking target as region for certain commands Aug 15, 2021
@minad
Copy link
Contributor

minad commented Aug 15, 2021

@jdtsmith I would love to see a consult-transient.el package in the consult repository, which adds transient interfaces to consult-x-multi and the consult-grep family of async commands. See also the consult wishlist and minad/consult#170 as a starting point.

@jdtsmith
Copy link
Author

jdtsmith commented Aug 15, 2021

"if an embark user uses a case command as an action, what do they expect to happen?"

Is this a good argument for just temporarily marking the region for all commands (past present and future) which conclude an embark invocation? Or is that asking for trouble?

@minad
Copy link
Contributor

minad commented Aug 15, 2021

@jdtsmith Yes, I think marking always is asking for trouble. I would prefer if Embark does not modify anything on its own without an explicit request.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

I wonder where the distinction between region and target should be now.

  • Remove all specialized mark/kill/narrow commands from the target maps given general commands with embark--mark-target.
  • Maybe even consider adding the region map under a prefix key "R" to the general map if not in the minibuffer. For every of these "R" commands the region should be marked automatically before executing the action. This goes in the same direction as the idea by @jdtsmith regarding always marking.

@oantolin
Copy link
Owner

@minad said:

Are some specific mark/kill commands made obsolete by the generic commands?

I'd say the kill-sexp command for expressions is made obsolete by kill-region. I want to do one of two things:

  • completely remove the binding of k to kill-sexp, and people can use C-w; or
  • bind k to kill-region instead, this would help for a perhaps obscure reason: the defun maps inherits from the expression map and in some programming modes not every defun is a sexp ---for example, currently k will correctly kill a python defun but not a C defun.

Basically: we should now use kill-region instead of kill-sexp as it works in more cases, and optionally we can remove the k binding and have people use C-w. I think maybe binding k to kill-region might be worth it for the indicators, but I'm sure my fingers will turn to C-w as they are used to.

As for marking commands, we currently have two: mark-sexp and mark-defun. We could replace them with a single marking command which works on all targets. That command could be bound in the expression map or even in the general map.

Or, we could remove all marking commands on the grounds that instead of marking a target and then using some command that uses the region, people should just skip the marking and use that command directly as an action.

@oantolin
Copy link
Owner

Oh, we were writing at the same time, @minad. Putting the region commands under a prefix is an interesting idea.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

The idea is basically to unify regions and targets. Marking could still be available as an action, but it will be mostly unnecessary. You can always act directly. In order to avoid too much shadowing I would use the R prefix. Only for the region target itself the region map is directly the top-level map.

@oantolin
Copy link
Owner

If we want a general mark action, I'd reuse the mark function, just for it's name. 😛

(push #'embark--mark-target (alist-get 'mark embark-pre-action-hooks))
(define-key embark-general-map " " #'mark)

@oantolin
Copy link
Owner

It might be useful to have mark as an action, for some random region command that isn't configured to use embark--mark-target as a pre-action hook.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

Yes and just to have a bridge to the outside of Embark.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

Do we need separate repeating and non-repeating mark commands? Or is the repeating one enough because people can just cancel embark-act (with RET 😄) after marking.

I thought about the exact same question and I am undecided 🤷

@oantolin
Copy link
Owner

Let's try just the repeating mark, and add a non-repeating one later if necessary. I think I'll use SPC instead of R for it.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

Yes SPC is much better. I only proposed R for the region prefix map.

@oantolin
Copy link
Owner

OK, done: b20de98

This binds SPC to mark everywhere including the minibuffer, but if that is bothersome we can special-case unbinding it there.

@oantolin
Copy link
Owner

I guess this issue got a highjacked by using targets as regions. That part seems mostly finished, and I will add sentence and paragraphs targets tonight (I have to go out right now). I do have one further question: what actions differ from sentences to paragraphs? I think they should share almost all of them, except transpose-{sentences,paragraphs}.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

Did it really get hijacked given the title "marking target as region for certain commands"? It is probably a good idea to create separate issues for separate ideas 😬

@oantolin
Copy link
Owner

The title was changed after the high-jacking! 😆

@jdtsmith
Copy link
Author

RE mixing issues: apologies, this thought sort of came out of nowhere.

I'm definitely in favor of "just act directly" for the most useful region-based commands. Similar to how I can "just act directly" and drop right into consult-line, etc. With all of this mark and repeat business, I'm actually a bit worried about an inconsistent experience: if you get used to C-w working on a target "directly", why wouldn't other such commands just work? Then you have to remember to reach for a SPC to mark them first? Would the region highlight change when you are targeting + marking (vs. just targeting)?

@jdtsmith
Copy link
Author

On the other hand, if you worry about embark "stealing the mark" without having been asked to do so, SPC makes more sense (esp. if the highlight face changes to indicate it). I guess a short user configurable list of commands which can "act directly on targets as if they were regions" might be a good compromise.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

Now we have a mix of both approaches. Some commands are available directly and some require you to mark first with spc. This makes sense since we would get too much keybinding shadowing if every region command is directly available with a top-level keybinding. Note that some users also expressed their preference to "hide" many of the general bindings. What we've got is a compromise. Either you act on the target or you press space first to switch to the region target. But region target and other targets are not automatically the same.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

I guess a short user configurable list of commands which can "act directly on targets as if they were regions" might be a good compromise.

This exists already (in an indirect form). It is the set of commands with embark--mark-target in their pre action hooks.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

One thing I should add - it probably makes sense to add embark--mark-target for every command acting on regions which is available as global binding. On top of that Embark can make more commands accessible via its region keymap.

@hmelman
Copy link

hmelman commented Aug 15, 2021

Also perhaps case conversion

I thought of those, but do you really want to put an entire string or defun in all caps that often? Although maybe that's the wrong question, maybe I should be thinking instead: "if an embark user uses a case command as an action, what do they expect to happen?" And I should configure these so they just work if someone decides to try them. OK, I think I talked myself into it. :D

In particular I could see using capitalize-region for sentences via embark when writing titles.

@hmelman
Copy link

hmelman commented Aug 15, 2021

I do have one further question: what actions differ from sentences to paragraphs? I think they should share almost all of them, except transpose-{sentences,paragraphs}.

Just to mention this, there is the obscure but sometimes useful command, repunctuate-sentences which puts two spaces at the end of sentences from point to the end of buffer. It doesn't respect the active region so it could be useful to use with a narrowed paragraph. I'm not sure if it deserves an embark (paragraph) binding, but it would certainly help me to remember it and find it given its unusual name.

@oantolin
Copy link
Owner

esp. if the highlight face changes to indicate it

We could have the highlight indicator not highlight region targets, so they get their normal face.

@minad
Copy link
Contributor

minad commented Aug 15, 2021

We could have the highlight indicator not highlight region targets, so they get their normal face.

While I see the appeal I also think it is misleading since then it is not obvious that you are embarking.

@oantolin
Copy link
Owner

oantolin commented Aug 15, 2021

While I see the appeal I also think it is misleading since then it is not obvious that you are embarking.

I doubt anyone is using just the highlighting indicator, maybe the other indicators are enough.

@oantolin oantolin reopened this Aug 15, 2021
@oantolin
Copy link
Owner

Off-topic, but I'm really enjoying putting point on the blank line after a defun and using <embark-act> i to duplicate it. (If you noticed I moved indent-pp-sexp from i to TAB a few days ago, this is why! 😉)

@oantolin
Copy link
Owner

So, @hmelman, you'd prefer we keep the defun target for text modes? Or at least for markdown-mode? (I was considering restricting it to modes derived from prog-mode.)

@oantolin
Copy link
Owner

oantolin commented Aug 15, 2021

I added the sentence and paragraph targets and actions (25ec9c3). Please let me know if any other actions would be good to bind. I made the motion and transpose actions repeatable (dragging stuff forward is super fun; dragging them backwards is, appropriately, a bit of a drag: C-- t C-- t C-- t ...).

I can't believe how quickly the default configuration is improving. This new indexed-by-actions format for the {pre,setup-post}-action hooks really encourages one to use them!

@oantolin
Copy link
Owner

oantolin commented Aug 15, 2021

I think I agree with @jdtsmith that we might as well configure lots and lots of region commands to mark the target, so that if someone uses them they aren't disappointed (the politely named "principle of least surprise"). So I added the ones easily accesible by keybinding:

  • those that have a global key binding by default,
  • those that are bound in some non-region embark keymap.

@hmelman
Copy link

hmelman commented Aug 16, 2021

So, @hmelman, you'd prefer we keep the defun target for text modes? Or at least for markdown-mode? (I was considering restricting it to modes derived from prog-mode.)

I'm not sure, I can't say I personally use defuns in markdown-mode, though I probably should. I'm not really an org user (another text-mode descendant), and I'm not sure how defuns are used in org-mode (would just it be in source code blocks?). I think, like in expand-region its more important to be able to easily configure the targets for different modes (not sure how this is currently implemented, I just see a single non-buffer-local embark-target-finders user option).

@oantolin
Copy link
Owner

not sure how this is currently implemented

A target finder returns either nil if not applicable or the target it found. So it can check for the mode (or any other condition) before even looking for the target.

For targets that are just (thing-at-point 'some-thing) I just added a macro embark-define-thingatpoint-target that you call with the thing and the list of applicable modes. For example the sentence target is defined by (embark-define-thingatpoint-target sentences text-mode help-mode Info-mode man-common) ---something like that, I may be forgetting the precise list of modes.

@hmelman
Copy link

hmelman commented Aug 16, 2021

Oh, each finder is responsible for checking if it works in the mode and all are in one global list. That wasn't the first arrangement that came to mind, but it does localize a prog-mode check for something like a defun finder. It's not as easy if you wanted to reorder finders for different modes, but maybe that's not important. Probably makes sense to just wait and see if any issue develop.

expand-region has a lot of programming mode specific finders, e.g., for python it can mark statements as well as inside and outside strings (referring to including or excluding the delimiters).

@jdtsmith
Copy link
Author

Since by default embark acts on any marked region, there's no reason not to use both expand-region and embark together. That said, it might be nice to have (optional) access to expand region capabilities after an embark is underway. In particular for reducing region and reset (so @, -, 0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants