Merge ~santoshbit2007/oxide:UITK_FilePicker_Impl into oxide:master

Thanks for working on this.

First of all, don't be afraid to refactor existing code. It would be nice to refactor the FilePicker layering in a similar way to recent changes I've made to the context menu / popup menu / JS dialogs implementations.

I've left a few comments inline.

In addition to those:

- I don't see where the fallback (legacy / non content-hub) file picker implementation gets loaded.
- Please remove qt::WebViewProxyClient::CreateFilePicker (and the corresponding implementation in qt/quick/api).

I'm probably not the best person to review the content-hub picker implementation.

While running the testcases, I am seeing protobuf issue
-tmpdir /tmp/tmp-oxide-runtestsnIpqZ4 --file tst_WebViewFilePicker.qml'
[New Thread 0x7ffff539f700 (LWP 12033)]
Oxide.Ubuntu is an experimental module - future versions may change in backwards-incompatible ways, and without bumping the module version. Please use with caution

[libprotobuf FATAL ../../third_party/protobuf/src/google/protobuf/stubs/common.cc:78] This program was compiled against version 2.6.1 of the Protocol Buffer runtime library, which is not compatible with the installed version (3.0.0). Contact the program author for an update. If you compiled the program yourself, make sure that your headers are from the same version of Protocol Buffers as your link-time library. (Version verification failed in "/build/mir-sajBv9/mir-0.26.1+16.04.20170209.1/obj-x86_64-linux-gnu/src/protobuf/mir_protobuf.pb.cc".)

Its happens only when content-hub is used.

Thanks, I've left some more comments.

Also, I've been gradually dropping the "oxide_" and "oxide_qt_" prefixes from filenames, keeping those only for files that implement Chromium interfaces

I think this looks ok (please see 1 comment inline though).

Also, I think I have an alternative idea for how the fallback could work. I'll open another bug for that though.

Ok, I opened bug 1672707 too

Thanks chris, I will check two new child bugs.

Regards
Santosh

On Tue, Mar 14, 2017 at 5:54 PM, Chris Coulson <email address hidden>
wrote:

> Ok, I opened bug 1672707 too
> --
> https://code.launchpad.net/~santoshbit2007/oxide/+git/oxide/+merge/317213
> You are the owner of ~santoshbit2007/oxide:UITK_FilePicker_Impl.
>

Just tested with webbrowser-app unconfined on unity7 desktop, and while I’m getting the list of apps correctly displayed, clicking on any of them (including browser itself) does nothing and I’m seeing the following errors in the logs:

void ContentTransfer::setTransfer(com::ubuntu::content::Transfer*) No valid transfer object passed: QObject(0x0)
bool ContentTransfer::start() Transfer can't be started

Also, see two minor comments inline.

Thanks for comments.

I am aware of this issue, I think you are testing legacy webview filepicker and here webbrowser-app filepicker is used, anyway this issue is related with content-hub.
Basically what happens is deb package of source app are not correctly registered in
content-hub with unity7/desktop. Ken VanDine had same saying on this when I had asked.IIRC same issue happens with webbrowser-app without this patch.
I will do double check though.

Best way to validate with unity8/mobile platform and app having UbuntuWebView

I will check other comments.

Nope, I am indeed testing the new webview (I have a branch of webbrowser-app that makes use of it). With the legacy webview (and webbrowser-app’s file picker), I am able to select a file from webbrowser-app itself as a source. With your branch, I can’t. It’s gotta work on desktop too, testing on mobile is not enough.

I tried to test with some apps(filemanager, gallary) but they doesn't seems to works well with content-hub, although gallery app works only once(needs reboot nexttime) and filepicker works well with that. There is some works going on ken team to fix that

unity7 result are as:

UbuntuWebView + oxidefilepicker
 gallary --> works
 webbrowser-app -- > doesn't ( need to fix in app side, as of now legacy webbrowser-app filepicker has code that shows the webbrowser-app local download page)
 This will not be valid in current case as now filepicker is app independent.

legacy webview + webbrowser-app filepicker : ->
gallary --. works
webbrowser-app --> works

Tested on unity8 with gallery app, works fine here

rebased on latest oxide

Please see my comments inline.

I have tested with a custom branch of webbrowser-app that uses the new Oxide.Ubuntu WebView, on unity7 and unconfined.

The fallback to the file dialog works if qml-module-qtquick-dialogs is installed.

I can confirm that the content-hub dialog works well once with the gallery app as a provider, but it doesn’t work subsequent times.

I disagree that the custom code in webbrowser-app to show the downloads page is not a valid use-case. We can’t afford this kind of regression. Besides, it seems a perfectly valid use case for other apps that may want to provide an integrated content picker, not just webbrowser-app. I’d say we need an additional API that lets embedders provide a custom view for when the app itself is picked as a content provider.

We also might want to consider allowing embedders to override the built-in file picker. Apps might want to use the Oxide.Ubuntu WebView while retaining a tight control of what content can be uploaded.

Thanks olivier for reviews,

I agree with that disagree, may be I created confusion before but I also mean the same.
What I really mean is that "custom code in webbrowser-app to show the downloads" should be
in different place not in the webbrowser-app filepicker. so that when oxide's internal filepicker is used(in this case webbrowser-app filepicker is not used), showing of download page still happens.

Regarding overriding filepicker, Its minor work in term of coding because OxideUbuntuWebview directly drives from OxideQQuickWebView, so all the api exposed by OxideQQuickWebView are also valid for OxideUbuntuWebView, But there is difference in how ui_factroy has been used in both webview. OxideUbuntuWebView supports uitk factory and OxideQQuickWebView uses legacy_aux_factory.
uitk factory doesn't support overriding but legacy_aux_factory supports(as used by webrowser-app right now).
So Its basically design decision and would need to confirm with chris. If agreed I will prefer to take it in seperate patch.

See answers inline.

The updates to the test case look ok. I'd still like to see proper testing of the content-hub based picker with mocks of the content-hub D-Bus API, but it looks like you want to implement that in a follow-up branch?

Hi Olivier,

Yes, I am planning to work testing part in separate branch, Its not very
straight way I need to do dig down in D-bus usage and also
would need to update oxide testing infrastructure. there was some updates
that content-hub project also plan to add some testing support
capability (not exactly clear what their plan).
Existing branch is already big to handle for me, and I don't to complicate
it further and mess up things, and I also feel review will be easier
in separate branch.

I already filed a bug for testing improvement
https://bugs.launchpad.net/oxide/+bug/1679628
Sure will look into this next.

Regards
Santosh

On Tue, Apr 4, 2017 at 9:43 PM, Olivier Tilloy <<email address hidden>
> wrote:

> The updates to the test case look ok. I'd still like to see proper testing
> of the content-hub based picker with mocks of the content-hub D-Bus API,
> but it looks like you want to implement that in a follow-up branch?
> --
> https://code.launchpad.net/~santoshbit2007/oxide/+git/oxide/+merge/317213
> You are the owner of ~santoshbit2007/oxide:UITK_FilePicker_Impl.
>