Bug 124474 – [EFL] Implement download directory selector on Minibrowser

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Jongwoo Choi]

EFL Minibrowser doesn't implement download directory selector.
To select destination download directory by user, Implement download directory selector on Minibrowser using existent fileselector API.

[Jongwoo Choi]

Created attachment 217164 [details]
Patch

[Jongwoo Choi]

Created attachment 217166 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 217166 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217166&action=review

Currently, MiniBrowser doesn't work on my environment. Let me review this patch further after fixing my problem.

> Tools/MiniBrowser/efl/main.c:615
> +    evas_object_event_callback_del(ds_data->parent->elm_window, EVAS_CALLBACK_DEL, on_directorypicker_parent_deletion);

Should we deregister this callback here ? If not, we can add close_director_picker() as close_file_picker(), then I think it is better to reuse the same function instead of duplicating code.

> Tools/MiniBrowser/efl/main.c:644
> +    ecore_main_loop_quit();

Why should we finish download job in on_download_progress though there is on_download_finished ?

[Jongwoo Choi]

Created attachment 217494 [details]
Patch

[Ryuan Choi]

Comment on attachment 217494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217494&action=review

> Tools/MiniBrowser/efl/main.c:679
> +    elm_fileselector_expandable_set(directory_selector, EINA_FALSE);

If I remember correctly, we don't need this because default value is EINA_FALSE.

> Tools/MiniBrowser/efl/main.c:680
> +    elm_fileselector_buttons_ok_cancel_set(directory_selector, EINA_FALSE);

Do you have any reason to use custom ok / cancel instead of builtin ?

[Jongwoo Choi]

(In reply to comment #3)
> (From update of attachment 217166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217166&action=review
> 
> Currently, MiniBrowser doesn't work on my environment. Let me review this patch further after fixing my problem.
> 
> > Tools/MiniBrowser/efl/main.c:615
> > +    evas_object_event_callback_del(ds_data->parent->elm_window, EVAS_CALLBACK_DEL, on_directorypicker_parent_deletion);
> 
> Should we deregister this callback here ? If not, we can add close_director_picker() as close_file_picker(), then I think it is better to reuse the same function instead of duplicating code.
> 
> > Tools/MiniBrowser/efl/main.c:644
> > +    ecore_main_loop_quit();
> 
> Why should we finish download job in on_download_progress though there is on_download_finished ?

I made close_directory_picker() similar to close_file_picker() and merged these two codes to reuse same function.

To avoid duplication, I moved deregister callback and ecore_main_loop_quit() from on_directorypicker_deletion() and on_download_progress() to close_directory_picker().

[Jongwoo Choi]

(In reply to comment #5)
> (From update of attachment 217494 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217494&action=review
> 
> > Tools/MiniBrowser/efl/main.c:679
> > +    elm_fileselector_expandable_set(directory_selector, EINA_FALSE);
> 
> If I remember correctly, we don't need this because default value is EINA_FALSE.

After built with exclude elm_fileselector_expandable_set(directory_selector, EINA_FALSE); line, directory selector set to tree view. I think that default value is EINA_TRUE.

> 
> > Tools/MiniBrowser/efl/main.c:680
> > +    elm_fileselector_buttons_ok_cancel_set(directory_selector, EINA_FALSE);
> 
> Do you have any reason to use custom ok / cancel instead of builtin ?

Maybe I didn't have fully knowledge about smartcallback when i first made this code.
Now, I understand this part and fixed it using built-in ok/cancel button.
To use built-in button, on_download_request() was changed to on_directoryselector_done() function.

[Jongwoo Choi]

Created attachment 217515 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 217515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217515&action=review

BTW, how can I see this directory picker ? Could you let me know an example ?

> Tools/MiniBrowser/efl/main.c:133
> +    Browser_Window* parent;

Wrong * place.

> Tools/MiniBrowser/efl/main.c:190
> +static void quit_event_loop(void *user_data, Evas_Object *obj, void *event_info);

Where is this function used ?

> Tools/MiniBrowser/efl/main.c:598
> +

Unneeded line.

> Tools/MiniBrowser/efl/main.c:599
> +static void on_directorypicker_parent_deletion(void *user_data, Evas *evas, Evas_Object *elm_window, void *event_info);

To be sync with other function, please use on_directory_picker_parent_deletion(...)

> Tools/MiniBrowser/efl/main.c:638
> +            char unique_path[] = "downloaded-file.XXXXXX";

Can't we support new name if there is already "downloaded-file.XXXXXX" in /tmp ? For instance, if there is downloaded-file-1.XXX, next no name file will be saved as download-file-2.XXX.

[Jongwoo Choi]

(In reply to comment #9)
> (From update of attachment 217515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217515&action=review
> 
> BTW, how can I see this directory picker ? Could you let me know an example ?
> 
> > Tools/MiniBrowser/efl/main.c:133
> > +    Browser_Window* parent;
> 
> Wrong * place.
> 
> > Tools/MiniBrowser/efl/main.c:190
> > +static void quit_event_loop(void *user_data, Evas_Object *obj, void *event_info);
> 
> Where is this function used ?
> 
> > Tools/MiniBrowser/efl/main.c:598
> > +
> 
> Unneeded line.
> 
> > Tools/MiniBrowser/efl/main.c:599
> > +static void on_directorypicker_parent_deletion(void *user_data, Evas *evas, Evas_Object *elm_window, void *event_info);
> 
> To be sync with other function, please use on_directory_picker_parent_deletion(...)
> 
> > Tools/MiniBrowser/efl/main.c:638
> > +            char unique_path[] = "downloaded-file.XXXXXX";
> 
> Can't we support new name if there is already "downloaded-file.XXXXXX" in /tmp ? For instance, if there is downloaded-file-1.XXX, next no name file will be saved as download-file-2.XXX.

Download picker, can be seen at the following address.
http://www.w3.org/TR/2000/REC-ATAG10-20000203/atag10.tgz

Filename consist with XXX(six or more'X') will be replaced unique file name  after mktemp(unique_path). For example, download-file.

[Jongwoo Choi]

Created attachment 217625 [details]
Patch

[Jongwoo Choi]

> Filename consist with XXX(six or more'X') will be replaced unique file name  after mktemp(unique_path). For example, download-file.

Sorry I missed example file name
after mktemp(), file name will be 
downloaded-file.ie7Ksm 
downloaded-file.aUFQUU 
downloaded-file.ADckXL
....

[Gyuyoung Kim]

Is this patch still valid ?

[Gyuyoung Kim]

Comment on attachment 217625 [details]
Patch

Cleared review? from attachment 217625 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.