bug#25764: 26.0.50; Make some usability improvements to sieve-script management

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Eric Abrahamsen]

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


The attached patch makes some usability improvements to the sieve script
management files: exposing more user commands in menus and help, and
using the buffer-modified mark to give the user a better sense of what's
going on.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Small-improvements-to-sieve-script-usability.patch --]
[-- Type: text/x-diff, Size: 3796 bytes --]

From e7c62b1bb8212034cba5502a7dec2c2786f4159f Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Thu, 16 Feb 2017 12:57:53 -0800
Subject: [PATCH] Small improvements to sieve script usability

* lisp/net/sieve-mode.el (sieve-mode-menu): Add
  `sieve-upload-and-kill' to the mode menu.

* lisp/net/sieve.el (sieve-manage-mode-menu): Add `sieve-manage-quit'
  to the mode menu.
  (sieve-help): Mention quitting in mode help.
  (sieve-manage-mode): Directly derive from special-mode.
  (sieve-edit-script): Start off with unmodified buffer.
  (sieve-upload): Set buffer unmodified after successful upload.

Changes are aimed at 1) increasing discoverability of commands and 2)
using buffer modification to help users understand what's happening.
---
 lisp/net/sieve-mode.el |  1 +
 lisp/net/sieve.el      | 22 ++++++++++++----------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/lisp/net/sieve-mode.el b/lisp/net/sieve-mode.el
index 87bb3a245b..cc5fb389e2 100644
--- a/lisp/net/sieve-mode.el
+++ b/lisp/net/sieve-mode.el
@@ -158,6 +158,7 @@ sieve-mode-map
   "Sieve Menu."
   '("Sieve"
     ["Upload script" sieve-upload t]
+    ["Upload script and kill" sieve-upload-and-kill t]
     ["Manage scripts on server" sieve-manage t]))
 
 ;; Code for Sieve editing mode.
diff --git a/lisp/net/sieve.el b/lisp/net/sieve.el
index 665a0a8e15..46cea316e3 100644
--- a/lisp/net/sieve.el
+++ b/lisp/net/sieve.el
@@ -137,16 +137,15 @@ sieve-manage-mode-map
   '("Manage Sieve"
     ["Edit script" sieve-edit-script t]
     ["Activate script" sieve-activate t]
-    ["Deactivate script" sieve-deactivate t]))
+    ["Deactivate script" sieve-deactivate t]
+    ["Quit and close connection" sieve-manage-quit t]))
 
-(define-derived-mode sieve-manage-mode fundamental-mode "Sieve-manage"
+(define-derived-mode sieve-manage-mode special-mode "Sieve-manage"
   "Mode used for sieve script management."
   (buffer-disable-undo (current-buffer))
   (setq truncate-lines t)
   (easy-menu-add sieve-manage-mode-menu sieve-manage-mode-map))
 
-(put 'sieve-manage-mode 'mode-class 'special)
-
 ;; Commands used in sieve-manage mode:
 
 (defun sieve-manage-quit ()
@@ -215,6 +214,7 @@ sieve-edit-script
     (sieve-mode)
     (setq sieve-buffer-script-name name)
     (goto-char (point-min))
+    (set-buffer-modified-p nil)
     (message
      (substitute-command-keys
       "Press \\[sieve-upload] to upload script to server."))))
@@ -257,7 +257,7 @@ sieve-help
       ;; would need minor-mode for log-edit-mode
       (describe-function 'sieve-mode)
     (message "%s" (substitute-command-keys
-	      "`\\[sieve-edit-script]':edit `\\[sieve-activate]':activate `\\[sieve-deactivate]':deactivate `\\[sieve-remove]':remove"))))
+	      "`\\[sieve-edit-script]':edit `\\[sieve-activate]':activate `\\[sieve-deactivate]':deactivate `\\[sieve-remove]':remove `\\[sieve-manage-quit]':quit"))))
 
 ;; Create buffer:
 
@@ -350,11 +350,13 @@ sieve-upload
       (with-current-buffer (get-buffer sieve-buffer)
 	(setq err (sieve-manage-putscript
                    (or name sieve-buffer-script-name (buffer-name))
-                   script sieve-manage-buffer))
-	(if (sieve-manage-ok-p err)
-	    (message (substitute-command-keys
-		      "Sieve upload done.  Use \\[sieve-manage] to manage scripts."))
-	  (message "Sieve upload failed: %s" (nth 2 err)))))))
+                   script sieve-manage-buffer)))
+      (if (sieve-manage-ok-p err)
+          (progn
+            (message (substitute-command-keys
+                      "Sieve upload done.  Use \\[sieve-manage] to manage scripts."))
+            (set-buffer-modified-p nil))
+        (message "Sieve upload failed: %s" (nth 2 err))))))
 
 ;;;###autoload
 (defun sieve-upload-and-bury (&optional name)
-- 
2.11.1


[-- Attachment #3: Type: text/plain, Size: 240 bytes --]



In GNU Emacs 26.0.50.26 (x86_64-unknown-linux-gnu, GTK+ Version 3.22.7)
 of 2017-02-14 built on clem
Repository revision: 71b90192dab8de9825904faaabbaf9548d3db2ab
Windowing system distributor 'The X.Org Foundation', version 11.0.11901000

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> The attached patch makes some usability improvements to the sieve script
> management files: exposing more user commands in menus and help, and
> using the buffer-modified mark to give the user a better sense of what's
> going on.

I think the patch looks good; please apply.

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

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Eric Abrahamsen]

On 06/25/19 01:03 AM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> The attached patch makes some usability improvements to the sieve script
>> management files: exposing more user commands in menus and help, and
>> using the buffer-modified mark to give the user a better sense of what's
>> going on.
>
> I think the patch looks good; please apply.

Done, thanks!

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Basil L. Contovounesios]

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

I don't use sieve scripts, but I noticed a minor nit in your patch:

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> @@ -215,6 +214,7 @@ sieve-edit-script
>      (sieve-mode)
>      (setq sieve-buffer-script-name name)
>      (goto-char (point-min))
> +    (set-buffer-modified-p nil)
>      (message
>       (substitute-command-keys
>        "Press \\[sieve-upload] to upload script to server."))))

[...]

> @@ -350,11 +350,13 @@ sieve-upload
>        (with-current-buffer (get-buffer sieve-buffer)
>  	(setq err (sieve-manage-putscript
>                     (or name sieve-buffer-script-name (buffer-name))
> -                   script sieve-manage-buffer))
> -	(if (sieve-manage-ok-p err)
> -	    (message (substitute-command-keys
> -		      "Sieve upload done.  Use \\[sieve-manage] to manage scripts."))
> -	  (message "Sieve upload failed: %s" (nth 2 err)))))))
> +                   script sieve-manage-buffer)))
> +      (if (sieve-manage-ok-p err)
> +          (progn
> +            (message (substitute-command-keys
> +                      "Sieve upload done.  Use \\[sieve-manage] to manage scripts."))
> +            (set-buffer-modified-p nil))
> +        (message "Sieve upload failed: %s" (nth 2 err))))))

In both hunks, 'message' is given an arbitrary string as its first
argument.  Any objections to the following cleanup of sieve.el text
formatting code?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Tidy-up-sieve.el-text-formatting.patch --]
[-- Type: text/x-diff, Size: 4323 bytes --]

From 77e1def846770526f50fceca41e269ec9be5080b Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Mon, 8 Jul 2019 21:05:14 +0100
Subject: [PATCH] Tidy up sieve.el text formatting

* lisp/net/sieve.el (sieve-edit-script, sieve-upload): Do not pass
arbitrary string as first argument to 'message' (bug#25764).
(sieve-help): Split long string across multiple lines.
(sieve-refresh-scriptlist): Use ngettext.  Fix grammar.
---
 lisp/net/sieve.el | 50 +++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/lisp/net/sieve.el b/lisp/net/sieve.el
index 55fea160f6..3337998bed 100644
--- a/lisp/net/sieve.el
+++ b/lisp/net/sieve.el
@@ -215,9 +215,8 @@ sieve-edit-script
     (setq sieve-buffer-script-name name)
     (goto-char (point-min))
     (set-buffer-modified-p nil)
-    (message
-     (substitute-command-keys
-      "Press \\[sieve-upload] to upload script to server."))))
+    (message "Press %s to upload script to server."
+             (substitute-command-keys "\\[sieve-upload]"))))
 
 (defmacro sieve-change-region (&rest body)
   "Turns off sieve-region before executing BODY, then re-enables it after.
@@ -256,8 +255,10 @@ sieve-help
   (if (eq last-command 'sieve-help)
       ;; would need minor-mode for log-edit-mode
       (describe-function 'sieve-mode)
-    (message "%s" (substitute-command-keys
-	      "`\\[sieve-edit-script]':edit `\\[sieve-activate]':activate `\\[sieve-deactivate]':deactivate `\\[sieve-remove]':remove `\\[sieve-manage-quit]':quit"))))
+    (message "%s" (substitute-command-keys "\
+`\\[sieve-edit-script]':edit `\\[sieve-activate]':activate \
+`\\[sieve-deactivate]':deactivate `\\[sieve-remove]':remove \
+`\\[sieve-manage-quit]':quit"))))
 
 ;; Create buffer:
 
@@ -312,20 +313,20 @@ sieve-refresh-scriptlist
     (delete-region (or sieve-buffer-header-end (point-max)) (point-max))
     (goto-char (point-max))
     ;; get list of script names and print them
-    (let ((scripts (sieve-manage-listscripts sieve-manage-buffer)))
-      (if (null scripts)
-	  (insert
-           (substitute-command-keys
-            (format
-             "No scripts on server, press \\[sieve-edit-script] on %s to create a new script.\n"
-             sieve-new-script)))
-	(insert
-         (substitute-command-keys
-          (format (concat "%d script%s on server, press \\[sieve-edit-script] on a script "
-                          "name edits it, or\npress \\[sieve-edit-script] on %s to create "
-                          "a new script.\n") (length scripts)
-                          (if (eq (length scripts) 1) "" "s")
-                          sieve-new-script))))
+    (let* ((scripts (sieve-manage-listscripts sieve-manage-buffer))
+           (count (length scripts))
+           (keys (substitute-command-keys "\\[sieve-edit-script]")))
+      (insert
+       (if (null scripts)
+           (format
+            "No scripts on server, press %s on %s to create a new script.\n"
+            keys sieve-new-script)
+         (format (concat (ngettext "%d script on server"
+                                   "%d scripts on server"
+                                   count)
+                         ", press %s on a script name to edit it, or"
+                         "\npress %s on %s to create a new script.\n")
+                 count keys keys sieve-new-script)))
       (save-excursion
 	(sieve-insert-scripts (list sieve-new-script))
 	(sieve-insert-scripts scripts)))
@@ -354,12 +355,11 @@ sieve-upload
 	(setq err (sieve-manage-putscript
                    (or name sieve-buffer-script-name script-name)
                    script sieve-manage-buffer))
-	(if (sieve-manage-ok-p err)
-            (progn
-	      (message (substitute-command-keys
-		        "Sieve upload done.  Use \\[sieve-manage] to manage scripts."))
-              (set-buffer-modified-p nil))
-	  (message "Sieve upload failed: %s" (nth 2 err)))))))
+        (if (not (sieve-manage-ok-p err))
+            (message "Sieve upload failed: %s" (nth 2 err))
+          (message "Sieve upload done.  Use %s to manage scripts."
+                   (substitute-command-keys "\\[sieve-manage]"))
+          (set-buffer-modified-p nil))))))
 
 ;;;###autoload
 (defun sieve-upload-and-bury (&optional name)
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 20 bytes --]


Thanks,

-- 
Basil

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I don't use sieve scripts, but I noticed a minor nit in your patch:

I think that looks correct (but I don't use Sieve, either).

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

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Eric Abrahamsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I don't use sieve scripts, but I noticed a minor nit in your patch:
>
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> @@ -215,6 +214,7 @@ sieve-edit-script
>>      (sieve-mode)
>>      (setq sieve-buffer-script-name name)
>>      (goto-char (point-min))
>> +    (set-buffer-modified-p nil)
>>      (message
>>       (substitute-command-keys
>>        "Press \\[sieve-upload] to upload script to server."))))
>
> [...]
>
>> @@ -350,11 +350,13 @@ sieve-upload
>>        (with-current-buffer (get-buffer sieve-buffer)
>>  	(setq err (sieve-manage-putscript
>>                     (or name sieve-buffer-script-name (buffer-name))
>> -                   script sieve-manage-buffer))
>> -	(if (sieve-manage-ok-p err)
>> -	    (message (substitute-command-keys
>> -		      "Sieve upload done.  Use \\[sieve-manage] to manage scripts."))
>> -	  (message "Sieve upload failed: %s" (nth 2 err)))))))
>> +                   script sieve-manage-buffer)))
>> +      (if (sieve-manage-ok-p err)
>> +          (progn
>> +            (message (substitute-command-keys
>> +                      "Sieve upload done.  Use \\[sieve-manage] to manage scripts."))
>> +            (set-buffer-modified-p nil))
>> +        (message "Sieve upload failed: %s" (nth 2 err))))))
>
> In both hunks, 'message' is given an arbitrary string as its first
> argument.  Any objections to the following cleanup of sieve.el text
> formatting code?

I do use sieve! But I didn't write this code. Your proposed chances seem
fine to me, though I'm wondering about the use of `ngettext' -- I saw
some arguments on these lists recently about international/localization,
and wasn't paying too much attention, but didn't see a clear resolution.
Is it the consensus that we're supposed to be using `ngettext' where possible?

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I do use sieve! But I didn't write this code. Your proposed chances seem
> fine to me, though I'm wondering about the use of `ngettext' -- I saw
> some arguments on these lists recently about international/localization,
> and wasn't paying too much attention, but didn't see a clear resolution.
> Is it the consensus that we're supposed to be using `ngettext' where possible?

Oh, I didn't notice that part.  No, I don't think there's any such
consensus...

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

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I do use sieve! But I didn't write this code. Your proposed chances seem
>> fine to me, though I'm wondering about the use of `ngettext' -- I saw
>> some arguments on these lists recently about international/localization,
>> and wasn't paying too much attention, but didn't see a clear resolution.
>> Is it the consensus that we're supposed to be using `ngettext' where possible?
>
> Oh, I didn't notice that part.  No, I don't think there's any such
> consensus...

There are 23 uses of `ngettext' in the sources right now, which says to
me: "there's no consensus, but it's also not a problem".

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Basil L. Contovounesios]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> I do use sieve! But I didn't write this code.

I know, just thought I may as well fix these trivial instances upon
seeing your patch.

>>> Your proposed chances seem fine to me, though I'm wondering about
>>> the use of `ngettext' -- I saw some arguments on these lists
>>> recently about international/localization, and wasn't paying too
>>> much attention, but didn't see a clear resolution.  Is it the
>>> consensus that we're supposed to be using `ngettext' where possible?
>>
>> Oh, I didn't notice that part.  No, I don't think there's any such
>> consensus...
>
> There are 23 uses of `ngettext' in the sources right now, which says to
> me: "there's no consensus, but it's also not a problem".

That's more or less my understanding as well:

[45c525a685]: Fig grammar of count-lines-page
  2019-06-09 17:00:05 -0700
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=45c525a685041c274a26c70caed4088726a790de

[7bba702e1a]: Use ngettext instead of dired-plural-s in Dired (bug#35287)
  2019-05-05 22:43:27 +0300
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=7bba702e1ad1ed343618e44cf5bbc2a1f079be0c

I wasn't involved in the discussion, but IIRC l10n (i.e. using a no-op
ngettext for now) is welcome and encouraged, it's just that we don't
have an agreed upon or concrete implementation yet.  I think it's
definitely better to mark the text as localisable now, while we're in
the area, than leave the previous string formatting trick for future
archaeologists to rediscover.  So I've now pushed my patch:

[7b70efb740]: Tidy up sieve.el text formatting
  2019-07-09 01:47:52 +0100
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=7b70efb740a04517e2764b0986c6e92305c1dd3e

Thanks for the review and sorry for hijacking this report.

-- 
Basil

bug#25764: 26.0.50; Make some usability improvements to sieve-script management

[Eric Abrahamsen]

On 07/09/19 01:50 AM, Basil L. Contovounesios wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>>
>>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>>
>>>> I do use sieve! But I didn't write this code.
>
> I know, just thought I may as well fix these trivial instances upon
> seeing your patch.
>
>>>> Your proposed chances seem fine to me, though I'm wondering about
>>>> the use of `ngettext' -- I saw some arguments on these lists
>>>> recently about international/localization, and wasn't paying too
>>>> much attention, but didn't see a clear resolution.  Is it the
>>>> consensus that we're supposed to be using `ngettext' where possible?
>>>
>>> Oh, I didn't notice that part.  No, I don't think there's any such
>>> consensus...
>>
>> There are 23 uses of `ngettext' in the sources right now, which says to
>> me: "there's no consensus, but it's also not a problem".
>
> That's more or less my understanding as well:
>
> [45c525a685]: Fig grammar of count-lines-page
>   2019-06-09 17:00:05 -0700
>   https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=45c525a685041c274a26c70caed4088726a790de
>
> [7bba702e1a]: Use ngettext instead of dired-plural-s in Dired (bug#35287)
>   2019-05-05 22:43:27 +0300
>   https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=7bba702e1ad1ed343618e44cf5bbc2a1f079be0c
>
> I wasn't involved in the discussion, but IIRC l10n (i.e. using a no-op
> ngettext for now) is welcome and encouraged, it's just that we don't
> have an agreed upon or concrete implementation yet.  I think it's
> definitely better to mark the text as localisable now, while we're in
> the area, than leave the previous string formatting trick for future
> archaeologists to rediscover.  So I've now pushed my patch:

Okay, fair enough...