Re: isearch region or thing at point.

[Ergus <spacibba@aol.com>]

On Tue, Apr 30, 2019 at 11:39:11PM +0100, Basil L. Contovounesios wrote:
>Ergus <spacibba@aol.com> 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