bug#39912: 27.0.90; Probably introduced at or before Emacs version ...

bug#39912: 27.0.90; Probably introduced at or before Emacs version ...

[Felician Nemeth]

"C-h f describe-function RET" provides a link to a NEWS file describing
the first mention of the given function (describe-function in this
case).  However, when I follow the link, it opens the NEWS file in
modifiable mode.  I think it would be better to enable view-mode for the
NEWS file similarly to "C-h N".

(Additionally, I think this information is really useful, but it is less
important than the actual documentation of the function.  So, I'd like
to see the "Probably introduced at or before Emacs version ..." text a
bit later in the help buffer.)

Thanks.

In GNU Emacs 27.0.90 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5)
 of 2020-03-04 built on betli
Repository revision: db37dd2e84573ceb2fc037dab040b79880ca298c
Repository branch: emacs-27
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

bug#39912: "Probably introduced at or before Emacs version ..." link should open NEWS read-only

[Nicholas Savage]

I'm currently using 28.0.50 and am not seeing the same behaviour. The NEWS file is being opened as read only, so somewhere between 27.0.90 and 28.0.50 this appears to have been fixed.

bug#39912: "Probably introduced at or before Emacs version ..." link should open NEWS read-only

[Lars Ingebrigtsen]

"Nicholas Savage" <nick@nicksavage.ca> writes:

> I'm currently using 28.0.50 and am not seeing the same behaviour. The
> NEWS file is being opened as read only, so somewhere between 27.0.90
> and 28.0.50 this appears to have been fixed.

Hm...  I just tried the same (I've never actually clicked on one of
those links before :-)), and I popped to the NEWS.22 file, and it wasn't
read-only here.  (This is with the current trunk, and emacs -Q.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39912: "Probably introduced at or before Emacs version ..." link should open NEWS read-only

[Andreas Schwab]

On Sep 07 2020, Lars Ingebrigtsen wrote:

> Hm...  I just tried the same (I've never actually clicked on one of
> those links before :-)), and I popped to the NEWS.22 file, and it wasn't
> read-only here.  (This is with the current trunk, and emacs -Q.)

That's because it is writable for you.  That won't happen with an
ordinarily installed Emacs.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#39912: "Probably introduced at or before Emacs version ..." link should open NEWS read-only

[Stefan Kangas]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Sep 07 2020, Lars Ingebrigtsen wrote:
>
>> Hm...  I just tried the same (I've never actually clicked on one of
>> those links before :-)), and I popped to the NEWS.22 file, and it wasn't
>> read-only here.  (This is with the current trunk, and emacs -Q.)
>
> That's because it is writable for you.  That won't happen with an
> ordinarily installed Emacs.

It will if you run as root.

bug#39912: "Probably introduced at or before Emacs version ..." link should open NEWS read-only

[Andreas Schwab]

On Sep 08 2020, Stefan Kangas wrote:

> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> On Sep 07 2020, Lars Ingebrigtsen wrote:
>>
>>> Hm...  I just tried the same (I've never actually clicked on one of
>>> those links before :-)), and I popped to the NEWS.22 file, and it wasn't
>>> read-only here.  (This is with the current trunk, and emacs -Q.)
>>
>> That's because it is writable for you.  That won't happen with an
>> ordinarily installed Emacs.
>
> It will if you run as root.

Q.E.D.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#39912: "Probably introduced at or before Emacs version ..." link should open NEWS read-only

[Lars Ingebrigtsen]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Sep 07 2020, Lars Ingebrigtsen wrote:
>
>> Hm...  I just tried the same (I've never actually clicked on one of
>> those links before :-)), and I popped to the NEWS.22 file, and it wasn't
>> read-only here.  (This is with the current trunk, and emacs -Q.)
>
> That's because it is writable for you.  That won't happen with an
> ordinarily installed Emacs.

Ah, make sense.  So this isn't a problem that exists in the distributed
version of Emacs (unless you're root), but it exists when people compile
Emacs themselves (and don't install).

In any case, I think the suggested fix here sounds good -- clicking the
button should switch on view-mode after loading the NEWS file.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39912: "Probably introduced at or before Emacs version ..." link should open NEWS read-only

[Nicholas Savage]

Uhh, no! I did not intend to leave out debbugs. Thanks for letting me know (I had got debbugs set up in emacs and didn't realize exactly how it worked).

But back to the matter at hand, now I can reproduce it. I'm going to attempt a fix for this, it seems like a simple enough fix for me to tackle for my first patch here.

On Tue, Sep 8, 2020, at 06:32, Lars Ingebrigtsen wrote:
> (I'm not sure whether you dropped the debbugs address from the Cc here
> on purpose or not, so I'm not adding it back. :-))
> 
> Nick Savage <nick@nicksavage.ca> writes:
> 
> > So that's interesting. I originally did not use 'emacs -Q', but now that
> > I am using that, it still is read-only. I'm also on the current trunk. I
> > don't have a good explanation for that. I also tried it out on one of my
> > other computers that's on 27.1.50 and it was also read only.
> >
> > So in the describe function for describe-function there are two links
> > - one for the defun and one
> > for the NEWS file. Both of those open for me as read-only.
> 
> Yeah, it sounded like Andreas found the reason why -- the installed
> files are normally not writeable by a user.
> 
>

bug#39912: "Probably introduced at or before Emacs version ..." link should open NEWS read-only

[Stefan Kangas]

"Nicholas Savage" <nick@nicksavage.ca> writes:

> I'm going to attempt a fix for this, it seems like a simple enough fix
> for me to tackle for my first patch here.

Thanks; this does look like an excellent choice for a first patch.

You can find some pointers on best practices for how to format your
patch, etc. in the CONTRIBUTE file.  Let us know if you have any
questions.

bug#39912: [PATCH] Open describe-function links in view-mode (Bug#39912)

[Nicholas Savage]

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

Hello,

I've written a patch for Bug #39912, as discussed in debbugs. I think I have formatted this correctly and have followed CONTRIBUTE as best as I could.

I've marked the patch as exempt from copyright assignment as my understanding is that 15 lines of code is the threshold. That being said, I have no problems assigning copyright for this and for future patches, and I would like to get the ball rolling on that now if possible.

One thing I should mention is that the thread discussed view-mode for NEWS links in describe-function. I've also changed it so the actual source code link opens in view-mode as well. I don't know if there are ramifications of that, so I'm happy to change it if necessary. My thought process was that describe-function is used as a reference to the code, not as a point of entry for editing functions, so changing that is consistent with changing the NEWS links, but I don't know what I don't know.

Thanks,
Nick

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Open-describe-function-links-in-view-mode-Bug-39912.patch --]
[-- Type: text/x-patch; name="0001-Open-describe-function-links-in-view-mode-Bug-39912.patch", Size: 1259 bytes --]

From 61bceceb2155d4521625f947be889743c8d3c27b Mon Sep 17 00:00:00 2001
From: Nick Savage <nick@nicksavage.ca>
Date: Tue, 8 Sep 2020 16:08:30 -0400
Subject: [PATCH] Open describe-function links in view-mode (Bug#39912)

* Open describe-function links in view-mode (Bug#39912) to make
behavior consistent depending on whether or not user has permission
to edit the files.

Copyright-paperwork-exempt: yes
---
 lisp/help-mode.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 9c2d1d7227..673e7bf304 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -204,7 +204,7 @@ help-function-def--button-function
     (let* ((location
             (find-function-search-for-symbol fun type file))
            (position (cdr location)))
-      (pop-to-buffer (car location))
+      (view-buffer (car location))
       (run-hooks 'find-function-after-hook)
       (if position
           (progn
@@ -308,7 +308,7 @@ 'help-news
   :supertype 'help-xref
   'help-function
   (lambda (file pos)
-    (pop-to-buffer (find-file-noselect file))
+    (view-buffer (find-file-noselect file))
     (goto-char pos))
   'help-echo (purecopy "mouse-2, RET: show corresponding NEWS announcement"))
 \f
-- 
2.20.1

bug#39912: [PATCH] Open describe-function links in view-mode (Bug#39912)

[Stefan Kangas]

"Nicholas Savage" <nick@nicksavage.ca> writes:

> I've written a patch for Bug #39912, as discussed in debbugs. I think I
> have formatted this correctly and have followed CONTRIBUTE as best as I
> could.

Thanks!  I have some comments.

(Someone else will have to help you with getting the assignment process
started.)

> I've also changed it so the actual source code link opens in view-mode
> as well. I don't know if there are ramifications of that, so I'm happy
> to change it if necessary. My thought process was that
> describe-function is used as a reference to the code, not as a point
> of entry for editing functions, so changing that is consistent with
> changing the NEWS links, but I don't know what I don't know.

Actually, I use it to edit the source code all the time.  That's what
that button is mostly used for, in my use.  :-)

So I think we'd better leave that part out.

> * Open describe-function links in view-mode (Bug#39912) to make
> behavior consistent depending on whether or not user has permission
> to edit the files.

The description is okay, but could be shorter (and should mention the
link to NEWS button as discussed above).  I'm also not sure if its
necessary to discuss file permissions here, since those details will be
in the bug report.  Mentioning view-mode should be enough.

Also, this doesn't seem to follow the ChangeLog format.  If you use VC
(or Magit) to edit the files, you can simply put point on the newly
added line and type `C-x 4 a' to get it automatically generated.  You
could even do it directly in the ELisp file, if point is in the changed
function.  (Read CONTRIBUTE for more details.)

> @@ -308,7 +308,7 @@ 'help-news
>    :supertype 'help-xref
>    'help-function
>    (lambda (file pos)
> -    (pop-to-buffer (find-file-noselect file))
> +    (view-buffer (find-file-noselect file))
>      (goto-char pos))
>    'help-echo (purecopy "mouse-2, RET: show corresponding NEWS announcement"))

This change looks good to me.

Best regards,
Stefan Kangas

bug#39912: [PATCH] Open describe-function links in view-mode (Bug#39912)

[Lars Ingebrigtsen]

Stefan Kangas <stefankangas@gmail.com> writes:

> (Someone else will have to help you with getting the assignment process
> started.)

(Sent off-list.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39912: [PATCH] Open describe-function links in view-mode (Bug#39912)

[Nicholas Savage]

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

Thanks for the comments. I've attached an updated patch, and have responded to your points below.

On Tue, Sep 8, 2020, at 17:01, Stefan Kangas wrote:
> "Nicholas Savage" <nick@nicksavage.ca> writes:
> 
> > I've written a patch for Bug #39912, as discussed in debbugs. I think I
> > have formatted this correctly and have followed CONTRIBUTE as best as I
> > could.
> 
> Thanks!  I have some comments.
> 
> (Someone else will have to help you with getting the assignment process
> started.)

This is now underway!

> 
> > I've also changed it so the actual source code link opens in view-mode
> > as well. I don't know if there are ramifications of that, so I'm happy
> > to change it if necessary. My thought process was that
> > describe-function is used as a reference to the code, not as a point
> > of entry for editing functions, so changing that is consistent with
> > changing the NEWS links, but I don't know what I don't know.
> 
> Actually, I use it to edit the source code all the time.  That's what
> that button is mostly used for, in my use.  :-)
> 
> So I think we'd better leave that part out.
> 

Yeah, that makes sense now that I'm thinking about it. I thought it may be beyond the pale, which is why I wanted to highlight it. I've removed it now in the new attached patch. 

> > * Open describe-function links in view-mode (Bug#39912) to make
> > behavior consistent depending on whether or not user has permission
> > to edit the files.
> 
> The description is okay, but could be shorter (and should mention the
> link to NEWS button as discussed above).  I'm also not sure if its
> necessary to discuss file permissions here, since those details will be
> in the bug report.  Mentioning view-mode should be enough.
> 
> Also, this doesn't seem to follow the ChangeLog format.  If you use VC
> (or Magit) to edit the files, you can simply put point on the newly
> added line and type `C-x 4 a' to get it automatically generated.  You
> could even do it directly in the ELisp file, if point is in the changed
> function.  (Read CONTRIBUTE for more details.)

I didn't know about `C-x 4 a'. Is that better now? I have reworded the description as well.

I appreciate all of your advice and help! This has been my first real attempt at working in a development environment, as my day job is in a different industry, so there's a bit of a learning curve but I'm eager to catch up and contribute as I can.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Open-describe-function-NEWS-links-in-view-mode-Bug-3.patch --]
[-- Type: text/x-patch; name="0001-Open-describe-function-NEWS-links-in-view-mode-Bug-3.patch", Size: 866 bytes --]

From 2e98047137e6f56494f8c41fdbf1d9d1275d0840 Mon Sep 17 00:00:00 2001
From: Nick Savage <nick@nicksavage.ca>
Date: Tue, 8 Sep 2020 16:08:30 -0400
Subject: [PATCH] Open describe-function NEWS links in view-mode (Bug#39912)

* lisp/help-mode.el (help-news): Open describe-function NEWS links in
view-mode (Bug#39912)

Copyright-paperwork-exempt: yes
---
 lisp/help-mode.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 9c2d1d7227..7a5d405c01 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -308,7 +308,7 @@ 'help-news
   :supertype 'help-xref
   'help-function
   (lambda (file pos)
-    (pop-to-buffer (find-file-noselect file))
+    (view-buffer (find-file-noselect file))
     (goto-char pos))
   'help-echo (purecopy "mouse-2, RET: show corresponding NEWS announcement"))
 \f
-- 
2.20.1

bug#39912: [PATCH] Open describe-function links in view-mode (Bug#39912)

[Lars Ingebrigtsen]

"Nicholas Savage" <nick@nicksavage.ca> writes:

> -    (pop-to-buffer (find-file-noselect file))
> +    (view-buffer (find-file-noselect file))

pop-to-buffer opens the NEWS buffer in a different window, though, while
view-buffer stays in the same window.  So perhaps it should pop first
and then call view-buffer?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39912: [PATCH] Open describe-function links in view-mode (Bug#39912)

[Nicholas Savage]

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

Good call. I've changed it to using view-buffer-other-window instead, which I think gets us where we want to be - it opens the NEWS file in view-mode, but also in another window as is consistent with previous behaviour.

On Wed, Sep 9, 2020, at 05:39, Lars Ingebrigtsen wrote:
> "Nicholas Savage" <nick@nicksavage.ca> writes:
> 
> > -    (pop-to-buffer (find-file-noselect file))
> > +    (view-buffer (find-file-noselect file))
> 
> pop-to-buffer opens the NEWS buffer in a different window, though, while
> view-buffer stays in the same window.  So perhaps it should pop first
> and then call view-buffer?
> 
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Open-describe-function-NEWS-links-in-view-mode-Bug-3.patch --]
[-- Type: text/x-patch; name="0001-Open-describe-function-NEWS-links-in-view-mode-Bug-3.patch", Size: 879 bytes --]

From b634e095982ff45bfb43363ae8fadaea20885800 Mon Sep 17 00:00:00 2001
From: Nick Savage <nick@nicksavage.ca>
Date: Tue, 8 Sep 2020 16:08:30 -0400
Subject: [PATCH] Open describe-function NEWS links in view-mode (Bug#39912)

* lisp/help-mode.el (help-news): Open describe-function NEWS links in
view-mode (Bug#39912)

Copyright-paperwork-exempt: yes
---
 lisp/help-mode.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 9c2d1d7227..0dc6c9ffae 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -308,7 +308,7 @@ 'help-news
   :supertype 'help-xref
   'help-function
   (lambda (file pos)
-    (pop-to-buffer (find-file-noselect file))
+    (view-buffer-other-window (find-file-noselect file))
     (goto-char pos))
   'help-echo (purecopy "mouse-2, RET: show corresponding NEWS announcement"))
 \f
-- 
2.20.1

bug#39912: [PATCH] Open describe-function links in view-mode (Bug#39912)

[Lars Ingebrigtsen]

"Nicholas Savage" <nick@nicksavage.ca> writes:

> Good call. I've changed it to using view-buffer-other-window instead,
> which I think gets us where we want to be - it opens the NEWS file in
> view-mode, but also in another window as is consistent with previous
> behaviour.

Thanks; I've now applied your patch to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no