Re: isearch region or thing at point.

[Basil L. Contovounesios]

Ergus <address@hidden> writes:

> I wrote this code today to do a search auto-insert text from region like
> behavior.

Thanks, I have some questions and minor comments:

> (defun isearch-forward-region (&optional arg)
>  "Do incremental search forward for text in active region.
> Like ordinary incremental search except that the text in the active
> region is added to the search string initially if variable
> `transient-mark-mode' is non nil.

Perhaps "if `transient-mark-mode' is enabled" instead.

>  See the command `isearch-forward-symbol' for more information.

Which information is that?  Why not refer to isearch-forward instead?

> With a prefix argument, search for ARGth symbol forward if ARG is
> positive, or search for ARGth symbol backward if ARG is negative."

I don't think this should mention symbols.

>  (interactive "P")
>  (isearch-forward nil 1)
>  (if-let* ((bounds (and (use-region-p) ;; Region and transient non-nil

RHS comments usually start with a single semicolon;
see (info "(elisp) Comment Tips").

>                        (string-empty-p isearch-string)

Why must this be empty?

>                        (region-bounds)))
>           (contiguous (= (length bounds) 1)) ;; Region is contiguous

Better to use the function region-noncontiguous-p.

Also, you shouldn't leave bound symbols unused.  The most common way to
short-circuit conditionals is via the special forms 'and' and 'or'.
In the case of if-let et al., you can also use the following syntax:

  (if-let* ((len (length foo))
            ((zerop len)))
      'empty
    'nonempty)

>           (region-beg (car (car bounds)))
>           (region-end (cdr (car bounds)))

Nitpick: caar/cdar, also not important.

>           (region-string (and (= (count-lines region-beg region-end) 1)
>                               (buffer-substring-no-properties
>                                region-beg region-end)))

Why can't the region span multiple lines?  Better to use
region-extract-function for this, as it is more flexible.

>           (noempty (not (string-blank-p region-string))))

Why can't the search string be blank?

>      (progn
>       (goto-char region-beg)
>       (setq mark-active nil
>             isearch--deactivated-mark t)

Where is isearch--deactivated-mark defined?  What does it do?
Shouldn't this set/call deactivate-mark or similar instead?

>       (isearch-yank-string region-string)
>
>       (when-let (count (and arg (prefix-numeric-value arg)))
>         (isearch-repeat-forward count)))

This can be simplified as per Noam's suggestion.

>    (setq isearch-error "Invalid region for isearch")
>    (isearch-push-state)
>    (isearch-update)))

Here's a suggestion which addresses some of my comments:

(defun isearch-forward-region (&optional arg)
  "Do incremental search forward for text in active region.
Like ordinary incremental search except that the text in the
active region is added to the search string initially
if`transient-mark-mode' is enabled.  See the command
`isearch-forward' for more information.
With a prefix argument, search for ARGth occurrence forward if
ARG is positive, or ARGth occurrence backward if ARG is
negative."
  (interactive "P")
  (isearch-forward nil 1)
  (let ((region (and (use-region-p)
                     (string-empty-p isearch-string)
                     (funcall region-extract-function nil))))
    (cond ((and (stringp region)
                (not (string-empty-p region)))
           (goto-char (region-beginning))
           (deactivate-mark)
           (isearch-yank-string region)
           (when arg
             (isearch-repeat-forward (prefix-numeric-value arg))))
          (t
           (setq isearch-error "Invalid region")
           (isearch-push-state)
           (isearch-update)))))

Feel free to adapt it as you please.

Thanks,

-- 
Basil