Re: Partial wdired (edit just filename at the point)

[Arthur Miller]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hi,
>
> These changes are looking quite good.  There are a few things to fix
> before we can install it, tho:
>
>> diff --git a/lisp/wdired.el b/lisp/wdired.el
>> index c495d8de34..ba71306e66 100644
>> --- a/lisp/wdired.el
>> +++ b/lisp/wdired.el
>> @@ -250,20 +250,11 @@ wdired-change-to-wdired-mode
>>    (setq buffer-read-only nil)
>>    (dired-unadvertise default-directory)
>>    (add-hook 'kill-buffer-hook 'wdired-check-kill-buffer nil t)
>> +  (add-hook 'before-change-functions 'wdired--preprocess-line nil t)
>>    (add-hook 'after-change-functions 'wdired--restore-properties nil t)
>
> [ Nitpick:
>   Please use #' rather than ' to quote functions (I know the rest of the
>   code here doesn't (yet), but we should move towards more systematic
>   use of #' so as to better catch obsolete functions and other issues).  ]

I skipped it I don't like to look at it in either CL nor Elisp; in general I
don't like much of special characters in syntax. I think it is pitty it
is introduced in Elisp. But if you want to have it, it is just to add
it, it's not something essential to argue about; I am just giving reason
why I didn't used it from the beginning :-).

>> +(defun wdired--preprocess-line (beg end)
>> +  "Preprocess current line under point to make it writable.  "
>
> This is incorrect: what it *should* do is to preprocess an area that
> covers BEG..END.  That's why I suggested to name it "...lines" rather
> than "...line" ;-)
> And the "current point" is not really relevant (it will *usually* be in
> the vicinity, but not always).
>
>> +  (let ((inhibit-read-only t))
>> +    (unless (get-text-property (line-beginning-position) 'front-sticky)
>> +      (buffer-disable-undo)
>
> As you can guess from the previous comment, we should loop over BEG...END
> to process all the lines involved.

Can you elaborate more on this please? Why is it wrong and why do you
want it to work with region? As I see it used in Dired, most of the time
beg and end points will be the same, which will effectively result in
lots of processing to change properties for one character. Usually, in
Dired buffer I would move to a line with name and change few characters
only.

Current code changes properties for entire line, if it worked on region
it would change only properties for one char at time. I think it turns
into very complicated processing for just changing one char properties.

I checked to do some editing and printed out region when normally
editing a file name:
(defun wdired--preprocess-line (beg end)
  "Preprocess current line under point to make it writable.  "
  (let ((inhibit-read-only t))
    (message "REG: %s %s" beg end)
    ( ... )

This is what I got for each char I typed in:

REG: 8397 8397
REG: 8398 8398
REG: 8399 8399
REG: 8400 8400
REG: 8401 8401
REG: 8402 8402
REG: 8402 8403
REG: 8401 8402
REG: 8400 8401
REG: 8399 8400
REG: 8398 8399
REG: 8397 8398

The way I did it, it works per line; it will process entire line at
time. I have also checked to change multiple file names with rectangle
commands and in it works well too. I guess due to how Emacs internally
anserts chars into buffer (sequentially).

> Also, I recommend you use `with-silent-modifications` which will cover
> both `inhibit-read-only` and the undo (and a few more potential
> problems, which admittedly should hopefully not matter here).
> [ That macro didn't exist back when wdired.el was written.  ]

Thanks; I wasn't aware of that macro either.

>> @@ -304,9 +303,51 @@ wdired-preprocess-files
>>                         (looking-at "l")
>>                         (search-forward " -> " (line-end-position) t)))
>>              (goto-char (line-end-position)))
>> -      (setq b-protection (point))
>> -        (forward-line))
>> -      (put-text-property b-protection (point-max) 'read-only t))))
>> +          (setq b-protection (point))
>> +          (put-text-property b-protection (line-end-position)
>> +                             'read-only t))
>> +
>> +        ;; Put a keymap property to the permission bits of the files,
>> +        ;; and store the original name and permissions as a property
>> +        (when wdired-allow-to-change-permissions
>> +          (goto-char (line-beginning-position))
>> +          (setq-local wdired-col-perm nil)
>> +          (when (and (not (looking-at dired-re-sym))
>> +                 (wdired-get-filename)
>> +                 (re-search-forward dired-re-perms
>> +                                        (line-end-position) 'eol))
>> +        (let ((begin (match-beginning 0))
>> +              (end (match-end 0)))
>> +          (unless wdired-col-perm
>> +            (setq wdired-col-perm (- (current-column) 9)))
>> +          (if (eq wdired-allow-to-change-permissions 'advanced)
>> +              (progn
>> +                (put-text-property begin end 'read-only nil)
>> +                ;; make first permission bit writable
>> +                (put-text-property
>> +                 (1- begin) begin 'rear-nonsticky '(read-only)))
>> +            ;; avoid that keymap applies to text following permissions
>> +            (add-text-properties
>> +             (1+ begin) end
>> +             `(keymap ,wdired-perm-mode-map rear-nonsticky (keymap))))
>> +          (put-text-property end (1+ end) 'end-perm t)
>> +          (put-text-property
>> +           begin (1+ begin) 'old-perm (match-string-no-properties 0)))))
>> +
>> +        ;; Put the needed properties to allow the user to change
>> +        ;; links' targets
>> +        (when (fboundp 'make-symbolic-link)
>> +          (goto-char (line-beginning-position))
>> +          (when (looking-at dired-re-sym)
>> +            (re-search-forward " -> \\(.*\\)$")
>> +        (put-text-property (1- (match-beginning 1))
>> +                           (match-beginning 1) 'old-link
>> +                           (match-string-no-properties 1))
>> +            (put-text-property (match-end 1) (1+ (match-end 1)) 'end-link t)
>> +            (unless wdired-allow-to-redirect-links
>> +              (put-text-property (match-beginning 0)
>> +                             (match-end 1) 'read-only t))))))
>> +    (buffer-enable-undo)))
>
> To minimize code changes (and to avoid making such a large function
> (which are sadly frequent in Emacs's code base)), I think you can keep
> the `wdired-preprocess-perms` and `wdired-preprocess-symlinks` (tho
> you'll want to change them by making them take the beg..end arguments.)
> and call them from here instead of moving their code into this new
> function.

There are much bigger monstrousites I have seen in Emacs lisp folder
than this :-).

I agree with you that we should avoid large functions, I had that in
mind when I refactored out those two methods, but I didn't thought this
was so big (~60 lines without comments). I merged them into one to fit
everything relevant into one spot and one page on the screen. I can
change it back to three smaller defuns, but I will still though prefer
to have them following each other in the code rather than scattered
around in wdired.el for no reason as it is now. They are not refered
from any other code.

Anyway, sharps and refactoring are non-important issue; I can change it
back, just please check more on that with region, before I do changes.