From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [elpa] master 74818d5: Brief Mode v5.86 release. Date: Thu, 18 Oct 2018 11:43:47 -0400 Message-ID: References: <20181018141131.22680.53035@vcs0.savannah.gnu.org> <20181018141132.6E8BA208EC@vcs0.savannah.gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: blaine.gmane.org 1539877323 27218 195.159.176.226 (18 Oct 2018 15:42:03 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 18 Oct 2018 15:42:03 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: Luke Lee Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Oct 18 17:41:58 2018 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gDAQv-0006xr-Mq for ged-emacs-devel@m.gmane.org; Thu, 18 Oct 2018 17:41:57 +0200 Original-Received: from localhost ([::1]:43106 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gDASz-0006mi-BY for ged-emacs-devel@m.gmane.org; Thu, 18 Oct 2018 11:44:05 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:36754) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gDASs-0006mb-E9 for emacs-devel@gnu.org; Thu, 18 Oct 2018 11:43:59 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gDASl-0005Oi-PQ for emacs-devel@gnu.org; Thu, 18 Oct 2018 11:43:58 -0400 Original-Received: from pruche.dit.umontreal.ca ([132.204.246.22]:38569) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gDASk-0005Lt-EZ for emacs-devel@gnu.org; Thu, 18 Oct 2018 11:43:51 -0400 Original-Received: from pastel.home (lechon.iro.umontreal.ca [132.204.27.242]) by pruche.dit.umontreal.ca (8.14.7/8.14.1) with ESMTP id w9IFhlYL012737; Thu, 18 Oct 2018 11:43:47 -0400 Original-Received: by pastel.home (Postfix, from userid 20848) id 6353B6A11A; Thu, 18 Oct 2018 11:43:47 -0400 (EDT) In-Reply-To: <20181018141132.6E8BA208EC@vcs0.savannah.gnu.org> (Luke Lee's message of "Thu, 18 Oct 2018 10:11:32 -0400 (EDT)") X-NAI-Spam-Flag: NO X-NAI-Spam-Level: X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0.1 X-NAI-Spam-Rules: 3 Rules triggered BODY_START_GREETING=0.1, EDT_SA_DN_PASS=0, RV6398=0 X-NAI-Spam-Version: 2.3.0.9418 : core <6398> : inlines <6936> : streams <1801677> : uri <2733125> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.22 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:230480 Archived-At: Hi Luke, Thanks for working on Brief mode. I have some comments on your patch, tho. > +BRIEFVERSION=3D${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=E2=84=A2? > +#!/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 s= tring. > +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 buffe= r. > +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-" keydes= c)))) > + `(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