support drag & drop file to device /sdcard #226

npes87184 · 2018-08-11T13:59:58Z

Hi, folks

With drop & drag file to device /sdcard, it will be very convenient to app developer to test app which need some file in the device.

In this patch, if people drop & drag an apk file, it will remain the same behavior. Otherwise, it will push the file to /sdcard.

Thanks.

Signed-off-by: npes87184 npes87184@gmail.com

rom1v · 2018-08-11T15:25:41Z

Ping #128.

Thank you for your contribution.

I cannot test for now (I'm on a phone), but this will block the main thread (and freeze the UI) during the file transfer. The execution+wait must be done on a separate thread.

I suggest to generalize the installer so that it can also handle file transfer requests in addition to apk installation requests.

npes87184 · 2018-08-12T01:11:23Z

Hi, @rom1v

Thank you for your advice.
I'll take a look today.

Regards.

npes87184 · 2018-08-12T02:47:56Z

Hi, @rom1v

I have integrated file transfer into installer.
In this series, I have made some changes.

fix a mutex leak when strdup failed.
rename installer to file_handler.
support file transfer in file_handler.

Please contact me, if you have any questions or concerns.

Thanks.

rom1v

Thank you, I pushed your first commit.

(see inline comments)

rom1v · 2018-08-12T13:07:43Z

app/src/file_handler.c

+        file_queue_take(&file_handler->queue, &current_file);
+#endif
+        LOGI("Processing %s...", current_file);
+        if (is_apk(current_file)) {


I think that this should be an explicit request from the caller (there would be a queue of requests, having a type which can take two values, for example INSTALL_APK or PUSH_FILE).

This test would be in case SDL_DROPFILE. Or better (I think): install an apk on dropfile (as it is), and push a file on Ctrl+dropfile (so that it will be possible to push apk). What do you think?

This could be done separately though. Tell me if you want to do this, otherwise I can do it later.

Hi, @rom1v

Give a type to every request is a good idea!
However, I think that push an apk without installing it make no sense. When do we need to just push an apk on the device and install it later?

Thank you for your time and effort.

It seems weird to be able to push any file except a specific type. One may want to use its device as an "usb key", and store apk to copy them somewhere afterwards.

It's not very important though.

Hi, @rom1v

Push an apk without installing it is an rare case. I don't think that break a good design because of unusual situations is a good idea. We should remain intuitive and convenient method (simply drag & drop) for user. Therefore, I propose these behavior:

Install apk (common case, just drag & drop)

Push file (common case, just drag & drop)

Push apk (rare case, ctrl + drag & drop)

What do you think?

Regards!

OK, so for simplicity, just forget 3. 😉

rom1v · 2018-08-12T13:13:12Z

app/src/file_handler.c

+        LOGI("Processing %s...", current_file);
+        if (is_apk(current_file)) {
+            process = adb_install(file_handler->serial, current_file);
+            strncpy(cmd, "adb install", sizeof(cmd));


You could declare cmd as const char *, then just:

cmd = "adb install";

rom1v · 2018-08-12T13:14:34Z

app/src/file_handler.c

@@ -0,0 +1,195 @@
+#include "file_handler.h"


<nitpick>
filehandler.c/filehandler.h for "consistency" with the other files (but still with _ for the structure). That's not a great convention, but at least it would be more consistent 😉
</nitpick>

Not a good idea. But I will do it to maintain consistency.

Thanks.

You're right, it's weird. Don't do it, I will rename the other files instead.

Signed-off-by: npes87184 <npes87184@gmail.com>

npes87184 · 2018-08-12T14:57:25Z

Hi, @rom1v

I have pushed commits based on review.
In this series, I rebase the change of master and introduce request struct. Caller only need to give a file name and what to do. Worker will take the request and consume it.

Hope you will like this change.

Regards!

rom1v

Hope you will like this change.

Yes, thank you, great work 👍

I commented inline for requesting some fixes and suggesting several minor changes.

To avoid many roundtrips, here is a diff of the changes I suggest (the reasons are hopefully given in my inline comments on this PR). If you agree with them, I can push your commits including these changes.

diff --git a/app/src/file_handler.c b/app/src/file_handler.c
index 4dc23ce..5feb008 100644
--- a/app/src/file_handler.c
+++ b/app/src/file_handler.c
@@ -6,12 +6,27 @@
 #include "lockutil.h"
 #include "log.h"
 
+struct request {
+    file_handler_action_t action;
+    const char *file;
+};
+
+struct request *request_new(file_handler_action_t action, const char *file) {
+    struct request *req = SDL_malloc(sizeof(*req));
+    if (!req) {
+        return NULL;
+    }
+    req->action = action;
+    req->file = file;
+    return req;
+}
+
 void request_free(struct request *req) {
     if (!req) {
         return;
     }
-    SDL_free(req->file);
-    free(req);
+    SDL_free((void *) req->file);
+    SDL_free((void *) req);
 }
 
 SDL_bool request_queue_is_empty(const struct request_queue *queue) {
@@ -105,9 +120,10 @@ static process_t push_file(const char *serial, const char *file) {
     return adb_push(serial, file, DEVICE_SDCARD_PATH);
 }
 
-SDL_bool file_handler_add_request(struct file_handler *file_handler, const char *file, req_action_t action) {
+SDL_bool file_handler_request(struct file_handler *file_handler,
+                              file_handler_action_t action,
+                              const char *file) {
     SDL_bool res;
-    struct request *req = (struct request *) malloc(sizeof(struct request));
 
     // start file_handler if it's used for the first time
     if (!file_handler->initialized) {
@@ -117,13 +133,11 @@ SDL_bool file_handler_add_request(struct file_handler *file_handler, const char
         file_handler->initialized = SDL_TRUE;
     }
 
-    LOGI("Adding request %s", file);
-    req->file = SDL_strdup(file);
-    req->action = action;
-    if (action == INSTALL_APK) {
-        req->func = install_apk;
-    } else {
-        req->func = push_file;
+    LOGI("Request to %s %s", action == ACTION_INSTALL_APK ? "install" : "push", file);
+    struct request *req = request_new(action, file);
+    if (!req) {
+        LOGE("Could not create request");
+        return SDL_FALSE;
     }
 
     mutex_lock(file_handler->mutex);
@@ -150,30 +164,37 @@ static int run_file_handler(void *data) {
             break;
         }
         struct request *req;
-        process_t process;
-        const char *cmd;
 #ifdef BUILD_DEBUG
         bool non_empty = request_queue_take(&file_handler->queue, &req);
         SDL_assert(non_empty);
 #else
         request_queue_take(&file_handler->queue, &req);
 #endif
-        LOGI("Processing %s...", req->file);
-        process = req->func(file_handler->serial, req->file);
+        process_t process;
+        if (req->action == ACTION_INSTALL_APK) {
+            LOGI("Installing %s...", req->file);
+            process = install_apk(file_handler->serial, req->file);
+        } else {
+            LOGI("Pushing %s...", req->file);
+            process = push_file(file_handler->serial, req->file);
+        }
         file_handler->current_process = process;
         mutex_unlock(file_handler->mutex);
 
-        if (req->action == INSTALL_APK) {
-            cmd = "adb install";
+        if (req->action == ACTION_INSTALL_APK) {
+            if (process_check_success(process, "adb install")) {
+                LOGI("%s successfully installed", req->file);
+            } else {
+                LOGE("Failed to install %s", req->file);
+            }
         } else {
-            cmd = "adb push";
+            if (process_check_success(process, "adb push")) {
+                LOGI("%s successfully pushed to /sdcard/", req->file);
+            } else {
+                LOGE("Failed to push %s to /sdcard/", req->file);
+            }
         }
 
-        if (process_check_success(process, cmd)) {
-            LOGI("Process %s successfully", req->file);
-        } else {
-            LOGE("Failed to process %s", req->file);
-        }
         request_free(req);
     }
     return 0;
diff --git a/app/src/file_handler.h b/app/src/file_handler.h
index 39dbf02..375db29 100644
--- a/app/src/file_handler.h
+++ b/app/src/file_handler.h
@@ -9,15 +9,9 @@
 #define REQUEST_QUEUE_SIZE 16
 
 typedef enum {
-    INSTALL_APK = 0,
-    PUSH_FILE,
-} req_action_t;
-
-struct request {
-    process_t (*func)(const char *, const char *);
-    char *file;
-    req_action_t action;
-};
+    ACTION_INSTALL_APK,
+    ACTION_PUSH_FILE,
+} file_handler_action_t;
 
 struct request_queue {
     struct request *reqs[REQUEST_QUEUE_SIZE];
@@ -43,6 +37,8 @@ SDL_bool file_handler_start(struct file_handler *file_handler);
 void file_handler_stop(struct file_handler *file_handler);
 void file_handler_join(struct file_handler *file_handler);
 
-SDL_bool file_handler_add_request(struct file_handler *file_handler, const char *filename, req_action_t action);
+SDL_bool file_handler_request(struct file_handler *file_handler,
+                              file_handler_action_t action,
+                              const char *file);
 
 #endif
diff --git a/app/src/scrcpy.c b/app/src/scrcpy.c
index 1993d77..cee4e12 100644
--- a/app/src/scrcpy.c
+++ b/app/src/scrcpy.c
@@ -56,7 +56,7 @@ static int event_watcher(void *data, SDL_Event *event) {
 }
 #endif
 
-static int is_apk(const char *file) {
+static SDL_bool is_apk(const char *file) {
     const char *ext = strrchr(file, '.');
     return ext && !strcmp(ext, ".apk");
 }
@@ -109,13 +109,16 @@ static SDL_bool event_loop(void) {
             case SDL_MOUSEBUTTONUP:
                 input_manager_process_mouse_button(&input_manager, &event.button);
                 break;
-            case SDL_DROPFILE:
+            case SDL_DROPFILE: {
+                file_handler_action_t action;
                 if (is_apk(event.drop.file)) {
-                    file_handler_add_request(&file_handler, event.drop.file, INSTALL_APK);
+                    action = ACTION_INSTALL_APK;
                 } else {
-                    file_handler_add_request(&file_handler, event.drop.file, PUSH_FILE);
+                    action = ACTION_PUSH_FILE;
                 }
+                file_handler_request(&file_handler, action, event.drop.file);
                 break;
+            }
         }
     }
     return SDL_FALSE;

rom1v · 2018-08-15T13:25:26Z

app/src/file_handler.c

+        return;
+    }
+    SDL_free(req->file);
+    free(req);


SDL_free((void *) req)

rom1v · 2018-08-15T13:25:43Z

app/src/scrcpy.c

@@ -56,6 +56,11 @@ static int event_watcher(void *data, SDL_Event *event) {
 }
 #endif

+static int is_apk(const char *file) {


return SDL_bool

rom1v · 2018-08-15T13:26:38Z

app/src/file_handler.h

+} req_action_t;
+
+struct request {
+    process_t (*func)(const char *, const char *);


Either store func+file or action+file, but not both. Since the action is useful later (for error messages), I suggest to remove func.

rom1v · 2018-08-15T13:27:03Z

app/src/file_handler.c

+
+SDL_bool file_handler_add_request(struct file_handler *file_handler, const char *file, req_action_t action) {
+    SDL_bool res;
+    struct request *req = (struct request *) malloc(sizeof(struct request));


check malloc failure

rom1v · 2018-08-15T13:27:23Z

app/src/file_handler.c

+
+SDL_bool file_handler_add_request(struct file_handler *file_handler, const char *file, req_action_t action) {
+    SDL_bool res;
+    struct request *req = (struct request *) malloc(sizeof(struct request));


do not leak req if file_handler_start() fails

rom1v · 2018-08-15T13:29:24Z

app/src/file_handler.h

+    PUSH_FILE,
+} req_action_t;
+
+struct request {


request is used only internally, it could be moved to file_handler.c.

I'll do the same later with request_queue.

EDIT: ah no, request_queue is used by file_handler in the .h.

rom1v · 2018-08-15T13:54:47Z

app/src/file_handler.h

+void file_handler_stop(struct file_handler *file_handler);
+void file_handler_join(struct file_handler *file_handler);
+
+SDL_bool file_handler_add_request(struct file_handler *file_handler, const char *filename, req_action_t action);


It's subjective, but I would reverse action and file (we request an action, and pass its parameter afterwards).

rom1v · 2018-08-15T13:56:19Z

app/src/file_handler.h

+typedef enum {
+    INSTALL_APK = 0,
+    PUSH_FILE,
+} req_action_t;


Since they are actions associated by the file_handler, I would prefer something like file_handler_action_t (especially once request is moved to the .c).

rom1v · 2018-08-15T14:00:50Z

app/src/file_handler.c

+        file_handler->initialized = SDL_TRUE;
+    }
+
+    LOGI("Adding request %s", file);


Should log the action too.

rom1v · 2018-08-15T14:03:26Z

app/src/file_handler.c

+#else
+        request_queue_take(&file_handler->queue, &req);
+#endif
+        LOGI("Processing %s...", req->file);


Different log messages for install and push would be better for the user.

npes87184 · 2018-08-15T14:32:21Z

Hi,

It is awesome, go ahead!

Thank you for your efforts.

support drag & drop file to device /sdcard

Rename foobar.ext to foo_bar.ext. <#226 (comment)>

rom1v · 2018-08-15T15:25:45Z

👍 Merged on dev branch.

Rename foobar.ext to foo_bar.ext. <#226 (comment)>

npes87184 force-pushed the push_file_to_sdcard branch 2 times, most recently from 24e95e9 to 57e42ac Compare August 12, 2018 02:44

npes87184 changed the title ~~support drop & drag file to device /sdcard~~ support drag & drop file to device /sdcard Aug 12, 2018

rom1v reviewed Aug 12, 2018

View reviewed changes

npes87184 added 2 commits August 12, 2018 22:49

installer -> file_handler

d0f8e42

Signed-off-by: npes87184 <npes87184@gmail.com>

Support drag & drop a file to transfer it to device

ddc9f58

Signed-off-by: npes87184 <npes87184@gmail.com>

npes87184 force-pushed the push_file_to_sdcard branch from 57e42ac to ddc9f58 Compare August 12, 2018 14:51

rom1v reviewed Aug 15, 2018

View reviewed changes

rom1v added a commit that referenced this pull request Aug 15, 2018

Merge branch 'npes87184:push_file_to_sdcard' into dev (#226)

92d1aff

support drag & drop file to device /sdcard

rom1v added a commit that referenced this pull request Aug 15, 2018

Separate multi-words filenames by '_'.

db6edfe

Rename foobar.ext to foo_bar.ext. <#226 (comment)>

rom1v closed this Aug 15, 2018

rom1v added a commit that referenced this pull request Aug 15, 2018

Separate multi-words filenames by '_'

536b318

Rename foobar.ext to foo_bar.ext. <#226 (comment)>

rom1v mentioned this pull request Aug 15, 2018

Move files between PC and device #128

Closed

npes87184 mentioned this pull request Aug 18, 2018

Drag and drop/File Maneger/ Copy-cut-paste #84

Closed

rom1v mentioned this pull request Oct 4, 2018

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

Closed

rom1v mentioned this pull request Aug 5, 2019

Request: offer more options for drag&drop of files #693

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support drag & drop file to device /sdcard #226

support drag & drop file to device /sdcard #226

npes87184 commented Aug 11, 2018

rom1v commented Aug 11, 2018

npes87184 commented Aug 12, 2018

npes87184 commented Aug 12, 2018 •

edited

Loading

rom1v left a comment •

edited

Loading

rom1v Aug 12, 2018

npes87184 Aug 12, 2018

rom1v Aug 12, 2018

npes87184 Aug 13, 2018

rom1v Aug 15, 2018

rom1v Aug 12, 2018

npes87184 Aug 12, 2018

rom1v Aug 12, 2018

npes87184 Aug 12, 2018

rom1v Aug 12, 2018

npes87184 commented Aug 12, 2018

rom1v left a comment

rom1v Aug 15, 2018

rom1v Aug 15, 2018

rom1v Aug 15, 2018

rom1v Aug 15, 2018

rom1v Aug 15, 2018

rom1v Aug 15, 2018 •

edited

Loading

rom1v Aug 15, 2018

rom1v Aug 15, 2018

rom1v Aug 15, 2018

rom1v Aug 15, 2018

npes87184 commented Aug 15, 2018

rom1v commented Aug 15, 2018

support drag & drop file to device /sdcard #226

support drag & drop file to device /sdcard #226

Conversation

npes87184 commented Aug 11, 2018

rom1v commented Aug 11, 2018

npes87184 commented Aug 12, 2018

npes87184 commented Aug 12, 2018 • edited Loading

rom1v left a comment • edited Loading

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

npes87184 commented Aug 12, 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

rom1v Aug 15, 2018 • edited Loading

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

npes87184 commented Aug 15, 2018

rom1v commented Aug 15, 2018

npes87184 commented Aug 12, 2018 •

edited

Loading

rom1v left a comment •

edited

Loading

rom1v Aug 15, 2018 •

edited

Loading