[Feature] Drag and drop to install apk files from your computer #133

AdoPi · 2018-04-28T22:28:38Z

I have just added a little feature to install apk files by using drag and drop.

So, just drag and drop an apk file and wait for its installation to complete.

rom1v · 2018-04-30T11:42:59Z

Oh, I saw the title of the PR in my mails, and thought it was a feature request, I didn't realize it was a pull request 😄

That's cool, I didn't know about SDL_DROPFILE 👍 Thank you.

However, currently, the installation blocks the main thread (so the UI is frozen), it should be executed in a separate thread instead.

(Besides, since it is a new feature, it should target dev branch)

AdoPi · 2018-04-30T17:33:30Z

Thanks :)
I am going to work on a version which uses separate thread so.
Sorry I don't know how to change the target of an existing pull request, do you know how can I do that?

AdoPi · 2018-04-30T17:57:03Z

dc68d99 commit is a quick fix.

Now, the UI should not be frozen.

rom1v · 2018-04-30T19:20:44Z

Now, the UI should not be frozen.

That's true. However, each "adb install" child process will not be reaped, and will stay zombies until the scrcpy process terminates.

Instead, I suggest to create a thread, keep a reference to it so that you can join it when scrcpy terminates. A priori, it should be similar to what decoder.c does.

By the way, all this APK installation stuff (what you currently implement in server.c) could be managed in a separate file (installapk.c or whatever), so that the implementation details¹ are hidden from the caller.

What do you think?

¹These implementation details include decisions about:

how to manage the separate thread? create a new one each time? reuse it?
what happens if we start a new installation before the previous one is complete? (serialized on the same thread, in parallel…)
what happens if we close scrcpy while an installation is running?

AdoPi · 2018-04-30T19:51:23Z

how to manage the separate thread? create a new one each time? reuse it?
what happens if we start a new installation before the previous one is complete? (serialized on the same thread, in parallel…)

My idea was to make an executor thread which takes commands from a command queue and execute them (thus serialized on the same thread).
The latest commit was just here to have something which works while I'm working on a better design which may take some time 👎

what happens if we close scrcpy while an installation is running?

It depends on adb, I think we can expect the same behaviour as when we disconnect the phone while adb is installing something on it.

rom1v · 2018-04-30T20:01:04Z

My idea was to make an executor thread which takes commands from a command queue and execute them (thus serialized on the same thread).

👍

The executor thread could be created only on the first time it is required (APK installations will probably be rare compared to scrcpy executions).

It depends on adb, I think we can expect the same behaviour as when we disconnect the phone while adb is installing something on it.

Yes, killing the process (like server.c does) is ok for me 👍

AdoPi · 2018-05-03T21:13:34Z

The latest commit should add this drag&drop feature in a more proper way.
What do you think? :)

If someday someone needs to execute other commands apart from "adb install" , it would be nice to have a command queue instead of a filename queue to be more generic.

rom1v · 2018-05-03T21:40:28Z

Thank you 👍 I will review this when I have some time, hopefully this week-end.

rom1v

Good work, it works well without freezing the UI 👍

You reused the structure of existing code, so this makes the new code consistent, that's great. 😄

I have two main requests before merging it:

the apk queue should not reserve space for MAX_FILENAME_SIZE (that's wasteful and unsafe if the filename is more than MAX_FILENAME_SIZE). Instead it should just store pointers to allocated strings (and free them as necessary) IMO.
the lazy initialization should be internal to apkinstaller.h. IMO the caller should always initially call installer_init() as if the installer queue/thread were not lazy initialized (it's an implementation detail, we should be able to change the lazy-initialization behavior without the caller being aware of it). In that case, installer_init() would just initialize the fiels, but not create the thread, etc. But if you want, after all, I think it's ok not to implement lazy initialization (this is probably a premature optimization that I suggested).

In addition, some suggestions:

pass -r argument to adb install to support upgrades: adb install -r file.apk
use the same name for the file and the struct (either installer.h/struct installer, either apkinstaller.h/struct apk_installer)

I also commented directly on the diff for tiny details.

rom1v · 2018-05-05T07:49:28Z

app/src/apkinstaller.c

+}
+
+void apk_queue_destroy(struct apk_queue *queue) {
+    // do nothing as we are on the stack


there is nothing to do, but we are not on the stack 😉

rom1v · 2018-05-05T07:51:39Z

app/src/apkinstaller.c

+    SDL_DestroyMutex(installer->mutex);
+    apk_queue_destroy(&installer->queue);
+    SDL_free((void *) installer->serial);
+    installer->initialized = SDL_FALSE;


After destroy, the installer is considered in an undefined state, there is no need to set its variables.

rom1v · 2018-05-05T07:54:19Z

app/src/apkinstaller.h

+void installer_join(struct installer *installer);
+
+// install an apk
+SDL_bool installer_push_apk(struct installer *installer, const char* filename);


The name is confusing, we push a request for apk installation into the queue, we don't "push" an apk (it may be confused with adb push).

rom1v · 2018-05-05T07:54:51Z

app/src/command.c

@@ -73,6 +73,11 @@ process_t adb_push(const char *serial, const char *local, const char *remote) {
    return adb_execute(serial, adb_cmd, ARRAY_LEN(adb_cmd));
 }

+process_t adb_install(const char *serial, const char *local) {
+    const char *const adb_cmd[] = {"install", local};


{"install", "-r", local} to support upgrades 😉

rom1v · 2018-05-05T08:00:22Z

app/src/scrcpy.c

@@ -102,6 +104,18 @@ static void event_loop(void) {
            case SDL_MOUSEBUTTONUP:
                input_manager_process_mouse_button(&input_manager, &event.button);
                break;
+            case SDL_DROPFILE:
+
+                if (!installer.initialized) {


IMO, the fact that the installer is "lazy initialized" should not leak to the caller (it's an implementation detail).

Cool! I was wondering whether or not it was an implementation detail.

You're right, it is possible not to hide the lazy initialization inside the installer: the installer could always be fully initialized on init, but the caller may call init only when necessary. In that case, the initialized flag would be on the caller side.

rom1v · 2018-05-05T09:32:12Z

app/src/apkinstaller.h

+#define MAX_FILENAME_SIZE 1024
+
+
+#include "apkinstaller.h"


Sorry about that 👎

rom1v · 2018-05-05T09:32:37Z

app/src/apkinstaller.h

+#define APK_INSTALLER_H
+
+#define APK_QUEUE_SIZE 16
+#define MAX_FILENAME_SIZE 1024


should be after the includes

rom1v · 2018-05-05T09:34:38Z

app/src/apkinstaller.c

+#include "lockutil.h"
+#include "log.h"
+#include "command.h"
+#include <string.h>


(nitpicking)

For consistency:

#include "apkinstaller.h" #include <string.h> #include "command.h" #include "lockutil.h" #include "log.h"

(first the associated .h, then the sorted <> includes, then the sorted "" includes)

rom1v · 2018-05-05T09:35:10Z

app/src/apkinstaller.h

+    .current_process = PROCESS_NONE \
+}
+
+


double empty line

rom1v · 2018-05-05T09:36:13Z

app/src/apkinstaller.c

+#include "command.h"
+#include <string.h>
+
+


double newline

AdoPi · 2018-05-05T10:11:59Z

Big thanks for your review and the time spent, I will work on it 👍

AdoPi · 2018-05-15T19:44:31Z

Thanks for the advice, it should be better now.
Keep me informed :)

rom1v · 2018-05-17T15:45:01Z

Thank you for you changes. I will probably not have time to review this week-end, but don't worry, I'm thinking about it 😉

rom1v · 2018-05-26T13:38:26Z

Hi,

Thank you for the feature.

I fixed some memory leaks and released the mutex while installing (I made me fix the controller for the same reason).

Here are the changes I suggest:

diff --git a/app/src/installer.c b/app/src/installer.c
index 95a15db..2bef19d 100644
--- a/app/src/installer.c
+++ b/app/src/installer.c
@@ -44,7 +44,8 @@ SDL_bool apk_queue_take(struct apk_queue *queue, char **apk) {
     if (apk_queue_is_empty(queue)) {
         return SDL_FALSE;
     }
-    *apk = SDL_strdup(queue->data[queue->tail]);
+    // transfer ownership
+    *apk = queue->data[queue->tail];
     queue->tail = (queue->tail + 1) % APK_QUEUE_SIZE;
     return SDL_TRUE;
 }
@@ -64,9 +65,14 @@ SDL_bool installer_init(struct installer *installer, const char *serial) {
         return SDL_FALSE;
     }
 
-    installer->serial = NULL;
     if (serial) {
         installer->serial = SDL_strdup(serial);
+        if (!installer->serial) {
+            LOGW("Cannot strdup serial");
+            return SDL_FALSE;
+        }
+    } else {
+        installer->serial = NULL;
     }
 
     // lazy initialization
@@ -104,35 +110,40 @@ SDL_bool installer_install_apk(struct installer *installer, const char *apk) {
     return res;
 }
 
-static SDL_bool process_install(struct installer *installer, const char *filename) {
-    LOGI("%s will be installed",filename);
-    process_t process = adb_install(installer->serial, filename);
-    installer->current_process = process;
-    return process_check_success(process, "adb install");
-}
-
 static int run_installer(void *data) {
     struct installer *installer = data;
 
-    char *current_apk;
-    mutex_lock(installer->mutex);
     for (;;) {
+        mutex_lock(installer->mutex);
         while (!installer->stopped && apk_queue_is_empty(&installer->queue)) {
             cond_wait(installer->event_cond, installer->mutex);
         }
         if (installer->stopped) {
             // stop immediately, do not process further events
+            mutex_unlock(installer->mutex);
             break;
         }
-        while (apk_queue_take(&installer->queue, &current_apk)) {
-            SDL_bool ok = process_install(installer,current_apk);
-            if (!ok) {
-                LOGD("Error during installation");
-            }
+        char *current_apk;
+#ifdef BUILD_DEBUG
+        bool non_empty = apk_queue_take(&installer->queue, &current_apk);
+        SDL_assert(non_empty);
+#else
+        apk_queue_take(&installer->queue, &current_apk);
+#endif
+
+        LOGI("Installing %s...", current_apk);
+        process_t process = adb_install(installer->serial, current_apk);
+        installer->current_process = process;
+
+        mutex_unlock(installer->mutex);
+
+        if (process_check_success(process, "adb install")) {
+            LOGI("%s installed successfully", current_apk);
+        } else {
+            LOGE("Failed to install %s", current_apk);
         }
+        SDL_free(current_apk);
     }
-    mutex_unlock(installer->mutex);
-    SDL_free(current_apk);
     return 0;
 }
 
@@ -152,7 +163,7 @@ void installer_stop(struct installer *installer) {
     mutex_lock(installer->mutex);
     installer->stopped = SDL_TRUE;
     cond_signal(installer->event_cond);
-    if (installer->current_process == PROCESS_NONE) {
+    if (installer->current_process != PROCESS_NONE) {
         if (!cmd_terminate(installer->current_process)) {
             LOGW("Cannot terminate install process");
         }
diff --git a/app/src/scrcpy.c b/app/src/scrcpy.c
index 03bdc19..4e74bb9 100644
--- a/app/src/scrcpy.c
+++ b/app/src/scrcpy.c
@@ -152,10 +152,10 @@ SDL_bool scrcpy(const char *serial, Uint16 local_port, Uint16 max_size, Uint32 b
         goto finally_destroy_server;
     }
 
-    if (!installer_init(&installer,server.serial)) {
+    if (!installer_init(&installer, server.serial)) {
         ret = SDL_FALSE;
         server_stop(&server);
-        goto finally_destroy_installer;
+        goto finally_destroy_frames;
     }
 
     decoder_init(&decoder, &frames, device_socket);
@@ -165,7 +165,7 @@ SDL_bool scrcpy(const char *serial, Uint16 local_port, Uint16 max_size, Uint32 b
     if (!decoder_start(&decoder)) {
         ret = SDL_FALSE;
         server_stop(&server);
-        goto finally_destroy_frames;
+        goto finally_destroy_installer;
     }
 
     if (!controller_init(&controller, device_socket)) {

Please review 😉 If it's ok, I'll squash all these commits into one on dev.

I'll also add a workaround for Windows to support spaces in path:

diff --git a/app/src/command.c b/app/src/command.c
index 463eb42..6a7c1b9 100644
--- a/app/src/command.c
+++ b/app/src/command.c
@@ -74,6 +74,16 @@ process_t adb_push(const char *serial, const char *local, const char *remote) {
 }
 
 process_t adb_install(const char *serial, const char *local) {
+#ifdef __WINDOWS__
+    // Windows will parse the string, so the local name must be quoted (see sys/win/command.c)
+    size_t len = strlen(local);
+    char quoted[len + 3];
+    memcpy(&quoted[1], local, len);
+    quoted[0] = '"';
+    quoted[len + 1] = '"';
+    quoted[len + 2] = '\0';
+    local = quoted;
+#endif
     const char *const adb_cmd[] = {"install", "-r", local};
     return adb_execute(serial, adb_cmd, ARRAY_LEN(adb_cmd));
 }

AdoPi · 2018-05-28T09:13:44Z

Oh, I was not aware of the spaces problem for Windows path, that's cool to know (I don't really use Windows a lot).

Something in the mutex code was interpelling me too but I couldn't find why, nice job 💯
It seems to be fine for me!

<#133>

rom1v · 2018-05-28T20:58:57Z

Commit e2a2973, and released in v1.2 👍 🎉

drag and drop to install apk files from computer

efc75e8

AdoPi changed the title ~~[Feature] Drag and drop to install apk files from your computer to Android~~ [Feature] Drag and drop to install apk files from your computer Apr 28, 2018

just don't wait for the end of cmd process

dc68d99

AdoPi added 9 commits May 1, 2018 00:36

wip: add apk installer

dde22ba

no need for server.h

30b03bf

no need for the remote path

716ca97

change free to SDL_free and log msg

0f9f3a3

apkinstaller integration into scrcpy

243a025

update meson file to add apkinstaller

6cbb207

installer queue works

d4f1de5

fix installer free

3722c4d

init installer only when it's used

ec95645

AdoPi added 3 commits May 4, 2018 08:44

try to terminate adb install command properly

43cbb9e

safe string copy

61f5647

fix destroy_apk_queue as we're currently on the stak

bf0b91c

rom1v reviewed May 5, 2018

View reviewed changes

AdoPi added 5 commits May 14, 2018 21:33

support upgrades for adb install

d018277

rename apkinstaller files to installer

ad4616b

fix meson build

cc32754

lazy loading + convert queue's char[] to pointer

34183ad

free installer's queue

9f4681d

AdoPi added 4 commits May 14, 2018 22:34

remove useless comments

527d55b

proper way to destroy and init installer

d6e35b0

clean installer

b5a81fa

fix installer's goto fail

c941e34

rom1v pushed a commit that referenced this pull request May 28, 2018

Drag and drop to install apk files from computer

e2a2973

<#133>

rom1v closed this May 28, 2018

rom1v mentioned this pull request Jul 17, 2018

Move files between PC and device #128

Closed

rom1v mentioned this pull request Oct 4, 2018

Drag n' Drop not working if filename has empty space #288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Drag and drop to install apk files from your computer #133

[Feature] Drag and drop to install apk files from your computer #133

AdoPi commented Apr 28, 2018

rom1v commented Apr 30, 2018 •

edited

Loading

AdoPi commented Apr 30, 2018

AdoPi commented Apr 30, 2018 •

edited

Loading

rom1v commented Apr 30, 2018

AdoPi commented Apr 30, 2018

rom1v commented Apr 30, 2018

AdoPi commented May 3, 2018 •

edited

Loading

rom1v commented May 3, 2018

rom1v left a comment

rom1v May 5, 2018

rom1v May 5, 2018

rom1v May 5, 2018

rom1v May 5, 2018

rom1v May 5, 2018

AdoPi May 5, 2018

rom1v May 5, 2018

rom1v May 5, 2018

AdoPi May 5, 2018

rom1v May 5, 2018

rom1v May 5, 2018

rom1v May 5, 2018

rom1v May 5, 2018

AdoPi commented May 5, 2018

AdoPi commented May 15, 2018

rom1v commented May 17, 2018

rom1v commented May 26, 2018 •

edited

Loading

AdoPi commented May 28, 2018

rom1v commented May 28, 2018 •

edited

Loading

[Feature] Drag and drop to install apk files from your computer #133

[Feature] Drag and drop to install apk files from your computer #133

Conversation

AdoPi commented Apr 28, 2018

rom1v commented Apr 30, 2018 • edited Loading

AdoPi commented Apr 30, 2018

AdoPi commented Apr 30, 2018 • edited Loading

rom1v commented Apr 30, 2018

AdoPi commented Apr 30, 2018

rom1v commented Apr 30, 2018

AdoPi commented May 3, 2018 • edited Loading

rom1v commented May 3, 2018

rom1v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdoPi commented May 5, 2018

AdoPi commented May 15, 2018

rom1v commented May 17, 2018

rom1v commented May 26, 2018 • edited Loading

AdoPi commented May 28, 2018

rom1v commented May 28, 2018 • edited Loading

rom1v commented Apr 30, 2018 •

edited

Loading

AdoPi commented Apr 30, 2018 •

edited

Loading

AdoPi commented May 3, 2018 •

edited

Loading

rom1v commented May 26, 2018 •

edited

Loading

rom1v commented May 28, 2018 •

edited

Loading