> (... 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