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

Add counsel-evil-registers #1368

Closed
wants to merge 1 commit into from
Closed

Add counsel-evil-registers #1368

wants to merge 1 commit into from

Conversation

jojojames
Copy link
Contributor

@jojojames jojojames commented Dec 10, 2017

Wanted to get an interest check on this. It adds support for seeing evil-registers.

Kind of similar to #1117 but for evil registers (so more similar to counsel-yank-pop).

I can spin this off somewhere else if it's not appropriate for ivy.

Couple questions:

  1. (ivy-format-function #'counsel--yank-pop-format-function) --> uses counsel-yank-pop's format function.

  2. counsel-evil-registers-height 5 --> I know it was mentioned that 5 seemed to be a pretty good middleground given the *3 factor. We could probably bump that default up just a tad since the default is actually pretty small. (i.e. It seems rare someone would have exactly 5 rows of yanks with 3 lines in them).

  3. I see we 'require features as we need them but that seems to assume they're there. We might want something like (counsel-require-program) but (counsel-require-feature) instead.

Lastly, a screenshot.

screenshot 2017-12-10 08 55 53

counsel.el Outdated

S will be of the form \"[register]: content\"."
(with-ivy-window
(setq last-command 'yank)
Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

Why is setting last-command necessary?

Copy link
Contributor Author

@jojojames jojojames Dec 10, 2017

Choose a reason for hiding this comment

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

Honestly, dunno. Copied it from counsel-yank-pop. I could remove it if we don't think it's necessary.

Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

In that case you can remove it - counsel-yank-pop needs to set it before calling yank-pop.

@dieggsy
Copy link
Contributor

@dieggsy dieggsy commented Dec 10, 2017

It might be nice to have a single option for this height and counsel-yank-pop-height? In any case, I think the height for counsel-evil-registers should probably default to counsel-yank-pop-height for consistency. Not sure.

@jojojames I think you can drop screenshots directly into the github issue, FWIW (unless you tried that and it's too large an image or something...)

counsel.el Outdated
(let ((ivy-format-function #'counsel--yank-pop-format-function)
(ivy-height counsel-evil-registers-height))
(ivy-read "evil-registers: "
(cl-loop for (key . val) in (evil-register-list)
Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

Perhaps it would be nicer to give an error message at the start of the command if evil is not available.

Copy link
Contributor Author

@jojojames jojojames Dec 10, 2017

Choose a reason for hiding this comment

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

Yeah, I agree. That was mainly question number 3.

Maybe I can add a function like (counsel-require-feature) that's similar to the other one... which we can use to gate this function.

What do you think?

Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

If you see enough places which could follow a "if feature X is unavailable, display an error message explaining this" pattern then sure, I support adding such a convenience function. IIUC, it would take two arguments, the required feature and the message to fail with. But this would belong in a separate PR. If there aren't enough call sites throughout this project, I don't think it would add much value.

@jojojames
Copy link
Contributor Author

@jojojames jojojames commented Dec 10, 2017

@dieggsy The screenshot was just to show this feature off (the height is at 15 instead of 5 though) and not complain about the defaults. :)

As for defaulting to the same value. Hmnn, I'm not sure if I wanna conjoin them but open to changing it if others felt the same way.

@dieggsy
Copy link
Contributor

@dieggsy dieggsy commented Dec 10, 2017

@jojojames I was saying that you don't need to provide screenshot through an external host like dropbox, github handles them if you drop the file directly into the issue

@jojojames
Copy link
Contributor Author

@jojojames jojojames commented Dec 10, 2017

@dieggsy Oh, thanks for that tip. I didn't know that. :(

I guess I should read the info more. "Attach files by dragging & dropping, selecting them, or pasting from the clipboard."

counsel.el Outdated
S will be of the form \"[register]: content\"."
(with-ivy-window
(insert
(string-trim-left s "^\\[.*\\]: "))))
Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

string-trim-left already adds a beginning-of-string (rather than beginning-of-line) anchor to its REGEXP argument so you don't have to.

But in general I think it's best to prefer built-in functions over inline ones defined in subr-x:

(replace-regexp-in-string "\\`\\[.*\\]: " "" s)

Copy link
Contributor Author

@jojojames jojojames Dec 10, 2017

Choose a reason for hiding this comment

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

Hmnn, what is the ` for (not extremely familiar with emacs regex.

I do think this particular replacement breaks compared to the first though.

For example, the second (replace-regexp) breaks (ironically) for this.

(defun counsel-evil-registers-action (s)
  "Paste contents of S, trimming the register part.

S will be of the form \"[register]: content\"."
  (with-ivy-window
    (insert
     (replace-regexp-in-string "\\`\\[.*\\]: " "" s)
     ;; (string-trim-left s "^\\[.*\\]: ")
     )))

As in copying that text and trying to paste it from evil-registers will past an incomplete version.

Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

^ matches the beginning of a string/buffer, but also any character following a newline character within said string/buffer. The backtick only matches the beginning of a string/buffer. The backtick is always preferred unless you're explicitly matching newlines. A rare example where ^ is wrong is when matching filenames, as they can conceivably include actual newline characters as part of their name. See (elisp) Regexp Spceial for an explanation of ^ and (elisp) Regexp Backslash for a description of the backtick.

I think the error might be arising from the use of a greedy wildcard operator. Try this instead:

(replace-regexp-in-string "\\`\\[.*?\\]: " "" s)

Copy link
Contributor Author

@jojojames jojojames Dec 10, 2017

Choose a reason for hiding this comment

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

Thanks, that works for me.

counsel.el Outdated
(cl-loop for (key . val) in (evil-register-list)
collect
(format "[%s]: %s" (char-to-string key)
(or (and val (stringp val) val) "")))
Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

This can be written

(format "[%c]: %s" key (if (stringp val) val ""))

but do we really want to include null vals in the list? I don't use evil so I am not familiar with the semantics of evil-register-list.

Copy link
Contributor Author

@jojojames jojojames Dec 10, 2017

Choose a reason for hiding this comment

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

Thanks, that looks much better.

Yeah, we do want to include empty strings because it shows that particular register doesn't have any contents in it yet.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 10, 2017

Looks good, thanks. Just fix the introduced byte-compile warnings (run make compile to check), and it's good to merge, I think.

@jojojames jojojames force-pushed the master branch 2 times, most recently from e7290eb to 6505e04 Compare Dec 10, 2017
@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 10, 2017

I think it would be nicer to have all lines of each register's contents indented to the same level, i.e. taking the [%c]: prefix into account. Thoughts?

@jojojames
Copy link
Contributor Author

@jojojames jojojames commented Dec 10, 2017

@basil-conto I don't have a strong opinion either way. Do you have a suggestion on how to do that?

(defun counsel-evil-registers ()
"Ivy replacement for `evil-show-registers'."
(interactive)
(if (fboundp 'evil-register-list)
Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

Just a tip, not requesting it - you can save on indentation with

(unless (fboundp 'evil-register-list)
  (user-error "..."))

as execution does not continue past errors.

Copy link
Contributor Author

@jojojames jojojames Dec 10, 2017

Choose a reason for hiding this comment

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

I think I need it so the byte compiler doesn't complain once I actually use that function.

Copy link
Collaborator

@basil-conto basil-conto Dec 10, 2017

Choose a reason for hiding this comment

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

Again, not suggesting you actually do this here, but in such cases you can just add a declare-function declaration.

Copy link
Contributor Author

@jojojames jojojames Dec 10, 2017

Choose a reason for hiding this comment

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

Thanks, sounds good.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 10, 2017

Thanks.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 10, 2017

@jojojames

Do you have a suggestion on how to do that?

A naive way would be to indent-rigidly all lines but the first, but then trimming the result for insertion becomes a bit messy. I don't know if text and display properties such as line-prefix and :align-to, respectively, could be used, but if so they could potentially lead to a cleaner implementation.

If no-one else is interested in looking into this I might just do so when I have too much work to procrastinate doing. :)

@jojojames
Copy link
Contributor Author

@jojojames jojojames commented Dec 11, 2017

@basil-conto I'd love to see the code if you ever get it going. :)

There's probably a few more things that can be done for evil-registers too, maybe an action to clear a register's contents, etc.

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