Re: [PATCH v5] Re: Improve the performance of `org-set-tags-command` on large `org-tag-alist`

[Ihor Radchenko]

"Christopher M. Miles" <numbchild@gmail.com> writes:

> Ok, I update and re-generated the patch, also add new document and Org NEWS 
> entries.

Thanks!
See my comments below.

> * lisp/org.el (org-fast-tag-selection): Filter out only maximum number
> limited tags has fast selection bound when `org-use-fast-tag-selection'
> is 'auto and tags has fast select bound keys.

Why only for 'auto?

> * doc/org-manual.org (org-fast-tag-selection-maximum-tags): Add new
> custom option document.

*documentation.

> --- a/doc/org-manual.org
> +++ b/doc/org-manual.org
> @@ -5073,6 +5073,16 @@ effect: start selection with {{{kbd(C-c C-c C-c)}}} 
> instead of
>  the special window is not even shown for single-key tag selection, it
>  comes up only when you press an extra {{{kbd(C-c)}}}.
>  
> +#+vindex: org-fast-tag-selection-maximum-tags
> +Limit the tags table when you have many tags. You can set the maximum

#+vindex entry will not be visible in the rendered manual. So, the first
 sentence of the paragraph reads awkwardly.

Also, please use double space between sentences. It is the convention
for Org manual, docstrings, and commit messages. See
doc/Documentation_Standards.org

> +(defcustom org-fast-tag-selection-maximum-tags 26
> +  "Set the maximum tags number for fast tag selection."
> +  :group 'org-tags
> +  :type 'number
> +  :safe #'numberp)

Why 26?
We use a-zA-Z{|}~

;; Characters available for auto-assignment.
         (tag-binding-char-list
          (eval-when-compile
            (string-to-list 
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ{|}~")))

Also, please add :package-version keyword in the defcustom.

>    (let* (;; Combined alist of all the tags and todo keywords.
> -         (tag-alist (append tag-table todo-table))
> +         (tag-alist (let* ((tags-table (append tag-table todo-table))
> +                           (binding-tags (seq-filter 'cdr tags-table)))
> +                      (cl-case org-use-fast-tag-selection
> +                        (auto (if (length> binding-tags 
> org-fast-tag-selection-maximum-tags)
> +                                  binding-tags
> +                                (seq-take tags-table 
> org-fast-tag-selection-maximum-tags)))
> +                        (t tags-table))))

May you please instead:

1. Store the total number of bound tags, but using a bit more complex
   filter (`(:startgroup "foo")', `(:endgroup "bar")' are also valid alist
   members, but do not correspond to tag bindings). See `(pcase
   tag-binding-spec ...)'
2. Store the total number of tag in tag groups. See `ingroup'.
3. Add these two numbers and compare them with
   `org-fast-tag-selection-maximum-tags', calculating how many extra
   tags can be displayed.
4. Do not filter `tag-alist', but instead plug a code into `(while (setq
   tag-binding-spec (pop tag-alist)) ...)', counting how many tags are in
   There, in `;; Insert the tag.', only insert the tag when (1) tag has
   binding - (cdr tag-binding-spec); (2) tag is in a group - ingroup;
   (3) we still have space for extra tags, according to (3), then also
   decrease the leftover tag count.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>