Re: [PATCH] Lazy wdired preprocessing

[Arthur Miller]

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

>> +(defvar wdired-perm-beg) ;; Column where the permission bits start
>> +(defvar wdired-perm-end) ;; Column where the permission bits stop
>
> I think this should use "--" in the names since they are internal variables.

I just followed naming as already was in wdired. In general I tried to
change as little as possible, so I didn't re-factor any existing code
unless needed. Partly to save my time, partly to not mess up
unnecessary. I have now changed all global vars to use '--' in name. I
was thinking about "private" functions too, but I'll leave that for some
other time or as exercise to someone else.

>>    (setq mode-name "Editable Dired")
>> -  (add-function :override (local 'revert-buffer-function) #'wdired-revert)
>> -  ;; I temp disable undo for performance: since I'm going to clear the
>> -  ;; undo list, it can save more than a 9% of time with big
>> -  ;; directories because setting properties modify the undo-list.
>> -  (buffer-disable-undo)
>> -  (wdired-preprocess-files)
>> -  (if wdired-allow-to-change-permissions
>> -      (wdired-preprocess-perms))
>> -  (if (fboundp 'make-symbolic-link)
>> -      (wdired-preprocess-symlinks))
>> -  (buffer-enable-undo) ; Performance hack. See above.
>> +  (setq revert-buffer-function 'wdired-revert)
>>    (set-buffer-modified-p nil)
>
> This reverts part of my recent change to the way we set
> `revert-buffer-function` (most likely an oversight while merging my changes).

You commited changes to wdired? :-)
Yeah, I copy-pasted entire function from my working file, so yes indeed,
it was an oversight on my part, fixed. 

>>    (setq buffer-undo-list nil)
>> +  ;; find one column with permissions and set permision text boundaries
>> +  (save-excursion
>> +    (goto-char (point-min))
>> +    (unless (re-search-forward dired-re-perms nil t 1)
>> +      (wdired-abort-changes)
>> +      (error "No files to be renamed - Exiting to Dired mode."))
>> +    (goto-char (match-beginning 0))
>> +    (setq-local wdired-perm-beg (current-column))
>> +    (goto-char (match-end 0))
>> +    (setq-local wdired-perm-end (current-column)))
>
> I'd recommend you put this in a separate function.

I have refactored that a bit more and am using directory-empty-p to exit
earlier. Hope we can use default-directory there. Check the code.

>> +  (define-key wdired-mode-map [remap self-insert-command] 
>> #'wdired--self-insert)
>
> Why is this done here instead of in the definition of `wdired-mode-map`?

Was typing as I was thinking ... :-). Fixed.

>> +(defun wdired--point-at-perms-p ()
>> +  (and (>= (current-column) wdired-perm-beg)
>> +       (<= (current-column) wdired-perm-end)))
>
> `current-column` can be somewhat costly, so we should refrain from
> calling it twice gratuitously.  And here we can even take advantage of
> the (rarely used and rarely applicable) multi-arg form of `<=` to fix
> that "for free":

Ok. Didn't know that (current-column) was expensive. I use now a good 'nuff
implementation for this purpose (wdired--current-column).

>      (<= wdired-perm-beg (current-column) wdired-perm-end)

Cool; thnks, didn't know we can write so.

>> +(defun wdired--self-insert ()
>> +  (interactive)
>> +  (if (wdired--point-at-perms-p)
>> +    (when (not (get-text-property (line-beginning-position) 'front-sticky))
>> +      (wdired--before-change-fn (line-beginning-position) 
>> (line-end-position))
>> +      (setq unread-command-events (nconc (listify-key-sequence
>> +                                          (this-command-keys))
>> +                                         unread-command-events)))
>> +    (call-interactively 'self-insert-command)))
>
> I think this deserves a comment about why we look at `front-sticky`.
> Better yet: move this test to a helper function to which you can give
> a meaningful name (like `wdired--processed-p`).

Sure, it makes code more self-documented.

> Also, instead of using `unread-command-events`, you can just call the
> appropriate command directly, no?

As I stated about refactoring above, I tried to change as
little as possible. Partly because I was lazy to look into the old code
and partly because I tried to "plug" into already existing (and
hopefully debugged) code. But sure toggle-bit is the one I think.

>> -  (remove-hook 'kill-buffer-hook #'wdired-check-kill-buffer t)
>> -  (remove-hook 'after-change-functions #'wdired--restore-properties t)
>> -  (remove-function (local 'revert-buffer-function) #'wdired-revert))
>> +  (remove-hook 'kill-buffer-hook 'wdired-check-kill-buffer t)
>> +  (remove-hook 'before-change-functions 'wdired--before-change-fn t)
>> +  (remove-hook 'after-change-functions 'wdired--restore-properties t)
>> +  (setq-local revert-buffer-function 'dired-revert))
>
> This also look like the merge wasn't done right.

No idea; mr. git format-patch seems to shuffle lines around, and I trust
it to do what it has to do.

>>  (defun wdired-abort-changes ()
>> -  "Abort changes and return to dired mode."
>> +  "Abort changes and return to dired mode.  "
>
> What happened here?

No idea. :-).

It's all yours now. I don't think I will have more time nor possibility
to work on this, so if this one is not good enough, you will to finnish
it on your own, or someone else could help. We are waiting a kid any
day now, so no hobby programming for quite some time over for me :-).

Thnks for the guidance.

Cheers

lazy-wdired.patch
Description: Text Data