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

[Thierry Volpiatto]

Hello, just taking this thread on the fly.

Normally one can pass a list of files with absolute path to dired to get
a dired buffer with only the needed file names to possibly edit with
wdired, however this is broken in Emacs since ages; Helm is fixing this
by advice, see
https://github.com/emacs-helm/helm/blob/master/helm-lib.el#L186.
This allow selecting files from helm-find-files and switching to a dired
buffer with only those files.

HTH.


Arthur Miller <arthur.miller@live.com> writes:

> 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.


-- 
Thierry

signature.asc
Description: PGP signature