[Utkarsh Singh] Re: [PATCH] emacs: Add more front ends for address completion
Start of forwarded message From: Utkarsh Singh To: Tomi Ollila Subject: Re: [PATCH] emacs: Add more front ends for address completion Date: Thu, 10 Feb 2022 08:58:49 +0530 Hello Tomi, On 2022-02-09, 23:59 +0200, Tomi Ollila wrote: > On Tue, Feb 08 2022, Utkarsh Singh wrote: > >> Hello maintainers, >> >> Emacs Lisp Package Archive (ELPA) now includes a package called 'corfu', >> according to its documentation: >> >> Corfu enhances the default completion in region function with a >> completion overlay. The current candidates are shown in a popup >> below or above the point. Corfu is the minimalistic >> ~completion-in-region~ counterpart of the >> [[https://github.com/minad/vertico][Vertico]] minibuffer UI. >> >> Hence, this patch tries to add support for `completion-in-region' in >> `notmuch-address-expand-name'. By default, this behaviour is turned off >> so that existing users can enjoy existing completion techniques. > > The current "default" (i.e. w/o any notmuch emacs mua configuration) is to > use completing-read to do the completion. If "company" is available, then > company is used by default (w/ all address harvesting and so on...). > > This is "messy" enough ;( (i.e the notmuch-address-selection-function > is called if company mode is not available or notmuch-address-command > is a string instead of 'internal or 'as-is (or whatnot, too tired to do > deep investigation there ;/) > > This change, contributes even more "complexity" there. To keep the > complexity to the same level would be adding more > notmuch-address-selection-functions and have the defcustom there list > the options (also probably the name of notmuch-address-selection-function > would need to be changed to notmuch-fallback-address-selection-function > ;/) > I think, in general, you're right about the complexity and we should try minimize it. But this patch was originally derived from a bug I was experiencing with `(global-corfu-mode 1)' in `notmuch-message-mode'. Here are the steps to reproduce the bug: 1. [Install](https://notmuchmail.org/#index7h2) `notmuch`. 2. In emacs -Q session, evaluate the following ```elisp (progn (add-to-list 'load-path "/usr/share/emacs/site-lisp") ; Notmuch Emacs interface (package-initialize) (package-install 'corfu) (package-install 'vertico) (require 'notmuch) (require 'corfu) (require 'vertico) (vertico-mode 1) (corfu-global-mode 1) (notmuch-mua-new-mail)) ``` 3. Press `TAB` to generate address completion, then press `RET` to select address of your choice. After selection, you will notice that `notmuch-address-expand-name` fails to insert desired address, that is, you will get the following: ``` To: ``` Note: Check the trailing spaces. > All this said, I think this is not simple to solve, as this otherwise fine > change would indicate :/ No problem, I'm willing to help the maintainers on this matter. -- Utkarsh Singh https://utkarshsingh.xyz/ End of forwarded message -- Utkarsh Singh https://utkarshsingh.xyz/ ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
Michael J Gruber writes: > If gdb is missing then some files are never written to so that the > comparisons of non-existing files succeeds for the wrong reason, > claiming that `notmch new` is idempotent when it was in fact never run. > > Catch this and (for lack of a better spot) set up the files with a > reason for the FAIL. > I'm a bit confused by this. For me if gdb is missing I get missing prerequisites: gdb(1) SKIP all tests in T380-atomicity ... All 1842 tests behaved as expected (8 expected failures). All tests in 1 file skipped. Do I misunderstand something, or is prerequisite detection not working for you? d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH] emacs: Add more front ends for address completion
On Tue, Feb 08 2022, Utkarsh Singh wrote: > Hello maintainers, > > Emacs Lisp Package Archive (ELPA) now includes a package called 'corfu', > according to its documentation: > > Corfu enhances the default completion in region function with a > completion overlay. The current candidates are shown in a popup > below or above the point. Corfu is the minimalistic > ~completion-in-region~ counterpart of the > [[https://github.com/minad/vertico][Vertico]] minibuffer UI. > > Hence, this patch tries to add support for `completion-in-region' in > `notmuch-address-expand-name'. By default, this behaviour is turned off > so that existing users can enjoy existing completion techniques. The current "default" (i.e. w/o any notmuch emacs mua configuration) is to use completing-read to do the completion. If "company" is available, then company is used by default (w/ all address harvesting and so on...). This is "messy" enough ;( (i.e the notmuch-address-selection-function is called if company mode is not available or notmuch-address-command is a string instead of 'internal or 'as-is (or whatnot, too tired to do deep investigation there ;/) This change, contributes even more "complexity" there. To keep the complexity to the same level would be adding more notmuch-address-selection-functions and have the defcustom there list the options (also probably the name of notmuch-address-selection-function would need to be changed to notmuch-fallback-address-selection-function ;/) Also, if name was notmuch-address-selection-function but its interface changed, current users using their own functions (I am in that list) would get error there (the interface would have to be (defun notmuch-address-selection-function (prompt collection initial-input beg end) to be backward compatible) If name was changed then their own function would not be used -- which is OK, things change and users can read from NEWS how to be compatible again... All this said, I think this is not simple to solve, as this otherwise fine change would indicate :/ Tomi > > Thank you, > Utkarsh Singh > -- > Utkarsh Singh > https://utkarshsingh.xyz/ > From fdc88b81fef763f7d7dcdc899aa8e90482c574fa Mon Sep 17 00:00:00 2001 > From: Utkarsh Singh > Date: Tue, 8 Feb 2022 19:17:26 +0530 > Subject: [PATCH] emacs: Add more front ends for address completion > > Add support for address completion through completion-in-region. > * notmuch-address.el (notmuch-address-use-completion-in-region): > Introduce customizable variable to activate the new front end. > (notmuch-address-selection-function, notmuch-address-expand-name): Use > it. > --- > emacs/notmuch-address.el | 28 ++-- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el > index 1a4cdda2..cfb56a3a 100644 > --- a/emacs/notmuch-address.el > +++ b/emacs/notmuch-address.el > @@ -123,10 +123,10 @@ you should make sure it is not somewhere publicly > readable." > (defcustom notmuch-address-selection-function > 'notmuch-address-selection-function >"The function to select address from given list. > > -The function is called with PROMPT, COLLECTION, and INITIAL-INPUT > -as arguments (subset of what `completing-read' can be called > -with). While executed the value of `completion-ignore-case' > -is t. See documentation of function > +The function is called with PROMPT, COLLECTION, INITIAL-INPUT, > +BEG and END as arguments (subset of what `completing-read' can be > +called with). While executed the value of > +`completion-ignore-case' is t. See documentation of function > `notmuch-address-selection-function' to know how address > selection is made by default." >:type 'function > @@ -150,13 +150,19 @@ matching `notmuch-address-completion-headers-regexp'." >:group 'notmuch-send >:group 'notmuch-address) > > +(defcustom notmuch-address-use-completion-in-region nil > + "Use `completion-in-region' for address completion." > + :type 'boolean > + :group 'notmuch-send > + :group 'notmuch-address) > + > ;;; Setup > > -(defun notmuch-address-selection-function (prompt collection initial-input) > - "Call (`completing-read' > - PROMPT COLLECTION nil nil INITIAL-INPUT 'notmuch-address-history)" > - (completing-read > - prompt collection nil nil initial-input 'notmuch-address-history)) > +(defun notmuch-address-selection-function (prompt collection initial-input > beg end) > + (if notmuch-address-use-completion-in-region > + (completion-in-region beg end collection) > +(completing-read > + prompt collection nil nil initial-input 'notmuch-address-history))) > > (defvar notmuch-address-completion-headers-regexp > > "^\\(Resent-\\)?\\(To\\|B?Cc\\|Reply-To\\|From\\|Mail-Followup-To\\|Mail-Copies-To\\):") > @@ -245,7 +251,9 @@ requiring external commands." >(funcall notmuch-address-selection-function >
Re: [PATCH 4/4] test: set up the outcount file for T380.1
On Wed, Feb 09 2022, Michael J. Gruber wrote:
> If gdb is present but for some reason `atomicity.py` fails to write to
> the output file then the test fails with some ugly bash errors in the
> wrong places (because the outcount variable is empty).
>
> Therefore, set up the outcount file with `0` to get the test script to
> rund and the test to fail fpr a clearer reason.
>
> Background: We noticed this with arch armhfp emulated on x86_64 in
> Fedora's COPR test build environment.
>
> Signed-off-by: Michael J Gruber
> ---
> test/T380-atomicity.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
> index 49df5c38..caac28a3 100755
> --- a/test/T380-atomicity.sh
> +++ b/test/T380-atomicity.sh
> @@ -64,6 +64,7 @@ if test_require_external_prereq gdb; then
> # -tty /dev/null works around a conflict between the 'timeout' wrapper
> # and gdb's attempt to control the TTY.
> export MAIL_DIR
> +echo -n 0 > outcount
printf 0 > outcount (is my suggestion)
> ${TEST_GDB} -tty /dev/null -batch -x $NOTMUCH_SRCDIR/test/atomicity.py
> notmuch 1>gdb.out 2>&1
>
> # Get the final, golden output
> --
> 2.35.1.306.ga00bde9711
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 3/4] test: reword T380.2 to be clearer
On Wed, Feb 09 2022, Michael J. Gruber wrote: > T380.2 gives a test description which depends on the actual test output, > rather than the expected outcome or actual test which is performed. > > So, when the test fails due missing abort points, the test describes > itself as `detected 0>10 abort points` so that it's not clear which part > or which number is the expectation. (Also, `0>10` is no number ...) > > When the test is not run for some reason and fails because of that, the > test describes itself as `detected >10 abort points`, which arguably is > better or worse. > > Reword it to say `detected more than 10 abort points`, which is the > actual expectation and what we test for. The failing test line still > gives the actual number. > > Signed-off-by: Michael J Gruber > --- > test/T380-atomicity.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh > index 7f618062..49df5c38 100755 > --- a/test/T380-atomicity.sh > +++ b/test/T380-atomicity.sh > @@ -99,7 +99,7 @@ fi > test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts' > test_expect_equal_file expectall searchall > > -test_begin_subtest "detected $outcount>10 abort points" > +test_begin_subtest "detected more than 10 abort points" probably ok (I trust the commit msg is right :D) > test_expect_success "test $outcount -gt 10" > > test_done > -- > 2.35.1.306.ga00bde9711 > > ___ > notmuch mailing list -- notmuch@notmuchmail.org > To unsubscribe send an email to notmuch-le...@notmuchmail.org ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/4] test: due not pass T380.1 for the wrong reasons
On Wed, Feb 09 2022, Michael J. Gruber wrote: > If gdb is missing then some files are never written to so that the > comparisons of non-existing files succeeds for the wrong reason, > claiming that `notmch new` is idempotent when it was in fact never run. > > Catch this and (for lack of a better spot) set up the files with a > reason for the FAIL. > > Signed-off-by: Michael J Gruber > --- > test/T380-atomicity.sh | 4 > 1 file changed, 4 insertions(+) > > diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh > index a6f1e037..7f618062 100755 > --- a/test/T380-atomicity.sh > +++ b/test/T380-atomicity.sh > @@ -90,6 +90,10 @@ if test_require_external_prereq gdb; then > i=$(expr $end - 1) > fi > done > +else > +echo -n "Test fails due to missing gdb." > searchall > +echo -n > expectall I am not much of a fan of 'echo -n' (I remember seeing -n (and newline echoed...), therefore first to use printf and second : > expectall (unless printf '' > expectall) > +outcount=0 > fi > > test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts' > -- > 2.35.1.306.ga00bde9711 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 1/4] test: correct comparison order in T380
On Wed, Feb 09 2022, Michael J. Gruber wrote: > Specifying test comparisons as "expected actual" gives a better readable > diff since the "-" indicates missing, "+" additional items compared to > the expectations. > > Signed-off-by: Michael J Gruber > --- > test/T380-atomicity.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh > index afe49d93..a6f1e037 100755 > --- a/test/T380-atomicity.sh > +++ b/test/T380-atomicity.sh > @@ -93,7 +93,7 @@ if test_require_external_prereq gdb; then > fi > > test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts' > -test_expect_equal_file searchall expectall > +test_expect_equal_file expectall searchall Good in princible, but if things are like decade ago, we still have hundreds of these in wrong order -- if so, all should be changed for consistency. If I am wrong and all / many of those are already changed, then this is OK. > > test_begin_subtest "detected $outcount>10 abort points" > test_expect_success "test $outcount -gt 10" > -- > 2.35.1.306.ga00bde9711 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 1/4] test: correct comparison order in T380
Specifying test comparisons as "expected actual" gives a better readable diff since the "-" indicates missing, "+" additional items compared to the expectations. Signed-off-by: Michael J Gruber --- test/T380-atomicity.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh index afe49d93..a6f1e037 100755 --- a/test/T380-atomicity.sh +++ b/test/T380-atomicity.sh @@ -93,7 +93,7 @@ if test_require_external_prereq gdb; then fi test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts' -test_expect_equal_file searchall expectall +test_expect_equal_file expectall searchall test_begin_subtest "detected $outcount>10 abort points" test_expect_success "test $outcount -gt 10" -- 2.35.1.306.ga00bde9711 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 3/4] test: reword T380.2 to be clearer
T380.2 gives a test description which depends on the actual test output, rather than the expected outcome or actual test which is performed. So, when the test fails due missing abort points, the test describes itself as `detected 0>10 abort points` so that it's not clear which part or which number is the expectation. (Also, `0>10` is no number ...) When the test is not run for some reason and fails because of that, the test describes itself as `detected >10 abort points`, which arguably is better or worse. Reword it to say `detected more than 10 abort points`, which is the actual expectation and what we test for. The failing test line still gives the actual number. Signed-off-by: Michael J Gruber --- test/T380-atomicity.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh index 7f618062..49df5c38 100755 --- a/test/T380-atomicity.sh +++ b/test/T380-atomicity.sh @@ -99,7 +99,7 @@ fi test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts' test_expect_equal_file expectall searchall -test_begin_subtest "detected $outcount>10 abort points" +test_begin_subtest "detected more than 10 abort points" test_expect_success "test $outcount -gt 10" test_done -- 2.35.1.306.ga00bde9711 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 2/4] test: due not pass T380.1 for the wrong reasons
If gdb is missing then some files are never written to so that the comparisons of non-existing files succeeds for the wrong reason, claiming that `notmch new` is idempotent when it was in fact never run. Catch this and (for lack of a better spot) set up the files with a reason for the FAIL. Signed-off-by: Michael J Gruber --- test/T380-atomicity.sh | 4 1 file changed, 4 insertions(+) diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh index a6f1e037..7f618062 100755 --- a/test/T380-atomicity.sh +++ b/test/T380-atomicity.sh @@ -90,6 +90,10 @@ if test_require_external_prereq gdb; then i=$(expr $end - 1) fi done +else +echo -n "Test fails due to missing gdb." > searchall +echo -n > expectall +outcount=0 fi test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts' -- 2.35.1.306.ga00bde9711 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 4/4] test: set up the outcount file for T380.1
If gdb is present but for some reason `atomicity.py` fails to write to
the output file then the test fails with some ugly bash errors in the
wrong places (because the outcount variable is empty).
Therefore, set up the outcount file with `0` to get the test script to
rund and the test to fail fpr a clearer reason.
Background: We noticed this with arch armhfp emulated on x86_64 in
Fedora's COPR test build environment.
Signed-off-by: Michael J Gruber
---
test/T380-atomicity.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/test/T380-atomicity.sh b/test/T380-atomicity.sh
index 49df5c38..caac28a3 100755
--- a/test/T380-atomicity.sh
+++ b/test/T380-atomicity.sh
@@ -64,6 +64,7 @@ if test_require_external_prereq gdb; then
# -tty /dev/null works around a conflict between the 'timeout' wrapper
# and gdb's attempt to control the TTY.
export MAIL_DIR
+echo -n 0 > outcount
${TEST_GDB} -tty /dev/null -batch -x $NOTMUCH_SRCDIR/test/atomicity.py
notmuch 1>gdb.out 2>&1
# Get the final, golden output
--
2.35.1.306.ga00bde9711
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 0/4] test: T380 rework
I've been testing with tests now (duh), i.e. enabling the tests for regular Fedora distribution builds. This seems to be mostly OK, no flaky fails so far. asan works locally (on x86_64) but we don't release build with asan; and we don't have sfsexp in Fedora yet (working on it). All other tests run. In this context I noticed a few pecularities with T380. They all have to do with the cases when either gdb is not available or fails to set breakpoints, and this series tries to improve test output (and make tests fail for the right reason). The series is kind of micro-granular with commit messages longer than the patch diff (in good old git.git fashion), to make it readable and also partially-pickable if you prefer. Michael J Gruber (4): test: correct comparison order in T380 test: due not pass T380.1 for the wrong reasons test: reword T380.2 to be clearer test: set up the outcount file for T380.1 test/T380-atomicity.sh | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) -- 2.35.1.306.ga00bde9711 ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: fcc and external attachments
David Bremner writes: > It seems likely this is an unintended consequence of > da302e1cbaaab89b2bbb32c0f59e1aa6ee708455. > > Since notmuch-emacs does it's own Fcc processing, it's a bit > hit-and-miss whether a given message-fcc-* variable is respected. I'll > have a look at how hard it is to fix, but first I need a reliable recipe > for triggering the externalization in message-mode (i.e. without > involving notmuch). I tried in emacs -q, customize > message-fcc-externalize-attachments, and running M-x compose-mail, but > that still saved the whole attachments. Maybe it was the type of > attachement or something, I don't understand it yet. > > d While trying to understand the problem I found out that gnus had a somewhat related issue, as you can see in this commit from emacs branch "emacs-28": https://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-28=d8bd7d015e626c73351938626a01288028ebe1c5 So I made this little modification to notmuch in the function notmuch-maildir-setup-message-for-saving: diff --git a/emacs/notmuch-maildir-fcc.el b/emacs/notmuch-maildir-fcc.el index 51020788..d58f209b 100644 --- a/emacs/notmuch-maildir-fcc.el +++ b/emacs/notmuch-maildir-fcc.el @@ -159,6 +159,8 @@ Otherwise set it according to `notmuch-fcc-dirs'." This should be called on a temporary copy. This is taken from the function message-do-fcc." + (when mml-externalize-attachments +(setq message-encoded-mail-cache nil)) (if (not message-encoded-mail-cache) (message-encode-message-body) (erase-buffer) Now I have again the original behaviour with external attachments in fcc copies. I understand that this forces a new encoding for the local copy in the fcc-externalize case and ignores the cached encoded message, which is maybe what you were trying to avoid in the first place. I did not test what happens to a signed message with attachments. ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
