Re: [PATCH] I updated patch by deleteing duplicate tags

[Kyle Meyer]

Christopher Miles writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> stardiviner writes:
>>>> By this patch, will merge both buffer-local tags and user defined global
>>>> `org-tags-alist`.
>>
>> It does a bit more than that.  It uses org-global-tags-completion-table,
>> which considers tags in all agenda files by default and takes into
>> account org-tag-alist (as well as org-tag-persistent-alist) via the use
>> of the org-current-tag-alist variable.
>
> That's what I want. Why obviously user pre-defined tags can't be used 
> globally.
> Right? It should be.

My point was that it's not just adding these pre-defined lists (#1);
it's also adding tags from agenda files (#2).  I think we certainly
don't want to do #2 unconditionally.  On the other hand, #1 is probably
okay, but as I said in my last email, I could see not wanting that
either.

Anyway, the patch I proposed (which you've adapted for your latest
round), avoids #2 by guarding the behavior behind the existing
org-complete-tags-always-offer-all-agenda-tags option, and I think
that's a good path forward.

> Subject: [PATCH] org.el: Complete tags from both global and buffer local
>
> * lisp/org.el (org-fast-tag-selection): Merge buffer local tags with
> global alist of tags. And it obey the option
> org-complete-tags-always-offer-all-agenda-tags.

s/obey/&s/

> * doc/org-manual.org: Update the TAB key doc in tags selection UI.
>
> * etc/ORG-NEWS: Mention the change in org-set-tags-command.
> ---
>  doc/org-manual.org |  6 +++---
>  etc/ORG-NEWS       |  5 +++++
>  lisp/org.el        | 24 ++++++++++++++----------
>  3 files changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/doc/org-manual.org b/doc/org-manual.org
> index b015b502c..01cec4b8d 100644
> --- a/doc/org-manual.org
> +++ b/doc/org-manual.org
> @@ -4860,9 +4860,9 @@ In this interface, you can also use the following 
> special keys:
>  
>    #+kindex: TAB
>    Enter a tag in the minibuffer, even if the tag is not in the
> -  predefined list.  You can complete on all tags present in the
> -  buffer.  You can also add several tags: just separate them with
> -  a comma.
> +  predefined list.  You can complete on all tags present in the buffer
> +  and globally pre-defined tags from ~org-tag{-persistent}-alist~.

Please spell these two options out: ~org-tag-alist~ and
~org-tag-persistent-alist~.

> +  You can also add several tags: just separate them with a comma.
>  
>  - {{{kbd(SPC)}}} ::
>  
> diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
> index 5e5f1954d..5e68d27c0 100644
> --- a/etc/ORG-NEWS
> +++ b/etc/ORG-NEWS
> @@ -149,6 +149,11 @@ Example:
>  A new =u= mode flag for Calc formulas in Org tables has been added to
>  enable Calc units simplification mode.
>  
> +*** =org-set-tags-command= select tags from 
> ~org-global-tags-completion-table~
> +
> +Let =org-set-tags-command= complete tags from global tags list (both
> +buffer-local tags and ~org-tag{-persistent}-alist~).

Same note about avoiding shorthand for the option names.

Also, please rewrite this to make it clear that this is relevant for TAB
in the fast completion interface.  It's probably also worth mentioning
that org-complete-tags-always-offer-all-agenda-tags is now honored.

And, very minor, but any reason for the mix of =...= and ~...~ markup
here?

> diff --git a/lisp/org.el b/lisp/org.el
> index 5b0ae389c..ba816dfa6 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -12139,7 +12139,7 @@ (defun org-fast-tag-selection (current inherited 
> table &optional todo-table)
>                                 fulltable))))
>        (buf (current-buffer))
>        (expert (eq org-fast-tag-selection-single-key 'expert))
> -      (buffer-tags nil)
> +      (tab-tags nil)
>        (fwidth (+ maxlen 3 1 3))
>        (ncol (/ (- (window-width) 4) fwidth))
>        (i-face 'org-done)
> @@ -12274,16 +12274,20 @@ (defun org-fast-tag-selection (current inherited 
> table &optional todo-table)
>                   (setq current nil)
>                   (when exit-after-next (setq exit-after-next 'now)))
>                  ((= c ?\t)
> -                 (condition-case nil

Unlike the patch I sent, you've dropped the condition-case here, which I
don't think was intentional.

Thanks.