Re: Enable adb port in tramp adb

Hi, Michael,

On Tue, Nov 11, 2014 at 9:25 PM, Michael Albinus <address@hidden> wrote:

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 :-)

Not at all! I real appreciate your review and helping! I've learned a lot in it.

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

Done.

> + :type '(choice (const nil) (const t)))

I would rather use :type 'boolean

Done.

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

Done

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

Done.

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

Done.

> - (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"?

No, I've set devices in local variable initialization: (devices (list exe-host)))

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.

Yes, it is a bug. I haven't fixed it yet in this update. I'll update the patch again when I solve this.

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

"device offline" is a error in adb itself, I guess we can't handle it in tramp. When got this error, I usually restart my device and try again... not a good solution though :/

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.

Best regards,
Zhongwei

From:	Ashi
Subject:	Re: Enable adb port in tramp adb
Date:	Wed, 12 Nov 2014 09:44:32 +0800