Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dynamic completion #10

Merged
merged 8 commits into from
Dec 1, 2019
Merged

Use dynamic completion #10

merged 8 commits into from
Dec 1, 2019

Conversation

thierryvolpiatto
Copy link
Member

DON'T MERGE, just a proof of concept, let me know people with large org files if it's faster.
Thanks. @alphapapa ping.

helm-org-startup-visibility filter have been disabled for now as it is
not working for some reasons (seems also it slowdown helm-org).

When submitting a pull request, please include the following information:

  • Emacs versions tested with:
    Emacs-26
  • Org versions tested with:
    The version coming with emacs-26
  • helm versions tested with:
    Last.
  • helm-core versions tested with:
    Last.

Thierry Volpiatto added 3 commits November 28, 2019 15:11
helm-org-startup-visibility filter have been disabled for now as it is
not working for some reasons (seems also it slowdown helm-org).
* helm-org.el (helm-org--get-candidates-in-file): Do it.
* helm-org.el (helm-source-org-headings-for-files): Do it.
@alphapapa
Copy link
Member

Hi Thierry,

Sorry, I don't understand what this new helm-dynamic-completion and match-dynamic thing is. I looked at some commits to the main Helm repo that add them, but I don't understand what it's doing or what its purpose is. Would you explain, please?

If this speeds up the headings-in-buffer sources, that would be great, as long as the heading visibility feature doesn't regress in functionality.

Thanks.

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Nov 29, 2019 via email

Thierry Volpiatto added 3 commits November 29, 2019 16:24
* helm-org.el (helm-org--get-candidates-in-file): Don't use a cons
cell but bind helm-realvalue prop.
it is not needed with the dynamic function that ensure all candidates
are computed and sorted.
@thierryvolpiatto
Copy link
Member Author

What can be done to speed up this in addition of dynamic completion is doing the same as what we are doing with helm-imenu.
AFAIU we are using find-file-noselect when fetching agenda files and parsing the resulting buffer for fetching headings, and the these buffers are staying open.
So when doing the initial call we could set 2 local vars in these buffers, one that handle the buffer-tick and one that handle a table containing all headers in this buffer.
For next calls of helm-org we could use the data saved in each buffer unless buffer-tick have changed, in this case rebuild the heading list in this buffer only.
Of course the first call will be the same than what we have actually but subsequent calls would be very fast even if something changed in one of the buffers.
I will perhaps implement this in next days...

for each buffer like in helm-imenu.

* helm-org.el (helm-org--headers-cache): New internals.
(helm-org--buffer-tick):                 New internals.
(helm-org-build-sources): New.
(helm-org--get-candidates-in-file): Set cache in each buffer.
(helm-org-agenda-files-headings): Use separate sources.
(helm-org-in-buffer-headings): Use new fn helm-org-build-sources.
(helm-org-parent-headings): Same.
@thierryvolpiatto
Copy link
Member Author

I have pushed the code with cache for individual buffers, please try, I would like to have feedback from people with huge org files.
Thanks.

@matthuszagh
Copy link

matthuszagh commented Nov 29, 2019

Thanks for taking a look at this @thierryvolpiatto! The solution works very well with my largest org file ~1.1MB. The lag used to be several seconds. I no longer notice any lag (after the cache is generated).

@alphapapa
Copy link
Member

Using :match-dynamic with helm-dynamic-completion allow computing directly the headings in :candidates with all-completions which is fast and provide as well in addition of usual multi matching what user set in completion-styles, it can be flex or helm-flex (similar to fuzzy) depending on the emacs version in use.

Thanks, but I don't understand what this new helm-dynamic-completion thing is or how it works. :) Why is it faster?

Regarding the caching of headings: That's a nice idea, but it seems like a lot of complexity to add to this package. Rather than implementing that in helm-org, you might consider looking at org-ql. I've already implemented a per-buffer cache, which caches the return value of functions at buffer positions, in the function org-ql--value-at: https://github.com/alphapapa/org-ql/blob/a1851ed0eb6587e822b62d18a884c1eabb1411c1/org-ql.el#L444 If you have a function which returns all headings in a buffer, you could call org-ql--value-at at point-min (in widened buffer) for your function, and it would cache the return value until the buffer tick changes. It could also be used to cache child or parent headings at a certain position. For example, see how it's called here: https://github.com/alphapapa/org-ql/blob/a1851ed0eb6587e822b62d18a884c1eabb1411c1/org-ql.el#L427

You might also consider using org-ql as a backend for listing headings. For example, this query returns all headings in a file, and it quickly retrieves them from the cache when run again:

(org-ql-select "~/org/main.org"
  '(regexp "*")
  :action #'org-ql--outline-path)

For retrieving headings matching certain regexps, a search with the terms can be used, like:

(org-ql-select "~/org/main.org"
  '(heading "word1" "word2")
  :action #'org-ql--outline-path)

The helm-org-ql command also makes such queries easy and fast: https://github.com/alphapapa/org-ql/blob/master/helm-org-ql.el For example, you can perform that same search with helm-org-ql by typing h:word1,word2. It would be easy to make a new command that searched only headings by keyword, regexp, etc, to avoid having to type the h: and commas.

Some ideas. :)

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Nov 29, 2019 via email

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Nov 29, 2019 via email

@thierryvolpiatto
Copy link
Member Author

About org-hide-leading-stars and org-startup-indented.
For the first one there is nothing to do (no need of a filtered-candidate-transformer like what was done before) as long as jit-lock ran in org buffer.
For the second, what is the purpose of indenting the helm buffer?
I propose to remove its support.
I already removed the filtered-candidate-transformer fn which IMO is usefulness and slowdown even more things.

@thierryvolpiatto thierryvolpiatto merged commit 0b9ba82 into master Dec 1, 2019
@alphapapa
Copy link
Member

For the second, what is the purpose of indenting the helm buffer? I propose to remove its support.

When outline paths are not displayed, subheadings are displayed indented to their level. Without that indentation, outline structure would appear flattened, only distinguishable by faces, which are not necessarily distinctive, depending on user configuration. Removing support for that feature would be a significant regression in functionality.

@alphapapa
Copy link
Member

alphapapa commented Dec 1, 2019

@thierryvolpiatto The new cache seems to return the wrong results when relevant settings have changed. For example:

  1. Call helm-org-in-buffer-headings.
  2. C-g.
  3. (setq helm-org-format-outline-path t).
  4. Call helm-org-in-buffer-headings again. The results from step 1 are displayed instead of the outline path.
  5. Insert a character into the Org buffer.
  6. Call helm-org-in-buffer-headings again. The outline paths are now displayed.

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Dec 1, 2019 via email

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Dec 1, 2019 via email

(replace-match "\\1\\2\\3" t nil (car i)))))))))
(cdr i)))))

(defun helm-org-get-candidates (filenames &optional parents)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thierryvolpiatto Spacemacs used this function syl20bnr/spacemacs#13123

Would you have a suggestion what function can be used instead? 🤔

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Dec 28, 2019 via email

@JAremko
Copy link

JAremko commented Dec 29, 2019

@thierryvolpiatto Will try, thanks. Too bad that helm-org--get-candidates-in-file looks like a private function: helm-org -- get-candidates-in-file

@thierryvolpiatto
Copy link
Member Author

thierryvolpiatto commented Dec 29, 2019 via email

smile13241324 pushed a commit to syl20bnr/spacemacs that referenced this pull request Jun 8, 2024
* Fix helm-spacemacs-help-faq

In #13136, helm-spacemacs-help-faq
supporting functions were rewritten because helm-org-get-candidates was removed
upstream (emacs-helm/helm-org#10).

One of the supporting functions, helm-spacemacs-help//get-faq-headings-list,
doesn't work. As a result, helm-spacemacs-help-faq (M-m h f) fails with "if:
Wrong type argument: stringp, helm".

It's unclear if this bug has existed since the Spacemacs PR because
helm-org-build-sources has changed since the helm-org
PR (https://github.com/emacs-helm/helm-org/commits/master/helm-org.el).

* Update CHANGELOG.develop
2ynn pushed a commit to 2ynn/spacemacs that referenced this pull request Jul 8, 2024
* Fix helm-spacemacs-help-faq

In syl20bnr#13136, helm-spacemacs-help-faq
supporting functions were rewritten because helm-org-get-candidates was removed
upstream (emacs-helm/helm-org#10).

One of the supporting functions, helm-spacemacs-help//get-faq-headings-list,
doesn't work. As a result, helm-spacemacs-help-faq (M-m h f) fails with "if:
Wrong type argument: stringp, helm".

It's unclear if this bug has existed since the Spacemacs PR because
helm-org-build-sources has changed since the helm-org
PR (https://github.com/emacs-helm/helm-org/commits/master/helm-org.el).

* Update CHANGELOG.develop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants