|
From: | Ashi |
Subject: | Re: Enable adb port in tramp adb |
Date: | Wed, 12 Nov 2014 09:44:32 +0800 |
Ashi <address@hidden> writes:
> Hi, Michael,
Hi Zhongwei,
> I've updated the patch:
> - add tramp-adb-connect-if-not-connected feature
> - remind user if unnecessary port number is provided in USB mode
> - discover port number for user if port number is not provided in TCP mode
Thanks. We're making progress!
As usual, I've played with this. And as usual, I do nitpick :-)
> +(defcustom tramp-adb-connect-if-not-connected nil
> + "Try to run adb connect if provided device is not connected currently. It is
> +in tramp-adb-get-host-for-execution."
The Emacs coding guidelines do require the first line of a docstring to
be exactly a sentence (or two sentences, if they fit). Everything else
must start in the next line. See (info "(elisp) Documentation")
> + :group 'tramp
> + :version "24.4"
Well, Emacs 24.4 has been released. Your patch cannot be merged to that
anymore ... use 24.5.
> + :type '(choice (const nil) (const t)))
I would rather use :type 'boolean
> +(defun tramp-adb-search-host-in-devices (host devices)
> + "Returns host:port string by searching host in the devices list. If
> +failed, return nil."
Again, docstring conventions.
> + (if (null devices) nil
(when devices
> + (if (tramp-adb-execute-adb-command
> + '["adb" nil "" "/" nil] "connect" exe-name)
You create a dummy vector, because tramp-adb-execute-adb-command adds an
additional argument when there is a host name. This looks like a hack;
better would be to fix this in tramp-adb-execute-adb-command. For
example, you could check whether (car args) is "shell". Or something
like this.
> + ;; If port is not provided, adb will connect to default port number
> + ;; 5555, which must be appended to return to get a full host name.
> + (if port
> + exe-name
> + (format "%s:%s" host "5555"))))
For the time being it might be OK to hardwire 5555. However, it is
always a good idea to make it configurable, for example via
tramp-methods. Look, how a default port is configured in tramp-sh.el.
> - (setq devices (mapcar 'cadr (tramp-adb-parse-device-names nil)))
> + (append devices (mapcar 'cadr (tramp-adb-parse-device-names nil)))
Did you forget "(setq devices"?
Well, then I made some tests. I have the following settings:
# adb devices
List of devices attached
015d24bc55282409 device
android-c05807abea645fdf.fritz.box:5555 device
In Emacs, I type "C-x f /adb: TAB". The completion buffer looks then
Possible completions are:
adb:015d24bc55282409: adb:192.168.0.141#5555:
adb:192.168.0.141:
adb:[android-c05807abea645fdf.fritz.box:5555]:
Hmm, the last entry isn't like it should be.
When I type "C-x f /adb:android-c05807abea645fdf: RET" everything is
OK. Now I try "C-x f /adb:192.168.0.141: RET" and I get an error
message. Still OK.
Now I set tramp-adb-connect-if-not-connected to t, and try again "C-x f
/adb:192.168.0.141: RET", and I don't get a Lisp error. Only in the
connection buffer, I see
Process *tramp/adb 192.168.0.141* exited abnormally with code 1
error: device offline
And when I check it from the shell, there is
# adb devices
List of devices attached
015d24bc55282409 device
android-c05807abea645fdf.fritz.box:5555 device
192.168.0.141:5555 offline
Is it intended like this?
> Best regards,
> Zhongwei
Best regards, Michael.
adb_port_enable.patch
Description: Text Data
[Prev in Thread] | Current Thread | [Next in Thread] |