Re: Enable adb port in tramp adb

[Michael Albinus]

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.