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

Touch-up counsel-outline #1700

Closed
wants to merge 5 commits into from

Conversation

basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Aug 5, 2018

Re: #1681, #1684
Cc: @ericdanan

Please review each commit in turn, I have tried to document and justify each relatively atomic change.

Questions

Should we be worried about all the markers accumulated by counsel-outline-candidates? Should we try to "free" them post-completion? Quoth (elisp) Overview of Markers:

   Insertion and deletion in a buffer must check all the markers and
relocate them if necessary.  This slows processing in a buffer with a
large number of markers.  For this reason, it is a good idea to make a
marker point nowhere if you are sure you don’t need it any more.
Markers that can no longer be accessed are eventually removed (see
Garbage Collection).

counsel.el Outdated
@@ -4047,6 +4044,7 @@ the current outline heading. See `counsel-outline-settings'."
current outline heading in org-mode buffers. See
`counsel-outline-settings'."
(apply 'org-get-heading (counsel--org-get-heading-args)))
(apply #'org-get-heading (counsel--org-get-heading-args)))
Copy link
Owner

@abo-abo abo-abo Aug 6, 2018

Choose a reason for hiding this comment

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

The same expression is called twice. Typo?

Copy link
Collaborator Author

@basil-conto basil-conto Aug 6, 2018

Choose a reason for hiding this comment

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

Oops, thanks. It's an artefact of trying to split the doc and code changes into separate commits, which is fixed by the subsequent doc fix. I'll clean up the commits now.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Aug 6, 2018

Thanks a lot. Looks good to me, except the suspected typo that I mentioned a minute earlier.

Should we be worried about all the markers accumulated by counsel-outline-candidates?

If the worries are backed up by some basic profiling, we should. But if the cost is negligible, we can ignore it.

Should we try to "free" them post-completion?

If it doesn't complicate the code too much.

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Aug 6, 2018

Looks good to me, except the suspected typo that I mentioned a minute earlier.

Thanks, the commits should be fixed now.

Should we be worried about all the markers accumulated by counsel-outline-candidates?

If the worries are backed up by some basic profiling, we should. But if the cost is negligible, we can ignore it.

Should we try to "free" them post-completion?

If it doesn't complicate the code too much.

I'll leave this investigation for a future time, if no-one else comments / steps up.

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Aug 6, 2018

I'll leave this investigation for a future time, if no-one else comments / steps up.

The future is now! (Is it obvious I'm procrastinating doing other work? :)

I've added a commit which frees any markers returned by counsel-outline-candidates.

I didn't notice or profile a performance hit before this commit, but what I did notice is that, after the first call to counsel-outline within an org-mode buffer, the buffer contained hundreds more positions with markers pointing to them (a ~7x increase). I did this by counting buffer-has-markers-at hits.

I'd rather err on the side of caution and simply free these markers after their use, given how simple it seems to do so (is it kosher to free them after the call to ivy-read?).

basil-conto added 5 commits Aug 6, 2018
(counsel-org-goto-all, counsel-outline-title-org)
(counsel-outline-candidates): #'-quote function symbols.
(counsel-org-goto): Remove unused internal aliases.
(counsel-org-agenda-headlines--candidates):
Use current name of obsolete varaliases.
(counsel-outline-face-style): Add documented option nil to :type.
(counsel-outline-settings, counsel-outline-title-markdown)
(counsel-outline-candidates, counsel-outline-action): Reindent.
(counsel-outline-title-latex): Do not unnecessarily set match data.
Align side comments.  Use inline comments for longer descriptions.
Do not expect forward-list to return resulting position, as this is
not documented.
(counsel-outline--preselect): Set initial value to zero.
(counsel-outline): Tweak docstring and prompt.  Use assq.
(counsel-outline-display-style, counsel-outline-path-separator)
(counsel-outline-face-style, counsel-outline-custom-faces)
(counsel-outline-settings, counsel-outline-title)
(counsel-outline-title-org, counsel-outline-title-markdown)
(counsel-outline-title-latex, counsel-outline-level-emacs-lisp)
(counsel-outline-candidates, counsel-outline--add-face):
Copy-edit documentation for clarity and formatting and according to
"(elisp) Documentation Tips".
(counsel-outline-candidates): Flatten nested let-expressions.
(counsel-outline--add-face): Modify string in place.
Use new name counsel-outline-face-style instead of obsolete name
counsel-org-goto-face-style.
The byte-compiler complains when a varaliased user option is used
before its declaration, so move the declarations to their call site.
Keep varaliases and declarations together.
(counsel--outline-free-markers): New function.
(counsel-org-goto-all, counsel-outline): Use it.
(counsel-outline-candidates):
Document onus on callers to free markers.

Re: abo-abo#1700
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Aug 7, 2018

Thanks.

is it kosher to free them after the call to ivy-read

If you want to be very precise, :unwind can be used (see e.g. swiper--ivy). That way, even if the user C-g, the resources will be cleaned up.

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Aug 7, 2018

If you want to be very precise, :unwind can be used (see e.g. swiper--ivy). That way, even if the user C-g, the resources will be cleaned up.

Doesn't :unwind get called too early (it's called before ivy-call)?

@basil-conto basil-conto deleted the blc/counsel-outline branch Aug 7, 2018
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Aug 8, 2018

You're right, I didn't consider that.

@ericdanan
Copy link
Contributor

@ericdanan ericdanan commented Aug 15, 2018

Sorry to reply late.

Freeing the markers has the drawback that ivy-resume no longer works after counsel-outline. If storing a marker for each candidate is too costly, an alternative could be to instead store the buffer name and value of point in the candidate, although I understand this would be less robust.

basil-conto added a commit to basil-conto/swiper that referenced this issue Aug 15, 2018
This reverts commit 66a1c36
"counsel.el: Free counsel-outline markers after use"
of 2018-08-06 23:06:02 +0300.

Re: abo-abo#1700
@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Aug 15, 2018

Good point, thanks @ericdanan. I've reverted the marker freeing commit in #1717; sorry about that.

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Aug 15, 2018

If storing a marker for each candidate is too costly, an alternative could be to instead store the buffer name and value of point in the candidate, although I understand this would be less robust.

It would definitely be less robust, but is ivy-resume ever realistically used after significant changes have been made to the source of the collection? More importantly, does anyone really expect it to work robustly in such cases?

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Aug 16, 2018

It would definitely be less robust, but is ivy-resume ever realistically used after significant changes have been made to the source of the collection?

We could try to make it more robust, similar to how g will re-update an ivy-occur buffer. But that would probably require the collection argument to be a function so that we would call it once more.

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.

None yet

3 participants