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

counsel.el (counsel-unicode-char): Make lazy #1204

Merged
merged 2 commits into from
Oct 7, 2017
Merged

Conversation

basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Sep 21, 2017

Changelog

  • On 2017-08-31, ucs-names was refactored as a hash-table in the Emacs development tree: support this
  • Use lazy completion table to speed up subsequent invocations and ensure collection is sorted from the outset
  • Improve completion transformer:
    • Ensure space between char name and glyph
    • Fit line width to default fill-column of 70, allowing for a maximum char-width of 4
  • Remove no-op minibuffer-allow-text-properties binding
  • Add :caller keyword to counsel-unicode-char
  • Fix a couple of incorrect :group customisation keywords

Before merging

This PR is not copyright exempt and I have previously contributed changes amounting to ~15 LOC to this project. I recently contacted assign@gnu.org about the copyright assignment forms and am currently waiting to hear back.

@abo-abo
Copy link
Owner

abo-abo commented Sep 21, 2017

Thanks for the contribution. Let me know when there's an update from FSF.

@basil-conto
Copy link
Collaborator Author

Status update: I just emailed the signed form back so I expect confirmation in the coming week.

@manuel-uberti
Copy link
Contributor

Any news on this one?

@basil-conto
Copy link
Collaborator Author

On Friday, I received an email apologising for the delay and stating the assignment will be confirmed today.

@kaushalmodi
Copy link
Contributor

Sorry, I seem to have done an independent bug fix of this in this PR: #1223

I started on this debug from the GNU Elpa side :P

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28688

@abo-abo
Copy link
Owner

abo-abo commented Oct 3, 2017

@basil-conto Sorry, I merged @kaushalmodi commit without remembering that this one is related.
Can you please have a look and see if anything from your implemenation can be used for improvement?

By the way, you're still not on copyright.list, I just checked. Maybe in a day or two...

@basil-conto
Copy link
Collaborator Author

@abo-abo @kaushalmodi No worries and thanks.

Can you please have a look and see if anything from your implemenation can be used for improvement?

Will do so as soon as I hear word from FSF to avoid keeping everyone waiting again.

Use lazy completion table to speed up subsequent invocations.
Amortised speedup is 2-3x in simulated benchmarks on the author's
machine, and subsequent invocations feel instantaneous in practice.

Ensure collection is sorted from the outset, as the result of
ucs-names is never entirely sorted. This is especially important in
later versions returning hash maps, as they have even weaker order
guarantees than alists.

Consolidate alist and hash table formatting functions: DRY.

Improve completion transformer: ensure space between char name and
glyph; fit line width to default fill-column of 70, allowing for a
maximum char-width of 4.

Remove no-op minibuffer-allow-text-properties binding.

Add :caller keyword to counsel-unicode-char.
@basil-conto
Copy link
Collaborator Author

@abo-abo Can you please confirm that I appear on copyright.list now?

@abo-abo @kaushalmodi I've rebased my changes and improved comments + commit message. Can you please review again?

Thanks again for your patience.

@basil-conto basil-conto changed the title counsel.el (counsel-unicode-char): Support hash-table ucs-names counsel.el (counsel-unicode-char): Make lazy Oct 6, 2017
@abo-abo abo-abo merged commit cd4e789 into abo-abo:master Oct 7, 2017
@abo-abo
Copy link
Owner

abo-abo commented Oct 7, 2017

@basil-conto You're on the list now. Thanks for the contribution.

@basil-conto basil-conto deleted the ucs branch October 7, 2017 19:23
astoff pushed a commit to astoff/swiper that referenced this pull request Jan 1, 2021
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

4 participants