Re: tramp-adb.el (Access Android devices filesystem using Emacs)

[Michael Albinus]

Jürgen Hötzel <juergen@hoetzel.info> writes:

Hi Juergen,

> Although The Android shell and file utilities are quite limited
> (missing commands like "test" or features  like "ls --dired"), tramp-
> adb is already quite useful for me:
>
> https://github.com/juergenhoetzel/tramp-adb
>
> Any feedback/contribution appreciated

Nice package. I don't use adb myself (yet), therefore just some comments
by code reading:

 25 (require 'tramp)
 26 
 27 (defcustom tramp-adb-sdk-dir "~/Android/sdk"
 28   "Set to the directory containing the Android SDK."
 29   :type 'string
 30   :group 'tramp-adb)
 31 
 32 (defconst tramp-adb-method "adb"
 33   "*When this method name is used, forward all calls to Android Debug 
Bridge.")

You do not use the ";;;###tramp-autoload" cookie. It would be helpful to
load tramp-adb.el without explicitely requiring it in .emacs.

 37 (add-to-list 'tramp-methods `(,tramp-adb-method))
 38 
 39 (add-to-list 'tramp-default-method-alist
 40          (list "\\`adb" nil tramp-adb-method))
 41 
 42 (add-to-list 'tramp-methods (cons tramp-adb-method nil))

Why do you add tramp-adb-method twice to tramp-methods?

 47 (defconst tramp-adb-file-name-handler-alist
 48   '((directory-file-name . tramp-handle-directory-file-name)
 49     (dired-uncache . tramp-handle-dired-uncache)
 50     (file-name-as-directory . tramp-handle-file-name-as-directory)
 51     (file-name-completion . tramp-handle-file-name-completion)
 52     (file-name-all-completions . tramp-adb-handle-file-name-all-completions)
 53     (file-attributes . tramp-adb-handle-file-attributes)
 54     (file-name-directory . tramp-handle-file-name-directory)
 55     (file-name-nondirectory . tramp-handle-file-name-nondirectory)
 56     (file-newer-than-file-p . tramp-handle-file-newer-than-file-p)
 57     (file-regular-p . tramp-handle-file-regular-p)
 58     (file-remote-p . tramp-handle-file-remote-p)
 59     (file-directory-p . tramp-adb-handle-file-directory-p)
 60     (file-symlink-p . tramp-handle-file-symlink-p)
 61     (file-exists-p . tramp-adb-handle-file-exists-p)
 62     (file-writable-p . tramp-adb-handle-file-writable-p)
 63     (file-local-copy . tramp-adb-handle-file-local-copy)
 64     (expand-file-name . tramp-adb-handle-expand-file-name)
 65     (find-backup-file-name . tramp-handle-find-backup-file-name)
 66     (directory-files . tramp-adb-handle-directory-files)
 67     (make-directory . tramp-adb-handle-make-directory)
 68     (delete-directory . tramp-adb-handle-delete-directory)
 69     (delete-file . tramp-adb-handle-delete-file)
 70     (load . tramp-handle-load)
 71     (insert-directory . tramp-adb-handle-insert-directory)
 72     (insert-file-contents . tramp-handle-insert-file-contents)
 73     (substitute-in-file-name . tramp-handle-substitute-in-file-name)
 74     (unhandled-file-name-directory . 
tramp-handle-unhandled-file-name-directory)
 75     (vc-registered . ignore)        ;no  vc control files on Android devices
 76     (write-region . tramp-adb-handle-write-region)
 77     (rename-file . tramp-sh-handle-rename-file))    
 78   "Alist of handler functions for Tramp ADB method.")

Some functions are missing. You have a handler for rename-file, but none
for copy-file. And if you want to reuse tramp-sh-handle-rename-file, you
would need to load tramp-sh.el.

 93     (with-current-buffer (get-buffer-create "my-buff")
 94       (insert (format "op: %s, args: %s\n" operation args)))

What's that good for? If you need traces, it might be better to use
tramp-message. Or, alternatively, you profile this function with elp.

113     (tramp-message v 5 "tramp-adb-handle-expand-file-name returns: %s" r)

You don't need to write the function name in tramp-message's
strings. The function name is always added by tramp-message itself.

117 (defun tramp-adb-handle-file-directory-p (filename)
118   "Like `file-directory-p' for Tramp files."
119   (and (file-exists-p filename)
120        (car (file-attributes filename))))

In theory, (car (file-attributes ...)) could also return a string (in
case of symlinks). The test would be better to check (eq t (car ...))

127       (tramp-message v 5 "adb: file attributes with ls: %s" localname)

Again, you dont need the "adb" identification in the message.

157 (defun tramp-adb--gnu-switches-to-ash
158   (switches)
159   "Almquist shell can't handle multiple arguments. Convert (\"-al\") to 
(\"-a\" \"-l\")"
160   (split-string (apply 'concat (mapcar (lambda (s)
161                                      (replace-regexp-in-string "\\(.\\)"  " 
-\\1" 
162                                                                
(replace-regexp-in-string "^-" "" s))) 
163                                    ;; FIXME: Warning about removed switches 
(long and non-dash)
164                                    (remove-if (lambda (s) 
165                                                 (string-match  
"\\(^--\\|^[^-]\\)" s)) switches)))))

Please restrict yourself to the 80 chars/line limit. The code is
readable, then.

208       (tramp-message v 6 "mkdir doesn't handle \"-p\" switch: mkdir \"%s\"" 
(tramp-shell-quote-argument localname)))

Verbose level 6 shall be restricted to sent commands and the returned
output. This allows reading of trace buffers.

209     (save-excursion
210       (tramp-barf-unless-okay
211        v (format "%s %s"
212              "mkdir"
213              (tramp-shell-quote-argument localname))
214        "Couldn't make directory %s" dir)

tramp-barf-unless-okay calls tramp-send-command, you use
tramp-adb-send-command. Are both functions equivalent?

221     (tramp-flush-file-property v (file-name-directory localname))
222     (tramp-flush-directory-property v localname)

You have kept the tramp-flush-* functions. It might be a good idea to
apply the property setting functions as well.

253       (tramp-adb-handle-directory-files directory)))))))

In general, we don't call the tramp-*-handle-* functions directly. This
bypasses the mechanisms in tramp-*-file-name-handler functions, which
prevent sometimes subtle locking situations. Therefore, we call the
primitive functions directly, whenever possible. This case,
directory-files could be called.

329 (defun tramp-adb-execute-adb-command (&rest args)
330   "Returns nil on success error-output on failure."
331   (let ((adb-program (concat (file-name-as-directory tramp-adb-sdk-dir) 
"platform-tools/adb")))
332     (with-temp-buffer 
333       (unless (zerop (apply 'call-process-shell-command adb-program nil t 
nil args))
334     (buffer-string)))))

I do not know too much about adb. Is it necessary to call adb again and
again? Or does it have an interactive mode (like smbclient), which could
be used? The latter case, if possible, would perform better IMHO.

383      (adb-program (concat (file-name-as-directory tramp-adb-sdk-dir) 
"platform-tools/adb")))

expand-file-name could be useful.

> Jürgen

Best regards, Michael.