Re: [elpa] master 74818d5: Brief Mode v5.86 release.

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[Stefan Monnier]

Hi Luke,

Thanks for working on Brief mode.  I have some comments on your patch, tho.

> +BRIEFVERSION=${BRIEFVERSION-"5.86"}
[...]
> +    BRIEFVERSION     ELPA brief version, default "5.86"

I'd recommend not to default to any specific version, because it's very
likely that someone will end up bumping the "Version:" header in
brief.el without realizing that it needs to be updated elsewhere.

Some with all the places where you say "5.86" in the README: better
reword it such that it doesn't need to be updated when the version
number changes.

> +# Search for brief.el or brief.elc through default path list

Why not just assume that the `brief` package was properly installed,
and hence `emacs -l brief` will just work™?

> +#!/bin/bash

Do you actually make use of bash-isms?  If so, can we avoid them
without too much extra work?  If not, we should say "/bin/sh".

> +exec ${EMACS} --load ${BRIEFPATH}/brief --eval \
> +"(progn \
> +  (setq-default truncate-lines t) \
> +  (setq scroll-step 1 \
> +        scroll-conservatively 101) \
> +  (setq hscroll-margin 1 \
> +        hscroll-step 1) \
> +  (scroll-bar-mode -1) \
> +  (brief-mode 1))" \
> +"${EMACSARGS[@]}"

Why are these (h)scroll settings here instead of adding them to
brief-mode?

If you want to keep `brief-mode` as a mode which doesn't impose
particular (h)scroll settings (which is probably a good idea, indeed),
then you could add a new function to brief.el and call that function
instead of `brief-mode`.

> diff --git a/packages/brief/b.el b/packages/brief/b.el
> new file mode 100644
> index 0000000..4a0ab6b
> --- /dev/null
> +++ b/packages/brief/b.el
> @@ -0,0 +1,9 @@
> +(setq-default truncate-lines t)
> +;;(setq-default global-visual-line-mode t)
> +(setq scroll-step 1
> +      scroll-conservatively 101)
> +(setq hscroll-margin 1
> +      hscroll-step 1)
> +(scroll-bar-mode -1)
> +(load "~/bin/elisp/brief/brief")
> +(brief-mode 1)

Several problems here:
- I don't see any place where this file is used.
- the chance that (load "~/bin/elisp/brief/brief") will work
  on the user's system is fairly slim.
- this file will be in the `load-path` so (load "b") will load it and so
  will (require 'b).  I think we'd rather keep such short names for more
  popular packages (like the `s` package).
- this Elisp file doesn't follow the convention that loading a file
  doesn't have significant effects (i.e. it just defines new vars and
  functions).

> diff --git a/packages/brief/brief.el b/packages/brief/brief.el
> index 1dd3181..66d3790 100644
> --- a/packages/brief/brief.el
> +++ b/packages/brief/brief.el
> @@ -1,11 +1,11 @@
> -;;; brief.el --- Brief Editor Emulator
> +;;; brief.el --- Brief Editor Emulator (Brief Mode)

The Commentary says

    ;; On Linux: (including native X11 Emacs, terminal mode Emacs on xterm
    ;;            and Linux X11 Emacs running on VcXsrv (1.19.3.4+) under
    ;;            Win10):
    ;;      Emacs 23.3.1, 24.4.50.2, 25.2.2, 26.0.50, 26.1 and 27.0.50.

has 5.86 still been tested with all those Emacs versions?
Any chance you'd be willing to drop compatibility with Emacs-23 (which
doesn't come with package.el)?

> +(defcustom brief-search-replace-using-regexp t
> +  "An option determine if search & replace using regular expression or string.
> +This is a buffer local variable with default value 't which means
> +regular expression is used for search & replace commands by default."
> +  :type  'boolean
> +  :group 'brief)

The `:group 'brief` is redundant (it defaults to the last group defined
in the same file).  Same applies to all other defcustoms.
Oh, and the docstring doesn't need to say "An option" ;-)

>  (defcustom brief-fake-region-face 'region ; 'secondary-selection
>    "The face (color) of fake region when `brief-search-fake-region-mark' is t."
> -  :type  'boolean
> +  :type  'sexp
>    :group 'brief)

You probably want `:type 'face` here.

> +(defvar-local brief-search-overlay nil
> +  "A buffer local overlay which records the current search selection.
> +It is deleted when the search ends or region deactivated.")

It should probably use a "--" in the name.

>  (defvar brief-latest-killed-buffer-info nil
> -  "This variable records the info of the most recently killed file
> -buffer.  If user accidentally killed a file buffer, it can be
> -recovered accordingly.
> +  "This variable records the info of the most recently killed file buffer.
> +If user accidentally killed a file buffer, it can be recovered
> +accordingly.
>  Information is a list of:

As above, the docstring doesn't need to say "This variable records".

> +(defmacro brief-meta-l-key (updown key)
> +  "Define key function and associated the key in `brief-prefix-meta-l'."
> +  (let* ((dir     (symbol-name updown))
> +         (keydesc (key-description key))
> +         (keyfunc (intern (concat "brief-mark-line-" dir "-with-" keydesc))))
> +    `(progn
> +       (defun ,keyfunc ()
> +         ,(concat "Mark line " dir " with " keydesc " key")
> +         (interactive)
> +         (,(intern (concat "brief-call-mark-line-" dir "-with-key"))
> +          ,key))
> +       (define-key brief-prefix-meta-l ,key ',keyfunc))))

This macro name and docstring makes it sound like it works for anything
one might want to have under the M-l prefix, yet the code seems to
limit this to those few operations named
brief-call-mark-line-*-with-key, so it's much more focused than announced.

>                          (call-interactively
>                           '(lambda (filename)

Please don't quote your lambdas.

> +  (let* (c1
> +         (p1 (save-excursion
> +               (end-of-visual-line)
> +               (setq c1 (following-char)) (point)))
> +         (p2 (save-excursion
> +               (end-of-line) (point)))) ;; `end-of-line' of course is at crlf

I think you can turn this into:

    (let* ((p1 (save-excursion
                 (end-of-visual-line)
                 (point)))
           (c1 (char-after p1))
           (p2 (line-end-position))) ;; `end-of-line' of course is at crlf

> +(defun brief-toggle-auto-backup ()
> +  "Toggle auto-backup on or off."
> +  (interactive)
> +  (message "Turn Emacs auto-backup %s"
> +           (if (setq auto-save-default (not auto-save-default))
> +               "ON" "OFF")))

You could use a minor mode, here:

    (define-minor-mode brief-auto-backup-mode
      "Whether auto-backup is done"
      :global t
      :variable auto-save-default)

> +(defun brief-beginning-of-file ()
> +  "Goto beginning of file."
> +  (interactive "^")
> +  (goto-char (point-min)))

Why not (defalias 'brief-beginning-of-file #'beginning-of-buffer) ?

> +(defun brief-end-of-file ()
> +  "Goto end of file."
> +  (interactive "^")
> +  (goto-char (point-max)))

Why not (defalias 'brief-end-of-file #'end-of-buffer) ?

> +(defun brief-open-new-line-next ()
> +  "Open a new line right below the current line and go there."
> +  (interactive)
> +  (move-end-of-line 1)
> +  (newline))

Why not (defalias 'brief-open-new-line-next #'open-line) ?


        Stefan

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[Stefan Monnier]

Ping?


        Stefan


PS: In the mean time, I had to remove b.el because its lack of copyright
header prevented the GNU ELPA scripts from building the package.


Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Hi Luke,
>
> Thanks for working on Brief mode.  I have some comments on your patch, tho.
>
>> +BRIEFVERSION=${BRIEFVERSION-"5.86"}
> [...]
>> +    BRIEFVERSION     ELPA brief version, default "5.86"
>
> I'd recommend not to default to any specific version, because it's very
> likely that someone will end up bumping the "Version:" header in
> brief.el without realizing that it needs to be updated elsewhere.
>
> Some with all the places where you say "5.86" in the README: better
> reword it such that it doesn't need to be updated when the version
> number changes.
>
>> +# Search for brief.el or brief.elc through default path list
>
> Why not just assume that the `brief` package was properly installed,
> and hence `emacs -l brief` will just work™?
>
>> +#!/bin/bash
>
> Do you actually make use of bash-isms?  If so, can we avoid them
> without too much extra work?  If not, we should say "/bin/sh".
>
>> +exec ${EMACS} --load ${BRIEFPATH}/brief --eval \
>> +"(progn \
>> +  (setq-default truncate-lines t) \
>> +  (setq scroll-step 1 \
>> +        scroll-conservatively 101) \
>> +  (setq hscroll-margin 1 \
>> +        hscroll-step 1) \
>> +  (scroll-bar-mode -1) \
>> +  (brief-mode 1))" \
>> +"${EMACSARGS[@]}"
>
> Why are these (h)scroll settings here instead of adding them to
> brief-mode?
>
> If you want to keep `brief-mode` as a mode which doesn't impose
> particular (h)scroll settings (which is probably a good idea, indeed),
> then you could add a new function to brief.el and call that function
> instead of `brief-mode`.
>
>> diff --git a/packages/brief/b.el b/packages/brief/b.el
>> new file mode 100644
>> index 0000000..4a0ab6b
>> --- /dev/null
>> +++ b/packages/brief/b.el
>> @@ -0,0 +1,9 @@
>> +(setq-default truncate-lines t)
>> +;;(setq-default global-visual-line-mode t)
>> +(setq scroll-step 1
>> +      scroll-conservatively 101)
>> +(setq hscroll-margin 1
>> +      hscroll-step 1)
>> +(scroll-bar-mode -1)
>> +(load "~/bin/elisp/brief/brief")
>> +(brief-mode 1)
>
> Several problems here:
> - I don't see any place where this file is used.
> - the chance that (load "~/bin/elisp/brief/brief") will work
>   on the user's system is fairly slim.
> - this file will be in the `load-path` so (load "b") will load it and so
>   will (require 'b).  I think we'd rather keep such short names for more
>   popular packages (like the `s` package).
> - this Elisp file doesn't follow the convention that loading a file
>   doesn't have significant effects (i.e. it just defines new vars and
>   functions).
>
>> diff --git a/packages/brief/brief.el b/packages/brief/brief.el
>> index 1dd3181..66d3790 100644
>> --- a/packages/brief/brief.el
>> +++ b/packages/brief/brief.el
>> @@ -1,11 +1,11 @@
>> -;;; brief.el --- Brief Editor Emulator
>> +;;; brief.el --- Brief Editor Emulator (Brief Mode)
>
> The Commentary says
>
>     ;; On Linux: (including native X11 Emacs, terminal mode Emacs on xterm
>     ;;            and Linux X11 Emacs running on VcXsrv (1.19.3.4+) under
>     ;;            Win10):
>     ;;      Emacs 23.3.1, 24.4.50.2, 25.2.2, 26.0.50, 26.1 and 27.0.50.
>
> has 5.86 still been tested with all those Emacs versions?
> Any chance you'd be willing to drop compatibility with Emacs-23 (which
> doesn't come with package.el)?
>
>> +(defcustom brief-search-replace-using-regexp t
>> +  "An option determine if search & replace using regular expression or string.
>> +This is a buffer local variable with default value 't which means
>> +regular expression is used for search & replace commands by default."
>> +  :type  'boolean
>> +  :group 'brief)
>
> The `:group 'brief` is redundant (it defaults to the last group defined
> in the same file).  Same applies to all other defcustoms.
> Oh, and the docstring doesn't need to say "An option" ;-)
>
>>  (defcustom brief-fake-region-face 'region ; 'secondary-selection
>>    "The face (color) of fake region when `brief-search-fake-region-mark' is t."
>> -  :type  'boolean
>> +  :type  'sexp
>>    :group 'brief)
>
> You probably want `:type 'face` here.
>
>> +(defvar-local brief-search-overlay nil
>> +  "A buffer local overlay which records the current search selection.
>> +It is deleted when the search ends or region deactivated.")
>
> It should probably use a "--" in the name.
>
>>  (defvar brief-latest-killed-buffer-info nil
>> -  "This variable records the info of the most recently killed file
>> -buffer.  If user accidentally killed a file buffer, it can be
>> -recovered accordingly.
>> +  "This variable records the info of the most recently killed file buffer.
>> +If user accidentally killed a file buffer, it can be recovered
>> +accordingly.
>>  Information is a list of:
>
> As above, the docstring doesn't need to say "This variable records".
>
>> +(defmacro brief-meta-l-key (updown key)
>> +  "Define key function and associated the key in `brief-prefix-meta-l'."
>> +  (let* ((dir     (symbol-name updown))
>> +         (keydesc (key-description key))
>> +         (keyfunc (intern (concat "brief-mark-line-" dir "-with-" keydesc))))
>> +    `(progn
>> +       (defun ,keyfunc ()
>> +         ,(concat "Mark line " dir " with " keydesc " key")
>> +         (interactive)
>> +         (,(intern (concat "brief-call-mark-line-" dir "-with-key"))
>> +          ,key))
>> +       (define-key brief-prefix-meta-l ,key ',keyfunc))))
>
> This macro name and docstring makes it sound like it works for anything
> one might want to have under the M-l prefix, yet the code seems to
> limit this to those few operations named
> brief-call-mark-line-*-with-key, so it's much more focused than announced.
>
>>                          (call-interactively
>>                           '(lambda (filename)
>
> Please don't quote your lambdas.
>
>> +  (let* (c1
>> +         (p1 (save-excursion
>> +               (end-of-visual-line)
>> +               (setq c1 (following-char)) (point)))
>> +         (p2 (save-excursion
>> +               (end-of-line) (point)))) ;; `end-of-line' of course is at crlf
>
> I think you can turn this into:
>
>     (let* ((p1 (save-excursion
>                  (end-of-visual-line)
>                  (point)))
>            (c1 (char-after p1))
>            (p2 (line-end-position))) ;; `end-of-line' of course is at crlf
>
>> +(defun brief-toggle-auto-backup ()
>> +  "Toggle auto-backup on or off."
>> +  (interactive)
>> +  (message "Turn Emacs auto-backup %s"
>> +           (if (setq auto-save-default (not auto-save-default))
>> +               "ON" "OFF")))
>
> You could use a minor mode, here:
>
>     (define-minor-mode brief-auto-backup-mode
>       "Whether auto-backup is done"
>       :global t
>       :variable auto-save-default)
>
>> +(defun brief-beginning-of-file ()
>> +  "Goto beginning of file."
>> +  (interactive "^")
>> +  (goto-char (point-min)))
>
> Why not (defalias 'brief-beginning-of-file #'beginning-of-buffer) ?
>
>> +(defun brief-end-of-file ()
>> +  "Goto end of file."
>> +  (interactive "^")
>> +  (goto-char (point-max)))
>
> Why not (defalias 'brief-end-of-file #'end-of-buffer) ?
>
>> +(defun brief-open-new-line-next ()
>> +  "Open a new line right below the current line and go there."
>> +  (interactive)
>> +  (move-end-of-line 1)
>> +  (newline))
>
> Why not (defalias 'brief-open-new-line-next #'open-line) ?
>
>
>         Stefan

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[Stefan Monnier]

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

Sorry 'bout that.  Was too busy these last few hours and didn't realize
it had not even been 24h since I sent the previous message!
Time to take a breath,


        Stef

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[路客]

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

> (... comments about the bash script 'b')

The purpose of this quick launcher bash script "b" is to allow
non-Emacs users a quick way to launch Brief mode even if one never use
Emacs before, in order to elicit potential new Emacs & Brief users.

This script also tries to mimic the "look and feel" of the original
Brief editor and hence those default setting changes. The wrapper file
'b.el' serves for the same purpose to show new users what Emacs setting
changes required to make scrolling not jumpy.  'b.el' was originally
used by 'b' but later I move those things into 'b' to make things more
obvious. With 'b', a new user can just install and launch Emacs with a
different default keystroke configuration.

Once they become a little bit more experienced with Emacs, this
launcher is no longer needed.  Users can change .emacs to achieve the
same thing.  'b' is not intended to be used for experienced users or
for long; therefore I have to assume users don't know how to install
Emacs packages in the first place.

>> +    BRIEFVERSION     ELPA brief version, default "5.86"
>I'd recommend not to default to any specific version, ...

The launcher could actually become more complicated to perform more
extensive search and get rid of this version, but the launch time
will be increased accordingly and give new users a bad impression
that Emacs is slow, compare to vi, vim or nano, just like the rumors
say.  As 'b' might be the first impression for new users I would like
to prevent this kind of possibility.  It's also simple enough so
any Linux user with some basic shell script capability can modify
the script by themselves.

>> +#!/bin/bash
>
>Do you actually make use of bash-isms?  If so, can we avoid them
>without too much extra work?  If not, we should say "/bin/sh".

Yes, quite a few bash specific things. Linux systems must have bash
installed so even tcsh/csh/zsh users still able to run it.

>> +  (setq scroll-step 1 \
>> +        scroll-conservatively 101) \
>> ...
>Why are these (h)scroll settings here instead of adding them to

It's basically telling new users what Emacs global settings are
affecting those (jumpy scrolling) behavior, without a need to look
into brief.el.

> diff --git a/packages/brief/b.el b/packages/brief/b.el

The purpose of this file is the same, sample code showing users
those settings.

>    ;;      Emacs 23.3.1, 24.4.50.2, 25.2.2, 26.0.50, 26.1 and 27.0.50.
> has 5.86 still been tested with all those Emacs versions?

Yes, but only some basic tests.

>>  (defcustom brief-fake-region-face 'region ; 'secondary-selection
> ...
>You probably want `:type 'face` here.

I found I need to change that to defface otherwise in Emacs GUI
it won't show up as "face" even if I changed to :type 'face.

>The `:group 'brief` is redundant (it defaults to the last group defined

I was following how the builtin "cua" package defines them, it
keeps all the ":group 'cua" too. This should be more convenient
if someday I move it to another file.

>> +(defcustom brief-search-replace-using-regexp t

>> +(defvar-local brief-search-overlay nil

>>  (defvar brief-latest-killed-buffer-info nil

>> +(defmacro brief-meta-l-key (updown key)

>> +  (let* (c1

> Why not (defalias 'brief-beginning-of-file #'beginning-of-buffer) ?

> Why not (defalias 'brief-end-of-file #'end-of-buffer) ?

Nice, all of the above are followed, thanks!

>> +(defun brief-toggle-auto-backup ()
>> +  "Toggle auto-backup on or off."
>> +  (interactive)
>> +  (message "Turn Emacs auto-backup %s"
>> +           (if (setq auto-save-default (not auto-save-default))
>> +               "ON" "OFF")))
> You could use a minor mode, here:
>
>    (define-minor-mode brief-auto-backup-mode
>      "Whether auto-backup is done"
>      :global t
>      :variable auto-save-default)

Wouldn't that become more complicated as I still need to define
a function `brief-toggle-auto-backup' to toggle it?

> Why not (defalias 'brief-open-new-line-next #'open-line) ?

They're different, `brief-open-new-line-next' shouldn't split
current line or it will be no different than the <Enter> key.
I added a comment there.

Thanks.

Best regards,
Luke Lee

Stefan Monnier <monnier@iro.umontreal.ca> 於 2018年10月18日 週四 下午11:43寫道：

> Hi Luke,
>
> Thanks for working on Brief mode.  I have some comments on your patch, tho.
>
> > +BRIEFVERSION=${BRIEFVERSION-"5.86"}
> [...]
> > +    BRIEFVERSION     ELPA brief version, default "5.86"
>
> I'd recommend not to default to any specific version, because it's very
> likely that someone will end up bumping the "Version:" header in
> brief.el without realizing that it needs to be updated elsewhere.
>
> Some with all the places where you say "5.86" in the README: better
> reword it such that it doesn't need to be updated when the version
> number changes.
>
> > +# Search for brief.el or brief.elc through default path list
>
> Why not just assume that the `brief` package was properly installed,
> and hence `emacs -l brief` will just work™?
>
> > +#!/bin/bash
>
> Do you actually make use of bash-isms?  If so, can we avoid them
> without too much extra work?  If not, we should say "/bin/sh".
>
> > +exec ${EMACS} --load ${BRIEFPATH}/brief --eval \
> > +"(progn \
> > +  (setq-default truncate-lines t) \
> > +  (setq scroll-step 1 \
> > +        scroll-conservatively 101) \
> > +  (setq hscroll-margin 1 \
> > +        hscroll-step 1) \
> > +  (scroll-bar-mode -1) \
> > +  (brief-mode 1))" \
> > +"${EMACSARGS[@]}"
>
> Why are these (h)scroll settings here instead of adding them to
> brief-mode?
>
> If you want to keep `brief-mode` as a mode which doesn't impose
> particular (h)scroll settings (which is probably a good idea, indeed),
> then you could add a new function to brief.el and call that function
> instead of `brief-mode`.
>
> > diff --git a/packages/brief/b.el b/packages/brief/b.el
> > new file mode 100644
> > index 0000000..4a0ab6b
> > --- /dev/null
> > +++ b/packages/brief/b.el
> > @@ -0,0 +1,9 @@
> > +(setq-default truncate-lines t)
> > +;;(setq-default global-visual-line-mode t)
> > +(setq scroll-step 1
> > +      scroll-conservatively 101)
> > +(setq hscroll-margin 1
> > +      hscroll-step 1)
> > +(scroll-bar-mode -1)
> > +(load "~/bin/elisp/brief/brief")
> > +(brief-mode 1)
>
> Several problems here:
> - I don't see any place where this file is used.
> - the chance that (load "~/bin/elisp/brief/brief") will work
>   on the user's system is fairly slim.
> - this file will be in the `load-path` so (load "b") will load it and so
>   will (require 'b).  I think we'd rather keep such short names for more
>   popular packages (like the `s` package).
> - this Elisp file doesn't follow the convention that loading a file
>   doesn't have significant effects (i.e. it just defines new vars and
>   functions).
>
> > diff --git a/packages/brief/brief.el b/packages/brief/brief.el
> > index 1dd3181..66d3790 100644
> > --- a/packages/brief/brief.el
> > +++ b/packages/brief/brief.el
> > @@ -1,11 +1,11 @@
> > -;;; brief.el --- Brief Editor Emulator
> > +;;; brief.el --- Brief Editor Emulator (Brief Mode)
>
> The Commentary says
>
>     ;; On Linux: (including native X11 Emacs, terminal mode Emacs on xterm
>     ;;            and Linux X11 Emacs running on VcXsrv (1.19.3.4+) under
>     ;;            Win10):
>     ;;      Emacs 23.3.1, 24.4.50.2, 25.2.2, 26.0.50, 26.1 and 27.0.50.
>
> has 5.86 still been tested with all those Emacs versions?
> Any chance you'd be willing to drop compatibility with Emacs-23 (which
> doesn't come with package.el)?
>
> > +(defcustom brief-search-replace-using-regexp t
> > +  "An option determine if search & replace using regular expression or
> string.
> > +This is a buffer local variable with default value 't which means
> > +regular expression is used for search & replace commands by default."
> > +  :type  'boolean
> > +  :group 'brief)
>
> The `:group 'brief` is redundant (it defaults to the last group defined
> in the same file).  Same applies to all other defcustoms.
> Oh, and the docstring doesn't need to say "An option" ;-)
>
> >  (defcustom brief-fake-region-face 'region ; 'secondary-selection
> >    "The face (color) of fake region when `brief-search-fake-region-mark'
> is t."
> > -  :type  'boolean
> > +  :type  'sexp
> >    :group 'brief)
>
> You probably want `:type 'face` here.
>
> > +(defvar-local brief-search-overlay nil
> > +  "A buffer local overlay which records the current search selection.
> > +It is deleted when the search ends or region deactivated.")
>
> It should probably use a "--" in the name.
>
> >  (defvar brief-latest-killed-buffer-info nil
> > -  "This variable records the info of the most recently killed file
> > -buffer.  If user accidentally killed a file buffer, it can be
> > -recovered accordingly.
> > +  "This variable records the info of the most recently killed file
> buffer.
> > +If user accidentally killed a file buffer, it can be recovered
> > +accordingly.
> >  Information is a list of:
>
> As above, the docstring doesn't need to say "This variable records".
>
> > +(defmacro brief-meta-l-key (updown key)
> > +  "Define key function and associated the key in `brief-prefix-meta-l'."
> > +  (let* ((dir     (symbol-name updown))
> > +         (keydesc (key-description key))
> > +         (keyfunc (intern (concat "brief-mark-line-" dir "-with-"
> keydesc))))
> > +    `(progn
> > +       (defun ,keyfunc ()
> > +         ,(concat "Mark line " dir " with " keydesc " key")
> > +         (interactive)
> > +         (,(intern (concat "brief-call-mark-line-" dir "-with-key"))
> > +          ,key))
> > +       (define-key brief-prefix-meta-l ,key ',keyfunc))))
>
> This macro name and docstring makes it sound like it works for anything
> one might want to have under the M-l prefix, yet the code seems to
> limit this to those few operations named
> brief-call-mark-line-*-with-key, so it's much more focused than announced.
>
> >                          (call-interactively
> >                           '(lambda (filename)
>
> Please don't quote your lambdas.
>
> > +  (let* (c1
> > +         (p1 (save-excursion
> > +               (end-of-visual-line)
> > +               (setq c1 (following-char)) (point)))
> > +         (p2 (save-excursion
> > +               (end-of-line) (point)))) ;; `end-of-line' of course is
> at crlf
>
> I think you can turn this into:
>
>     (let* ((p1 (save-excursion
>                  (end-of-visual-line)
>                  (point)))
>            (c1 (char-after p1))
>            (p2 (line-end-position))) ;; `end-of-line' of course is at crlf
>
> > +(defun brief-toggle-auto-backup ()
> > +  "Toggle auto-backup on or off."
> > +  (interactive)
> > +  (message "Turn Emacs auto-backup %s"
> > +           (if (setq auto-save-default (not auto-save-default))
> > +               "ON" "OFF")))
>
> You could use a minor mode, here:
>
>     (define-minor-mode brief-auto-backup-mode
>       "Whether auto-backup is done"
>       :global t
>       :variable auto-save-default)
>
> > +(defun brief-beginning-of-file ()
> > +  "Goto beginning of file."
> > +  (interactive "^")
> > +  (goto-char (point-min)))
>
> Why not (defalias 'brief-beginning-of-file #'beginning-of-buffer) ?
>
> > +(defun brief-end-of-file ()
> > +  "Goto end of file."
> > +  (interactive "^")
> > +  (goto-char (point-max)))
>
> Why not (defalias 'brief-end-of-file #'end-of-buffer) ?
>
> > +(defun brief-open-new-line-next ()
> > +  "Open a new line right below the current line and go there."
> > +  (interactive)
> > +  (move-end-of-line 1)
> > +  (newline))
>
> Why not (defalias 'brief-open-new-line-next #'open-line) ?
>
>
>         Stefan
>


-- 
Best regards,
Luke Lee

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

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[Stefan Monnier]

>> (... comments about the bash script 'b')
> The purpose of this quick launcher bash script "b" is to allow
> non-Emacs users a quick way to launch Brief mode even if one never use
> Emacs before, in order to elicit potential new Emacs & Brief users.

OK.

> same thing.  'b' is not intended to be used for experienced users or
> for long; therefore I have to assume users don't know how to install
> Emacs packages in the first place.

Not sure what you mean exactly, here: `b` is a script that's part of the
`brief` GNU ELPA package.  So to get this script, they've had to
"install" (at the very least download and unpack) that package already.
The easiest way to install it is with `M-x package-install RET`, AFAIK
which will take care of downloading it, unpacking it in a good spot etc...

What other/simpler method of installation are you thinking about?
I could see an argument for trying to handle the case where the user
downloads brief-NN.tar.gz by hand, unpacks it somewhere and then runs
`b`, but to support that case, `b` would have to use $0 to find its
brief.el neighbor and I don't see any such code in `b`.

I only see you searching through:

    || find_brief ~/.emacs.d/elpa/brief-${BRIEFVERSION} \
    || find_brief ~/.emacs.d/elpa/brief \
    || find_brief ~/.emacs.d/brief \
    || find_brief ~/bin/elisp/brief \

where
- ~/.emacs.d/elpa/brief-${BRIEFVERSION} is the normal place where
  package.el places the package, but in that case, most likely the package
  is already properly installed by package.el so we don't need to know
  where it is because Emacs will already know where to find brief.el.
- all the other places seem rather arbitrary (I'd only expect
  ~/.emacs.d/brief to cover some non-negligible proportion of users, and
  those are likely to be familiar enough with Emacs to know how to use
  package.el).

>>> +    BRIEFVERSION     ELPA brief version, default "5.86"
>>I'd recommend not to default to any specific version, ...
> The launcher could actually become more complicated to perform more
> extensive search and get rid of this version, but the launch time
> will be increased accordingly and give new users a bad impression
> that Emacs is slow, compare to vi, vim or nano, just like the rumors
> say.

Not sure how this relates to what I wrote.  Keeping BRIEFVERSION unset
by default should just stop the script from considering
~/.emacs.d/elpa/brief-${BRIEFVERSION}, which will still work anyway if
the script falls back to assuming that `brief` was installed via
package.el.
I don't see why/how it would slow things down.

>>> +#!/bin/bash
>>Do you actually make use of bash-isms?  If so, can we avoid them
>>without too much extra work?  If not, we should say "/bin/sh".
> Yes, quite a few bash specific things.

I find it difficult to find those, but I usually find it easy to change
the code not to use them, so if you can point them out I can try and
see if I can remove this dependency without making the code worse.

> Linux systems must have bash
> installed so even tcsh/csh/zsh users still able to run it.

[ You mean GNU/Linux, I think, because most Android systems are very
  much using Linux but don't have bash installed.  ]

My remark was unrelated to the interactive shell used by the users: some
GNU/Linux systems can run without bash (using e.g. dash as the /bin/sh).

>>> +  (setq scroll-step 1 \
>>> +        scroll-conservatively 101) \
>>> ...
>>Why are these (h)scroll settings here instead of adding them to
> It's basically telling new users what Emacs global settings are
> affecting those (jumpy scrolling) behavior, without a need to look
> into brief.el.

Are they likely to dig into the code of `b` to find that?  Or look for
a b.el file in the package (e.g. if they installed it via package.el
they may not even know where to look to find b.el)?

Maybe you could advertise those in a different way, e.g. in the README,
in some special entry in the menu-bar, or in some help buffer that you
could pop up on first-use (or if the init file is empty)?

>>>  (defcustom brief-fake-region-face 'region ; 'secondary-selection
>> ...
>>You probably want `:type 'face` here.
> I found I need to change that to defface otherwise in Emacs GUI
> it won't show up as "face" even if I changed to :type 'face.

It's not the same: one declares a face, the other declares a variable
whose value is the name of another face.  But yes, it's usually better
to define a new face than a Custom variable that tells which existing
face to use.

>>The `:group 'brief` is redundant (it defaults to the last group defined
> I was following how the builtin "cua" package defines them, it
> keeps all the ":group 'cua" too.

Lots and lots of Elisp has those redundant :group because in the distant
past they were not redundant.

>>> +(defun brief-toggle-auto-backup ()
>>> +  "Toggle auto-backup on or off."
>>> +  (interactive)
>>> +  (message "Turn Emacs auto-backup %s"
>>> +           (if (setq auto-save-default (not auto-save-default))
>>> +               "ON" "OFF")))
>> You could use a minor mode, here:
>>
>>    (define-minor-mode brief-auto-backup-mode
>>      "Whether auto-backup is done"
>>      :global t
>>      :variable auto-save-default)
>
> Wouldn't that become more complicated as I still need to define
> a function `brief-toggle-auto-backup' to toggle it?

I don't follow: the above definition already defines the
`brief-auto-backup-mode` command which toggles.

>> Why not (defalias 'brief-open-new-line-next #'open-line) ?
> They're different, `brief-open-new-line-next' shouldn't split
> current line or it will be no different than the <Enter> key.

Oops, sorry you're right.  I use C-o so little that I was confused about
what it does.  You just reminded me how useless is the default C-o
keybinding ;-)


        Stefan

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[路客]

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

> Not sure what you mean exactly, here: `b` is a script that's part of the
> `brief` GNU ELPA package.  So to get this script, they've had to
> "install" (at the very least download and unpack) that package already.
> The easiest way to install it is with `M-x package-install RET`, AFAIK

In EmacsWiki page (or "README.org") I suggest this command line:

$ emacs -Q -eval "(progn (package-initialize) (package-refresh-contents) \
(package-install 'brief) (save-buffers-kill-emacs))"

so basically a new user only need to copy&paste this command...

> (regarding other discussions about 'b'...)

However I am facing another more basic problem that I can't make
either 'b' or 'README.org' packed into brief package.  I read thru all
the multi-file related info but they seems only applies to .el files.
I am still trying, if I still can't even pack 'b' or README.org I will just
put them in github and modify EmacsWiki and README.org about the above
command line without even using package.  Of course the 'b' would need
to be modified accordingly.

> > Yes, quite a few bash specific things.
> I find it difficult to find those, but I usually find it easy to change
> the code not to use them, so if you can point them out I can try and
> see if I can remove this dependency without making the code worse.

Mainly about the array things.  I would like to get rid of the
possibility that white space exists in some argument (like file name
or directory with white space) so bash arrays are more convenient to
me.  There must be 'sh' ways to achieve this but maybe later when
I have time.  Maybe 'b' need to be revised for github anyway.

> Are they likely to dig into the code of `b` to find that?  Or look for
> a b.el file in the package (e.g. if they installed it via package.el
> they may not even know where to look to find b.el)?
> ...
> Maybe you could advertise those in a different way, e.g. in the README,

Once users find the behavior of launching 'b' and of Emacs+briefpkg
differs, they will findout in the 'b' script.  Surely better be in
README.org.

> I don't follow: the above definition already defines the
> `brief-auto-backup-mode` command which toggles.

Arg, you're right, will do that later.

Best regards,
Luke Lee

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

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[Stefan Monnier]

>> Not sure what you mean exactly, here: `b` is a script that's part of the
>> `brief` GNU ELPA package.  So to get this script, they've had to
>> "install" (at the very least download and unpack) that package already.
>> The easiest way to install it is with `M-x package-install RET`, AFAIK
>
> In EmacsWiki page (or "README.org") I suggest this command line:
>
> $ emacs -Q -eval "(progn (package-initialize) (package-refresh-contents) \
> (package-install 'brief) (save-buffers-kill-emacs))"
>
> so basically a new user only need to copy&paste this command...

But that's also the "normal" package.el installation, so there's no need
to search for brief.el: after the above command, Emacs will know where
to find brief.el and `b` could just fire `emacs -f brief-mode` and be
done with it.

> However I am facing another more basic problem that I can't make
> either 'b' or 'README.org' packed into brief package.  I read thru all
> the multi-file related info but they seems only applies to .el files.
> I am still trying, if I still can't even pack 'b' or README.org I will just
> put them in github and modify EmacsWiki and README.org about the above
> command line without even using package.  Of course the 'b' would need
> to be modified accordingly.

IIUC you found the answer in the mean time: the package header needed
a `Package-Type: multi`.

> Once users find the behavior of launching 'b' and of Emacs+briefpkg
> differs, they will findout in the 'b' script.  Surely better be in
> README.org.

That's also why I suggested introducing another function (like
`brief-mode-full`) which would call brief plus setup those
(h)scroll vars.


        Stefan

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[路客]

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

> But that's also the "normal" package.el installation, so there's no need
> to search for brief.el: after the above command, Emacs will know where
> to find brief.el and `b` could just fire `emacs -f brief-mode` and be
> done with it.

Aside from the major purpose of 'b' for new users, there is a minor
purpose of 'b' -- a quick convenient shortcut for experienced users,
a pure & clean Emacs+Brief environment.  Just like the "-q" or "-Q"
argument is for Emacs. You might notice by default I add "-q" in 'b'
and gave an "-nq" to reverse it.  I would like 'b' to behave just like
"emacs -q" or "emacs -Q", but, with BriefMode as it's default keymap.

For myself, I from time to time need to launch a clean emacs env (to
bypass my lengthy .emacs init seq) for some quick editing, sometimes
emacsclient is just not adequate, therefore the "emacs -q" was
supposed to be good enough for that but just lack of the brief keymap.
Therefore with "-q" so I can't just fire it with "-f brief-mode" and
therefore it came along with the version/path things.

If you have a better way to launch Emacs with -q while being able to do
similar things like "-f brief-mode", *please definitively let me know*.
I myself don't like those default version things but I got no better
way.  For a personal script I don't have that locally; but when it
extended for other users or new users, with Brief as an installed
Emacs package, I can't find a faster way to direct access
"~/.emacs.d/elpa/brief-XXX" without any searching.  As I mentioned
in my earlier reply I can make things more complicated by things like
a 'find ~/.emacs.d/elpa -name b' but then I will need to sort
"by versions" if there are several versions returned (which involves
*version format* comparison like: v5.100 > v5.90). Of course, this
won't happen immediately as currently there is only v5.86.  But once
there are more other versions installed it suppose to slow down things.

> That's also why I suggested introducing another function (like
> `brief-mode-full`) which would call brief plus setup those
> (h)scroll vars.

Yap, that's a good idea and I can get rid of those default settings
in 'b' altogether.  I will do that in my next release.

Best regards,
Luke Lee

Stefan Monnier <monnier@iro.umontreal.ca> 於 2018年10月21日 週日 上午2:51寫道：

> >> Not sure what you mean exactly, here: `b` is a script that's part of the
> >> `brief` GNU ELPA package.  So to get this script, they've had to
> >> "install" (at the very least download and unpack) that package already.
> >> The easiest way to install it is with `M-x package-install RET`, AFAIK
> >
> > In EmacsWiki page (or "README.org") I suggest this command line:
> >
> > $ emacs -Q -eval "(progn (package-initialize) (package-refresh-contents)
> \
> > (package-install 'brief) (save-buffers-kill-emacs))"
> >
> > so basically a new user only need to copy&paste this command...
>
> But that's also the "normal" package.el installation, so there's no need
> to search for brief.el: after the above command, Emacs will know where
> to find brief.el and `b` could just fire `emacs -f brief-mode` and be
> done with it.
>
> > However I am facing another more basic problem that I can't make
> > either 'b' or 'README.org' packed into brief package.  I read thru all
> > the multi-file related info but they seems only applies to .el files.
> > I am still trying, if I still can't even pack 'b' or README.org I will
> just
> > put them in github and modify EmacsWiki and README.org about the above
> > command line without even using package.  Of course the 'b' would need
> > to be modified accordingly.
>
> IIUC you found the answer in the mean time: the package header needed
> a `Package-Type: multi`.
>
> > Once users find the behavior of launching 'b' and of Emacs+briefpkg
> > differs, they will findout in the 'b' script.  Surely better be in
> > README.org.
>
> That's also why I suggested introducing another function (like
> `brief-mode-full`) which would call brief plus setup those
> (h)scroll vars.
>
>
>         Stefan
>


-- 
Best regards,
Luke Lee

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

Re: [elpa] master 74818d5: Brief Mode v5.86 release.

[Stefan Monnier]

> If you have a better way to launch Emacs with -q while being able to do
> similar things like "-f brief-mode", *please definitively let me know*.

I think that would be:

    emacs -q -f package-initialize -f brief-mode


-- Stefan