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

Wordatpt dwim #1871

Closed
wants to merge 2 commits into from
Closed

Wordatpt dwim #1871

wants to merge 2 commits into from

Conversation

@mookid
Copy link
Contributor

@mookid mookid commented Dec 24, 2018

This is a follow-up to #1868.

I propose this new default function for grep-like commands.
It provides a 'do what I mean' behaviour to get a meaningful initial input to search.

In particular, using the active region plays well with packages like expand-region.

To be discussed?

@mookid
Copy link
Contributor Author

@mookid mookid commented Dec 24, 2018

it will probably get commented, so I'll address that here:

  • yes, add-to-list is used in a loop. It ensures that several evaluations of the whole counsel buffer do not add several times the things into the alist (what the usages of cl-pushnew fail to do in the current other similar customizations).

ivy.el Outdated
@@ -4426,6 +4426,20 @@ EVENT gives the mouse position."
(expand-file-name "doc/ivy-help.org"))))
"The file for `ivy-help'.")

(defun ivy-word-at-point-dwim ()
"Extracts a meaningful part of the current buffer.
Copy link
Contributor Author

@mookid mookid Dec 24, 2018

The elevator pitch part of that docstring sucks. If anyone has a better proposal...

"Return either text from region (if active) or `current-word'."

ivy.el Outdated
@@ -4426,6 +4426,20 @@ EVENT gives the mouse position."
(expand-file-name "doc/ivy-help.org"))))
"The file for `ivy-help'.")

(defun ivy-word-at-point-dwim ()
"Extracts a meaningful part of the current buffer.
"Return either text from region (if active) or `current-word'."

ivy.el Outdated
@@ -4424,6 +4426,20 @@ EVENT gives the mouse position."
(expand-file-name "doc/ivy-help.org"))))
"The file for `ivy-help'.")

(defun ivy-word-at-point-dwim ()

ivy-region-text-or-current-word.

ivy.el Outdated Show resolved Hide resolved
ivy.el Outdated
(defun ivy-word-at-point-dwim ()
"Extracts a meaningful part of the current buffer.

This function tries to DWIM. It either uses the beginning of the region if activated, or the current word.

This could be superfluous given the above correction.

counsel.el Outdated
(cons cmd 'ivy-word-at-point-dwim)
nil
(lambda (cell1 cell2) (eq (car cell1) (car cell2)))))

Copy link

@Alexander-Shukaev Alexander-Shukaev Dec 24, 2018

This looks ugly and also may result in side effects. Never do any logic on the top level apart from maybe

;;;###autoload
(add-to-list 'auto-mode-alist '("\\.foo\\'" . foo-mode))

because you'd otherwise alter user's configuration in an unexpected way (see The Emacs Lisp Style Guide for a good summary). As a result, this code should really be part of defcustom. I'd just let a local list there, where I'd push (or better add-to-list as you said) in that loop, and then I'd nconc it with '((org-refile . "^") ....

Copy link
Contributor Author

@mookid mookid Dec 25, 2018

I fail to understand the difference between the last part of your answer and the current proposal.

Copy link

@Alexander-Shukaev Alexander-Shukaev Dec 25, 2018

The difference will become visible if one setq-default the ivy-initial-inputs-alist to something custom before loading ivy. That is the result would be custom value in ivy-initial-inputs-alist plus whatever you now forcefully inject into it with add-to-list on the top level upon load. These are side effects and are considered bad practice.

Further problem is that if ivy is loaded before counsel, then ivy-highlight-grep-commands is simply nil and that modification of initial-inputs-alist does nothing. Fragile.

Further problem is that if ivy is loaded before counsel, then ivy-highlight-grep-commands is simply nil and that modification of initial-inputs-alist does nothing. Fragile.

I take back the last comment as I overlooked that you added this code to counsel instead of ivy. Forceful default initialization of ivy-initial-inputs-alist still applies though.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Dec 24, 2018

The solution is good overall though it still does not fully address #1867 as it keeps on tampering with ivy-initial-inputs-alist, and that's not what we want here. Give one more read to the final proposal made in #1867. The major advantage of it over the current (ivy-initial-inputs-alist) is that it will not require one to delete the initial input if one rather wants to type out some custom pattern. That is the function you implemented now should rather be part of ivy-read-def-alist, which is yet to be implemented.

@mookid
Copy link
Contributor Author

@mookid mookid commented Dec 25, 2018

The solution is good overall though it still does not fully address #1867 as it keeps on tampering with ivy-initial-inputs-alist, and that's not what we want here

i am not that worried about that issue; someone else should feel free to implement it.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Dec 25, 2018

Right, then your default initialization of ivy-initial-inputs-alist has to become more clever. I would not welcome its current default if this gets merged (in particular, as already mentioned, for me it is unacceptable to have to manually clear initial input if I want to type some custom pattern). Thus, this default initialization logic needs to be redesigned. Here is what comes to my mind:

(defcustom counsel-initial-input-default
  #'ivy-region-text-or-current-word)

(defun counsel-set-initial-inputs-alist (symbol value)
  (when (boundp symbol)
    (dolist (entry (symbol-value symbol))
      (setq ivy-initial-inputs-alist
        (assq-delete-all (car entry) ivy-initial-inputs-alist))))
  (dolist (entry value)
    (add-to-list 'ivy-initial-inputs-alist
                 entry
                 nil
                 #'(lambda (entry-1 entry-2)
                     (eq (car entry-1) (car entry-2)))))
  (set-default symbol value))

(defcustom counsel-initial-inputs-alist-default
  nil)

(defcustom counsel-initial-inputs-alist
  (when (and counsel-initial-inputs-alist-default
             counsel-initial-input-default)
    (let (alist)
      (dolist (command ivy-highlight-grep-commands)
        (push (cons command counsel-initial-input-default) alist))
      alist))
  :set #'counsel-set-initial-inputs-alist)

This way, first of all, it is disabled by default thanks to counsel-initial-inputs-alist-default being nil. You can enable it by changing it to t. Secondly, we have fine control over its initialization by either setting counsel-initial-input-default or counsel-initial-inputs-alist directly. Furthermore, with the above approach, ivy-initial-inputs-alist will be correctly modified even if counsel-initial-inputs-alist is customized after counsel has been loaded.

@mookid
Copy link
Contributor Author

@mookid mookid commented Dec 26, 2018

Right, then your default initialization of ivy-initial-inputs-alist has to become more clever.

I think that this is becoming too complicated.
I propose to just stop making that the default, and let users deal with that defcustom themselves.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Dec 27, 2018

I disagree. The point is that what you proposed is indeed a good "second-class" citizen default which, if not implemented in the form of the (tested) code, has to be be somehow documented in order to be advertised to the users. And we all know how much better it is to write actual self-documenting code rather than convoluted comments or docstrings.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Jan 18, 2019

Why is this closed? I'm personally looking forward to this being merged. Do you plain to make a separate PR for this or?

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 21, 2019

I'm also interested why it's closed. @mookid please ping if you still want this merged.

@mookid
Copy link
Contributor Author

@mookid mookid commented Jan 31, 2019

@abo-abo @Alexander-Shukaev feel free to take some code from here, of course. I will be happy to see these features integrated. I just don't have the time to work on swiper at the moment.

@abo-abo thanks again for the continuous work you are doing on emacs packages!

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Feb 5, 2019

@mookid Thanks for the explanation and for your past contributions. Feel free to reopen when you have more time.

@Alexander-Shukaev If you're interested in this feature, feel free to take over testing the code and making sure it's integrated.

Currently I have a backlog of 53 unread threads on Github (it was 100 around two weeks ago). I try to resolve those first, but new threads always keep coming up. I'm afraid if someone doesn't want to keep track this of PR, it will not be merged.

abo-abo added a commit that referenced this issue Feb 5, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Feb 5, 2019

@mookid Thanks, merged (there were other issues referencing this PR). I chose to extend ivy-thing-at-point instead of introducing a new function though.

astoff added a commit to astoff/swiper that referenced this issue 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants