emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: isearch region or thing at point.


From: Ergus
Subject: Re: isearch region or thing at point.
Date: Wed, 1 May 2019 01:16:16 +0200
User-agent: NeoMutt/20180716

On Tue, Apr 30, 2019 at 11:39:11PM +0100, Basil L. Contovounesios wrote:
Ergus <address@hidden> writes:


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?

Yes, The docstring was not finished actually, I was just trying the code
functionality..

 (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").

I will delete those comments

                         (string-empty-p isearch-string)

Why must this be empty?

This is a condition I will change. The idea is that if the search-string
is empty in this moment we need to goto the region-beginning, That's a
latter change I will make.

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

Better to use the function region-noncontiguous-p.

I already had the bounds.. I didn't want to do the funcall again... but
I know it is probably not important.. I am just an obsessed.

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.

I don't thing that searching multiple lines will be very useful in
practice and could potentially produce some corner cases. But I
will think about that a bit more.

            (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?

This is somewhere else, but I did't included it here because is was not
related with the issue I had
        (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.
Yes, sorry, I was concerned if somehow the prefix could be non-nil, but
(prefix-numeric-value arg) could return nil... I don't really know the
fancy valued that prefix could potentially have. But probably there is
not any danger there.

   (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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]