Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open deep links in app, items already discovered (1st round) #3598

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

fesave
Copy link
Contributor

@fesave fesave commented Apr 5, 2022

Related Issues

App: #3573

  • Added changelog files for the fixed issues in folder changelog/unreleased. More info here

QA

Test plan: https://github.com/owncloud/QA/blob/master/Mobile/Android/Release_2.21/3598-Universal%20Link%20I.md

Reports:

Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First iteration 💯 Some suggestions here @fesave

@fesave fesave force-pushed the feature/deep_links branch from b4fb9f6 to ad15940 Compare April 6, 2022 09:53
@fesave fesave requested a review from abelgardep April 6, 2022 09:53
@fesave fesave force-pushed the feature/deep_links branch from ad15940 to 214061a Compare April 6, 2022 11:16
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

owncloudApp/src/main/res/values/setup.xml Outdated Show resolved Hide resolved
@jesmrec
Copy link
Collaborator

jesmrec commented Apr 11, 2022

(1) [FIXED]

  1. Install app and add account
  2. Browse inside any folder in root, in order to discover it.
  3. Get the link of the folder browsed in 2.
  4. Kill the app, so that it will be closed when link is clicked
  5. Click on link over the folder

Current: Folder not opened, it is shown the root folder. Check the video, the link corresponds with the Documents folder. Documentsappears in the top bar but the content is the root:

device-2022-04-11-171049.mp4

Nexus 6P Android7
Huawei P20 Android 9
214061ad

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 11, 2022

(2) [WONT FIX] -> new architecture.

Prerequisite:

  1. Create a folder structure like /A/B/C
  2. Inside C, upload an openable file (f. ex, an image), and a non-openable file (f. ex a PDF)

Two scenarios:

Open via link the image -> image is opened
Browse back with the arrow -> details view
Browse back from details view -> "C" folder content is displayed

Open via link the pdf -> details view is opened
Browse back with the arrow -> root folder

that means, depending on the content's openability, browsing works in a different way. Is this expected somehow? Could if be fixed?

Nexus 6P Android7
214061ad

@fesave fesave force-pushed the feature/deep_links branch from e6135fb to 20c5564 Compare April 13, 2022 12:17
@fesave
Copy link
Contributor Author

fesave commented Apr 13, 2022

that means, depending on the content's operability, browsing works in a different way. Is this expected somehow? Could it be fixed?

I think that with the new architecture the current behavior can be improved, but for the moment it is the first version.

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 18, 2022

(3) [FIXED]

Not posible to build the branch after the latest changes. Some manifest error happens:

/Documents/android/owncloudApp/src/debug/AndroidManifest.xml:86:9-106:20 Error:
	android:exported needs to be explicitly specified for element <activity#com.owncloud.android.ui.activity.FileDisplayActivity>. Apps targeting Android 12 and higher are required to specify an explicit value for `android:exported` when the corresponding component has an intent filter defined. See https://developer.android.com/guide/topics/manifest/activity-element#exported for details.

BitRise build also fails with same error: https://app.bitrise.io/build/f4c6256e-f684-4d9f-a22c-467179da0ddf

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 18, 2022

(4) [FIXED]

  1. Enable passcode/pattern
  2. Open deeplink from other app

Current: First time, passcode/pattern is asked. If app is killed (no lock delay), next times the deeplink is opened, pattern/passcode is not asked.

Expected: Every time a link is opened, pattern/passcode is asked

Nexus 6P Android 7
6b4b0303

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 18, 2022

(2) will need a new issue with New Architecture label

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 19, 2022

(5) [FIXED]

Checking the feature in some servers, i realised that the private link does not always is "plain". That means, it could contain some extra chunks. For example, in cloud.damken.com the private links are like https://cloud.damken.com/index.php/f/28728865 with an extra index.php. The oC internal instance works in that way too.

With Android 7, the link is opened in the browser. With Android 12, it can not be manually added as supported.

I also tried adding index.php , * and some combinations to setup.xml with no success in any of the Android versions.

Is there anything we could do with that?

@fesave fesave force-pushed the feature/deep_links branch from 938a4ab to f71c65f Compare April 19, 2022 08:40
@fesave
Copy link
Contributor Author

fesave commented Apr 19, 2022

(3)

Not posible to build the branch after the latest changes. Some manifest error happens:

/Documents/android/owncloudApp/src/debug/AndroidManifest.xml:86:9-106:20 Error:
	android:exported needs to be explicitly specified for element <activity#com.owncloud.android.ui.activity.FileDisplayActivity>. Apps targeting Android 12 and higher are required to specify an explicit value for `android:exported` when the corresponding component has an intent filter defined. See https://developer.android.com/guide/topics/manifest/activity-element#exported for details.

BitRise build also fails with same error: https://app.bitrise.io/build/f4c6256e-f684-4d9f-a22c-467179da0ddf

fixed :)

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 20, 2022

Regarding (5). Problem comes from the android:pathPrefix setup. We should add the extra chunks. For the example above

https://cloud.damken.com/index.php/f/28728865

android:host="cloud.damken.com"
android:pathPrefix="/index.php/f/"

Checked, this setup works.

So, one option is creating a new parameter in setup.xml together with deep_link_host (something like deep_link_suffix) in which those links containing subdomains or other chunks will add this part there:

If deep_link_suffix is empty -> android:pathPrefix="/f/" as before
If deep_link_suffix is not empty -> android:pathPrefix="/" + deep_link_suffix + /f/"

(logic not completed, so the leading slashes should be handled in the same line since no ifs are allowed in Manifest files)

is this feasible @fesave?

@fesave
Copy link
Contributor Author

fesave commented Apr 20, 2022

Regarding (5). Problem comes from the android:pathPrefix setup. We should add the extra chunks. For the example above

https://cloud.damken.com/index.php/f/28728865

android:host="cloud.damken.com"
android:pathPrefix="/index.php/f/"

Checked, this setup works.

So, one option is creating a new parameter in setup.xml together with deep_link_host (something like deep_link_suffix) in which those links containing subdomains or other chunks will add this part there:

If deep_link_suffix is empty -> android:pathPrefix="/f/" as before If deep_link_suffix is not empty -> android:pathPrefix="/" + deep_link_suffix + /f/"

(logic not completed, so the leading slashes should be handled in the same line since no ifs are allowed in Manifest files)

is this feasible @fesave?

AFAIK, the AndroidManifest.xml file is a configuration file where we can apply the basic settings of our application. Therefore, it is not possible to add logic to control pathPrefix.

I think the best option is to configure a new field inside our setup.xml file and have its value come from the server as we do with other fields.

We would have to consider the possibility that the value could be of two types:

  • /f/ -> default.
  • /deepLinkSuffix/f/ -> in case the value is filled in the server.

Are you in charge of creating the issue with the detailed solution and the name of the new field or do you prefer me to do it in this branch? @jesmrec

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 20, 2022

Are you in charge of creating the issue with the detailed solution and the name of the new field or do you prefer me to do it in this branch? @jesmrec

it's ok to do in this branch.

@fesave
Copy link
Contributor Author

fesave commented Apr 21, 2022

(4)

  1. Enable passcode/pattern
  2. Open deeplink from other app

Current: First time, passcode/pattern is asked. If app is killed (no lock delay), next times the deeplink is opened, pattern/passcode is not asked.

Expected: Every time a link is opened, pattern/passcode is asked

Nexus 6P Android 7 6b4b0303

First of all, I have only reproduced this error with image files and their preview. I have coded a solution to avoid it, but, we have lost the automatic preview of the images. I think it is not bad at all because we can unify all the previews with the three action buttons that we should add after the design discussion here

@fesave fesave force-pushed the feature/deep_links branch from 7b5a4a5 to cf0d294 Compare April 21, 2022 06:46
@jesmrec
Copy link
Collaborator

jesmrec commented Apr 21, 2022

That means, that deeplinks will point always to Details , no matter if the file is preview-able inside the app or not, right @fesave ?

is that OK @michaelstingl? should we create issue or develop this to mitigate such effect?

@fesave
Copy link
Contributor Author

fesave commented Apr 21, 2022

That means, that deeplinks will point always to Details , no matter if the file is preview-able inside the app or not, right @fesave ?

is that OK @michaelstingl? should we create issue or develop this to mitigate such effect?

Yes, with the current state, all files (except videos) show their preview screen if the user accesses them through a deep link.

@fesave fesave force-pushed the feature/deep_links branch from cf0d294 to 10697b2 Compare April 21, 2022 07:46
@michaelstingl
Copy link
Contributor

is that OK @michaelstingl? should we create issue or develop this to mitigate such effect?

Yes, with the current state, all files (except videos) show their preview screen if the user accesses them through a deep link.

What do you mean with "preview screen"? Images, Audio & Video should be shown or played if the app supports it. Only unsupported formats should open Details for this file. #3573 (comment) isn't a solution, this would open the image in a 3rd party app, not in the ownCloud app.

@fesave fesave force-pushed the feature/deep_links branch from 10697b2 to dfffdce Compare April 26, 2022 10:22
@fesave fesave requested a review from abelgardep April 27, 2022 11:50
@fesave fesave force-pushed the feature/deep_links branch from 76692f3 to 08b091e Compare April 28, 2022 07:00
@jesmrec
Copy link
Collaborator

jesmrec commented Apr 29, 2022

(6) [FIXED]

  1. Install the app, no accounts added
  2. Click on any deeplink openable by the app

Current: crash happens. This is the stacktrace:

2022-04-29 09:40:12.377 31539-31539/? E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.owncloud.android.debug, PID: 31539
    java.lang.RuntimeException: Unable to resume activity {com.owncloud.android.debug/com.owncloud.android.ui.activity.FileDisplayActivity}: java.lang.NullPointerException: Attempt to invoke virtual method 'com.owncloud.android.datamodel.OCFile com.owncloud.android.datamodel.FileDataStorageManager.getFileByPrivateLink(java.lang.String)' on a null object reference
        at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3430)
        at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3470)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2733)
        at android.app.ActivityThread.-wrap12(ActivityThread.java)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1478)
        at android.os.Handler.dispatchMessage(Handler.java:102)
        at android.os.Looper.loop(Looper.java:154)
        at android.app.ActivityThread.main(ActivityThread.java:6121)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:889)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:779)
     Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'com.owncloud.android.datamodel.OCFile com.owncloud.android.datamodel.FileDataStorageManager.getFileByPrivateLink(java.lang.String)' on a null object reference
        at com.owncloud.android.ui.activity.FileDisplayActivity.getFileDiscovered(FileDisplayActivity.kt:1676)
        at com.owncloud.android.ui.activity.FileDisplayActivity.refreshListOfFilesFragment(FileDisplayActivity.kt:428)
        at com.owncloud.android.ui.activity.FileDisplayActivity.onResume(FileDisplayActivity.kt:668)
        at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1269)
        at android.app.Activity.performResume(Activity.java:6786)

Expected: error message or similar

Pixel5 Android12, Nexus6P Android7
08b091e6

@fesave
Copy link
Contributor Author

fesave commented May 3, 2022

(6)

  1. Install the app, no accounts added
  2. Click on any deeplink openable by the app

Current: crash happens. This is the stacktrace:

2022-04-29 09:40:12.377 31539-31539/? E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.owncloud.android.debug, PID: 31539
    java.lang.RuntimeException: Unable to resume activity {com.owncloud.android.debug/com.owncloud.android.ui.activity.FileDisplayActivity}: java.lang.NullPointerException: Attempt to invoke virtual method 'com.owncloud.android.datamodel.OCFile com.owncloud.android.datamodel.FileDataStorageManager.getFileByPrivateLink(java.lang.String)' on a null object reference
        at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3430)
        at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3470)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2733)
        at android.app.ActivityThread.-wrap12(ActivityThread.java)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1478)
        at android.os.Handler.dispatchMessage(Handler.java:102)
        at android.os.Looper.loop(Looper.java:154)
        at android.app.ActivityThread.main(ActivityThread.java:6121)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:889)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:779)
     Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'com.owncloud.android.datamodel.OCFile com.owncloud.android.datamodel.FileDataStorageManager.getFileByPrivateLink(java.lang.String)' on a null object reference
        at com.owncloud.android.ui.activity.FileDisplayActivity.getFileDiscovered(FileDisplayActivity.kt:1676)
        at com.owncloud.android.ui.activity.FileDisplayActivity.refreshListOfFilesFragment(FileDisplayActivity.kt:428)
        at com.owncloud.android.ui.activity.FileDisplayActivity.onResume(FileDisplayActivity.kt:668)
        at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1269)
        at android.app.Activity.performResume(Activity.java:6786)

Expected: error message or similar

Pixel5 Android12, Nexus6P Android7 08b091e6

Fixed

@fesave fesave force-pushed the feature/deep_links branch from abbab05 to 696c71d Compare May 3, 2022 07:54
@jesmrec
Copy link
Collaborator

jesmrec commented May 3, 2022

Approved.

At this point:

  • Deeplinks opened only over items that are already discovered
  • Deeplinks opened only in single account mode
  • Android12: servers with name and URL must be branded and validated in device's Settings (not valid a * in setup.xml)
  • If server URL contains some extra chunk (subdomain or similar), needs to be branded as well
  • Clicking on thumbnail inside Details will download and open the pointed file/folder

Known restriction:

After opening an item, navigation is not totally smooth. Browsing back from Details or the preview itself goes directly to root folder, not matter the location of the opened item (it could be placed very deep in the folder structure). This is a tricky one, that we expect to mitigate in the new architecture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Open deeplinks in app, items already discovered (1st round)
4 participants