* Re: tramp-adb.el (Access Android devices filesystem using Emacs)
[not found] <BANLkTi=RX9NW3tfZj4qdnM72pQ8qVe7m-A@mail.gmail.com>
@ 2011-05-15 19:55 ` Michael Albinus
0 siblings, 0 replies; only message in thread
From: Michael Albinus @ 2011-05-15 19:55 UTC (permalink / raw
To: Jürgen Hötzel; +Cc: help-gnu-emacs, tramp-devel
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.
^ permalink raw reply [flat|nested] only message in thread