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