Adding refactoring capabilities to Emacs

Adding refactoring capabilities to Emacs

[Eli Zaretskii]

I originally wrote the below in a discussion of a bug report, but it
really belongs here.  So here's a repost:

  > Okay, I'm convinced.  I'll defer this functionality to the future
  > refactoring support in Emacs, built with project.el and eglot no doubt.

  Eglot can serve as the back-end, and it isn't the only one that comes
  to mind (I sincerely hope that at least some of the simpler
  refactoring jobs will not require an LSP, but could be done using
  built-in capabilities).

  But back-end is just one part of this.  We should IMO begin by our own
  research into the UI parts of this: how does the user specify the
  requested refactoring?  Several alternatives are possible, and we
  should study them and decide what is best for Emacs.

  Would someone please step forward and work on adding refactoring to
  Emacs?

The main point of this is that it's high time Emacs had sophisticated
and flexible support for code refactoring.  Let's start working on
this!

Re: Adding refactoring capabilities to Emacs

[Eshel Yaron]

Eli Zaretskii <eliz@gnu.org> writes:

> The main point of this is that it's high time Emacs had sophisticated
> and flexible support for code refactoring.  Let's start working on
> this!

That'd be really great!  I'd love to assist, if I can.

My initial thinking is that it may be useful to consider two categories
of refactoring operations:

1. Common transformations that make sense in most languages.  These
include renaming identifiers, reordering definitions/directives (such as
import statements), extracting blocks of code as standalone functions
and probably a few more.  IMO it'd be great if Emacs had standard
commands and user options for these refactor operations across major
modes.  For some languages, it might be difficult to provide some of
these transformations even though they're clearly useful, so some major
mode might want to indicate that they do not support some of these
transformations.  For instance, renaming identifiers requires very
precise knowledge of scoping and namespacing that might not be available
to some major modes (at least not without a lot more work).

2. Transformations that only make sense for a specific set of languages.
For example, in Prolog we may want to move the unification of some term
from the head of a clause to the start of its body.  That's a useful
transformation that doesn't alter the program's semantics, but I can't
think of a corresponding transformation in functional/imperative
languages.  Perhaps for such operations Emacs could allow major modes to
provide language-specific extensions to the refactoring facility.


-- 
Best,

Eshel

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: emacs-devel@gnu.org
> Date: Sat, 19 Aug 2023 12:58:00 +0200
> 
> My initial thinking is that it may be useful to consider two categories
> of refactoring operations:
> 
> 1. Common transformations that make sense in most languages.  These
> include renaming identifiers, reordering definitions/directives (such as
> import statements), extracting blocks of code as standalone functions
> and probably a few more.  IMO it'd be great if Emacs had standard
> commands and user options for these refactor operations across major
> modes.  For some languages, it might be difficult to provide some of
> these transformations even though they're clearly useful, so some major
> mode might want to indicate that they do not support some of these
> transformations.  For instance, renaming identifiers requires very
> precise knowledge of scoping and namespacing that might not be available
> to some major modes (at least not without a lot more work).
> 
> 2. Transformations that only make sense for a specific set of languages.
> For example, in Prolog we may want to move the unification of some term
> from the head of a clause to the start of its body.  That's a useful
> transformation that doesn't alter the program's semantics, but I can't
> think of a corresponding transformation in functional/imperative
> languages.  Perhaps for such operations Emacs could allow major modes to
> provide language-specific extensions to the refactoring facility.

Yes, there's a place for both language-independent and
language-dependent refactoring.  And I think more sub-categories are
in order, because some of them can be done with relatively simple
means which don't require intimate knowledge of the language syntax,
and others will require support from the likes of tree-sitter and LSP.

It would be good to start developing these capabilities, first for the
relatively simple transformations.  Patches are welcome.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 19/08/2023 09:03, Eli Zaretskii wrote:
>    But back-end is just one part of this.  We should IMO begin by our own
>    research into the UI parts of this: how does the user specify the
>    requested refactoring?  Several alternatives are possible, and we
>    should study them and decide what is best for Emacs.

If we had a better UI for renaming than query-replace, we could plug it 
into project-find-regexp and xref-find-references' output buffers.

Where you currently can hit 'r' to switch to renaming action, it could 
instead show the proposed operation (in some fashion) and how the text 
will look "after". That would cover the regexp-replace and the 
rename-symbol refactorings. Just the basics, but still. Those could also 
be combined into separate commands (such as 
xref-find-references-and-replace).

LSP clients, with their more sophisticated operations, could use the UI 
as well, as long as it's possible to agree on the input instructions format.

And there could be some language-specific refactorings implemented in 
Lisp as well.

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> Date: Sun, 20 Aug 2023 04:19:46 +0300
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 19/08/2023 09:03, Eli Zaretskii wrote:
> >    But back-end is just one part of this.  We should IMO begin by our own
> >    research into the UI parts of this: how does the user specify the
> >    requested refactoring?  Several alternatives are possible, and we
> >    should study them and decide what is best for Emacs.
> 
> If we had a better UI for renaming than query-replace, we could plug it 
> into project-find-regexp and xref-find-references' output buffers.

I indeed think that these capabilities will need a better, novel UI,
and that developing it should be part of such an effort.

> Where you currently can hit 'r' to switch to renaming action, it could 
> instead show the proposed operation (in some fashion) and how the text 
> will look "after". That would cover the regexp-replace and the 
> rename-symbol refactorings. Just the basics, but still. Those could also 
> be combined into separate commands (such as 
> xref-find-references-and-replace).
> 
> LSP clients, with their more sophisticated operations, could use the UI 
> as well, as long as it's possible to agree on the input instructions format.
> 
> And there could be some language-specific refactorings implemented in 
> Lisp as well.
 
Yup.

Re: Adding refactoring capabilities to Emacs

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> If we had a better UI for renaming than query-replace, we could plug it 
>> into project-find-regexp and xref-find-references' output buffers.
>
> I indeed think that these capabilities will need a better, novel UI,
> and that developing it should be part of such an effort.

Just in case, leaving a link to
https://github.com/Wilfred/emacs-refactor that is an existing
implementation of some refactoring features.

It might be used as a source of ideas.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Adding refactoring capabilities to Emacs

[Yuri Khan]

On Sun, 20 Aug 2023 at 08:20, Dmitry Gutov <dmitry@gutov.dev> wrote:

> If we had a better UI for renaming than query-replace, we could plug it
> into project-find-regexp and xref-find-references' output buffers.
>
> Where you currently can hit 'r' to switch to renaming action, it could
> instead show the proposed operation (in some fashion) and how the text
> will look "after". That would cover the regexp-replace and the
> rename-symbol refactorings. Just the basics, but still. Those could also
> be combined into separate commands (such as
> xref-find-references-and-replace).

Renaming is already possible in Eglot, with the following UI: You
stand on a symbol and invoke eglot-rename. It asks you for the new
name, then does the right thing.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 20/08/2023 11:44, Yuri Khan wrote:
> On Sun, 20 Aug 2023 at 08:20, Dmitry Gutov<dmitry@gutov.dev>  wrote:
> 
>> If we had a better UI for renaming than query-replace, we could plug it
>> into project-find-regexp and xref-find-references' output buffers.
>>
>> Where you currently can hit 'r' to switch to renaming action, it could
>> instead show the proposed operation (in some fashion) and how the text
>> will look "after". That would cover the regexp-replace and the
>> rename-symbol refactorings. Just the basics, but still. Those could also
>> be combined into separate commands (such as
>> xref-find-references-and-replace).
> Renaming is already possible in Eglot, with the following UI: You
> stand on a symbol and invoke eglot-rename. It asks you for the new
> name, then does the right thing.

Indeed: we do have access to a bunch of refactorings already through 
Eglot. Some general, and some language-specific ones.

What we don't have is any advanced UI coming with that. Traditional IDEs 
(and apparently even VS Code now: 
https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp) 
have been featuring the "preview changes" feature for years. One where 
you could see which files will be affected, and even opt out from some 
of the changes.

It seems like the LSP protocol provides enough information for this to 
work (the response to the "rename" action is a list of changes to be 
performed on the client), so the UI can definitely be extended there.

Re: Adding refactoring capabilities to Emacs

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On Sun, Aug 20, 2023, 23:52 Dmitry Gutov <dmitry@gutov.dev> wrote:

> On .
>
> What we don't have is any advanced UI coming with that. Traditional IDEs
> (and apparently even VS Code now:
>
> https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp)
>
> have been featuring the "preview changes" feature for years. One where
> you could see which files will be affected, and even opt out from some
> of the changes.
>
> It seems like the LSP protocol provides enough information for this to
> work (the response to the "rename" action is a list of changes to be
> performed on the client), so the UI can definitely be extended there.
>

Philip K. has proposed a patch to Eglot that implements this in bug#60338.
It is not without problems, but was generally agreeable to me. Would you
have a look, Dmitry? We stalled while thinking about the user confirmation
model...

Anyway, are we aiming at making Eglot and LSP the only provider of
refactoring in Emacs? If we aren't, then I think we should be working on a
compatibility layer (which can be modeled after LSP's request/proposal
mechanism) even if -- for the time being -- LSP/Eglot is the only provider.
That move would inherit a lot of code from Eglot related to applying the
changes, confirming etc, meaning those details would already be solved.

João

>

[-- Attachment #2: Type: text/html, Size: 2166 bytes --]

Re: Adding refactoring capabilities to Emacs

[Dr. Arne Babenhauserheide]

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]


João Távora <joaotavora@gmail.com> writes:

> Anyway, are we aiming at making Eglot and LSP the only provider of refactoring in Emacs? If we aren't, then I think we should be working on a
> compatibility layer (which can be modeled after LSP's request/proposal mechanism) even if -- for the time being -- LSP/Eglot is the only
> provider. That move would inherit a lot of code from Eglot related to applying the changes, confirming etc, meaning those details would
> already be solved. 

I also use js2-refactor, multi-cursor (mc/mark-all-symbols-like-this),
and helm-multi-swoop-projectile + C-c C-e for refactoring.

Is Python rope-mode still a thing?
https://github.com/python-rope/ropemacs
(there’s also an lsp for rope)

lispy--replace also provides refactoring, but locally.

And as far as i can tell projectile-replace-regexp does multi-file
refactoring, too.

Of these helm-swoop already gives a kind of preview (because it uses a "do
changes then apply with C-c C-c" model).

Best wishes,
Arne
-- 
Unpolitisch sein
heißt politisch sein,
ohne es zu merken.
draketo.de

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1125 bytes --]

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 29/08/2023 13:53, João Távora wrote:
> 
> On Sun, Aug 20, 2023, 23:52 Dmitry Gutov <dmitry@gutov.dev 
> <mailto:dmitry@gutov.dev>> wrote:
> 
>     On .
> 
>     What we don't have is any advanced UI coming with that. Traditional
>     IDEs
>     (and apparently even VS Code now:
>     https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp <https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp>)
>     have been featuring the "preview changes" feature for years. One where
>     you could see which files will be affected, and even opt out from some
>     of the changes.
> 
>     It seems like the LSP protocol provides enough information for this to
>     work (the response to the "rename" action is a list of changes to be
>     performed on the client), so the UI can definitely be extended there.
> 
> 
> Philip K. has proposed a patch to Eglot that implements this in 
> bug#60338.  It is not without problems, but was generally agreeable to 
> me. Would you have a look, Dmitry? We stalled while thinking about the 
> user confirmation model...

Hmmm. Some screenshots would go a long way, I haven't tried the patch 
yet, but from what I can tell from the description, it's a pretty 
power-user-ish approach to UI.

Similar to what we ended up doing with checkin-patch in VC -- powerful, 
but no very obvious to a non-pro user in how it can be operated. It is 
surely a good addition to Eglot, but the refactoring interface I was 
thinking of would have been more graphical (very vaguely in the style of 
Xref), looking a little closer to the VS Code screenshot I posted.

But those are just my expectations -- opinions welcome.

> Anyway, are we aiming at making Eglot and LSP the only provider of 
> refactoring in Emacs? If we aren't, then I think we should be working on 
> a compatibility layer (which can be modeled after LSP's request/proposal 
> mechanism) even if -- for the time being -- LSP/Eglot is the only 
> provider. That move would inherit a lot of code from Eglot related to 
> applying the changes, confirming etc, meaning those details would 
> already be solved.

We have refactorings in various places: as Arne mentioned, there are 
some established third-party packages, of different degrees of 
popularity. And from built-in ones we also have, at the very least, some 
commands which present long lists of search results with Xref interface 
(some plugged into by Eglot when it's enabled, and some not), those 
could also use a better interface for global replacements.

Regarding compatibility layer, I'm not sure I see a request-response 
mechanism here. I'd expect to see something synchronous, e.g. a call

   (refact-start CHANGES)

where CHANGES are a list of changes in some pre-determined format (with 
file names and buffer/byte positions or lines and columns). Then the UI 
takes over, shows the user the proposed changes, allows paging through 
them, maybe disabling certain ones (for all changes in a file, or all 
files in certain dir, or individual changes too), and applying in bulk 
after a confirmation.

Maybe a callback at the end could be useful, though, e.g. to update any 
views that need to be updated. Could always be added later.

And here's a general question to such UIs: what happens if one of the 
changes (in the middle of the list) becomes un-apply-able. At which step 
the user finds out about this. I don't have this solved very well in 
xref-query-replace-in-results, but an attempt was made.

Re: Adding refactoring capabilities to Emacs

[Stefan Kangas]

Dmitry Gutov <dmitry@gutov.dev> writes:

> Similar to what we ended up doing with checkin-patch in VC -- powerful,
> but no very obvious to a non-pro user in how it can be operated. It is
> surely a good addition to Eglot, but the refactoring interface I was
> thinking of would have been more graphical (very vaguely in the style of
> Xref), looking a little closer to the VS Code screenshot I posted.

Do we need to choose between them, or could we support both?

IOW, could we have a nice clickable display that also comes with the
fast commands and well-thought-out key bindings?

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 30/08/2023 21:46, Stefan Kangas wrote:
> Dmitry Gutov<dmitry@gutov.dev>  writes:
> 
>> Similar to what we ended up doing with checkin-patch in VC -- powerful,
>> but no very obvious to a non-pro user in how it can be operated. It is
>> surely a good addition to Eglot, but the refactoring interface I was
>> thinking of would have been more graphical (very vaguely in the style of
>> Xref), looking a little closer to the VS Code screenshot I posted.
> Do we need to choose between them, or could we support both?
> 
> IOW, could we have a nice clickable display that also comes with the
> fast commands and well-thought-out key bindings?

Naturally, we could have a customization point that supports several 
displays (like xref-show-xrefs-function), but in this case as well any 
new display would have to reimplement almost all functionality (I guess 
that's unavoidable), and the format of the input data (CHANGES) need to 
be usable for such different displays.

I can roughly understand how to produce a diff from a list ((FILE 
START-POS END-POS NEW-TEXT) ...), but it seems more difficult to do the 
reverse. Though also possible, I suppose.

Re: Adding refactoring capabilities to Emacs

[João Távora]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 29/08/2023 13:53, João Távora wrote:
>> On Sun, Aug 20, 2023, 23:52 Dmitry Gutov <dmitry@gutov.dev
>> <mailto:dmitry@gutov.dev>> wrote:
>>     On .
>>     What we don't have is any advanced UI coming with
>> that. Traditional
>>     IDEs
>>     (and apparently even VS Code now:
>>     https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp <https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp>)
>>     have been featuring the "preview changes" feature for years. One where
>>     you could see which files will be affected, and even opt out from some
>>     of the changes.
>>     It seems like the LSP protocol provides enough information for
>> this to
>>     work (the response to the "rename" action is a list of changes to be
>>     performed on the client), so the UI can definitely be extended there.
>> Philip K. has proposed a patch to Eglot that implements this in
>> bug#60338.  It is not without problems, but was generally agreeable
>> to me. Would you have a look, Dmitry? We stalled while thinking
>> about the user confirmation model...
>
> Hmmm. Some screenshots would go a long way,

Just imagine a diff like *vc-diff*.

> I haven't tried the patch yet, but from what I can tell from the
> description, it's a pretty power-user-ish approach to UI.

IMO showing a diff to a user such a is not a power-user-ish thing.  Or
rather, it might be, but I don't see any better way to show potentially
complex code changes to users.

> Similar to what we ended up doing with checkin-patch in VC --
> powerful, but no very obvious to a non-pro user in how it can be
> operated. It is surely a good addition to Eglot, but the refactoring
> interface I was thinking of would have been more graphical (very
> vaguely in the style of Xref), looking a little closer to the VS Code
> screenshot I posted.

I don't know how VSCode does it, but refactorings -- at least the kind
provided by LSP servers -- aren't in general as trivial as simple
renamings.  They are arbitrary sets of changes to code, like moving
whole functions from one file to another, adding new sections or new
files, removing sections or whole files, etc, etc.

In fact, they are just like patches.

And I don't know any better way today to display patches to users other
than diff.

> Regarding compatibility layer, I'm not sure I see a request-response
> mechanism here. I'd expect to see something synchronous, e.g. a call

Synchronous is doable (Eglot does it, and the user shouldn't be doing
changes to the code while the refactoring backend is computing the
changes).  What I meant by request-proposal is that there is this
sequence

1. a request by the user selecting one of multiple offered
   refactorings.

2. a synchronous response proposing changes

3. The decision by the user to apply some or all of these proposed
   changes.  This decision might be taken in advance in step 1, if the
   user trusts the refactoring engine enough.

Note: Files are not saved automatically, because this is a different
      concern.

João

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 30/08/2023 23:37, João Távora wrote:
> Dmitry Gutov <dmitry@gutov.dev> writes:
> 
>> On 29/08/2023 13:53, João Távora wrote:
>>> On Sun, Aug 20, 2023, 23:52 Dmitry Gutov <dmitry@gutov.dev
>>> <mailto:dmitry@gutov.dev>> wrote:
>>>      On .
>>>      What we don't have is any advanced UI coming with
>>> that. Traditional
>>>      IDEs
>>>      (and apparently even VS Code now:
>>>      https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp <https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp>)
>>>      have been featuring the "preview changes" feature for years. One where
>>>      you could see which files will be affected, and even opt out from some
>>>      of the changes.
>>>      It seems like the LSP protocol provides enough information for
>>> this to
>>>      work (the response to the "rename" action is a list of changes to be
>>>      performed on the client), so the UI can definitely be extended there.
>>> Philip K. has proposed a patch to Eglot that implements this in
>>> bug#60338.  It is not without problems, but was generally agreeable
>>> to me. Would you have a look, Dmitry? We stalled while thinking
>>> about the user confirmation model...
>>
>> Hmmm. Some screenshots would go a long way,
> 
> Just imagine a diff like *vc-diff*.

Sure.

>> I haven't tried the patch yet, but from what I can tell from the
>> description, it's a pretty power-user-ish approach to UI.
> 
> IMO showing a diff to a user such a is not a power-user-ish thing.  Or
> rather, it might be, but I don't see any better way to show potentially
> complex code changes to users.
> 
>> Similar to what we ended up doing with checkin-patch in VC --
>> powerful, but no very obvious to a non-pro user in how it can be
>> operated. It is surely a good addition to Eglot, but the refactoring
>> interface I was thinking of would have been more graphical (very
>> vaguely in the style of Xref), looking a little closer to the VS Code
>> screenshot I posted.
> 
> I don't know how VSCode does it, but refactorings -- at least the kind
> provided by LSP servers -- aren't in general as trivial as simple
> renamings.  They are arbitrary sets of changes to code, like moving
> whole functions from one file to another, adding new sections or new
> files, removing sections or whole files, etc, etc.

I previously posted a link to a screenshot from VS Code, here it is 
again: 
https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp

Indeed, refactorings could be very arbitrary changes, though most of the 
time they are classified in certain types where each changes has some 
semantic meaning where the IDE previews knows how to render it (be it a 
renaming of a file, or of a function, or extracting a variable, etc). 
It's also helpful to show two related changes that are intermixed in the 
diff (e.g. when renaming a class also renames the file it's defined in).

It might be useful to experiment with different changes (simpler and 
more complex) in VS Code and see what kind of previews it managed to 
show for them, naturally based on the data format returned by LSP.

> In fact, they are just like patches.
> 
> And I don't know any better way today to display patches to users other
> than diff.
> 
>> Regarding compatibility layer, I'm not sure I see a request-response
>> mechanism here. I'd expect to see something synchronous, e.g. a call
> 
> Synchronous is doable (Eglot does it, and the user shouldn't be doing
> changes to the code while the refactoring backend is computing the
> changes).

It's possible that the backend malfunctions, or e.g. auto-revert-mode 
applies some changes while the user is still considering the changeset.

> What I meant by request-proposal is that there is this
> sequence
> 
> 1. a request by the user selecting one of multiple offered
>     refactorings.
> 
> 2. a synchronous response proposing changes
> 
> 3. The decision by the user to apply some or all of these proposed
>     changes.  This decision might be taken in advance in step 1, if the
>     user trusts the refactoring engine enough.
> 
> Note: Files are not saved automatically, because this is a different
>        concern.

"Not saving files automatically" is a power-user approach too.

We have that in query-replace and naturally in 
xref-query-replace-in-results too, but having a large refactoring across 
many files end up in a not-synced-to-disk condition is a complication. 
Not everyone knows about save-some-buffers and auto-revert-mode.

Re: Adding refactoring capabilities to Emacs

[Stefan Kangas]

Dmitry Gutov <dmitry@gutov.dev> writes:

> I previously posted a link to a screenshot from VS Code, here it is
> again:
> https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp

So fundamentally, it is a one-line diff along the lines of "git diff
--word-diff", plus some folding, and check-boxes to select which diff
chunks to apply.  Or is there more to it?

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 31/08/2023 01:01, Stefan Kangas wrote:
> Dmitry Gutov<dmitry@gutov.dev>  writes:
> 
>> I previously posted a link to a screenshot from VS Code, here it is
>> again:
>> https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp
> So fundamentally, it is a one-line diff along the lines of "git diff
> --word-diff", plus some folding, and check-boxes to select which diff
> chunks to apply.  Or is there more to it?

I don't know, I haven't tried this feature in VS Code yet. I'm not 
really using that editor.

It was actually a surprise for me that they do have some refactoring 
preview, and I think it'll be a good idea to explore its limitations 
experimentally.

Re: Adding refactoring capabilities to Emacs

[Rudolf Schlatte]

Stefan Kangas <stefankangas@gmail.com> writes:

> Dmitry Gutov <dmitry@gutov.dev> writes:
>
>> I previously posted a link to a screenshot from VS Code, here it is
>> again:
>> https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp
>
> So fundamentally, it is a one-line diff along the lines of "git diff
> --word-diff", plus some folding, and check-boxes to select which diff
> chunks to apply.  Or is there more to it?

This sounds very similar to magit's staging interface to me, which is a
nice emacs-y way to pick and choose from changes files or individual
changes within files.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Mon, Sep 4, 2023 at 11:48 AM Rudolf Schlatte <rudi@constantly.at> wrote:
>
> Stefan Kangas <stefankangas@gmail.com> writes:
>
> > Dmitry Gutov <dmitry@gutov.dev> writes:
> >
> >> I previously posted a link to a screenshot from VS Code, here it is
> >> again:
> >> https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp
> >
> > So fundamentally, it is a one-line diff along the lines of "git diff
> > --word-diff", plus some folding, and check-boxes to select which diff
> > chunks to apply.  Or is there more to it?
>
> This sounds very similar to magit's staging interface to me, which is a
> nice emacs-y way to pick and choose from changes files or individual
> changes within files.

Can you show a screenshot of that interface or give a recipe
to try it out?  Is it very different from regular "diff-mode"?
I'm not a Magit user but I feel some of its UI is indeed
good, and maybe they could be reused without an explicit dependency
on broader Magit. (For example I'm still using a pretty good
version of git-rebase-mode.el before it was integrated into
Magit).

João

Re: Adding refactoring capabilities to Emacs

[Rudolf Schlatte]

João Távora <joaotavora@gmail.com> writes:

> On Mon, Sep 4, 2023 at 11:48 AM Rudolf Schlatte <rudi@constantly.at> wrote:
>>
>> Stefan Kangas <stefankangas@gmail.com> writes:
>>
>> > Dmitry Gutov <dmitry@gutov.dev> writes:
>> >
>> >> I previously posted a link to a screenshot from VS Code, here it is
>> >> again:
>> >> https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp
>> >
>> > So fundamentally, it is a one-line diff along the lines of "git diff
>> > --word-diff", plus some folding, and check-boxes to select which diff
>> > chunks to apply.  Or is there more to it?
>>
>> This sounds very similar to magit's staging interface to me, which is a
>> nice emacs-y way to pick and choose from changes files or individual
>> changes within files.
>
> Can you show a screenshot of that interface or give a recipe
> to try it out?  Is it very different from regular "diff-mode"?

Sure--it's easy to try if you have a git repository checked out locally
that contains uncommitted changes.  Here's how to navigate that part of
magit:

- Open the repository in magit (`M-x magit-status`)

- Depending on the local state of the repository, you will see multiple
  sections in the resulting buffer ("Untracked files", "Stashes",
  "Recent commits", etc.)  The section we are interested in is called
  "Unstaged Changes".

- You can use Tab to fold and unfold any of magit's sections, like in
  outline-mode.  Also, M-1 .. M-4 fold or unfold all sections in the
  magit buffer to the specified level, and the keys 1 .. 4 fold/unfold
  the current subtree to the specified level; it's easier to try this
  out than to explain it succinctly :-)

- In "Unstaged chages", you will see the list of files that contain
  changes.  (Note that this list does not contain files whose buffers
  contain unsaved changes, only on-disk state; a refactoring UI might
  want to include modified unsaved buffers here.)

- You can use `magit-stage` (bound to "s") to stage ("accept") all
  changes in the file that point is on.  The file will vanish from
  "Unstaged changes" and appear in a section "Staged changes".  It can
  be moved back via `magit-unstage` (bound to "u") with point on the
  staged file.

- You can also use `magit-stage` with point on the "Unstaged changes"
  section header to stage all changes in one go.

- You can use TAB (`magit-section-toggle`) to unfold the file (or use
  the "4" key); this gives you a diff view with all unstaged changes.
  Move point to a hunk and use "s" to stage that individual hunk.  You
  can observe the file will now appear both in the "Unstaged changes"
  and "Staged changes" sections, with the entries showing the staged vs
  committed / unstaged vs staged diffs, respectively.  You can unstage
  one hunk of a diff by expanding a file in the "Staged changes" section
  and pressing "u" with point in the hunk.

- In the diff view, you can mark part of the diff and stage only that
  part (pressing "s" when the region is active stages only the region).
  The same goes for unstaging part of a diff.

  ... this last feature is where people who do not use Emacs become very
  motivated to install and learn it, since this detailed control over
  what source changes get committed is seemingly not available in other
  tools.  I believe a similar level of control would be beneficial for
  source changes proposed by a refactoring tool, which is why I brought
  up magit.

I hope that helped a bit; I'll be happy to explain more of course.

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> Date: Thu, 31 Aug 2023 00:49:53 +0300
> Cc: Yuri Khan <yuri.v.khan@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>  emacs-devel <emacs-devel@gnu.org>, "Philip K." <philipk@posteo.net>
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> > I don't know how VSCode does it, but refactorings -- at least the kind
> > provided by LSP servers -- aren't in general as trivial as simple
> > renamings.  They are arbitrary sets of changes to code, like moving
> > whole functions from one file to another, adding new sections or new
> > files, removing sections or whole files, etc, etc.
> 
> I previously posted a link to a screenshot from VS Code, here it is 
> again: 
> https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp

Having never used VSCode, I have difficulty understanding what I see
there.  Could you (or someone else) please post a detailed description
of what is shown there and what kind of refactoring is going on?  TIA.

Re: Adding refactoring capabilities to Emacs

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

On Thu, Aug 31, 2023, 06:03 Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Thu, 31 Aug 2023 00:49:53 +0300
> > Cc: Yuri Khan <yuri.v.khan@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
> >  emacs-devel <emacs-devel@gnu.org>, "Philip K." <philipk@posteo.net>
> > From: Dmitry Gutov <dmitry@gutov.dev>
> >
> > > I don't know how VSCode does it, but refactorings -- at least the kind
> > > provided by LSP servers -- aren't in general as trivial as simple
> > > renamings.  They are arbitrary sets of changes to code, like moving
> > > whole functions from one file to another, adding new sections or new
> > > files, removing sections or whole files, etc, etc.
> >
> > I previously posted a link to a screenshot from VS Code, here it is
> > again:
> >
> https://bobbyhadz.com/images/blog/rename-variable-vscode/refactor-preview.webp
>
> Having never used VSCode, I have difficulty understanding what I see
> there.  Could you (or someone else) please post a detailed description
> of what is shown there and what kind of refactoring is going on?  TIA.
>

I had seen this screenshot already. I think it is a very simple rename. If
like to see how it handles clangd's move function to out-of-line action,
for example.

João

>

[-- Attachment #2: Type: text/html, Size: 2405 bytes --]

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 31/08/2023 11:02, João Távora wrote:
> I had seen this screenshot already. I think it is a very simple rename. 
> If like to see how it handles clangd's move function to out-of-line 
> action, for example.

Do you mean what's also commonly referred to as "Extract function"? Have 
you seen it in practice?

I've tried to find a pieces of code in several C/C++ projects where 
clangd would offer such action, but no such luck. Only "extract 
variable" in some places. And renames, of course.

"Extract variable" doesn't seem to offer any preview either, opting for 
a more snippet-like interactive UI where you're typing the name and it's 
updated in both places on the fly. Maybe wherever "Extract function" 
works (newer clangd? I have 15.0.7 installed), it would do the same.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 04/09/2023 18:45, Dmitry Gutov wrote:
> Maybe wherever "Extract function" works (newer clangd? I have 15.0.7 
> installed), it would do the same.

Apparently not quite (there is a "Refactor with Preview..." action on 
the video).

I found this gif with "Extract function": 
https://i.stack.imgur.com/YLFvw.gif

I does look similar to an one-line diff, but lines are not truncated on 
newline (multiple lines are folded into one instead with a special char 
denoting newline, as long as they fit into the available width).

There is also something that implies a difference but is not: the top 
node is not a file name, but I'm guessing that's only when the tab 
(buffer?) is not saved to a file yet.

Re: Adding refactoring capabilities to Emacs

[Juri Linkov]

>> Note: Files are not saved automatically, because this is a different
>>        concern.
>
> "Not saving files automatically" is a power-user approach too.
>
> We have that in query-replace and naturally in
> xref-query-replace-in-results too, but having a large refactoring across
> many files end up in a not-synced-to-disk condition is a complication. Not
> everyone knows about save-some-buffers and auto-revert-mode.

This is a real problem.  Diff mode requires typing a long sequence of
'C-c C-a C-c C-a C-c C-a C-c C-a ...' to apply the whole diff.

A possible solution would be to support a region by 'C-c C-a'.
Then it will be possible to activate the region on the whole buffer
or on a few hunks, and type a single 'C-c C-a'.

Another problem that the changes are not saved automatically
on multiple files.  This limitation forces to use tricks like
'M-| git apply RET' after selecting the whole diff buffer with 'C-x h'
or a region of the diff buffer.  A "poor man's auto-save".

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   >> Note: Files are not saved automatically, because this is a different
   >>        concern.
   >
   > "Not saving files automatically" is a power-user approach too.
   >
   > We have that in query-replace and naturally in
   > xref-query-replace-in-results too, but having a large refactoring across
   > many files end up in a not-synced-to-disk condition is a complication. Not
   > everyone knows about save-some-buffers and auto-revert-mode.

   This is a real problem.  Diff mode requires typing a long sequence of
   'C-c C-a C-c C-a C-c C-a C-c C-a ...' to apply the whole diff.

Can't that be solved using something like,

(goto-char (point-min))
(while (not (eobp))
  (diff-apply-hunk))

Or some such and have it bound to something sensible, maybe collecting
the files it touched for a later save.

   A possible solution would be to support a region by 'C-c C-a'.

A region might not be a valid thing to apply -- it is better to work
with hunks.

   Then it will be possible to activate the region on the whole buffer
   or on a few hunks, and type a single 'C-c C-a'.

   Another problem that the changes are not saved automatically
   on multiple files.  This limitation forces to use tricks like
   'M-| git apply RET' after selecting the whole diff buffer with 'C-x h'
   or a region of the diff buffer.  A "poor man's auto-save".

Re: Adding refactoring capabilities to Emacs

[Juri Linkov]

> Can't that be solved using something like,
>
> (goto-char (point-min))
> (while (not (eobp))
>   (diff-apply-hunk))

It's basically what Philip proposed in bug#60338.

> Or some such and have it bound to something sensible, maybe collecting
> the files it touched for a later save.

Good idea, then with a prefix argument e.g. 'C-u C-c C-A' could save
the buffers after applying the entire patch.

>    A possible solution would be to support a region by 'C-c C-a'.
>
> A region might not be a valid thing to apply -- it is better to work
> with hunks.

It's possible to detect hunk boundaries that start/end inside the selected region.

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   > Or some such and have it bound to something sensible, maybe collecting
   > the files it touched for a later save.

   Good idea, then with a prefix argument e.g. 'C-u C-c C-A' could save
   the buffers after applying the entire patch.

That doesn't rhyme with the behaviour of C-c C-a which is to reverse
the hunk.  It would make more sense to provide a "Save all files
touched by diff-mode" key, that lists the files, and you can answer
yes or now.  This could then also be used when applying hunks in a
singular fashion.

   >    A possible solution would be to support a region by 'C-c C-a'.
   >
   > A region might not be a valid thing to apply -- it is better to work
   > with hunks.

   It's possible to detect hunk boundaries that start/end inside the
   selected region.

Too much DWIM for my taste ... and might have weird behaviour.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 04/09/2023 20:23, Juri Linkov wrote:
> A possible solution would be to support a region by 'C-c C-a'.
> Then it will be possible to activate the region on the whole buffer
> or on a few hunks, and type a single 'C-c C-a'.

Another problem is trying to do an atomic revert if a large diff, across 
multiple files, fails to apply somewhere in the middle.

> Another problem that the changes are not saved automatically
> on multiple files.  This limitation forces to use tricks like
> 'M-| git apply RET' after selecting the whole diff buffer with 'C-x h'
> or a region of the diff buffer.  A "poor man's auto-save".

This approach might help with the above, though.

Re: Adding refactoring capabilities to Emacs

[Juri Linkov]

>> A possible solution would be to support a region by 'C-c C-a'.
>> Then it will be possible to activate the region on the whole buffer
>> or on a few hunks, and type a single 'C-c C-a'.
>
> Another problem is trying to do an atomic revert if a large diff, across
> multiple files, fails to apply somewhere in the middle.
>
>> Another problem that the changes are not saved automatically
>> on multiple files.  This limitation forces to use tricks like
>> 'M-| git apply RET' after selecting the whole diff buffer with 'C-x h'
>> or a region of the diff buffer.  A "poor man's auto-save".
>
> This approach might help with the above, though.

Indeed, and an atomic revert in case of no errors (just when the
user changes the mind) is to do 'C-c C-r' ('diff-reverse-direction'),
and then 'M-| git apply RET' again.

But the drawback of this approach is that visited files will be out of sync
that requires reverting with losing their content.

Re: Adding refactoring capabilities to Emacs

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>>> Note: Files are not saved automatically, because this is a different
>>>        concern.
>>
>> "Not saving files automatically" is a power-user approach too.
>>
>> We have that in query-replace and naturally in
>> xref-query-replace-in-results too, but having a large refactoring across
>> many files end up in a not-synced-to-disk condition is a complication. Not
>> everyone knows about save-some-buffers and auto-revert-mode.
>
> This is a real problem.  Diff mode requires typing a long sequence of
> 'C-c C-a C-c C-a C-c C-a C-c C-a ...' to apply the whole diff.

I had proposed adding a command to diff-mode, that would apply the
entire buffer.  It appears that was not applied.

>
> A possible solution would be to support a region by 'C-c C-a'.
> Then it will be possible to activate the region on the whole buffer
> or on a few hunks, and type a single 'C-c C-a'.
>
> Another problem that the changes are not saved automatically
> on multiple files.  This limitation forces to use tricks like
> 'M-| git apply RET' after selecting the whole diff buffer with 'C-x h'
> or a region of the diff buffer.  A "poor man's auto-save".

Re: Adding refactoring capabilities to Emacs

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

> If we had a better UI for renaming than query-replace, we could plug it
> into project-find-regexp and xref-find-references' output buffers.

Thanks for mentioning query-replace.  I wished query-replace could do
something like what 'd' does for previewing changes before saving
the buffer with 'C-x s':

```
    (?d ,(lambda (buf)
           (if (null (buffer-file-name buf))
               (message "Not applicable: no file")
             (require 'diff)            ;for diff-no-select.
             (let ((diffbuf (diff-no-select (buffer-file-name buf) buf
                                            nil 'noasync)))
```

And now Eglot does this for previewing renaming.

So below is a command that does the same for replace-string.
Instead of boringly typing a long sequence of y y y y y y y ...
to check if all replacements are correct, now it will be possible
to preview the diff buffer that will take much less time:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: replace-string-as-diff.patch --]
[-- Type: text/x-diff, Size: 3013 bytes --]

diff --git a/lisp/replace.el b/lisp/replace.el
index eeac734f3bd..a5704e6966f 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -691,6 +697,28 @@ replace-string
 	   (use-region-noncontiguous-p))))
   (perform-replace from-string to-string nil nil delimited nil nil start end backward region-noncontiguous-p))
 
+(defun replace-string-as-diff (from-string to-string &optional delimited start end backward region-noncontiguous-p)
+  (interactive (apply (cadr (interactive-form 'replace-string))))
+  (require 'diff)
+  (let ((file-name buffer-file-name)
+        (orig-buffer (current-buffer))
+        (diff-buffer (get-buffer-create "*replace-diff*")))
+    (with-current-buffer diff-buffer
+      (buffer-disable-undo (current-buffer))
+      (let ((inhibit-read-only t))
+	(erase-buffer))
+      (diff-mode))
+    (with-temp-buffer
+      (insert-buffer-substring orig-buffer)
+      (goto-char (point-min))
+      (perform-replace from-string to-string nil nil delimited nil nil start end backward region-noncontiguous-p)
+      (diff-no-select orig-buffer (current-buffer) nil t diff-buffer (concat file-name "~") file-name))
+    (with-current-buffer diff-buffer
+      (setq-local buffer-read-only t)
+      (buffer-enable-undo (current-buffer))
+      (font-lock-ensure))
+    (pop-to-buffer diff-buffer)))
+
 (defun replace-regexp (regexp to-string &optional delimited start end backward region-noncontiguous-p)
   "Replace things after point matching REGEXP with TO-STRING.
 Preserve case in each match if `case-replace' and `case-fold-search'
diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index a411d98da31..a682b88976f 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -149,7 +149,7 @@ diff-check-labels
 		    (call-process diff-command nil t nil "--help"))
 	      (if (search-backward "--label" nil t) t))))))
 
-(defun diff-no-select (old new &optional switches no-async buf)
+(defun diff-no-select (old new &optional switches no-async buf label-old label-new)
   ;; Noninteractive helper for creating and reverting diff buffers
   "Compare the OLD and NEW file/buffer.
 If the optional SWITCHES is nil, the switches specified in the
@@ -180,11 +180,13 @@ diff-no-select
                                   (and (or old-alt new-alt)
 				       (eq diff-use-labels t)
 				       (list "--label"
-					     (if (stringp old) old
-					       (prin1-to-string old))
+                                             (cond ((stringp old) old)
+                                                   ((stringp label-old) label-old)
+                                                   ((prin1-to-string old)))
 					     "--label"
-					     (if (stringp new) new
-					       (prin1-to-string new))))
+					     (cond ((stringp new) new)
+                                                   ((stringp label-new) label-new)
+					           ((prin1-to-string new)))))
                                   (list (or old-alt old)
                                         (or new-alt new)))))
 		     " "))

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 04/09/2023 20:15, Juri Linkov wrote:
> So below is a command that does the same for replace-string.
> Instead of boringly typing a long sequence of y y y y y y y ...
> to check if all replacements are correct, now it will be possible
> to preview the diff buffer that will take much less time:

A diff buffer for all the replacements combined? That's pretty cool.

Re: Adding refactoring capabilities to Emacs

[Alexander Adolf]

[-- Attachment #1.1: Type: text/plain, Size: 687 bytes --]


> On 4. Sep 2023, at 20:03, Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
> On 04/09/2023 20:15, Juri Linkov wrote:
>> So below is a command that does the same for replace-string.
>> Instead of boringly typing a long sequence of y y y y y y y ...
>> to check if all replacements are correct, now it will be possible
>> to preview the diff buffer that will take much less time:
> 
> A diff buffer for all the replacements combined? That's pretty cool.
> 

That’d be just like  https://github.com/benma/visual-regexp.el and that’s why I’m using that. OTOH it’s for string replacement only; no code refactoring. But perhaps the UI could be used da source of inspiration?

[-- Attachment #1.2: Type: text/html, Size: 1255 bytes --]

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 1944 bytes --]

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 05/09/2023 16:56, Alexander Adolf wrote:
> That’d be just like https://github.com/benma/visual-regexp.el 
> <https://github.com/benma/visual-regexp.el> and that’s why I’m using 
> that. OTOH it’s for string replacement only; no code refactoring. But 
> perhaps the UI could be used da source of inspiration?

I don't know. One issue is that for a multi-file refactoring you would 
want to see all the changes in one place.

OTOH, if we were talking about a more compact diff, 'git diff' has a 
thing called "word diff" which doesn't take up two lines for one change 
(the modification is shown in-line).

Re: Adding refactoring capabilities to Emacs

[Alexander Adolf]

Dmitry Gutov <dmitry@gutov.dev> writes:

> [...]
> I don't know. One issue is that for a multi-file refactoring you would 
> want to see all the changes in one place.

Yes, one would want that indeed.

It would be similar to an indirect buffer, but based on a list of
(buffer . (region-beg region-end)) tuples.

> OTOH, if we were talking about a more compact diff, 'git diff' has a 
> thing called "word diff" which doesn't take up two lines for one change 
> (the modification is shown in-line).

That's the reason I name-dropped visual-regexp; the change is shown
in-line there, too.


  --alexander

Re: Adding refactoring capabilities to Emacs

[sbaugh]

Eli Zaretskii <eliz@gnu.org> writes:
> I originally wrote the below in a discussion of a bug report, but it
> really belongs here.  So here's a repost:
>
>   > Okay, I'm convinced.  I'll defer this functionality to the future
>   > refactoring support in Emacs, built with project.el and eglot no doubt.
>
>   Eglot can serve as the back-end, and it isn't the only one that comes
>   to mind (I sincerely hope that at least some of the simpler
>   refactoring jobs will not require an LSP, but could be done using
>   built-in capabilities).
>
>   But back-end is just one part of this.  We should IMO begin by our own
>   research into the UI parts of this: how does the user specify the
>   requested refactoring?  Several alternatives are possible, and we
>   should study them and decide what is best for Emacs.
>
>   Would someone please step forward and work on adding refactoring to
>   Emacs?
>
> The main point of this is that it's high time Emacs had sophisticated
> and flexible support for code refactoring.  Let's start working on
> this!

I thought the UI in
https://www.masteringemacs.org/article/combobulate-editing-searching-new-query-builder
was quite interesting.

Something heavily based on tree-sitter is interesting because such
queries could easily be stored and used programmatically outside Emacs,
you simply need to link against the tree-sitter library.  That's
somewhat more convenient than running Emacs as a subprocess to do bulk
automated refactoring, e.g. across 100k files or on every new file
that's committed.

Re: Adding refactoring capabilities to Emacs

[João Távora]

>  Let's start working on this!

I'm posting a top-level reply to Eli's email with the content
of a reply to Dmitry in the scope of bug#60338.

That which concerns a significant enhancement to Eglot's
refactoring capability first proposed by Philip and finished by
myself.  It's closed.  So it's better to consolidate the
discussion here.

Especially I think that, to answer Eli's request successfully, we
have to take a look at the forest, not just one particular tree.

By the way, Stefan I CC'ed you so you could comment on the feasibility
of using (elisp--local-variables) for refactoring purposes, in
this case renaming.  Do we have source-tracking info there?

On Thu, Sep 7, 2023 at 2:00 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> On 02/09/2023 12:55, João Távora wrote:
> > Anyway I invite everyone to have a look and try to improve it, perhaps
> > moving it out of Eglot into the shiny new "refactoring interface" if
> > those ever become a thing.
>
> Regarding the diff approach vs. "shiny new", I've run a little
> comparison with VS Code: doing a rename of 'glyph_row' across the Emacs
> codebase (a moderately sized project) takes about ~1.44s to produce the
> diff.

FWIW, I got 0.85s on that same test on a GNU/Linux VM running on a 6
yo laptop with 4 cores assigned.  IMO this is more than acceptable for
this contrived example which renames a symbol across many files.
A much more typical example of renaming a symbol in a single function
resolved in 0.02s.  Most examples of applying quick fixes and moving
.hpp inlined member functions to out-of-line in .cpp resolve in similar
quasi-instantaneous fashion.

Now, let me clarify what I meant by "shiny new refactoring interface".
It's not what you think I meant, I believe. What I meant is a framework
for:

1. collecting 'potential sets of changes' -- for clarity let's call
   "actions" after LSP nomenclature -- from a number of different
   backends.  Examples of actions: "rename thing at point", "organize
   imports", "write as while loops as cl-loop"

2. presenting actions names to the user -- either via the minibuffer
   or by annotating buffer position where these tweaks apply.

3. after the user chooses an action, develop it into a set of changes
   -- again, for clarity and consonance with LSP nomenclature, we
   may call these "edits".

4. presenting these edits to the user via a number of different
   user-confirmation strategies.

For very common "rename" or "organize imports" actions, steps 1
and 2 may be coalesced and a special command may be provided that
goes directly to step 3.  LSP supports these "shortcuts".

Regarding 4 (which seems to be the current focal point of the
discussion) Eglot has three main types of confirmation strategies
available.  See eglot-confirm-server-edits for more.

* 'diff', which just presents the edits
* 'summary', which summarizes the edits in the minibuffer and prompts
  to apply.
* nil, which means no confirmation at all

There is also 'maybe-diff' and 'maybe-summary' which behave like
nil if all the files in the change list are already visited buffers.

That said, it's the whole sequence 1 to 4, not just 4, that needs
to be generalized out of Eglot and into Emacs.  Note that some parts
of 1..4, like the annotation of actions in buffers or the support for
file creation and deletion, aren't yet in Eglot, so there's good work
to be done.  Some of the "annotation" logic already exists in Flymake,
and Flymake must be taken into the equation too, and early.

Also, it would be very good if we could have an early backend which is
*not* LSP.  An obvious candidate is Elisp.  I'd like to know what is
the minimal effort to write a lisp function based on the new
macroexpanding tricks that Stefan Monnier developed that says
precisely where a given symbol is used as a variable in a function.
As far as I understand,  these techniques are currently being used for
accurate completion, but they could maybe be used for accurate
refactorings.

> Not to belittle the new addition, though - it's a good step for Eglot.

As someone who actually uses Eglot daily and is an avid consumer
of refactoring functionality, my opinion is that the "diff"
confirmation strategy that Philip proposed and implemented is freaking
great, even if the implementation is a bit contorted (though mostly
isolated).

IMO diff is the lingua franca for communicating changes to source
code around the world, even in those pretty web interfaces that
you and many others like.  So it makes full sense to support it
and to support it well.

> - I would probably want to bind the originally proposed
> 'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"),
> but that's already taken in diff-mode.

> - 'git diff' has a less-frequently used option called '--word-diff'
> which could reduce the length of the diff in the case I am testing (but
> I guess diff-mode would have to be updated to support that output). And
> another way in that direction would be to reduce the size of the diff
> context (e.g. to 0).

I'm not at all opposed to implementing other confirmation strategies
that look more like what VSCode or other editors show users.  Or
augmenting the existing 'diff' strategy with new shortcuts and stuff.
But I wouldn't overthink UI details at this point in the game either.

João

Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

> Also, it would be very good if we could have an early backend which is
> *not* LSP.  An obvious candidate is Elisp.  I'd like to know what is
> the minimal effort to write a lisp function based on the new
> macroexpanding tricks that Stefan Monnier developed that says
> precisely where a given symbol is used as a variable in a function.
> As far as I understand,  these techniques are currently being used for
> accurate completion, but they could maybe be used for accurate
> refactorings.

Hmm... the `elisp--local-variables` functionality (beside the
associated security issues and occasional errors) provides only
a (somewhat) reliable list of variables that are in-scope.  So it could
potentially be extended to offer a way to jump to the declaration of
a local variable from a reference to it, but it doesn't know how to find
all (and only) the references to a given variable.

We could extend it to a full code-walker that can distinguish between
identifiers that refer to functions vs variables and which skips "data"
and obeys scope, and thus collect reliably all the references.
But `cconv-analyze-form` is probably a better starting point :-)

Going back from there to the actual source code can be a fair bit more
tricky, but Alan's symbols-with-positions should provide just what we need.

> IMO diff is the lingua franca for communicating changes to source
> code around the world, even in those pretty web interfaces that
> you and many others like.  So it makes full sense to support it
> and to support it well.

+1, tho I think the general framework should be agnostic to the specific
way a change is presented to the user.

>> - I would probably want to bind the originally proposed
>> 'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"),

Agreed.

>> but that's already taken in diff-mode.

I don't think the current `C-c C-c` binding in `diff-mode` should get in
the way.  That command is available via different bindings already, and
this is a specific use of diff-mode where `diff-apply-everything` makes
a lot of sense (as opposed to `C-x v =` where the changes are usually
already applied anyway so `diff-apply-everything` is rarely what we want).

>> - 'git diff' has a less-frequently used option called '--word-diff'
>> which could reduce the length of the diff in the case I am testing (but
>> I guess diff-mode would have to be updated to support that output). And
>> another way in that direction would be to reduce the size of the diff
>> context (e.g. to 0).

[ Side note: we could also extend diff-mode to do such "rewrite" inside
  Emacs.  Hiding some of the context should be fairly easy, for example.
  Also I've often wished for `diff-refine-hunk` to try and interlace
  the lines so as to minimize the distance between the "before" and
  "after" lines.  ]

> But I wouldn't overthink UI details at this point in the game either.

+1


        Stefan

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Thu, Sep 7, 2023 at 5:20 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> We could extend it to a full code-walker that can distinguish between
> identifiers that refer to functions vs variables and which skips "data"
> and obeys scope, and thus collect reliably all the references.
> But `cconv-analyze-form` is probably a better starting point :-)

But cconv-analyze-form takes a pre-read form, you've lost all source code
info by then.

> Going back from there to the actual source code can be a fair bit more
> tricky,

Exactly.

> but Alan's symbols-with-positions should provide just what we need.

Great! but how?

Some years ago I presented a full source-tracking reader for Common Lisp
with just Lisp to a conference ;-) Needs a programmable reader though :-(
As you may remember, I then paired it with a full code walker to create
a stepper.

Is lread.c used for symbols-with-positions, or is code read in some
other completely new way?

> > IMO diff is the lingua franca for communicating changes to source
> > code around the world, even in those pretty web interfaces that
> > you and many others like.  So it makes full sense to support it
> > and to support it well.
>
> +1, tho I think the general framework should be agnostic to the specific
> way a change is presented to the user.

Of course, but I don't understand the "tho", since that's exactly
what I described.  Presenting edits to the user is just the last step
after collecting them.   We should have a goodinternal representation
for a change, which is easy to manipulate, easy to apply to buffers,
and easy to view from many formats.  LSP's representation (a range
and a string with a null string for deletions) is not bad IMO, and
I see no reason to stray from it.

João

Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

>> Going back from there to the actual source code can be a fair bit more
>> tricky,
> Exactly.
>> but Alan's symbols-with-positions should provide just what we need.
> Great! but how?
> Some years ago I presented a full source-tracking reader for Common Lisp
> with just Lisp to a conference ;-) Needs a programmable reader though :-(
> As you may remember, I then paired it with a full code walker to create
> a stepper.

We do have a "full source-tracking reader" writen in ELisp inside `edebug.el`.
It's not 100% faithful to `lread.c`, but it seems to work well enough
that we haven't heard too many bug reports about it over the years.

Still, can't be used with `cconv` because the the "sexp with
source-tracking info" don't have the shape expected by macros.

> Is lread.c used for symbols-with-positions, or is code read in some
> other completely new way?

Yes, the symbols-with-positions thingy was done by extending `lread.c`.
It keeps track of the source position only for symbols rather than for
all Lisp objects, but it does it in a way which can survive
macro-expansion.

It's what's used in the byte-compiler to provide position info in
the warnings.

> Of course, but I don't understand the "tho",

Yeah, it was probably not a correct usage.  Please ignore it.

> since that's exactly what I described.

Yes, we agree.


        Stefan

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Thu, Sep 7, 2023 at 6:06 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> >> Going back from there to the actual source code can be a fair bit more
> >> tricky,
> > Exactly.
> >> but Alan's symbols-with-positions should provide just what we need.
> > Great! but how?
> > Some years ago I presented a full source-tracking reader for Common Lisp
> > with just Lisp to a conference ;-) Needs a programmable reader though :-(
> > As you may remember, I then paired it with a full code walker to create
> > a stepper.
>
> We do have a "full source-tracking reader" writen in ELisp inside `edebug.el`.
> It's not 100% faithful to `lread.c`, but it seems to work well enough
> that we haven't heard too many bug reports about it over the years.
>
> Still, can't be used with `cconv` because the the "sexp with
> source-tracking info" don't have the shape expected by macros.
>
> > Is lread.c used for symbols-with-positions, or is code read in some
> > other completely new way?
>
> Yes, the symbols-with-positions thingy was done by extending `lread.c`.
> It keeps track of the source position only for symbols rather than for
> all Lisp objects, but it does it in a way which can survive
> macro-expansion.

Great!  Indeed that's exactly what's needed.   Might as well
plug my paper here: https://zenodo.org/record/3742759
where I gave it the pompous name of "mnesic macroexpansion".
There I used an existing codewalker with hook support and a
simple 'eq' lookup table.

Though when I used that for a stepper I run into some ambiguities
with atoms after macro expansion (described in section 4.1).  Might
not be a problem here, as that problem was related to knowing
which manifestations a given atom is actually evaluated.  And I'd
say that's not a problem for the "rename" refactoring action.

> It's what's used in the byte-compiler to provide position info in
> the warnings.

Hmmm, and we know it doesn't always get things right... :-/ Namely
it fails when there is more than one manifestation of the warning.
But then again, maybe that's a problem of compilation, not
reading, source-tracking or macro-expansion.

João

Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

> Though when I used that for a stepper I run into some ambiguities
> with atoms after macro expansion (described in section 4.1).  Might
> not be a problem here, as that problem was related to knowing
> which manifestations a given atom is actually evaluated.  And I'd
> say that's not a problem for the "rename" refactoring action.

The symbols-with-position approach avoids this problem (for symbols,
tho not for other data without identity list fixnums) because every
occurrence of a symbol returns a different "symbol with position" (and
then we need a hack to allow those different symbol-with-positions to
be `eq` nevertheless because that's what is needed at too many places
we can't control).

>> It's what's used in the byte-compiler to provide position info in
>> the warnings.
>
> Hmmm, and we know it doesn't always get things right... :-/ Namely
> it fails when there is more than one manifestation of the warning.
> But then again, maybe that's a problem of compilation, not
> reading, source-tracking or macro-expansion.

[ I can already hear Alan prepare his reply :-)  ]
Seriously, tho, the position is pretty damn exactly right almost every
time nowadays.  When it's not, it's not because the
"symbol-with-position" info is wrong but because it's missing, or
something else like that.


        Stefan

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Thu, Sep 7, 2023 at 6:54 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > Though when I used that for a stepper I run into some ambiguities
> > with atoms after macro expansion (described in section 4.1).  Might
> > not be a problem here, as that problem was related to knowing
> > which manifestations a given atom is actually evaluated.  And I'd
> > say that's not a problem for the "rename" refactoring action.
>
> The symbols-with-position approach avoids this problem (for symbols,
> tho not for other data without identity list fixnums) because every
> occurrence of a symbol returns a different "symbol with position" (and
> then we need a hack to allow those different symbol-with-positions to
> be `eq` nevertheless because that's what is needed at too many places
> we can't control).

Making different things 'eq' is a possibility, but won't it
slow down 'eq'?

Again, I don't know if this is actually a problem specifically
when refactoring.

In that simple example

  (lambda (x) x)

The ambiguity is a problem when stepping, but it's no problem when
refactoring.

A trickier problem is

  (lambda (x) (let (x) x) x)

where renaming one of the symbols should not rename the others.  But
I think I implemented  a solution to that (at least in my model),
because symbols have parent forms, and the forms also have
source information.

Anyway, you seem to have envisioned a good strategy for this,  so
I won't bother you more with my vague memories of alternatives.

> >> It's what's used in the byte-compiler to provide position info in
> >> the warnings.
> >
> > Hmmm, and we know it doesn't always get things right... :-/ Namely
> > it fails when there is more than one manifestation of the warning.
> > But then again, maybe that's a problem of compilation, not
> > reading, source-tracking or macro-expansion.
>
> [ I can already hear Alan prepare his reply :-)  ]
> Seriously, tho, the position is pretty damn exactly right almost every
> time nowadays.  When it's not, it's not because the
> "symbol-with-position" info is wrong but because it's missing, or
> something else like that.

I didn't say it was :-) I just meant cases like this (this is the
simplest one)

   (defun foo () bar bar baz)

Only the first bar and the baz gets the warning.  If there was a lot
of code between the two 'bar's, it's not easy for the user to spot
(say in a Flymake overlay) that the bar isn't defined.

In contrast, a code analyzer like clangd gets this case correctly
(in C/C++ of course) and highlights the two occurances of the
undeclared identifier.

But this is a "diagnostics" problem and here we're concerned with
refactorings.  There, I'm really not sure at all it will be a problem.

João

Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

> Making different things 'eq' is a possibility, but won't it
> slow down 'eq'?

No need to use the future tense: if you're using a recent enough Emacs
your `eq` is slowed down by this already.

> I didn't say it was :-) I just meant cases like this (this is the
> simplest one)
>
>    (defun foo () bar bar baz)
>
> Only the first bar and the baz gets the warning.

AFAIK this is done on purpose to avoid repeating the same warnings over
and over again.  If you want them all, it should be easy to tweak the
compiler to stop silencing the non-first ones.


        Stefan

Re: Adding refactoring capabilities to Emacs

[Lynn Winebarger]

On Thu, Sep 7, 2023 at 1:08 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> >> Going back from there to the actual source code can be a fair bit more
> >> tricky,
> > Exactly.
> >> but Alan's symbols-with-positions should provide just what we need.
> > Great! but how?
> > Some years ago I presented a full source-tracking reader for Common Lisp
> > with just Lisp to a conference ;-) Needs a programmable reader though :-(
> > As you may remember, I then paired it with a full code walker to create
> > a stepper.
>
> We do have a "full source-tracking reader" writen in ELisp inside `edebug.el`.
> It's not 100% faithful to `lread.c`, but it seems to work well enough
> that we haven't heard too many bug reports about it over the years.
>
> Still, can't be used with `cconv` because the the "sexp with
> source-tracking info" don't have the shape expected by macros.

I have a package "rewriting-pcase" which contains the code I use to
textually rewrite some very specific, small sexps in packages so the
data files and subdirectories do not have to be located in a
particular location relative to the source library.  The text is
rewritten to avoid dramatic changes in the formatting of the original
code, but it only works because the sexp patterns are not complicated,
and the replacement sexps are atoms.

To expand the functionality to be true to its name, I was planning to
augment the wrap the reader with an elisp function to create
"syntax-objects" (as they are called in Scheme macro expanders) that
augment the data produced by the reader with various metadata,
including things like source file, line number, position, as well as
lexical information for expansion hygiene.  For rewriting-pcase, I'd
also track comments that should "attach" to the syntactic construct so
comments would move around with the code they describe when that code
is substituted while rewriting.

"symbol-with-position" appears to just be a handle on a symbol with a
position.  Would it be acceptable to change this to a more generic
syntax-object, then factor the reader into an enhanced reader that
produces syntax-objects, and provide the traditional read function as
a wrapper that discards the syntax?  Is the existence of position
information relied on in many places in the code?  I would think those
places could use the syntax-object-producing version of the reader.

My current approach is to recursively use the reader to deduce the
required syntactic information, but in addition to being expensive,
it's also unreliable in many cases that seem like they would be corner
cases but do occur in the wild, where the same datum can be
represented in multiple ways and are tricky to navigate with the usual
syntax-traversing functions used in working with elisp buffers.

Lynn

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 07/09/2023 19:20, Stefan Monnier wrote:
>> Also, it would be very good if we could have an early backend which is
>> *not* LSP.  An obvious candidate is Elisp.  I'd like to know what is
>> the minimal effort to write a lisp function based on the new
>> macroexpanding tricks that Stefan Monnier developed that says
>> precisely where a given symbol is used as a variable in a function.
>> As far as I understand,  these techniques are currently being used for
>> accurate completion, but they could maybe be used for accurate
>> refactorings.
> 
> Hmm... the `elisp--local-variables` functionality (beside the
> associated security issues and occasional errors) provides only
> a (somewhat) reliable list of variables that are in-scope.  So it could
> potentially be extended to offer a way to jump to the declaration of
> a local variable from a reference to it, but it doesn't know how to find
> all (and only) the references to a given variable.
> 
> We could extend it to a full code-walker that can distinguish between
> identifiers that refer to functions vs variables and which skips "data"
> and obeys scope, and thus collect reliably all the references.
> But `cconv-analyze-form` is probably a better starting point :-)

I think the Helpful packages used some of the approach. Though I'm not 
sure how precise/thorough it is, and combining the outputs of 
xref-find-references and elisp--local-variables is likely to be faster. 
As a first/initial implementation anyway.

If we're only talking about 'rename', at least.

>>> but that's already taken in diff-mode.
> 
> I don't think the current `C-c C-c` binding in `diff-mode` should get in
> the way.  That command is available via different bindings already, and

diff-apply-everything is a little destructive, though (there is no 
'undo' across multiple files), so maybe a warning could help. Or make it 
an initially-disabled command, for example.

> this is a specific use of diff-mode where `diff-apply-everything` makes
> a lot of sense (as opposed to `C-x v =` where the changes are usually
> already applied anyway so `diff-apply-everything` is rarely what we want).

So... the binding wouldn't be used in "regular" Diff buffers, it it just 
would stop with a "hunk already applied" message? If it's the former, 
we'd need to distinguish the "refactoring diff" buffers by look somehow.

>>> - 'git diff' has a less-frequently used option called '--word-diff'
>>> which could reduce the length of the diff in the case I am testing (but
>>> I guess diff-mode would have to be updated to support that output). And
>>> another way in that direction would be to reduce the size of the diff
>>> context (e.g. to 0).
> 
> [ Side note: we could also extend diff-mode to do such "rewrite" inside
>    Emacs.  Hiding some of the context should be fairly easy, for example.
>    Also I've often wished for `diff-refine-hunk` to try and interlace
>    the lines so as to minimize the distance between the "before" and
>    "after" lines.  ]

Interesting.

Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

>>>> but that's already taken in diff-mode.
>> I don't think the current `C-c C-c` binding in `diff-mode` should get in
>> the way.  That command is available via different bindings already, and
> diff-apply-everything is a little destructive, though (there is no 'undo'
> across multiple files), so maybe a warning could help. Or make it an
> initially-disabled command, for example.

But in the context of a refactoring operation, it wouldn't make sense to
disable it.

>> this is a specific use of diff-mode where `diff-apply-everything` makes
>> a lot of sense (as opposed to `C-x v =` where the changes are usually
>> already applied anyway so `diff-apply-everything` is rarely what we want).
> So... the binding wouldn't be used in "regular" Diff buffers,

That's indeed a possible option (e.g. use a mode that's derived from
`diff-mode` but with a different `C-c C-c` binding, or activate
a special "refactoring minor mode" to provide that binding).


        Stefan

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 01:03, Stefan Monnier wrote:
>>>>> but that's already taken in diff-mode.
>>> I don't think the current `C-c C-c` binding in `diff-mode` should get in
>>> the way.  That command is available via different bindings already, and
>> diff-apply-everything is a little destructive, though (there is no 'undo'
>> across multiple files), so maybe a warning could help. Or make it an
>> initially-disabled command, for example.
> But in the context of a refactoring operation, it wouldn't make sense to
> disable it.

We would if we keep the same major mode. And if we decide that it's 
dangerous to set off accidentally.

>>> this is a specific use of diff-mode where `diff-apply-everything` makes
>>> a lot of sense (as opposed to `C-x v =` where the changes are usually
>>> already applied anyway so `diff-apply-everything` is rarely what we want).
>> So... the binding wouldn't be used in "regular" Diff buffers,
> That's indeed a possible option (e.g. use a mode that's derived from
> `diff-mode` but with a different `C-c C-c` binding, or activate
> a special "refactoring minor mode" to provide that binding).

But it would have to look different from diff-mode somehow, I suppose? 
Or is having a different buffer name good enough?

Anyway, the same command would probably be useful when applying patches 
received from others as well. So perhaps we should just always have it 
available in diff-mode.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Thu, Sep 7, 2023 at 5:20 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> >> but that's already taken in diff-mode.
>
> I don't think the current `C-c C-c` binding in `diff-mode` should get in
> the way.  That command is available via different bindings already, and

I didn't notice this suggestion earlier.  Please, let's not do this.
C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
I have it most hardwired in my brain.

IOW, I see a diff about my code, I don't care if it was LSP or git or someone's
patch, I want the same interface always.

João

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 01:18, João Távora wrote:
> On Thu, Sep 7, 2023 at 5:20 PM Stefan Monnier<monnier@iro.umontreal.ca>  wrote:
> 
>>>> but that's already taken in diff-mode.
>> I don't think the current `C-c C-c` binding in `diff-mode` should get in
>> the way.  That command is available via different bindings already, and
> I didn't notice this suggestion earlier.  Please, let's not do this.
> C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
> I have it most hardwired in my brain.

Really? For me it's more like "finish this commit" or "send this email". 
Or perhaps "kill process", which is somewhat close enough.

I'm not married to the particular key sequence, but it does have 
precedent in other parts of Emacs. And there aren't too many great 
alternatives.

> IOW, I see a diff about my code, I don't care if it was LSP or git or someone's
> patch, I want the same interface always.

diff-apply-everything could work with "someone's patches" just as well. 
That's actually a reason to just add it to diff-mode.

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> Date: Fri, 8 Sep 2023 01:39:43 +0300
> Cc: Eli Zaretskii <eliz@gnu.org>, "Philip K." <philipk@posteo.net>,
>  emacs-devel@gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 08/09/2023 01:18, João Távora wrote:
> > On Thu, Sep 7, 2023 at 5:20 PM Stefan Monnier<monnier@iro.umontreal.ca>  wrote:
> > 
> >>>> but that's already taken in diff-mode.
> >> I don't think the current `C-c C-c` binding in `diff-mode` should get in
> >> the way.  That command is available via different bindings already, and
> > I didn't notice this suggestion earlier.  Please, let's not do this.
> > C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
> > I have it most hardwired in my brain.
> 
> Really? For me it's more like "finish this commit" or "send this email". 
> Or perhaps "kill process", which is somewhat close enough.

"C-c C-c" in Emacs generally means "I'm done editing/typing input,
proceed with what needs to be done to complete this transaction".

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 09:18, Eli Zaretskii wrote:
>> Date: Fri, 8 Sep 2023 01:39:43 +0300
>> Cc: Eli Zaretskii<eliz@gnu.org>, "Philip K."<philipk@posteo.net>,
>>   emacs-devel@gnu.org
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 08/09/2023 01:18, João Távora wrote:
>>> On Thu, Sep 7, 2023 at 5:20 PM Stefan Monnier<monnier@iro.umontreal.ca>   wrote:
>>>
>>>>>> but that's already taken in diff-mode.
>>>> I don't think the current `C-c C-c` binding in `diff-mode` should get in
>>>> the way.  That command is available via different bindings already, and
>>> I didn't notice this suggestion earlier.  Please, let's not do this.
>>> C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
>>> I have it most hardwired in my brain.
>> Really? For me it's more like "finish this commit" or "send this email".
>> Or perhaps "kill process", which is somewhat close enough.
> "C-c C-c" in Emacs generally means "I'm done editing/typing input,
> proceed with what needs to be done to complete this transaction".

That seems to rhyme with my proposal. Except instead of "done editing" 
it will be "done reviewing" (though some rare users might engage in 
editing the diff as well).

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 8, 2023 at 7:25 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> That seems to rhyme with my proposal. Except instead of "done editing"
> it will be "done reviewing" (though some rare users might engage in
> editing the diff as well).

It doesn't rhyme at all, unless you really twist your ears.

What's wrong with C-c RET, easily typed ad as C-c C-m?  Or
just RET, for that matter?

Also why should a discussion on refactoring capabilities have
to kick off with the proposal for the destruction of one of
the most longstanding useful shortcuts in diff-mode?

João

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 21:35, João Távora wrote:
> On Fri, Sep 8, 2023 at 7:25 PM Dmitry Gutov<dmitry@gutov.dev>  wrote:
> 
>> That seems to rhyme with my proposal. Except instead of "done editing"
>> it will be "done reviewing" (though some rare users might engage in
>> editing the diff as well).
> It doesn't rhyme at all, unless you really twist your ears.
> 
> What's wrong with C-c RET, easily typed ad as C-c C-m?  Or
> just RET, for that matter?

RET is already taken. C-c RET is not as discoverable and doesn't match 
any other mode.

> Also why should a discussion on refactoring capabilities have
> to kick off with the proposal for the destruction of one of
> the most longstanding useful shortcuts in diff-mode?

Because we have now a feature which uses diff-mode for confirming a 
refactoring?

Anyway, I don't understand the strength of your reaction, but I'm not 
going to push this.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 8, 2023 at 7:38 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> > What's wrong with C-c RET, easily typed ad as C-c C-m?  Or
> > just RET, for that matter?
>
> RET is already taken.

So is C-c C-c !

> C-c RET is not as discoverable and doesn't match
> any other mode.

And how does "matching another mode" matter here as this is a
completely new command for applying all diff hunks?  There aren't
any other modes to match!

> > Also why should a discussion on refactoring capabilities have
> > to kick off with the proposal for the destruction of one of
> > the most longstanding useful shortcuts in diff-mode?
>
> Because we have now a feature which uses diff-mode for confirming a
> refactoring?

A feature you don't use, right?  In your new word-diff-based confirmation
mode, when it comes, feel free to bind C-c C-c there at will.

> Anyway, I don't understand the strength of your reaction,

It'd royally mess up my workflow, especially when using Emacs -Q
across versions.

> but I'm not going to push this.

Thanks.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 21:44, João Távora wrote:
>>> Also why should a discussion on refactoring capabilities have
>>> to kick off with the proposal for the destruction of one of
>>> the most longstanding useful shortcuts in diff-mode?
>> Because we have now a feature which uses diff-mode for confirming a
>> refactoring?
> A feature you don't use, right? 

Correct. I would probably use the new command in diff-mode for applying 
multi-file patches, though.

 > In your new word-diff-based confirmation
 > mode, when it comes, feel free to bind C-c C-c there at will.

Sure.

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 8 Sep 2023 19:35:41 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca, philipk@posteo.net, 
> 	emacs-devel@gnu.org
> 
> On Fri, Sep 8, 2023 at 7:25 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
> > That seems to rhyme with my proposal. Except instead of "done editing"
> > it will be "done reviewing" (though some rare users might engage in
> > editing the diff as well).
> 
> It doesn't rhyme at all, unless you really twist your ears.

Actually, yes, it does.

Mind you, I didn't say that we should rebind "C-c C-c" in diff-mode, I
just explained what we use that key sequence for in Emacs.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 8, 2023 at 7:57 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Fri, 8 Sep 2023 19:35:41 +0100
> > Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca, philipk@posteo.net,
> >       emacs-devel@gnu.org
> >
> > On Fri, Sep 8, 2023 at 7:25 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
> >
> > > That seems to rhyme with my proposal. Except instead of "done editing"
> > > it will be "done reviewing" (though some rare users might engage in
> > > editing the diff as well).
> >
> > It doesn't rhyme at all, unless you really twist your ears.
>
> Actually, yes, it does.

I guess rhyming is in the ears of the beholder.

It rhymes with "DWIM" for me, and that's a much broader spectrum.

> Mind you, I didn't say that we should rebind "C-c C-c" in diff-mode, I
> just explained what we use that key sequence for in Emacs.

OK.  Dmitry and I gave some other examples that may or may not
rhyme depending on how you listen.

João

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Thu, Sep 7, 2023 at 11:39 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> > C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
> > I have it most hardwired in my brain.
>
> Really?

Yes, really.

> For me it's more like "finish this commit" or "send this email".

That too of course.  But these are other modes, no?

> > IOW, I see a diff about my code, I don't care if it was LSP or git or someone's
> > patch, I want the same interface always.
>
> diff-apply-everything could work with "someone's patches" just as well.
> That's actually a reason to just add it to diff-mode.

I never said there wasn't.  But don't touch C-c C-c please.

João Távora

Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

>> > C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
>> > I have it most hardwired in my brain.
>> Really?
> Yes, really.

Curious: I always use RET instead.  I'm actually having a hard time
remembering/imagining how/why we ended up binding `C-c C-c` to
`diff-goto-source` in `diff-mode`.

>> For me it's more like "finish this commit" or "send this email".
> That too of course.  But these are other modes, no?

To me the "show diff during refactoring" is a different situation from
cases like `vc-diff` or `C-x C-f foo.diff`: it's more like showing you
all the details of your order before you confirm that you really want to
do it.

>> diff-apply-everything could work with "someone's patches" just as well.
>> That's actually a reason to just add it to diff-mode.
> I never said there wasn't.  But don't touch C-c C-c please.

Maybe we should start by removing that `C-c C-c` binding from
`diff-mode` (users like you who really like it can re-add it easily).


        Stefan

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 8, 2023 at 4:42 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> >> > C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
> >> > I have it most hardwired in my brain.
> >> Really?
> > Yes, really.
>
> Curious: I always use RET instead.
> I'm actually having a hard time
> remembering/imagining how/why we ended up binding `C-c C-c` to
> `diff-goto-source` in `diff-mode`.

Seems some young hotshot added it back in the day

commit 610a6418564684d52f7490b8e00f92443abb3253
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Sat Oct 9 23:52:39 1999 +0000

    Initial revision, known outside of Emacs as version 1.8.

> >> For me it's more like "finish this commit" or "send this email".
> > That too of course.  But these are other modes, no?
>
> To me the "show diff during refactoring" is a different situation from
> cases like `vc-diff` or `C-x C-f foo.diff`: it's more like showing you
> all the details of your order before you confirm that you really want to
> do it.
>
> >> diff-apply-everything could work with "someone's patches" just as well.
> >> That's actually a reason to just add it to diff-mode.
> > I never said there wasn't.  But don't touch C-c C-c please.
>
> Maybe we should start by removing that `C-c C-c` binding from
> `diff-mode` (users like you who really like it can re-add it easily).

Strongly object to this.  Remove a 20yo binding?  Aren't there
really other alternatives??  Especially in a mode that's read only
much more often than not?

João

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 8, 2023 at 5:05 PM João Távora <joaotavora@gmail.com> wrote:
>
> On Fri, Sep 8, 2023 at 4:42 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >
> > >> > C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
> > >> > I have it most hardwired in my brain.
> > >> Really?
> > > Yes, really.
> >
> > Curious: I always use RET instead.
> > I'm actually having a hard time
> > remembering/imagining how/why we ended up binding `C-c C-c` to
> > `diff-goto-source` in `diff-mode`.
>
> Seems some young hotshot added it back in the day

More realistically, the reason may have been that
diff-mode isn't necessarily a read-only mode, and
so you need a command such as this which doesn't
imply modifying the buffer (RET is no good here).
There is also M-o and M-RET but these were (and
most importantly probably still are) harder to hit or
bind on some terminals.

João

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 19:20, João Távora wrote:
> On Fri, Sep 8, 2023 at 5:05 PM João Távora<joaotavora@gmail.com>  wrote:
>> On Fri, Sep 8, 2023 at 4:42 PM Stefan Monnier<monnier@iro.umontreal.ca>  wrote:
>>>>>> C-c C-c is "go to place mentioned in this hunk".  It must not apply anything,
>>>>>> I have it most hardwired in my brain.
>>>>> Really?
>>>> Yes, really.
>>> Curious: I always use RET instead.
>>> I'm actually having a hard time
>>> remembering/imagining how/why we ended up binding `C-c C-c` to
>>> `diff-goto-source` in `diff-mode`.
>> Seems some young hotshot added it back in the day
> More realistically, the reason may have been that
> diff-mode isn't necessarily a read-only mode, and
> so you need a command such as this which doesn't
> imply modifying the buffer (RET is no good here).
> There is also M-o and M-RET but these were (and
> most importantly probably still are) harder to hit or
> bind on some terminals.

(Sorry, for resurrecting this subthread, but...)

Should we make diff-mode always start in read-only-mode? It's currently 
inconsistent: read-only in some cases (vc-diff) and writable in others 
(visiting a patch file on disk).

Whereas, I think, in both cases editing the diff itself is a relatively 
rare operation. Most of the time, I visit a patch, press some key (such 
as n or p), curse slightly, press 'undo', turn on read-only-mode, and 
then continue with the real work.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 02:11, Dmitry Gutov wrote:
> Should we make diff-mode always start in read-only-mode? It's currently 
> inconsistent: read-only in some cases (vc-diff) and writable in others 
> (visiting a patch file on disk).

Ah, there actually is a user option for that. And it's only 20 years young.

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   (Sorry, for resurrecting this subthread, but...)

   Should we make diff-mode always start in read-only-mode? It's currently 
   inconsistent: read-only in some cases (vc-diff) and writable in others 
   (visiting a patch file on disk).

Does it need to be consitent?  These are two I would think entierly
different use cases of diff-mode... and warrent different behaviour.

If you have a diff on file, you are most probobly going to apply it,
and also probobly going to remove a hunk or two or edit the diff in
some manner.  (That this is "relatively rare" I disagree from my own
usage and experience).  Not to mention that visiting a file on disk,
that is read-write, and Emacs making it read-only would be very
strange.

With vc-diff (similar, diff-buffer-with-file) you are diffing whatever
you have against VC (or file), there is little point to edit via the
diff, and you'd more likley just save this to file or an email.  So in
those cases the buffer being read-only makes more sense.

   Whereas, I think, in both cases editing the diff itself is a relatively 
   rare operation. Most of the time, I visit a patch, press some key (such 
   as n or p), curse slightly, press 'undo', turn on read-only-mode, and 
   then continue with the real work.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Tue, Sep 26, 2023 at 6:36 AM Alfred M. Szmidt <ams@gnu.org> wrote:

> If you have a diff on file, you are most probobly going to apply it,
> and also probobly going to remove a hunk or two or edit the diff in
> some manner.  (That this is "relatively rare" I disagree from my own
> usage and experience).  Not to mention that visiting a file on disk,
> that is read-write, and Emacs making it read-only would be very
> strange.

I completely agree with these two points.  Even non-file diff-mode
buffers, such as the ones provided by piping git diff into Emacs
(yes, I can do that :-) ) are generally better left read-write,
since I frequently edit them to kill hunks I'm not interested in.

João

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 11:06, João Távora wrote:
> On Tue, Sep 26, 2023 at 6:36 AM Alfred M. Szmidt<ams@gnu.org>  wrote:
> 
>> If you have a diff on file, you are most probobly going to apply it,
>> and also probobly going to remove a hunk or two or edit the diff in
>> some manner.  (That this is "relatively rare" I disagree from my own
>> usage and experience).  Not to mention that visiting a file on disk,
>> that is read-write, and Emacs making it read-only would be very
>> strange.
> I completely agree with these two points.  Even non-file diff-mode
> buffers, such as the ones provided by piping git diff into Emacs
> (yes, I can do that 😄 ) are generally better left read-write,
> since I frequently edit them to kill hunks I'm not interested in.

'k' (or M-k), 'C-c C-s' and 'C-_' all work fine in a read-only diff-mode 
buffer. 'C-x C-s' also works, of course.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Tue, Sep 26, 2023 at 11:57 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> On 26/09/2023 11:06, João Távora wrote:
> > On Tue, Sep 26, 2023 at 6:36 AM Alfred M. Szmidt<ams@gnu.org>  wrote:
> >
> >> If you have a diff on file, you are most probobly going to apply it,
> >> and also probobly going to remove a hunk or two or edit the diff in
> >> some manner.  (That this is "relatively rare" I disagree from my own
> >> usage and experience).  Not to mention that visiting a file on disk,
> >> that is read-write, and Emacs making it read-only would be very
> >> strange.
> > I completely agree with these two points.  Even non-file diff-mode
> > buffers, such as the ones provided by piping git diff into Emacs
> > (yes, I can do that 😄 ) are generally better left read-write,
> > since I frequently edit them to kill hunks I'm not interested in.
>
> 'k' (or M-k), 'C-c C-s' and 'C-_' all work fine in a read-only diff-mode
> buffer. 'C-x C-s' also works, of course.

I think it's very inconsistent to have specialized commands to modify
a buffers contents and not allow all the other regular commands that
modify a buffer do their work.  I don't have unlimited brain address
space for keybindings and I think C-SPC C-n a few times C-w does
the job just fine.

Opening regular files of a special type read-only mode would be a
spectacular failure in the basic ergonomics of an editor.

João

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]


   On Tue, Sep 26, 2023 at 11:57 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
   >
   > On 26/09/2023 11:06, João Távora wrote:
   > > On Tue, Sep 26, 2023 at 6:36 AM Alfred M. Szmidt<ams@gnu.org>  wrote:
   > >
   > >> If you have a diff on file, you are most probobly going to apply it,
   > >> and also probobly going to remove a hunk or two or edit the diff in
   > >> some manner.  (That this is "relatively rare" I disagree from my own
   > >> usage and experience).  Not to mention that visiting a file on disk,
   > >> that is read-write, and Emacs making it read-only would be very
   > >> strange.
   > > I completely agree with these two points.  Even non-file diff-mode
   > > buffers, such as the ones provided by piping git diff into Emacs
   > > (yes, I can do that 😄 ) are generally better left read-write,
   > > since I frequently edit them to kill hunks I'm not interested in.
   >
   > 'k' (or M-k), 'C-c C-s' and 'C-_' all work fine in a read-only diff-mode
   > buffer. 'C-x C-s' also works, of course.

   I think it's very inconsistent to have specialized commands to modify
   a buffers contents and not allow all the other regular commands that
   modify a buffer do their work.  I don't have unlimited brain address
   space for keybindings and I think C-SPC C-n a few times C-w does
   the job just fine.

Or fixing a typo, or renaming a variable, or some such ...

   Opening regular files of a special type read-only mode would be a
   spectacular failure in the basic ergonomics of an editor.

1+

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 14:24, João Távora wrote:
> On Tue, Sep 26, 2023 at 11:57 AM Dmitry Gutov<dmitry@gutov.dev>  wrote:
>> On 26/09/2023 11:06, João Távora wrote:
>>> On Tue, Sep 26, 2023 at 6:36 AM Alfred M. Szmidt<ams@gnu.org>   wrote:
>>>
>>>> If you have a diff on file, you are most probobly going to apply it,
>>>> and also probobly going to remove a hunk or two or edit the diff in
>>>> some manner.  (That this is "relatively rare" I disagree from my own
>>>> usage and experience).  Not to mention that visiting a file on disk,
>>>> that is read-write, and Emacs making it read-only would be very
>>>> strange.
>>> I completely agree with these two points.  Even non-file diff-mode
>>> buffers, such as the ones provided by piping git diff into Emacs
>>> (yes, I can do that 😄 ) are generally better left read-write,
>>> since I frequently edit them to kill hunks I'm not interested in.
>> 'k' (or M-k), 'C-c C-s' and 'C-_' all work fine in a read-only diff-mode
>> buffer. 'C-x C-s' also works, of course.
> I think it's very inconsistent to have specialized commands to modify
> a buffers contents and not allow all the other regular commands that
> modify a buffer do their work.  I don't have unlimited brain address
> space for keybindings and I think C-SPC C-n a few times C-w does
> the job just fine.

Is that better than typing 'k'?

Or being able to use hunk navigation with n/p?

> Opening regular files of a special type read-only mode would be a
> spectacular failure in the basic ergonomics of an editor.

Like you said: you edit both kinds of diff buffers (vc and files on 
disk) in the same way. But we assign them different read-only status.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Tue, Sep 26, 2023 at 12:34 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> Is that better than typing 'k'?

'k' inserts k.

> Or being able to use hunk navigation with n/p?

I use M-n sometimes when I remember it, but mostly I use C-s like
everywhere.

> > Opening regular files of a special type read-only mode would be a
> > spectacular failure in the basic ergonomics of an editor.
>
> Like you said: you edit both kinds of diff buffers (vc and files on
> disk) in the same way. But we assign them different read-only status.

When I pipe the results of `git diff`, `git log`, etc into Emacs,
I don't make them read only.  And I use this more way more frequently
than vc-diff.

Frankly I don't care if you think it's "better" to use your
specialized keybindings.  It's your right to think so. But I
do care about being able to use the keybindings I've always used
for such basic file and buffer-editing functionality.

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

I can sorta see where Dimitry is coming from.

You, and I at least, consider diffs "files" -- so we prefer to keep
them as you'd normally C-x C-f on then. 

But consider RMAIL, RMAIL handles the file specially to keep whatever
internal structure sane.  So the only way to edit is to use whatever
mode bindings you have at your fingers.  

The major difference here is that if you C-x C-f a RMAIL file, you get
the file (if I remeber correct, this might be a regression from when
the format was changed, since I do recall that BABYL files had a mode
line and the file would open up in RMAIL mode?).  WHere as with diff,
if you name the file .diff or .patch or whatever, you get diff-mode
like you'd get c-mode or similar.

To open a RMAIL file in RMAIL mode you need M-x rmail, which puts the
burden on the user to decide what they want to do (break the rules of
the format or use the dedicated mode).


Now .. I don't personally favor this, and it seems that there is a
simple switch to make diff files read-only anyway, so maybe leave it
at that?

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 16:09, Alfred M. Szmidt wrote:
> But consider RMAIL, RMAIL handles the file specially to keep whatever
> internal structure sane.  So the only way to edit is to use whatever
> mode bindings you have at your fingers.

And if you split and kill the hunks (in diff-mode) using the dedicated 
commands, the format is indeed preserved better: e.g. the line numbers 
and counters in the hunk headers.

To break the syntax and make a patch un-applyable is also very easy with 
free-form editing.

 > Now .. I don't personally favor this, and it seems that there is a
simple switch to make diff files read-only anyway, so maybe leave it
at that?

When I bring such questions up, it's usually to consider the standpoint 
of some new-ish user. More consistency and fewer chances to mess things 
up = better.

Re: Adding refactoring capabilities to Emacs

[Philip Kaludercic]

João Távora <joaotavora@gmail.com> writes:

> On Tue, Sep 26, 2023 at 11:57 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
>>
>> On 26/09/2023 11:06, João Távora wrote:
>> > On Tue, Sep 26, 2023 at 6:36 AM Alfred M. Szmidt<ams@gnu.org>  wrote:
>> >
>> >> If you have a diff on file, you are most probobly going to apply it,
>> >> and also probobly going to remove a hunk or two or edit the diff in
>> >> some manner.  (That this is "relatively rare" I disagree from my own
>> >> usage and experience).  Not to mention that visiting a file on disk,
>> >> that is read-write, and Emacs making it read-only would be very
>> >> strange.
>> > I completely agree with these two points.  Even non-file diff-mode
>> > buffers, such as the ones provided by piping git diff into Emacs
>> > (yes, I can do that 😄 ) are generally better left read-write,
>> > since I frequently edit them to kill hunks I'm not interested in.
>>
>> 'k' (or M-k), 'C-c C-s' and 'C-_' all work fine in a read-only diff-mode
>> buffer. 'C-x C-s' also works, of course.
>
> I think it's very inconsistent to have specialized commands to modify
> a buffers contents and not allow all the other regular commands that
> modify a buffer do their work.  I don't have unlimited brain address
> space for keybindings and I think C-SPC C-n a few times C-w does
> the job just fine.

While I get this perspective, I also think that being able to close a
diff using 'q' is a nice, ergonomic-wise.

> Opening regular files of a special type read-only mode would be a
> spectacular failure in the basic ergonomics of an editor.
>
> João

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Tue, Sep 26, 2023 at 2:38 PM Philip Kaludercic <philipk@posteo.net> wrote:

> While I get this perspective, I also think that being able to close a
> diff using 'q' is a nice, ergonomic-wise.

That would be nice for many types, I guess, if `q` wasn't already
much more useful and understood as a tool for inserting the letter
q in file-visiting buffers.

João

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 17:06, João Távora wrote:
> On Tue, Sep 26, 2023 at 2:38 PM Philip Kaludercic<philipk@posteo.net>  wrote:
> 
>> While I get this perspective, I also think that being able to close a
>> diff using 'q' is a nice, ergonomic-wise.
> That would be nice for many types, I guess, if `q` wasn't already
> much more useful and understood as a tool for inserting the letter
> q in file-visiting buffers.

C-x C-f etc/images/attach.xpm RET
q

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Tue, Sep 26, 2023 at 3:31 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> On 26/09/2023 17:06, João Távora wrote:
> > On Tue, Sep 26, 2023 at 2:38 PM Philip Kaludercic<philipk@posteo.net>  wrote:
> >
> >> While I get this perspective, I also think that being able to close a
> >> diff using 'q' is a nice, ergonomic-wise.
> > That would be nice for many types, I guess, if `q` wasn't already
> > much more useful and understood as a tool for inserting the letter
> > q in file-visiting buffers.
>
> C-x C-f etc/images/attach.xpm RET
> q

Yup, inserts the letter 'q' here.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 17:51, João Távora wrote:
> On Tue, Sep 26, 2023 at 3:31 PM Dmitry Gutov<dmitry@gutov.dev>  wrote:
>> On 26/09/2023 17:06, João Távora wrote:
>>> On Tue, Sep 26, 2023 at 2:38 PM Philip Kaludercic<philipk@posteo.net>   wrote:
>>>
>>>> While I get this perspective, I also think that being able to close a
>>>> diff using 'q' is a nice, ergonomic-wise.
>>> That would be nice for many types, I guess, if `q` wasn't already
>>> much more useful and understood as a tool for inserting the letter
>>> q in file-visiting buffers.
>> C-x C-f etc/images/attach.xpm RET
>> q
> Yup, inserts the letter 'q' here.

It quits the buffer here and with 'emacs -Q'.

Re: Adding refactoring capabilities to Emacs

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

On .

> >> C-x C-f etc/images/attach.xpm RET
> >> q
> > Yup, inserts the letter 'q' here.
>
> It quits the buffer here and with 'emacs -Q'.
>

Also using Emacs -Q here. And using it as a text editor while I'm at it.

>

[-- Attachment #2: Type: text/html, Size: 706 bytes --]

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   On .

    >> C-x C-f etc/images/attach.xpm RET
    >> q
    > Yup, inserts the letter 'q' here.

Ditto, inserts a q -- this is master, ./src/emacs -Q.

I am guessing someone might be using a X11 or other GUI Emacs...

    It quits the buffer here and with 'emacs -Q'.

   Also using Emacs -Q here. And using it as a text editor while I'm at it. 

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 18:35, Alfred M. Szmidt wrote:
>     On .
> 
>      >> C-x C-f etc/images/attach.xpm RET
>      >> q
>      > Yup, inserts the letter 'q' here.
> 
> Ditto, inserts a q -- this is master, ./src/emacs -Q.
> 
> I am guessing someone might be using a X11 or other GUI Emacs...

Seems like it.

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   On 26/09/2023 18:35, Alfred M. Szmidt wrote:
   >     On .
   > 
   >      >> C-x C-f etc/images/attach.xpm RET
   >      >> q
   >      > Yup, inserts the letter 'q' here.
   > 
   > Ditto, inserts a q -- this is master, ./src/emacs -Q.
   > 
   > I am guessing someone might be using a X11 or other GUI Emacs...

   Seems like it.

So much for consistency ... :-)

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 18:47, Alfred M. Szmidt wrote:
>     On 26/09/2023 18:35, Alfred M. Szmidt wrote:
>     >     On .
>     >
>     >      >> C-x C-f etc/images/attach.xpm RET
>     >      >> q
>     >      > Yup, inserts the letter 'q' here.
>     >
>     > Ditto, inserts a q -- this is master, ./src/emacs -Q.
>     >
>     > I am guessing someone might be using a X11 or other GUI Emacs...
> 
>     Seems like it.
> 
> So much for consistency ... 😄

If we could do something for consistency for terminal users (e.g. render 
xpms with pseudographics or somesuch), we should.

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 839 bytes --]


   On 26/09/2023 18:47, Alfred M. Szmidt wrote:
   >     On 26/09/2023 18:35, Alfred M. Szmidt wrote:
   >     >     On .
   >     >
   >     >      >> C-x C-f etc/images/attach.xpm RET
   >     >      >> q
   >     >      > Yup, inserts the letter 'q' here.
   >     >
   >     > Ditto, inserts a q -- this is master, ./src/emacs -Q.
   >     >
   >     > I am guessing someone might be using a X11 or other GUI Emacs...
   > 
   >     Seems like it.
   > 
   > So much for consistency ... 😄

   If we could do something for consistency for terminal users (e.g. render 
   xpms with pseudographics or somesuch), we should.

Please, no.  It does not make sense, specially for the case of XPM
files, they are fully editable using normal text and making the
display the content would make it worse since you loose out of that
ability.

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: joaotavora@gmail.com, philipk@posteo.net,
> 	monnier@iro.umontreal.ca, eliz@gnu.org, emacs-devel@gnu.org
> Date: Tue, 26 Sep 2023 12:10:45 -0400
> 
> 
>    On 26/09/2023 18:47, Alfred M. Szmidt wrote:
>    >     On 26/09/2023 18:35, Alfred M. Szmidt wrote:
>    >     >     On .
>    >     >
>    >     >      >> C-x C-f etc/images/attach.xpm RET
>    >     >      >> q
>    >     >      > Yup, inserts the letter 'q' here.
>    >     >
>    >     > Ditto, inserts a q -- this is master, ./src/emacs -Q.
>    >     >
>    >     > I am guessing someone might be using a X11 or other GUI Emacs...
>    > 
>    >     Seems like it.
>    > 
>    > So much for consistency ... 😄
> 
>    If we could do something for consistency for terminal users (e.g. render 
>    xpms with pseudographics or somesuch), we should.
> 
> Please, no.  It does not make sense, specially for the case of XPM
> files, they are fully editable using normal text and making the
> display the content would make it worse since you loose out of that
> ability.

This is again your personal preferences speaking, coupled with the
fact that you aren't using a GUI Emacs frequently enough.  Next time
when you visit in a GUI Emacs an image file that can also be edited as
a text file, please make a point of reading what Emacs tells you right
away in the echo-area after you visit the file.

Once again, this whole discussion thread is completely useless.

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   > Please, no.  It does not make sense, specially for the case of XPM
   > files, they are fully editable using normal text and making the
   > display the content would make it worse since you loose out of that
   > ability.

   This is again your personal preferences speaking, 

Are users not allowed to raise a voice for objectional behaviour in
Emacs?

   coupled with the
   fact that you aren't using a GUI Emacs frequently enough.

You have no knowledge of that.  

   Once again, this whole discussion thread is completely useless.

Raising objections to behaviour that is annoying is not "completely
useless".

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: dmitry@gutov.dev, joaotavora@gmail.com, philipk@posteo.net,
> 	monnier@iro.umontreal.ca, emacs-devel@gnu.org
> Date: Fri, 29 Sep 2023 08:36:49 -0400
> 
>    > Please, no.  It does not make sense, specially for the case of XPM
>    > files, they are fully editable using normal text and making the
>    > display the content would make it worse since you loose out of that
>    > ability.
> 
>    This is again your personal preferences speaking, 
> 
> Are users not allowed to raise a voice for objectional behaviour in
> Emacs?

They are allowed to raise such a voice, but they are much less allowed
to impose their preferences on others where Emacs already have some
useful default behavior and ample possibilities to customize that for
someone who doesn't like the defaults.

>    coupled with the
>    fact that you aren't using a GUI Emacs frequently enough.
> 
> You have no knowledge of that.  

I can read, you know.  And I understand what I read.

>    Once again, this whole discussion thread is completely useless.
> 
> Raising objections to behaviour that is annoying is not "completely
> useless".

Raising them once or twice isn't, but keeping pushing for them to be
the only Right Thing is.

Re: Adding refactoring capabilities to Emacs

[Yuri Khan]

On Tue, 26 Sept 2023 at 23:04, Dmitry Gutov <dmitry@gutov.dev> wrote:

> If we could do something for consistency for terminal users (e.g. render
> xpms with pseudographics or somesuch), we should.

Could do better than that. Could display actual graphics using the
protocol implemented in several terminal emulators.

https://sw.kovidgoyal.net/kitty/graphics-protocol/

(For toggling between previewing images and editing them as text,
there is ‘C-c C-c’ bound in ‘image-minor-mode’. I checked and it works
for both SVG and XPM images.)

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 19:31, Yuri Khan wrote:
> On Tue, 26 Sept 2023 at 23:04, Dmitry Gutov<dmitry@gutov.dev>  wrote:
> 
>> If we could do something for consistency for terminal users (e.g. render
>> xpms with pseudographics or somesuch), we should.
> Could do better than that. Could display actual graphics using the
> protocol implemented in several terminal emulators.
> 
> https://sw.kovidgoyal.net/kitty/graphics-protocol/

Indeed.

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: Yuri Khan <yuri.v.khan@gmail.com>
> Date: Tue, 26 Sep 2023 23:31:18 +0700
> Cc: "Alfred M. Szmidt" <ams@gnu.org>, joaotavora@gmail.com, philipk@posteo.net, 
> 	monnier@iro.umontreal.ca, eliz@gnu.org, emacs-devel@gnu.org
> 
> On Tue, 26 Sept 2023 at 23:04, Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
> > If we could do something for consistency for terminal users (e.g. render
> > xpms with pseudographics or somesuch), we should.
> 
> Could do better than that. Could display actual graphics using the
> protocol implemented in several terminal emulators.
> 
> https://sw.kovidgoyal.net/kitty/graphics-protocol/

Incorporating this into Emacs should be possible, but would need a
thorough surgery of the TTY part of the display engine, which
currently assumes that each "display element" on TTY frames is one
"pixel" wide and 1 "pixel" high.

(It is also not clear to me from a cursory reading of the spec how to
figure out how many text lines and text columns will a given image
need to be displayed unclipped.  This is important for the layout to
work correctly.  Since Emacs generally doesn't know the pixel
resolution of the terminal screen, it cannot easily compute that, and
probably needs some help from the terminal emulator/driver, but I saw
no commands for such a query.  I probably missed something.)

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: joaotavora@gmail.com, philipk@posteo.net,
>  monnier@iro.umontreal.ca, eliz@gnu.org, emacs-devel@gnu.org
> Date: Tue, 26 Sep 2023 11:47:24 -0400
> 
> 
>    On 26/09/2023 18:35, Alfred M. Szmidt wrote:
>    >     On .
>    > 
>    >      >> C-x C-f etc/images/attach.xpm RET
>    >      >> q
>    >      > Yup, inserts the letter 'q' here.
>    > 
>    > Ditto, inserts a q -- this is master, ./src/emacs -Q.
>    > 
>    > I am guessing someone might be using a X11 or other GUI Emacs...
> 
>    Seems like it.
> 
> So much for consistency ... :-)

THERE'S NO REASON TO BE CONSISTENT IN WHAT IS BASICALLY PERSONAL
PREFERENCES!!!

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

I suggest you go read
https://www.gnu.org/philosophy/kind-communication.html ... and go and
calm down

RE: [External] : Re: Adding refactoring capabilities to Emacs

[Drew Adams]

(Caveat: I'm not following this thread much.)

Diff output follows any of several formats,
OK.  But it's nevertheless plain text, no?  As
such, it makes sense to be able to use all of
the general text-editing commands/keys on it,
if you want to change it in arbitrary/whatever
ways.

Is this whole discussion, or even much of it,
really about whether to keep diff-mode buffers
read-only by default, and whether to provide
refactoring commands/keys that let you change
the content in special ways when the buffer is
read-only?

I don't really see a fundamental difference
between a diff buffer and, say, a grep or an
occur buffer.  (But yes, you often _apply_ the
diff content, so you want to ensure it keeps to
a proper formatting.)

We already have, and are used to, the universal
idiom of `C-x C-q' to toggle a buffer read-only.
I use that in grep and occur buffers without a
second thought.  Why isn't that "sufficient" for
diff buffers as well?

I can see that some might want commands/keys that
act on the buffer content, including to change it
in structured/"legitimate"/refactoring ways, while
keeping it read-only.

But for that, why not just add a new minor mode?

That way, by default, diff buffers would still be
read-only by default, quitable with `q', but also
modifiable in all the usual ways after `C-x C-q'.

And anyone who wants to could toggle the minor
mode ON, to act only in special, allowed ways
while in read-only mode, using keys defined for
that mode.  The minor mode could ensure read-only.

(Maybe this or similar has already been proposed
in this thread?  If so, mille excuses...)

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   Is this whole discussion, or even much of it, really about whether
   to keep diff-mode buffers read-only by default, and whether to
   provide refactoring commands/keys that let you change the content
   in special ways when the buffer is read-only?

You need to define what a "diff buffer" is first.  Is it a file you
open, is it the output from M-x diff-buffer-with-file, is it the
output from C-x v = ?

   I don't really see a fundamental difference between a diff buffer
   and, say, a grep or an occur buffer.  (But yes, you often _apply_
   the diff content, so you want to ensure it keeps to a proper
   formatting.)

Occur and grep buffers are by default read-only (and there is a
regression in that too -- you can no longer edit a grep/occur buffer
easily due to strange font lock or other properties).

They are also not files, if you open a file (any file really) you will
expect to be able to edit it using normal commands not specific to the
mode (i.e. self-inser-command ...).  vc-diff etc, are more akin to
occur and grep -- the user doesn't work against the file, but rather a
buffer.

   We already have, and are used to, the universal idiom of `C-x C-q'
   to toggle a buffer read-only.  I use that in grep and occur buffers
   without a second thought.  Why isn't that "sufficient" for diff
   buffers as well?

Because we are also talking about files on disk.  If you save a
compilation buffer (essentially the same thing as M-x grep or M-x
occur) to disk, and re-open it, it will not be read-only -- and it
would make little sense for it to be so.  Where as M-x compile _will_
be read-only.

   I can see that some might want commands/keys that act on the buffer
   content, including to change it in
   structured/"legitimate"/refactoring ways, while keeping it
   read-only.

One can have both, diff-mode already allows you to edit the file, and
act on it in a structure manner -- much like any programming mode.

   That way, by default, diff buffers would still be read-only by
   default, quitable with `q', but also modifiable in all the usual
   ways after `C-x C-q'.

But diff buffers aren't read-only -- it depends on their context.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: dmitry@gutov.dev, joaotavora@gmail.com, philipk@posteo.net,
> 	monnier@iro.umontreal.ca, eliz@gnu.org, emacs-devel@gnu.org
> Date: Tue, 26 Sep 2023 11:22:20 -0400
> 
> Occur and grep buffers are by default read-only (and there is a
> regression in that too

It is only a "regression" if you still have your muscle memory from 20
years ago, since it's when we started making the compilation-mode
buffers read-opnly.

> -- you can no longer edit a grep/occur buffer
> easily due to strange font lock or other properties).

Of course, you can: this is just one "C-x C-q" away (as in any other
read-only buffer).  Let's not look for problems where there are none.

> They are also not files, if you open a file (any file really) you will
> expect to be able to edit it using normal commands not specific to the
> mode (i.e. self-inser-command ...).  vc-diff etc, are more akin to
> occur and grep -- the user doesn't work against the file, but rather a
> buffer.

That depends on your workflows and preferences, of course.  I can
easily imagine someone working with *.diff and *.patrh files a lot.

>    We already have, and are used to, the universal idiom of `C-x C-q'
>    to toggle a buffer read-only.  I use that in grep and occur buffers
>    without a second thought.  Why isn't that "sufficient" for diff
>    buffers as well?
> 
> Because we are also talking about files on disk.  If you save a
> compilation buffer (essentially the same thing as M-x grep or M-x
> occur) to disk, and re-open it, it will not be read-only -- and it
> would make little sense for it to be so.  Where as M-x compile _will_
> be read-only.

I don't see how this is a problem.

There are kinds of files and buffers where sometimes it makes sense to
have them editable and sometimes not, sometimes it makes sense to
treat them as normal text files, and sometimes as specially-formatted
files supported by special modes.  Emacs lets you handle these cases
in all of the above possibilities, and I see no reason to argue about
what is The Only Right Thing here, because there isn't one.

>    I can see that some might want commands/keys that act on the buffer
>    content, including to change it in
>    structured/"legitimate"/refactoring ways, while keeping it
>    read-only.
> 
> One can have both, diff-mode already allows you to edit the file, and
> act on it in a structure manner -- much like any programming mode.

And "C-x C-q" or "M-x fundamental-mode" or "M-x text-mode" lets you
edit them if you need.

>    That way, by default, diff buffers would still be read-only by
>    default, quitable with `q', but also modifiable in all the usual
>    ways after `C-x C-q'.
> 
> But diff buffers aren't read-only -- it depends on their context.

The context depends on the use case and the workflow, and cannot be
reliably determined by Emacs.  So we picked one possibility, and the
others are just one command away.

So let's please stop this futile dispute, which once again is entirely
about personal preferences.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Robert Pluim]

>>>>> On Fri, 29 Sep 2023 13:36:22 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: "Alfred M. Szmidt" <ams@gnu.org>
    >> Cc: dmitry@gutov.dev, joaotavora@gmail.com, philipk@posteo.net,
    >> monnier@iro.umontreal.ca, eliz@gnu.org, emacs-devel@gnu.org
    >> Date: Tue, 26 Sep 2023 11:22:20 -0400
    >> 
    >> Occur and grep buffers are by default read-only (and there is a
    >> regression in that too

    Eli> It is only a "regression" if you still have your muscle memory from 20
    Eli> years ago, since it's when we started making the compilation-mode
    Eli> buffers read-opnly.

"C-x C-q" in an "occur" buffer still leaves some non-useful key
bindings around, like for "DEL". To usefully edit an "occur" buffer
you need to run `occur-edit-mode', bound to "e" by default. We could
fix that wart if we wanted to.

Robert
-- 

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

> "C-x C-q" in an "occur" buffer still leaves some non-useful key
> bindings around, like for "DEL". To usefully edit an "occur" buffer
> you need to run `occur-edit-mode', bound to "e" by default. We could
> fix that wart if we wanted to.

We should "merge" `C-x C-q` and `e`, indeed, like we do for Dired.


        Stefan

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   > "C-x C-q" in an "occur" buffer still leaves some non-useful key
   > bindings around, like for "DEL". To usefully edit an "occur" buffer
   > you need to run `occur-edit-mode', bound to "e" by default. We could
   > fix that wart if we wanted to.

   We should "merge" `C-x C-q` and `e`, indeed, like we do for Dired.

Sounds like a nice idea!

Re: [External] : Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 29, 2023 at 2:11 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > "C-x C-q" in an "occur" buffer still leaves some non-useful key
> > bindings around, like for "DEL". To usefully edit an "occur" buffer
> > you need to run `occur-edit-mode', bound to "e" by default. We could
> > fix that wart if we wanted to.
>
> We should "merge" `C-x C-q` and `e`, indeed, like we do for Dired.

Yes, and isn't a buffer-local value for read-only-mode-hook the
suitable way to fix that?

Re: [External] : Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 29, 2023 at 2:16 PM João Távora <joaotavora@gmail.com> wrote:
>
> On Fri, Sep 29, 2023 at 2:11 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >
> > > "C-x C-q" in an "occur" buffer still leaves some non-useful key
> > > bindings around, like for "DEL". To usefully edit an "occur" buffer
> > > you need to run `occur-edit-mode', bound to "e" by default. We could
> > > fix that wart if we wanted to.
> >
> > We should "merge" `C-x C-q` and `e`, indeed, like we do for Dired.
>
> Yes, and isn't a buffer-local value for read-only-mode-hook the
> suitable way to fix that?

For context, I'm asking because I don't think rebinding `C-x C-q`
to a different command (like in the case of Dired) is as good
as using the existing hook.  It should be much more consistent
to use the standard facilities of the existing minor mode.

Likewise, I don't think having "-read-only" or "-edit" variations
of each major mode is a good idea.  We can activate and deactivate
keymaps on the fly right?

João

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

>> > We should "merge" `C-x C-q` and `e`, indeed, like we do for Dired.
>> Yes, and isn't a buffer-local value for read-only-mode-hook the
>> suitable way to fix that?

I must admit I haven't looked at what the implementation
would/should/could look like.

> For context, I'm asking because I don't think rebinding `C-x C-q`
> to a different command (like in the case of Dired) is as good
> as using the existing hook.  It should be much more consistent
> to use the standard facilities of the existing minor mode.

In general, I agree.  There are degrees, tho.
E.g. Dired doesn't touch `C-x C-q`, strictly speaking, instead it remaps
`read-only-mode` which is much cleaner already.

But admittedly, looking at the code of `dired-toggle-read-only` I can't
see an obvious technical reason why it couldn't use
`read-only-mode-hook` instead (tho maybe the interaction with
`view-read-only` could be a problem [I'd argue the problem is in
`view-read-only`, tho]).  Yet, at the same time changing the minor mode
from within `read-only-mode-hook` doesn't sound super healthy (from
a purely philosophical standpoint, at least), so I think such a change
would probably make more sense if we were to change wdired so it's not
a wholly separate major mode.

> Likewise, I don't think having "-read-only" or "-edit" variations
> of each major mode is a good idea.  We can activate and deactivate
> keymaps on the fly right?

Again, I'd tend to agree but I'd have to see the details to be sure.
In the case of Dired the two states are very different (completely
different key-bindings and sets of operations), so while I think the use
of a separate major mode is a historical accident, it's not too bad of
a choice.

In contrast in `diff-mode` I originally didn't even use
a `read-only-mode-hook`; instead I used
`minor-mode-overriding-map-alist` to dynamically activate/deactivate the
key bindings according to the value of `buffer-read-only`.


        Stefan

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> > We should "merge" `C-x C-q` and `e`, indeed, like we do for Dired.
>>> Yes, and isn't a buffer-local value for read-only-mode-hook the
>>> suitable way to fix that?
>
> I must admit I haven't looked at what the implementation
> would/should/could look like.
>
>> For context, I'm asking because I don't think rebinding `C-x C-q`
>> to a different command (like in the case of Dired) is as good
>> as using the existing hook.  It should be much more consistent
>> to use the standard facilities of the existing minor mode.
>
> In general, I agree.  There are degrees, tho.
> E.g. Dired doesn't touch `C-x C-q`, strictly speaking, instead it remaps
> `read-only-mode` which is much cleaner already.

Agreed.  But until someone cleans that up, perhaps we're already making
progress if we install this simple change?

diff --git a/lisp/replace.el b/lisp/replace.el
index 6b06e48c384..b40843da141 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1315,6 +1315,7 @@ occur-mode-map
     (define-key map "r" 'occur-rename-buffer)
     (define-key map "c" 'clone-buffer)
     (define-key map "\C-c\C-f" 'next-error-follow-minor-mode)
+    (define-key map [remap read-only-mode] #'occur-edit-mode)
     (bindings--define-key map [menu-bar occur] (cons "Occur" occur-menu-map))
     map)
   "Keymap for `occur-mode'.")

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Howard Melman]

Stefan Kangas <stefankangas@gmail.com> writes:

>> In general, I agree.  There are degrees, tho.
>> E.g. Dired doesn't touch `C-x C-q`, strictly speaking, instead it remaps
>> `read-only-mode` which is much cleaner already.
>
> Agreed.  But until someone cleans that up, perhaps we're already making
> progress if we install this simple change?

I'm all for progress via simple changes.

FWIW, Emacs bug#16214 has a bunch more on the
inconsistencies of wdired, woccur, and wgrep.

-- 

Howard

RE: [External] : Re: Adding refactoring capabilities to Emacs

[Drew Adams]

> > "C-x C-q" in an "occur" buffer still leaves some non-useful key
> > bindings around, like for "DEL". To usefully edit an "occur" buffer
> > you need to run `occur-edit-mode', bound to "e" by default. We could
> > fix that wart if we wanted to.
> 
> We should "merge" `C-x C-q` and `e`, indeed, like we do for Dired.

+1

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: "Alfred M. Szmidt" <ams@gnu.org>,  drew.adams@oracle.com,
>   dmitry@gutov.dev,  joaotavora@gmail.com,  philipk@posteo.net,
>   monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Fri, 29 Sep 2023 14:30:30 +0200
> 
> >>>>> On Fri, 29 Sep 2023 13:36:22 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
>     >> From: "Alfred M. Szmidt" <ams@gnu.org>
>     >> Cc: dmitry@gutov.dev, joaotavora@gmail.com, philipk@posteo.net,
>     >> monnier@iro.umontreal.ca, eliz@gnu.org, emacs-devel@gnu.org
>     >> Date: Tue, 26 Sep 2023 11:22:20 -0400
>     >> 
>     >> Occur and grep buffers are by default read-only (and there is a
>     >> regression in that too
> 
>     Eli> It is only a "regression" if you still have your muscle memory from 20
>     Eli> years ago, since it's when we started making the compilation-mode
>     Eli> buffers read-opnly.
> 
> "C-x C-q" in an "occur" buffer still leaves some non-useful key
> bindings around, like for "DEL". To usefully edit an "occur" buffer
> you need to run `occur-edit-mode', bound to "e" by default. We could
> fix that wart if we wanted to.

I know that, right?  My point was that if the mode doesn't give you a
special-purpose editing sub-mode (like Occur and Dired, etc.), then
"C-x C-q" should do.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

To wit, the difference Dimitry is raising is:

C-x C-f ~/RMAIL RET			Opens ~/RMAIL in Fundamental mode
C-x C-f ~/emacs-build.gcov RET		Opens file in Compilation-mode

vs.

M-x rmail RET				Opens ~/RMAIL using the RMAIL "program"
M-x compile RET make RET		.. similar

The two later, the thing you are working is by "dedicated mode"; the
first ones you are working with the file in the specific mode.  This
is similar to diff, occur, etc.  The issue is that sometimes, a mode
might want more or less .. RMAIL is a good example where it wants
more.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: dmitry@gutov.dev, joaotavora@gmail.com, philipk@posteo.net,
> 	monnier@iro.umontreal.ca, eliz@gnu.org, emacs-devel@gnu.org
> Date: Tue, 26 Sep 2023 11:27:26 -0400
> 
> To wit, the difference Dimitry is raising is:
> 
> C-x C-f ~/RMAIL RET			Opens ~/RMAIL in Fundamental mode
> C-x C-f ~/emacs-build.gcov RET		Opens file in Compilation-mode
> 
> vs.
> 
> M-x rmail RET				Opens ~/RMAIL using the RMAIL "program"
> M-x compile RET make RET		.. similar
> 
> The two later, the thing you are working is by "dedicated mode"; the
> first ones you are working with the file in the specific mode.  This
> is similar to diff, occur, etc.  The issue is that sometimes, a mode
> might want more or less .. RMAIL is a good example where it wants
> more.

I see no need for consistency in these matters.  We sometimes let "C-x
C-f" open files in specialized modes, and sometimes not.  If you are
unhappy with either default, you can always customize your Emacs to do
what you prefer.

So let's please stop this futile discussion about personal
preferences.  It did nothing except raising the noise level on the
list by two orders of magnitude.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   So let's please stop this futile discussion about personal
   preferences.  It did nothing except raising the noise level on the
   list by two orders of magnitude.

Can you please stop this condescending tone? Everything at the end of
the day is about personal preference, be it whatever the maintainers
add or what the users prefer.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: drew.adams@oracle.com, dmitry@gutov.dev, joaotavora@gmail.com,
> 	philipk@posteo.net, monnier@iro.umontreal.ca,
> 	emacs-devel@gnu.org
> Date: Fri, 29 Sep 2023 08:36:49 -0400
> 
>    So let's please stop this futile discussion about personal
>    preferences.  It did nothing except raising the noise level on the
>    list by two orders of magnitude.
> 
> Can you please stop this condescending tone?

Sorry, no.  When a discussion is blown out of proportion and increases
the noise level too much, it is part of my job to step in and try to
lower the noise.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

Up to now, before you bardged, in this discussion was entierly
amicable.  You're entierly free to not participate, specially if you
consider yelling at people is somehow being a maintainer, or telling
that their opinions do not matter when they raised them _once_.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: drew.adams@oracle.com, dmitry@gutov.dev, joaotavora@gmail.com,
> 	philipk@posteo.net, monnier@iro.umontreal.ca,
> 	emacs-devel@gnu.org
> Date: Fri, 29 Sep 2023 11:40:09 -0400
> 
> Up to now, before you bardged, in this discussion was entierly
> amicable.

Not what I saw.

> You're entierly free to not participate, specially if you consider
> yelling at people is somehow being a maintainer, or telling that
> their opinions do not matter when they raised them _once_.

Now you are being unkind, even rude.  Are GNU Kind Communications
Guidelines only for others to follow?

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   > You're entierly free to not participate, specially if you consider
   > yelling at people is somehow being a maintainer, or telling that
   > their opinions do not matter when they raised them _once_.

   Now you are being unkind, even rude.  Are GNU Kind Communications
   Guidelines only for others to follow?

None of the above is unkind or rude.  It is pointing out of line
behaviour.  So please, kindly, go get an ice cream and talk to you
later, keep the apology for someone else -- we all can have bad days.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: drew.adams@oracle.com, dmitry@gutov.dev, joaotavora@gmail.com,
> 	philipk@posteo.net, monnier@iro.umontreal.ca,
> 	emacs-devel@gnu.org
> Date: Fri, 29 Sep 2023 12:32:00 -0400
> 
>    > You're entierly free to not participate, specially if you consider
>    > yelling at people is somehow being a maintainer, or telling that
>    > their opinions do not matter when they raised them _once_.
> 
>    Now you are being unkind, even rude.  Are GNU Kind Communications
>    Guidelines only for others to follow?
> 
> None of the above is unkind or rude.

Yeah, right.  Suggest you ask a friend for a second opinion.

> It is pointing out of line behaviour.  So please, kindly, go get an
> ice cream and talk to you later, keep the apology for someone else
> -- we all can have bad days.

So being condescending is okay for you, but not for others, I guess.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   So being condescending is okay for you, but not for others, I guess.

I was not condescending.  At this point, I expect to get an apology
from you.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> From: "Alfred M. Szmidt" <ams@gnu.org>
> Cc: drew.adams@oracle.com, dmitry@gutov.dev, joaotavora@gmail.com,
>  philipk@posteo.net, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> Date: Fri, 29 Sep 2023 13:32:36 -0400
> 
>    So being condescending is okay for you, but not for others, I guess.
> 
> I was not condescending.  At this point, I expect to get an apology
> from you.

I see no reason to apologize.  I'm just doing my job, according to all
the known good principles.

I have nothing more to say to you in this off-topic sub-sub-thread,
except that you should respect the opinions of the project maintainer
a bit more than you do, even if they seem to contradict yours.

Re: [External] : Re: Adding refactoring capabilities to Emacs

[Stefan Monnier]

Hi Eli,

Just a reminder that in the long run, you're always better off just
leaving a discussion when it becomes unpleasant, rather than try and
convince others that they were unpleasant.

[ I'm even tempted to say that seen from the outside, the one who stops
  first is usually the one who was right, to start with.  ]


        Stefan


Eli Zaretskii [2023-09-29 19:51:47] wrote:
>> From: "Alfred M. Szmidt" <ams@gnu.org>
>> Cc: drew.adams@oracle.com, dmitry@gutov.dev, joaotavora@gmail.com,
>> 	philipk@posteo.net, monnier@iro.umontreal.ca,
>> 	emacs-devel@gnu.org
>> Date: Fri, 29 Sep 2023 12:32:00 -0400
>> 
>>    > You're entierly free to not participate, specially if you consider
>>    > yelling at people is somehow being a maintainer, or telling that
>>    > their opinions do not matter when they raised them _once_.
>> 
>>    Now you are being unkind, even rude.  Are GNU Kind Communications
>>    Guidelines only for others to follow?
>> 
>> None of the above is unkind or rude.
>
> Yeah, right.  Suggest you ask a friend for a second opinion.
>
>> It is pointing out of line behaviour.  So please, kindly, go get an
>> ice cream and talk to you later, keep the apology for someone else
>> -- we all can have bad days.
>
> So being condescending is okay for you, but not for others, I guess.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 08:36, Alfred M. Szmidt wrote:
> With vc-diff (similar, diff-buffer-with-file) you are diffing whatever
> you have against VC (or file), there is little point to edit via the
> diff, and you'd more likley just save this to file or an email.  So in
> those cases the buffer being read-only makes more sense.

Sounds like you missed the new feature: committing a patch (C-v C-v in a 
vc-diff buffer). You would usually edit it as well.

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   > With vc-diff (similar, diff-buffer-with-file) you are diffing whatever
   > you have against VC (or file), there is little point to edit via the
   > diff, and you'd more likley just save this to file or an email.  So in
   > those cases the buffer being read-only makes more sense.

   Sounds like you missed the new feature: committing a patch (C-v C-v in a 
   vc-diff buffer). You would usually edit it as well.

I really hope you typoed that, rebinding C-v to something other than
scrolling is a beyond terrible idea.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 26/09/2023 15:03, Alfred M. Szmidt wrote:
>     > With vc-diff (similar, diff-buffer-with-file) you are diffing whatever
>     > you have against VC (or file), there is little point to edit via the
>     > diff, and you'd more likley just save this to file or an email.  So in
>     > those cases the buffer being read-only makes more sense.
> 
>     Sounds like you missed the new feature: committing a patch (C-v C-v in a
>     vc-diff buffer). You would usually edit it as well.
> 
> I really hope you typoed that, rebinding C-v to something other than
> scrolling is a beyond terrible idea.

Correction: C-x v v.

Re: Adding refactoring capabilities to Emacs

[Alfred M. Szmidt]

   >     > With vc-diff (similar, diff-buffer-with-file) you are diffing whatever
   >     > you have against VC (or file), there is little point to edit via the
   >     > diff, and you'd more likley just save this to file or an email.  So in
   >     > those cases the buffer being read-only makes more sense.
   > 
   >     Sounds like you missed the new feature: committing a patch (C-v C-v in a
   >     vc-diff buffer). You would usually edit it as well.
   > 
   > I really hope you typoed that, rebinding C-v to something other than
   > scrolling is a beyond terrible idea.

   Correction: C-x v v.

Thanks, and for making me jump from my seat, here is a bug report:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66211

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> Date: Tue, 26 Sep 2023 15:11:17 +0300
> Cc: joaotavora@gmail.com, monnier@iro.umontreal.ca, eliz@gnu.org,
>  philipk@posteo.net, emacs-devel@gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 26/09/2023 15:03, Alfred M. Szmidt wrote:
> >     > With vc-diff (similar, diff-buffer-with-file) you are diffing whatever
> >     > you have against VC (or file), there is little point to edit via the
> >     > diff, and you'd more likley just save this to file or an email.  So in
> >     > those cases the buffer being read-only makes more sense.
> > 
> >     Sounds like you missed the new feature: committing a patch (C-v C-v in a
> >     vc-diff buffer). You would usually edit it as well.
> > 
> > I really hope you typoed that, rebinding C-v to something other than
> > scrolling is a beyond terrible idea.
> 
> Correction: C-x v v.

That functionality of "C-x v v" is only available for Git (and
basically is undocumented), so it's too early to declare victory.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 29/09/2023 09:52, Eli Zaretskii wrote:
>> Date: Tue, 26 Sep 2023 15:11:17 +0300
>> Cc:joaotavora@gmail.com,monnier@iro.umontreal.ca,eliz@gnu.org,
>>   philipk@posteo.net,emacs-devel@gnu.org
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 26/09/2023 15:03, Alfred M. Szmidt wrote:
>>>      > With vc-diff (similar, diff-buffer-with-file) you are diffing whatever
>>>      > you have against VC (or file), there is little point to edit via the
>>>      > diff, and you'd more likley just save this to file or an email.  So in
>>>      > those cases the buffer being read-only makes more sense.
>>>
>>>      Sounds like you missed the new feature: committing a patch (C-v C-v in a
>>>      vc-diff buffer). You would usually edit it as well.
>>>
>>> I really hope you typoed that, rebinding C-v to something other than
>>> scrolling is a beyond terrible idea.
>> Correction: C-x v v.
> That functionality of "C-x v v" is only available for Git (and
> basically is undocumented), so it's too early to declare victory.

It's available for Git, for Hg (two separate dedicated implementations), 
and for the other VCSes too on best-effort basis (see the function 
vc-default-checkin-patch).

Here was me asking for feedback from SVN/CVS users:

https://lists.gnu.org/archive/html/emacs-devel/2022-10/msg01355.html
https://lists.gnu.org/archive/html/emacs-devel/2022-10/msg01954.html

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> Date: Fri, 29 Sep 2023 13:21:43 +0300
> Cc: ams@gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca,
>  philipk@posteo.net, emacs-devel@gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> >> Correction: C-x v v.
> > That functionality of "C-x v v" is only available for Git (and
> > basically is undocumented), so it's too early to declare victory.
> 
> It's available for Git, for Hg (two separate dedicated implementations), 
> and for the other VCSes too on best-effort basis (see the function 
> vc-default-checkin-patch).

Yes, I found this out by now, thanks.  Which makes it even worse that
no documentation of "C-x v v" mentions that, anywhere!  (And many
things that "C-x v v" did for years aren't documented, either, sigh.)

We should do better than that.

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 29/09/2023 18:20, Eli Zaretskii wrote:
>> Date: Fri, 29 Sep 2023 13:21:43 +0300
>> Cc: ams@gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca,
>>   philipk@posteo.net, emacs-devel@gnu.org
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>>>> Correction: C-x v v.
>>> That functionality of "C-x v v" is only available for Git (and
>>> basically is undocumented), so it's too early to declare victory.
>>
>> It's available for Git, for Hg (two separate dedicated implementations),
>> and for the other VCSes too on best-effort basis (see the function
>> vc-default-checkin-patch).
> 
> Yes, I found this out by now, thanks.  Which makes it even worse that
> no documentation of "C-x v v" mentions that, anywhere!  (And many
> things that "C-x v v" did for years aren't documented, either, sigh.)
> 
> We should do better than that.

We should. But the problem started at the "no feedback" step.

When there is no interest even among the greybeards on this list, why 
spend extra effort advertising something untested?

Re: Adding refactoring capabilities to Emacs

[Eli Zaretskii]

> Date: Fri, 29 Sep 2023 20:20:46 +0300
> Cc: ams@gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca,
>  philipk@posteo.net, emacs-devel@gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 29/09/2023 18:20, Eli Zaretskii wrote:
> >> Date: Fri, 29 Sep 2023 13:21:43 +0300
> >> Cc: ams@gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca,
> >>   philipk@posteo.net, emacs-devel@gnu.org
> >> From: Dmitry Gutov <dmitry@gutov.dev>
> >>
> >>>> Correction: C-x v v.
> >>> That functionality of "C-x v v" is only available for Git (and
> >>> basically is undocumented), so it's too early to declare victory.
> >>
> >> It's available for Git, for Hg (two separate dedicated implementations),
> >> and for the other VCSes too on best-effort basis (see the function
> >> vc-default-checkin-patch).
> > 
> > Yes, I found this out by now, thanks.  Which makes it even worse that
> > no documentation of "C-x v v" mentions that, anywhere!  (And many
> > things that "C-x v v" did for years aren't documented, either, sigh.)
> > 
> > We should do better than that.
> 
> We should. But the problem started at the "no feedback" step.
> 
> When there is no interest even among the greybeards on this list, why 
> spend extra effort advertising something untested?

I'm not sure I follow: the undocumented features I had in mind were
released with Emacs 29.1.  So if it was untested, why did we release
it?

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 29/09/2023 20:36, Eli Zaretskii wrote:
>> Date: Fri, 29 Sep 2023 20:20:46 +0300
>> Cc:ams@gnu.org,joaotavora@gmail.com,monnier@iro.umontreal.ca,
>>   philipk@posteo.net,emacs-devel@gnu.org
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 29/09/2023 18:20, Eli Zaretskii wrote:
>>>> Date: Fri, 29 Sep 2023 13:21:43 +0300
>>>> Cc:ams@gnu.org,joaotavora@gmail.com,monnier@iro.umontreal.ca,
>>>>    philipk@posteo.net,emacs-devel@gnu.org
>>>> From: Dmitry Gutov<dmitry@gutov.dev>
>>>>
>>>>>> Correction: C-x v v.
>>>>> That functionality of "C-x v v" is only available for Git (and
>>>>> basically is undocumented), so it's too early to declare victory.
>>>> It's available for Git, for Hg (two separate dedicated implementations),
>>>> and for the other VCSes too on best-effort basis (see the function
>>>> vc-default-checkin-patch).
>>> Yes, I found this out by now, thanks.  Which makes it even worse that
>>> no documentation of "C-x v v" mentions that, anywhere!  (And many
>>> things that "C-x v v" did for years aren't documented, either, sigh.)
>>>
>>> We should do better than that.
>> We should. But the problem started at the "no feedback" step.
>>
>> When there is no interest even among the greybeards on this list, why
>> spend extra effort advertising something untested?
> I'm not sure I follow: the undocumented features I had in mind were
> released with Emacs 29.1.  So if it was untested, why did we release
> it?

Same as we do most other new features' interaction with old VCSes (CVS, 
RCS, SCCS and the rest of the zoo): they had 9 months to "stabilize".

FWIW, if nobody's using it, it's not hurting anybody either.

Anyway, Juri did test it a little with Bzr, and I - with Hg, before the 
latter grew a dedicated implementation, but there was no feedback 
regarding any of the others. Would we document it as "definitely works 
with Git/Hg, seemingly with Bzr, and probably with some of the others"?

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 19:05, João Távora wrote:
>>>> diff-apply-everything could work with "someone's patches" just as well.
>>>> That's actually a reason to just add it to diff-mode.
>>> I never said there wasn't.  But don't touch C-c C-c please.
>> Maybe we should start by removing that `C-c C-c` binding from
>> `diff-mode` (users like you who really like it can re-add it easily).
> Strongly object to this.  Remove a 20yo binding?  Aren't there
> really other alternatives??

Obviously there are some, but can you suggest a good one?

Following the semantics of 'C-c C-c' of others modes can make the new 
feature easier to use and discover too.

> Especially in a mode that's read only
> much more often than not?

When I simply visit a patch (which I might want to apply), it doesn't 
open in read-only. Still, to visit the source I've always switched to 
read-only and pressed RET too.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 8, 2023 at 7:46 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> (This one arrived in private, BTW)

Thanks for noticing, hoping it's fine to re-add emacs-devel

> On 08/09/2023 21:39, João Távora wrote:
> > On Fri, Sep 8, 2023 at 7:35 PM Dmitry Gutov<dmitry@gutov.dev>  wrote:
> >> On 08/09/2023 19:05, João Távora wrote:
> >>>>>> diff-apply-everything could work with "someone's patches" just as well.
> >>>>>> That's actually a reason to just add it to diff-mode.
> >>>>> I never said there wasn't.  But don't touch C-c C-c please.
> >>>> Maybe we should start by removing that `C-c C-c` binding from
> >>>> `diff-mode` (users like you who really like it can re-add it easily).
> >>> Strongly object to this.  Remove a 20yo binding?  Aren't there
> >>> really other alternatives??
> >> Obviously there are some, but can you suggest a good one?
> > Just proposed C-c RET.  C-c M-c is also good IMO, so many
> > good shortcuts.
> >
> >> Following the semantics of 'C-c C-c' of others modes can make the new
> >> feature easier to use and discover too.
> > It's not following any such "semantics", unless you bend them really
> > hard.
>
> When I finish a commit, I press 'C-c C-c'. Seems quite logical to me.
>
> Especially if we consider the case where the user edits the diff (e.g.
> with C-k) first.
>
> > C-c C-c means "compile current function" for all SLIME users
> > (and I suspect CIDER users too).
>
> But note that this also has the meaning of "I've updated a piece of
> work, proceed with it", conveyed to the compiler. Not too far off.

I guess, but in effect, nothing's "too far off" if you squint hard
enough.

> > If it has any semantics at all,
> > it's vaguely DWIM.  And the longstanding "Meaning" in the DWIM here
> > is "goto source for this hunk".
>
> There's also change-log-mode which does this. And I think that's it.
>
> Ah, and there's also cc-mode which uses the same binding for
> comment-region. *shrug*

Yeah, it's a DWIM shortcut with "What I Mean" being determined by
historical usage.

> >>> Especially in a mode that's read only
> >>> much more often than not?
> >> When I simply visit a patch (which I might want to apply), it doesn't
> >> open in read-only. Still, to visit the source I've always switched to
> >> read-only and pressed RET too.
> > Wow, that's so much easier than typing C-c C-c which has existed
> > forever.
>
> It's not. But the switch to read-only is a one-time thing and jumping to
> different hunks is often done repeatedly anyways. So I'm probably saving
> on keys, at least in some cases.

OK.  I'm not against that workflow, but it's not my workflow.

Re: Adding refactoring capabilities to Emacs

[Felician Nemeth]

I'm lacking the basics here, so sorry to butt in, but ...

>> 'diff-apply-everything'

How would a user undo such an operation in case of a multi-file edit?
Does the undo system support this?

Thanks.

Re: Adding refactoring capabilities to Emacs

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

On Thu, Sep 7, 2023, 20:06 Felician Nemeth <felician.nemeth@gmail.com>
wrote:

> I'm lacking the basics here, so sorry to butt in, but ...
>
> >> 'diff-apply-everything'
>
> How would a user undo such an operation in case of a multi-file edit?
> Does the undo system support this?
>

No, but someone suggested reversing the diff and applying again. It's not
really undo, but decent.

João

>

[-- Attachment #2: Type: text/html, Size: 999 bytes --]

Re: Adding refactoring capabilities to Emacs

[Ihor Radchenko]

Felician Nemeth <felician.nemeth@gmail.com> writes:

> I'm lacking the basics here, so sorry to butt in, but ...
>
>>> 'diff-apply-everything'
>
> How would a user undo such an operation in case of a multi-file edit?
> Does the undo system support this?

No, but it is still possible. We do multi-file undo in Org agenda via
`org-with-remote-undo'.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Adding refactoring capabilities to Emacs

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

>> How would a user undo such an operation in case of a multi-file edit?
>> Does the undo system support this?

Actually, it might be possible:

In `buffer-undo-list', one entry type is

    An entry (apply FUN-NAME . ARGS) means undo the change with
    (apply FUN-NAME ARGS).
    
    An entry (apply DELTA BEG END FUN-NAME . ARGS) supports selective undo
    in the active region.  BEG and END is the range affected by this entry
    and DELTA is the number of characters added or deleted in that range by
    this change.

This can be used to do anything for undo, including editing other files.

> No, but it is still possible. We do multi-file undo in Org agenda via
> `org-with-remote-undo'.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Thu, Sep 7, 2023 at 8:27 PM Ihor Radchenko <yantar92@posteo.net> wrote:
>
> Ihor Radchenko <yantar92@posteo.net> writes:
>
> >> How would a user undo such an operation in case of a multi-file edit?
> >> Does the undo system support this?
>
> Actually, it might be possible:
>
> In `buffer-undo-list', one entry type is
>
>     An entry (apply FUN-NAME . ARGS) means undo the change with
>     (apply FUN-NAME ARGS).
>
>     An entry (apply DELTA BEG END FUN-NAME . ARGS) supports selective undo
>     in the active region.  BEG and END is the range affected by this entry
>     and DELTA is the number of characters added or deleted in that range by
>     this change.
>
> This can be used to do anything for undo, including editing other files.

In which of the buffer-undo-list's of changed buffers would you place
the entry?  And how would you handle redo, if at all?  Hurts my brain
just to think about it, but I'd love to see a demo.

João

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 07/09/2023 22:06, Felician Nemeth wrote:
> I'm lacking the basics here, so sorry to butt in, but ...
> 
>>> 'diff-apply-everything'
> How would a user undo such an operation in case of a multi-file edit?
> Does the undo system support this?

'C-u C-c C-c', if we're only talking about the interface.

As others have pointed out, the function could also save the list of 
affected buffers and undo points in them.

Finally, it might make sense to offer an option to undo the whole 
changeset if somewhere in the middle a hunk fails to apply (because 
that's a situation that can be difficult to fix otherwise).

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 07/09/2023 17:39, João Távora wrote:
> On Thu, Sep 7, 2023 at 2:00 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
>>
>> On 02/09/2023 12:55, João Távora wrote:
>>> Anyway I invite everyone to have a look and try to improve it, perhaps
>>> moving it out of Eglot into the shiny new "refactoring interface" if
>>> those ever become a thing.
>>
>> Regarding the diff approach vs. "shiny new", I've run a little
>> comparison with VS Code: doing a rename of 'glyph_row' across the Emacs
>> codebase (a moderately sized project) takes about ~1.44s to produce the
>> diff.
> 
> FWIW, I got 0.85s on that same test on a GNU/Linux VM running on a 6
> yo laptop with 4 cores assigned.  IMO this is more than acceptable for
> this contrived example which renames a symbol across many files.

Not that many: these are 260 edits just across 3 files.

And I see nothing contrived about such an example. There exist much 
bigger projects anyway, which we should ideally support well too.

> A much more typical example of renaming a symbol in a single function
> resolved in 0.02s.  Most examples of applying quick fixes and moving
> .hpp inlined member functions to out-of-line in .cpp resolve in similar
> quasi-instantaneous fashion.

That's great, but for such small examples one could almost do without 
advanced interfaces too, couldn't they?

> Now, let me clarify what I meant by "shiny new refactoring interface".
> It's not what you think I meant, I believe. What I meant is a framework
> for:
> 
> 1. collecting 'potential sets of changes' -- for clarity let's call
>     "actions" after LSP nomenclature -- from a number of different
>     backends.  Examples of actions: "rename thing at point", "organize
>     imports", "write as while loops as cl-loop"
> 
> 2. presenting actions names to the user -- either via the minibuffer
>     or by annotating buffer position where these tweaks apply.

These could be split into two:

- A hook which would return the available actions at point. This seems 
to be the primary mode of operations of LSP editors: first you choose 
the location, and then find out which which operations apply to it.

- Some way (possibly also a hook) to find all the "points of interest" 
in a buffer -- these could also be annotated with a lightbulb or etc in 
the margin or fringe. Not sure which place this has when using LSP 
(perhaps some diagnostics with edits attached?), but I vaguely remember 
being suggested lists of refactorings by IDEs I tried in the past (<10 
years ago).

Aside from "renames", which would probably apply almost everywhere 
(though e.g. clangd doesn't know how to rename macros; I wonder at what 
step that we would indicate that to the user).

> 3. after the user chooses an action, develop it into a set of changes
>     -- again, for clarity and consonance with LSP nomenclature, we
>     may call these "edits".

Sure. Possibly carrying more-or-less same information that LSP's edits 
do. Though we could see whether it can be useful to annotate them with 
additional info, e.g. name/type/etc (where we'd deduce that a function 
called X is being renamed, or a file is being moved, or both; which 
could be reflects in the changeset's display more intelligently).

> 4. presenting these edits to the user via a number of different
>     user-confirmation strategies.
> 
> For very common "rename" or "organize imports" actions, steps 1
> and 2 may be coalesced and a special command may be provided that
> goes directly to step 3.  LSP supports these "shortcuts".

Sure. I wouldn't know what position "organize imports" should apply to 
anyway. But to do with backends which don't support it?

> Regarding 4 (which seems to be the current focal point of the
> discussion) Eglot has three main types of confirmation strategies
> available.  See eglot-confirm-server-edits for more.
> 
> * 'diff', which just presents the edits
> * 'summary', which summarizes the edits in the minibuffer and prompts
>    to apply.
> * nil, which means no confirmation at all
> 
> There is also 'maybe-diff' and 'maybe-summary' which behave like
> nil if all the files in the change list are already visited buffers.

Not sure why the cutoff is there: and not for example "if all the 
changes are in the current file", or "are all visible on the screen".

Anyway, the more the merrier, I suppose.

> That said, it's the whole sequence 1 to 4, not just 4, that needs
> to be generalized out of Eglot and into Emacs.  Note that some parts
> of 1..4, like the annotation of actions in buffers or the support for
> file creation and deletion, aren't yet in Eglot, so there's good work
> to be done.  Some of the "annotation" logic already exists in Flymake,
> and Flymake must be taken into the equation too, and early.

The reason for talking about 4 first is twofold:

- I understand 4 better than the rest at this point of time, and it 
could be plugged into some existing bits of functionality already (e.g. 
Eglot's refactorings and Xref/Project names).

- There will likely be two entry points to the new library, just like 
with Xref we have xref-backend-functions and xref-show-xrefs. The former 
depends on the latter. By choosing a good enough data model for step 4 
(e.g. how changes are represented), we can better build requirements for 
step 1, given that whatever hook(s) we add, they would already need to 
be documented with the details about the data in/out flow.

> Also, it would be very good if we could have an early backend which is
> *not* LSP.  An obvious candidate is Elisp.  I'd like to know what is
> the minimal effort to write a lisp function based on the new
> macroexpanding tricks that Stefan Monnier developed that says
> precisely where a given symbol is used as a variable in a function.
> As far as I understand,  these techniques are currently being used for
> accurate completion, but they could maybe be used for accurate
> refactorings.

Xref could easily be an early backend which supports renaming only. 
Eglisp's xref-backend-references could make use of the macroexpanding 
thing to include local variables too.

>> Not to belittle the new addition, though - it's a good step for Eglot.
> 
> As someone who actually uses Eglot daily and is an avid consumer
> of refactoring functionality, my opinion is that the "diff"
> confirmation strategy that Philip proposed and implemented is freaking
> great, even if the implementation is a bit contorted (though mostly
> isolated).
> 
> IMO diff is the lingua franca for communicating changes to source
> code around the world, even in those pretty web interfaces that
> you and many others like.  So it makes full sense to support it
> and to support it well.

I'm okay with diffs myself obviously, and we should keep this UI option 
if feasible, but I'd also prefer to have something more compact 
available, for the "web interface lovers" of the world. Diffs do have 
some barrier to understanding, even we are very much used to them. And 
diff-with-context can be pretty long too.

>> - I would probably want to bind the originally proposed
>> 'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"),
>> but that's already taken in diff-mode.
> 
>> - 'git diff' has a less-frequently used option called '--word-diff'
>> which could reduce the length of the diff in the case I am testing (but
>> I guess diff-mode would have to be updated to support that output). And
>> another way in that direction would be to reduce the size of the diff
>> context (e.g. to 0).
> 
> I'm not at all opposed to implementing other confirmation strategies
> that look more like what VSCode or other editors show users.  Or
> augmenting the existing 'diff' strategy with new shortcuts and stuff.
> But I wouldn't overthink UI details at this point in the game either.

The first suggestion in particular looked like a modest enough tweak 
that does not depend on the rest of this discussion (and it would likely 
have a diff-specific implementation). But if we try to make it atomic, 
that's an increase in complexity, of course.

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Thu, Sep 7, 2023 at 11:39 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> Not that many: these are 260 edits just across 3 files.

glyph_row? As in 'struct glyph_row' of src/dispextern.h?  My clangd
suggested changes to 7 files.   Maybe not contrived but certainly
not common either, and well worth the subsecond latency considering
the time you'd want to spend reviewing such a mammoth change.

> > A much more typical example of renaming ab symbol in a single function
> > resolved in 0.02s.  Most examples of applying quick fixes and moving
> > .hpp inlined member functions to out-of-line in .cpp resolve in similar
> > quasi-instantaneous fashion.
>
> That's great, but for such small examples one could almost do without
> advanced interfaces too, couldn't they?

For local renames, yes, which is precisely why the reason why eglot-rename
has a special 'maybe-summary' confirmation method (I use maybe-diff)
whose point is to skips the confirmation if it detects the change to be "local
enough"

For others,  like extracting methods, etc, not at all.  They may be
simple, but you generally still want to have a look at what's being
especially if it targets on or

> > Now, let me clarify what I meant by "shiny new refactoring interface".
> > It's not what you think I meant, I believe. What I meant is a framework
> > for:
> >
> > 1. collecting 'potential sets of changes' -- for clarity let's call
> >     "actions" after LSP nomenclature -- from a number of different
> >     backends.  Examples of actions: "rename thing at point", "organize
> >     imports", "write as while loops as cl-loop"
> >
> > 2. presenting actions names to the user -- either via the minibuffer
> >     or by annotating buffer position where these tweaks apply.
> - Some way (possibly also a hook) to find all the "points of interest"
> in a buffer -- these could also be annotated with a lightbulb or etc in

That's what I meant yes "annotate".  But I'd leave the annotation
interface to a second phase.  As you note, it's not obvious when to collect
these actions with special affinity to buffer positions .  In LSP, some of
them are already being collected because of diagnostics.

Flymake is the main existing Emacs interface that periodically annotates
the buffer with pieces of information about your code gotten from a server.
It knows how to display things in the fringe, and is fairly customizable.
You can have multiple backends, etc, it works asynchronously and generally
has a lot of common logic for knowing how to update only parts of the
buffer etc.

It could be argued we should use it for this purpose, but then again, it
could be argued we shouldn't, especially if we suspect equating "localized
actions" to diagnostics is simply wrong.  This feature request came up in the
past in the Eglot tracker and I gave it a go once or twice with Flymake.
But maybe I just didn't try hard enough.

> Sure. Possibly carrying more-or-less same information that LSP's edits
> do. Though we could see whether it can be useful to annotate them with
> additional info, e.g. name/type/etc (where we'd deduce that a function
> called X is being renamed, or a file is being moved, or both; which
> could be reflects in the changeset's display more intelligently).

That's not a bad idea, but is an implementation detail.  We
should probably not expose this representation (at least not too early)
to conserve the freedom to change it.

> Sure. I wouldn't know what position "organize imports" should apply to
> anyway. But to do with backends which don't support it?

Maybe at least one of the backends will support it.  Else issue an
error, that's not much more I can see.  Some backends may
not even support rename.  It's not like in xref where "find definitions"
and "find references" which are things that almost all xref backends
would be able to do.


> > Regarding 4 (which seems to be the current focal point of the
> > discussion) Eglot has three main types of confirmation strategies
> > available.  See eglot-confirm-server-edits for more.
> >
> > * 'diff', which just presents the edits
> > * 'summary', which summarizes the edits in the minibuffer and prompts
> >    to apply.
> > * nil, which means no confirmation at all
> >
> > There is also 'maybe-diff' and 'maybe-summary' which behave like
> > nil if all the files in the change list are already visited buffers.
>
> Not sure why the cutoff is there: and not for example "if all the
> changes are in the current file", or "are all visible on the screen".
>
> Anyway, the more the merrier, I suppose.
>
> > That said, it's the whole sequence 1 to 4, not just 4, that needs
> > to be generalized out of Eglot and into Emacs.  Note that some parts
> > of 1..4, like the annotation of actions in buffers or the support for
> > file creation and deletion, aren't yet in Eglot, so there's good work
> > to be done.  Some of the "annotation" logic already exists in Flymake,
> > and Flymake must be taken into the equation too, and early.
>
> The reason for talking about 4 first is twofold:
>
> - I understand 4 better than the rest at this point of time, and it
> could be plugged into some existing bits of functionality already (e.g.
> Eglot's refactorings and Xref/Project names).
>
> - There will likely be two entry points to the new library, just like
> with Xref we have xref-backend-functions and xref-show-xrefs. The former
> depends on the latter. By choosing a good enough data model for step 4
> (e.g. how changes are represented), we can better build requirements for
> step 1, given that whatever hook(s) we add, they would already need to
> be documented with the details about the data in/out flow.
>
> > Also, it would be very good if we could have an early backend which is
> > *not* LSP.  An obvious candidate is Elisp.  I'd like to know what is
> > the minimal effort to write a lisp function based on the new
> > macroexpanding tricks that Stefan Monnier developed that says
> > precisely where a given symbol is used as a variable in a function.
> > As far as I understand,  these techniques are currently being used for
> > accurate completion, but they could maybe be used for accurate
> > refactorings.
>
> Xref could easily be an early backend which supports renaming only.
> Eglisp's xref-backend-references could make use of the macroexpanding
> thing to include local variables too.
>
> >> Not to belittle the new addition, though - it's a good step for Eglot.
> >
> > As someone who actually uses Eglot daily and is an avid consumer
> > of refactoring functionality, my opinion is that the "diff"
> > confirmation strategy that Philip proposed and implemented is freaking
> > great, even if the implementation is a bit contorted (though mostly
> > isolated).
> >
> > IMO diff is the lingua franca for communicating changes to source
> > code around the world, even in those pretty web interfaces that
> > you and many others like.  So it makes full sense to support it
> > and to support it well.
>
> I'm okay with diffs myself obviously, and we should keep this UI option
> if feasible, but I'd also prefer to have something more compact
> available, for the "web interface lovers" of the world. Diffs do have
> some barrier to understanding, even we are very much used to them. And
> diff-with-context can be pretty long too.
>
> >> - I would probably want to bind the originally proposed
> >> 'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"),
> >> but that's already taken in diff-mode.
> >
> >> - 'git diff' has a less-frequently used option called '--word-diff'
> >> which could reduce the length of the diff in the case I am testing (but
> >> I guess diff-mode would have to be updated to support that output). And
> >> another way in that direction would be to reduce the size of the diff
> >> context (e.g. to 0).
> >
> > I'm not at all opposed to implementing other confirmation strategies
> > that look more like what VSCode or other editors show users.  Or
> > augmenting the existing 'diff' strategy with new shortcuts and stuff.
> > But I wouldn't overthink UI details at this point in the game either.
>
> The first suggestion in particular looked like a modest enough tweak
> that does not depend on the rest of this discussion (and it would likely
> have a diff-specific implementation). But if we try to make it atomic,
> that's an increase in complexity, of course.



--
João Távora

Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 14:10, João Távora wrote:
> On Thu, Sep 7, 2023 at 11:39 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
>> Not that many: these are 260 edits just across 3 files.
> 
> glyph_row? As in 'struct glyph_row' of src/dispextern.h?  My clangd
> suggested changes to 7 files.   Maybe not contrived but certainly
> not common either, and well worth the subsecond latency considering
> the time you'd want to spend reviewing such a mammoth change.

Not the type but the identifier with the same name (struct glyph_row 
*glyph_row). Anyway, 3 or 7, it might as well be 70 files in another case.

Although the impact somewhat depends on whether the current performance 
is proportional to the number of files, or the number of edits.

>>> A much more typical example of renaming ab symbol in a single function
>>> resolved in 0.02s.  Most examples of applying quick fixes and moving
>>> .hpp inlined member functions to out-of-line in .cpp resolve in similar
>>> quasi-instantaneous fashion.
>>
>> That's great, but for such small examples one could almost do without
>> advanced interfaces too, couldn't they?
> 
> For local renames, yes, which is precisely why the reason why eglot-rename
> has a special 'maybe-summary' confirmation method (I use maybe-diff)
> whose point is to skips the confirmation if it detects the change to be "local
> enough"

But a rename can just as well be global. Which is simple in theory, but 
it can be a good idea to double-check whether all identifiers are found 
anyway (or check for false positives). Though perhaps it's my habit due 
to working with Grep-based renames a lot.

> For others,  like extracting methods, etc, not at all.  They may be
> simple, but you generally still want to have a look at what's being
> especially if it targets on or

Sure.

>>> Now, let me clarify what I meant by "shiny new refactoring interface".
>>> It's not what you think I meant, I believe. What I meant is a framework
>>> for:
>>>
>>> 1. collecting 'potential sets of changes' -- for clarity let's call
>>>      "actions" after LSP nomenclature -- from a number of different
>>>      backends.  Examples of actions: "rename thing at point", "organize
>>>      imports", "write as while loops as cl-loop"
>>>
>>> 2. presenting actions names to the user -- either via the minibuffer
>>>      or by annotating buffer position where these tweaks apply.
>> - Some way (possibly also a hook) to find all the "points of interest"
>> in a buffer -- these could also be annotated with a lightbulb or etc in
> 
> That's what I meant yes "annotate".  But I'd leave the annotation
> interface to a second phase.  As you note, it's not obvious when to collect
> these actions with special affinity to buffer positions .  In LSP, some of
> them are already being collected because of diagnostics.

Yes, when to collect and how to display. The collection method could 
also be "pull" or "push": with hooks we usually do the "pull" thing, but 
since flymake (and often other diagnostics) work asynchronously a "push" 
mechanism might make more sense.

> Flymake is the main existing Emacs interface that periodically annotates
> the buffer with pieces of information about your code gotten from a server.
> It knows how to display things in the fringe, and is fairly customizable.
> You can have multiple backends, etc, it works asynchronously and generally
> has a lot of common logic for knowing how to update only parts of the
> buffer etc.
> 
> It could be argued we should use it for this purpose, but then again, it
> could be argued we shouldn't, especially if we suspect equating "localized
> actions" to diagnostics is simply wrong.  This feature request came up in the
> past in the Eglot tracker and I gave it a go once or twice with Flymake.
> But maybe I just didn't try hard enough.

We should probably abstract over Flymake either way. But I agree that 
all this can be left until later.

>> Sure. Possibly carrying more-or-less same information that LSP's edits
>> do. Though we could see whether it can be useful to annotate them with
>> additional info, e.g. name/type/etc (where we'd deduce that a function
>> called X is being renamed, or a file is being moved, or both; which
>> could be reflects in the changeset's display more intelligently).
> 
> That's not a bad idea, but is an implementation detail.  We
> should probably not expose this representation (at least not too early)
> to conserve the freedom to change it.

You skipped over the remainder of my email where I explained why I liked 
to start with step 4.

We can still want to be able to tweak it during Emacs 30's development, 
of course, and maybe even later, but it's impossible not to discuss and 
document if we want customizable/swappable UIs for confirmation anyway.

>> Sure. I wouldn't know what position "organize imports" should apply to
>> anyway. But to do with backends which don't support it?
> 
> Maybe at least one of the backends will support it.  Else issue an
> error, that's not much more I can see.  Some backends may
> not even support rename.  It's not like in xref where "find definitions"
> and "find references" which are things that almost all xref backends
> would be able to do.

Backends which don't support rename could fall back to xref which worst 
case falls back to grep. Or something like that.

Anyway, I guess we'll implicitly decide that both "rename" and "organize 
imports" are always available and create commands with dedicated 
bindings for them?

Why the latter, BTW? Or are there more commands like that? From what I'm 
reading, "organize imports" is not a part of the official LSP protocol, 
though there are extensions.

>>> Regarding 4 (which seems to be the current focal point of the
>>> discussion) Eglot has three main types of confirmation strategies
>>> available.  See eglot-confirm-server-edits for more.
>>>
>>> * 'diff', which just presents the edits
>>> * 'summary', which summarizes the edits in the minibuffer and prompts
>>>     to apply.
>>> * nil, which means no confirmation at all
>>>
>>> There is also 'maybe-diff' and 'maybe-summary' which behave like
>>> nil if all the files in the change list are already visited buffers.
>>
>> Not sure why the cutoff is there: and not for example "if all the
>> changes are in the current file", or "are all visible on the screen".
>>
>> Anyway, the more the merrier, I suppose.
>>
>>> That said, it's the whole sequence 1 to 4, not just 4, that needs
>>> to be generalized out of Eglot and into Emacs.  Note that some parts
>>> of 1..4, like the annotation of actions in buffers or the support for
>>> file creation and deletion, aren't yet in Eglot, so there's good work
>>> to be done.  Some of the "annotation" logic already exists in Flymake,
>>> and Flymake must be taken into the equation too, and early.
>>
>> The reason for talking about 4 first is twofold:
>>
>> - I understand 4 better than the rest at this point of time, and it
>> could be plugged into some existing bits of functionality already (e.g.
>> Eglot's refactorings and Xref/Project names).
>>
>> - There will likely be two entry points to the new library, just like
>> with Xref we have xref-backend-functions and xref-show-xrefs. The former
>> depends on the latter. By choosing a good enough data model for step 4
>> (e.g. how changes are represented), we can better build requirements for
>> step 1, given that whatever hook(s) we add, they would already need to
>> be documented with the details about the data in/out flow.
>>
>>> Also, it would be very good if we could have an early backend which is
>>> *not* LSP.  An obvious candidate is Elisp.  I'd like to know what is
>>> the minimal effort to write a lisp function based on the new
>>> macroexpanding tricks that Stefan Monnier developed that says
>>> precisely where a given symbol is used as a variable in a function.
>>> As far as I understand,  these techniques are currently being used for
>>> accurate completion, but they could maybe be used for accurate
>>> refactorings.
>>
>> Xref could easily be an early backend which supports renaming only.
>> Eglisp's xref-backend-references could make use of the macroexpanding
>> thing to include local variables too.
>>
>>>> Not to belittle the new addition, though - it's a good step for Eglot.
>>>
>>> As someone who actually uses Eglot daily and is an avid consumer
>>> of refactoring functionality, my opinion is that the "diff"
>>> confirmation strategy that Philip proposed and implemented is freaking
>>> great, even if the implementation is a bit contorted (though mostly
>>> isolated).
>>>
>>> IMO diff is the lingua franca for communicating changes to source
>>> code around the world, even in those pretty web interfaces that
>>> you and many others like.  So it makes full sense to support it
>>> and to support it well.
>>
>> I'm okay with diffs myself obviously, and we should keep this UI option
>> if feasible, but I'd also prefer to have something more compact
>> available, for the "web interface lovers" of the world. Diffs do have
>> some barrier to understanding, even we are very much used to them. And
>> diff-with-context can be pretty long too.
>>
>>>> - I would probably want to bind the originally proposed
>>>> 'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"),
>>>> but that's already taken in diff-mode.
>>>
>>>> - 'git diff' has a less-frequently used option called '--word-diff'
>>>> which could reduce the length of the diff in the case I am testing (but
>>>> I guess diff-mode would have to be updated to support that output). And
>>>> another way in that direction would be to reduce the size of the diff
>>>> context (e.g. to 0).
>>>
>>> I'm not at all opposed to implementing other confirmation strategies
>>> that look more like what VSCode or other editors show users.  Or
>>> augmenting the existing 'diff' strategy with new shortcuts and stuff.
>>> But I wouldn't overthink UI details at this point in the game either.
>>
>> The first suggestion in particular looked like a modest enough tweak
>> that does not depend on the rest of this discussion (and it would likely
>> have a diff-specific implementation). But if we try to make it atomic,
>> that's an increase in complexity, of course.
> 
> 
> 
> --
> João Távora

Re: Adding refactoring capabilities to Emacs

[João Távora]

Dmitry Gutov <dmitry@gutov.dev> writes:

> Although the impact somewhat depends on whether the current
> performance is proportional to the number of files, or the number of
> edits.

I think it depends on both, but a large number of edits is relatively
slow anyway.  That's probably because you have to make lots of
arrangements to get correct undo behaviour and call the self-admittedly
"very slow" replace-buffer-contents C functions.  Anyway, I never looked
at performance because noone ever complained until you (and it didn't
and still doesn't bother me).  But maybe there's some low-hanging fruit
there.

> You skipped over the remainder of my email where I explained why I
> liked to start with step 4.

Probably didn't have anything useful to add.  You can focus on whatever
you part you like, of course.

> We can still want to be able to tweak it during Emacs 30's
> development, of course, and maybe even later, but it's impossible not
> to discuss and document if we want customizable/swappable UIs for
> confirmation anyway.

I think everyone already agreed we want that.

> Anyway, I guess we'll implicitly decide that both "rename" and
> "organize imports" are always available and create commands with
> dedicated bindings for them?

At least for "rename".  For the others, not so sure yet.

> Why the latter, BTW? Or are there more commands like that? From what
> I'm reading, "organize imports" is not a part of the official LSP
> protocol, though there are extensions.

It's a common code for an LSP action.  Other common names:

 "source.organizeImports"
 "refactor.extract"
 "refactor.inline"
 "refactor.rewrite"
 "quickfix"

>>> Not sure why the cutoff is there: and not for example "if all the
>>> changes are in the current file", or "are all visible on the
>>> screen".

Seemed reasonable and easy to implement, so I did it.  Noone complained
and I quite like it.  Your ideas are also quite good, so other cutoff
criteria welcome (even if just to Eglot right now) if you want to keep
shaving this interface yak :-)

Re: Adding refactoring capabilities to Emacs

[Eshel Yaron]

João Távora <joaotavora@gmail.com> writes:

> Also, it would be very good if we could have an early backend which is
> *not* LSP.  An obvious candidate is Elisp.

FWIW, I think `sweeprolog`, my NonGNU ELPA package for SWI-Prolog can
also a good non-LSP backend.  Elisp would be perfect, no doubt.
But `sweeprolog` is interesting because it does a lot of code analysis
that's useful for refactoring.  Namely it provides precise project-wide
cross-reference data that can be used for renaming predicates.  Renaming
local variables is already implemented, with a simple UI: you type `C-c
C-r`, it prompts you for a variable to rename in the current clause
(with completion), you hit `RET` to choose the variable at point, you
insert a new name, and it renames the variable at once.  That's alright
for renaming local variables, but it would be great to simply plug in to
a generic refactoring UI.

So I can probably try and implement any "refactoring backend functions"
the new refactoring interface includes, and see how it goes.


Best,

Eshel

Re: Adding refactoring capabilities to Emacs

[João Távora]

On Fri, Sep 8, 2023 at 1:46 PM Eshel Yaron <me@eshelyaron.com> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > Also, it would be very good if we could have an early backend which is
> > *not* LSP.  An obvious candidate is Elisp.
>
> FWIW, I think `sweeprolog`, my NonGNU ELPA package for SWI-Prolog can
> also a good non-LSP backend.  Elisp would be perfect, no doubt.
> But `sweeprolog` is interesting because it does a lot of code analysi
[...]
> insert a new name, and it renames the variable at once.  That's alright
> for renaming local variables, but it would be great to simply plug in to
> a generic refactoring UI.

Great.  Precise project-wide analysis (as opposed to more-or-less dumb
grep-based) is exactly the main thing we should be targeting IMO.
 Is "rename" the only thing it does or can you, say, extract subexpressions
to variables, organize imports, etc?

> So I can probably try and implement any "refactoring backend functions"
> the new refactoring interface includes, and see how it goes.

Sounds like a plan!

João

Re: Adding refactoring capabilities to Emacs

[Eshel Yaron]

João Távora <joaotavora@gmail.com> writes:

>> > Also, it would be very good if we could have an early backend which is
>> > *not* LSP.  An obvious candidate is Elisp.
>>
>> FWIW, I think `sweeprolog`, my NonGNU ELPA package for SWI-Prolog can
>> also a good non-LSP backend.  Elisp would be perfect, no doubt.
>> But `sweeprolog` is interesting because it does a lot of code analysi
> [...]
>> insert a new name, and it renames the variable at once.  That's alright
>> for renaming local variables, but it would be great to simply plug in to
>> a generic refactoring UI.
>
> Great.  Precise project-wide analysis (as opposed to more-or-less dumb
> grep-based) is exactly the main thing we should be targeting IMO.
> Is "rename" the only thing it does or can you, say, extract subexpressions
> to variables, organize imports, etc?

Yes, there are some other things already in place, like updating imports
and adding exports.  I have a work-in-progress implementation for
extracting subexpressions (subterms, in Prolog), but nothing in terms of
existing commands.  A relevant feature that's already in place is
creating a definition for an undefined predicate at point.  Its relevant
because I had to consider where to put this new definition, which
applies also to extracting subexpressions.  Currently there's a user
option that controls where to put new definitions, by default it's right
below the predicate at point (the one that references the new predicate).

Extract to new definition (was: Adding refactoring capabilities to Emacs)

[Eshel Yaron]

Hi,

>> ...can you, say, extract subexpressions to variables, organize
>> imports, etc?
>
> Yes, there are some other things already in place, like updating imports
> and adding exports.  I have a work-in-progress implementation for
> extracting subexpressions (subterms, in Prolog), but nothing in terms of
> existing commands.

Since writing this message, I've added that "extract" command to
`sweeprolog`.  I'm still working on some final tweaks, but I'm pretty
happy with the result so I thought I'd share some details and insights.

The new command is called `sweeprolog-extract-region-to-predicate`.  You
call it with point and mark surrounding some code inside one definition
that you want to extract to a new, separate definition.  You do this
either because you want to reuse this piece of computation somewhere
else or simply to break a large complicated definition into smaller
parts.  For example, with the following code in a `sweeprolog-mode`
buffer, where ^ and $ denote the region:

--8<---------------cut here---------------start------------->8---
foo(X, Y, Z) :-
   ^bar1(X,A), bar2(A,B), bar3(C,Y)$,
    baz(Y,Z).
--8<---------------cut here---------------end--------------->8---

Typing `M-x sweeprolog-extract-region-to-predicate RET bar RET` yields:

--8<---------------cut here---------------start------------->8---
foo(X, Y, Z) :-
    bar(X,Y),
    baz(Y,Z).

bar(X, Y) :-
    bar1(X,A), bar2(A,B), bar3(C,Y).
--8<---------------cut here---------------end--------------->8---

The complex goal (expression) with `bar1`, `bar2` and `bar3` makes way
for a single call to `bar`, while a definition of `bar` is created in
the current buffer.  Crucially, this command analyzes the selected goal
and its context to determine how many and which arguments the new
definition should have.

In the simplest case, you mark a piece of code, call the command, it
prompts you for the name of the new definition, and performs the
extraction at once.  There are, however, some subtleties to consider:

- The selection may be invalid.  Invalid means that the contents of the
  region do not constitute a valid Prolog goal, that's a clear user
  error, so this command complains and bails.

- Some extraction operations are unsafe.  Unsafe means that extracting
  the selected piece of code may change the semantics of the program.
  (Un)safety is very language-specific, and it's more difficult to
  determine than validity, since safety is a semantic property while
  validity is syntactic.  For Prolog, extracting a goal that contains a
  "cut" can have a semantic effect, because the "cut" operates on the
  defintion in which it occurs.  In fact, it's not generally decidable
  whether extraction will have an affect on the semantics of the program
  or not.  That's why `sweeprolog-extract-region-to-predicate` asks for
  confirmation if it can't be certain that extracting the specified goal
  preserves the semantics of the surrounding code.

- The name you give to the new definition might already be in use.
  That's something `sweeprolog-extract-region-to-predicate` does not
  currently protect you from, but I intend to address that soon.
  Ideally, the command should detect this collision early, and bail or
  ask for confirmation before changing the buffer.

My hope is that when we get that generic refactoring interface in Emacs,
it'll support these distinctions and handle them appropriately.

This command also has an extension on top of the basic local extraction
operation: If you call it with a prefix argument, then after performing
the extraction as usual, it searches the current buffer for other goals
that the extracted goal subsumes, and suggests replacing them one after
the other with calls to the newly defined predicate with appropriate
arguments.  This is useful when you have several occurrences of some
complex idiom scattered around that you want to extract to a stand alone
definition.

In terms of user interface, there's also a user option that says where
to insert the new definition, and that's about it.
(A couple more details are mentioned in the manual:
https://eshelyaron.com/man/sweep/Extract-Goal.html)


Best,

Eshel

[semi off topic] grep-based refactoring [was: Adding refactoring capabilities to Emacs]

[tomas]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

On Fri, Sep 08, 2023 at 01:52:53PM +0100, João Távora wrote:

[...]

> Great.  Precise project-wide analysis (as opposed to more-or-less dumb
> grep-based) is exactly the main thing we should be targeting IMO.
>  Is "rename" the only thing it does or can you, say, extract subexpressions
> to variables, organize imports, etc?

As someone who has done quite a bit of "grep based refactoring", one
observation: of course, this can only me made semi-manually. But then,
the process has found things for me a syntax based refactoring tool
wouldn't ever have, like a variable name's mention in a comment (or
docstring, or even more insidious: possibly temporarily un-commented
code!) where I went "oh, yes, I want to change that too".

Food for thought.

Cheers
-- 
t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [semi off topic] grep-based refactoring [was: Adding refactoring capabilities to Emacs]

[João Távora]

On Fri, Sep 8, 2023 at 2:31 PM <tomas@tuxteam.de> wrote:
>
> On Fri, Sep 08, 2023 at 01:52:53PM +0100, João Távora wrote:
>
> [...]
>
> > Great.  Precise project-wide analysis (as opposed to more-or-less dumb
> > grep-based) is exactly the main thing we should be targeting IMO.
> >  Is "rename" the only thing it does or can you, say, extract subexpressions
> > to variables, organize imports, etc?
>
> As someone who has done quite a bit of "grep based refactoring", one
> observation: of course, this can only me made semi-manually. But then,
> the process has found things for me a syntax based refactoring tool
> wouldn't ever have,

Not sure about that.  Clangd mentions this limitation specifically
in their web page, so they acknowledge it and possibly are looking
to fix it.  It's not too wild to think that an LSP server might
to suggest additional edits beyond what it has from syntactic
analysis, who knows maybe even using language models or by using
a parser that understands doxygen or common types of documentation
systems using comments.

So the only thing we know for sure is that grep-based isn't enough.

João

Re: [semi off topic] grep-based refactoring [was: Adding refactoring capabilities to Emacs]

[Dmitry Gutov]

On 08/09/2023 20:53, João Távora wrote:
> On Fri, Sep 8, 2023 at 2:31 PM<tomas@tuxteam.de>  wrote:
>> On Fri, Sep 08, 2023 at 01:52:53PM +0100, João Távora wrote:
>>
>> [...]
>>
>>> Great.  Precise project-wide analysis (as opposed to more-or-less dumb
>>> grep-based) is exactly the main thing we should be targeting IMO.
>>>   Is "rename" the only thing it does or can you, say, extract subexpressions
>>> to variables, organize imports, etc?
>> As someone who has done quite a bit of "grep based refactoring", one
>> observation: of course, this can only me made semi-manually. But then,
>> the process has found things for me a syntax based refactoring tool
>> wouldn't ever have,
> Not sure about that.  Clangd mentions this limitation specifically
> in their web page, so they acknowledge it and possibly are looking
> to fix it.  It's not too wild to think that an LSP server might
> to suggest additional edits beyond what it has from syntactic
> analysis, who knows maybe even using language models or by using
> a parser that understands doxygen or common types of documentation
> systems using comments.

As long as the identifiers are referred to in the docstrings using the 
proper markup. And they could also mention the same term in the 
free-form text which one might want to see renamed.

> So the only thing we know for sure is that grep-based isn't enough.

Good thing we'll have access to both anyway.

Re: [semi off topic] grep-based refactoring [was: Adding refactoring capabilities to Emacs]

[tomas]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

On Fri, Sep 08, 2023 at 09:24:12PM +0300, Dmitry Gutov wrote:
> On 08/09/2023 20:53, João Távora wrote:
> > On Fri, Sep 8, 2023 at 2:31 PM<tomas@tuxteam.de>  wrote:

[identifiers in comments, etc.]

> > Not sure about that.  Clangd mentions this limitation specifically
> > in their web page, so they acknowledge it and possibly are looking
> > to fix it.  It's not too wild to think that an LSP server might
> > to suggest additional edits beyond what it has from syntactic
> > analysis, who knows maybe even using language models or by using
> > a parser that understands doxygen or common types of documentation
> > systems using comments.
> 
> As long as the identifiers are referred to in the docstrings using the
> proper markup. And they could also mention the same term in the free-form
> text which one might want to see renamed.
> 
> > So the only thing we know for sure is that grep-based isn't enough.
> 
> Good thing we'll have access to both anyway.

And good thing in the first place that people are aware :-)

Cheers
-- 
t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]