thunder-app / thunder

Thunder - An open-source cross-platform Lemmy client for iOS and Android built with Flutter
https://thunderapp.dev
GNU Affero General Public License v3.0
732 stars 62 forks source link

Add support for opening lemmy links #832

Closed ggichure closed 7 months ago

ggichure commented 7 months ago

Pull Request Description

Add support for opening Lemmy links in app using uni_links

Android support for now.

Issue Being Fixed

Issue Number: #60

Screenshots / Recordings

Image 1 Image 2 Image 3

Checklist

micahmo commented 7 months ago

This is awesome, thanks for working on this!!

I know it's still a draft, so I didn't officially review, but I wanted to jot down some thoughts before I forgot. Note that I'm not asking for you to do any of these things. Just notes for reference. 😊

Again, super awesome, and please don't take any comments as criticism or asking for extra work. 😊

ggichure commented 7 months ago

Any reason for picking uni_links over app_links?

I have in the past used uni_links

We should have graceful handling of any non-post link (I'm assuming we're only supporting posts to start).

Correct, only posts for now. Although, I'll look into the comments and user profiles too.

Does the user need to manually enable link handling for each supported domain in the app's Android settings? (I believe that's required unless an app publisher can verify that it owns a domain.) If so, we should document that process.

Given that we are using deep links we don't have to verify we own the domains. Deep links are general links that point to specific content within an app.

App links on the other hand are designed to verify and ensure that links are opened within the designated app rather than through a web browser, if the app is installed.

Again, super awesome, and please don't take any comments as criticism or asking for extra work.

I thoroughly enjoy working on Thunder, and I welcome constructive criticism.

micahmo commented 7 months ago

Does the user need to manually enable link handling for each supported domain in the app's Android settings? (I believe that's required unless an app publisher can verify that it owns a domain.) If so, we should document that process.

Given that we are using deep links we don't have to verify we own the domains. Deep links are general links that point to specific content within an app.

App links on the other hand are designed to verify and ensure that links are opened within the designated app rather than through a web browser, if the app is installed.

I understand that we don't need to (and can't) verify all the domains. However, I believe we still need to instruct users to enable link handling for all of those domains in the Android settings. For example, if I grab and run your branch, Thunder does not automatically handle those domains. I have to enable them, like this.

https://github.com/thunder-app/thunder/assets/7417301/18a21e2d-4640-4ead-b62f-f24f63a2c787

This is what I mean when I say we should document the process to enable link handling (unless I am misunderstanding something!).

I thoroughly enjoy working on Thunder, and I welcome constructive criticism.

Me too. 😊 Thanks for all the help!

ggichure commented 7 months ago

Does the user need to manually enable link handling for each supported domain in the app's Android settings? (I believe that's required unless an app publisher can verify that it owns a domain.) If so, we should document that process.

Given that we are using deep links we don't have to verify we own the domains. Deep links are general links that point to specific content within an app. App links on the other hand are designed to verify and ensure that links are opened within the designated app rather than through a web browser, if the app is installed.

I understand that we don't need to (and can't) verify all the domains. However, I believe we still need to instruct users to enable link handling for all of those domains in the Android settings. For example, if I grab and run your branch, Thunder does not automatically handle those domains. I have to enable them, like this. qemu-system-x86_64_ow54t3xYYh.mp4

This is what I mean when I say we should document the process to enable link handling (unless I am misunderstanding something!).

I thoroughly enjoy working on Thunder, and I welcome constructive criticism.

Me too. 😊 Thanks for all the help!

Yeah, we need to add a guide.

micahmo commented 7 months ago

Hey again! I opened PR #834 to help with auto-generating the supported domain list in the Android manifest file via the GitHub workflows. It assumes that the file already contains...

...
<intent-filter>
  <action android:name="android.intent.action.VIEW" />
  <category android:name="android.intent.category.DEFAULT" />
  <category android:name="android.intent.category.BROWSABLE" />
  <data android:scheme="https"/>
   <!-- TODO(lemmy_server_domain_finder): Setup a script to update available lemmy domains  -->
  <!-- Obtained from github.com/dessalines/jerboa/blob/main/app/src/main/AndroidManifest.xml-->
  <!--#AUTO_GEN_INSTANCE_LIST_DO_NOT_TOUCH#-->
  ...
  <data android:host="WHATEVER" />
  ...
  <!--#INSTANCE_LIST_END#-->
</intent-filter>
...

We can either...

I have no preference. :)

ggichure commented 7 months ago

Hey again! I opened PR #834 to help with auto-generating the supported domain list in the Android manifest file via the GitHub workflows. It assumes that the file already contains...

...
<intent-filter>
  <action android:name="android.intent.action.VIEW" />
  <category android:name="android.intent.category.DEFAULT" />
  <category android:name="android.intent.category.BROWSABLE" />
  <data android:scheme="https"/>
   <!-- TODO(lemmy_server_domain_finder): Setup a script to update available lemmy domains  -->
  <!-- Obtained from github.com/dessalines/jerboa/blob/main/app/src/main/AndroidManifest.xml-->
  <!--#AUTO_GEN_INSTANCE_LIST_DO_NOT_TOUCH#-->
  ...
  <data android:host="WHATEVER" />
  ...
  <!--#INSTANCE_LIST_END#-->
</intent-filter>
...

We can either...

* Copy my changes and incorporate them into your PR

* Do my PR separately from your changes

I have no preference. :)

Your PR can go in separately. Thanks for this.

ggichure commented 7 months ago

@micahmo @hjiangsu

I have a commentId , navigateToPost requires at least a postId or PostViewMedia , any insights on how to navigate user to PostPage just using commentId?

micahmo commented 7 months ago

I have a commentId , navigateToPost requires at least a postId or PostViewMedia , any insights on how to navigate user to PostPage just using commentId?

I had intended to do a followup to #818 by refactoring a navigateToComment method and using it for in-app navigation as well (you can see my TODO here). Let me see if I can knock that out for you!

EDIT: #835 adds a navigateToComment method. 😊


P.S. I was thinking you could use the instance page from #824 if they navigate to the root domain and maybe also if they navigate to an unsupported link.

hjiangsu commented 7 months ago

This is pretty awesome, thanks again for initiating work on this! I don't have too much experience with integrating deep links but I'll see what I can do on the iOS side of things to make it work as well.

This is what I mean when I say we should document the process to enable link handling (unless I am misunderstanding something!).

I wonder if we can make it easier by having a button which opens up the app settings page for the user to be able to select the applicable domains? I'm not sure if this is possible but it would be a nice QoL feature!

ggichure commented 7 months ago

I wonder if we can make it easier by having a button which opens up the app settings page for the user to be able to select the applicable domains? I'm not sure if this is possible but it would be a nice QoL feature!

Using permission_handler you can open app settings.

micahmo commented 7 months ago

I don't have too much experience with integrating deep links but I'll see what I can do on the iOS side of things

Hopefully it should just involve creating/editing an entitlements file, and the code should be the same for both platforms.

I wonder if we can make it easier by having a button which opens up the app settings page for the user to be able to select the applicable domains?

That's a great idea, and I'm pretty sure it's possible on Android!

ggichure commented 7 months ago

Open settings done. open_app_settings

ggichure commented 7 months ago

@micahmo
getLemmyCommentId ResolveObject doesn't "always" work . instance.dart#L114

Default instance being used lemmy.ml On lemmy.world it works. https://lemmy.world/comment/3078559

 final ResolveObjectResponse resolveObjectResponse = await lemmy.run(ResolveObject(q: text));

LemmyApiException: couldnt_find_object

micahmo commented 7 months ago

Open settings done.

That's awesome! Is there any way to get it to the specific settings page for links? Maybe not with permission_manager but maybe with app_settings? Also do you want to add an icon to that setting so that it aligns with the other ones?

getLemmyCommentId ResolveObject doesn't "always" work .

It won't be able to resolve an object that is not federated to the current instance. I'm not sure what we can do about that. Maybe just show a message? We could still open it in the browser in the app I suppose.

ggichure commented 7 months ago

app_settings seems like it will do the job.

Edit: app_settings doesn't launch specific settings page for links.

Maybe just show a message? We could still open it in the browser in the app I suppose.

Sure.

Also do you want to add an icon to that setting so that it aligns with the other ones?

Sure

hjiangsu commented 7 months ago

Hopefully it should just involve creating/editing an entitlements file, and the code should be the same for both platforms.

So I've played around a little bit with this yesterday, and unfortunately, there doesn't seem to be a good way to handle this on iOS (that I know of so far).

Using Universal Links means that we would need to verify the domains we add to the entitlements/info.plist files. However, since we don't directly own those domains, thats not a possibility.

We can use Custom URL schemes, but it doesn't work entirely the same as it does on Android. What happens here is that we need to add any unique custom url schemes that we want to associate with our app which also does not clash with another installed app (e.g., thunder://). I tried to test out using https://lemmy.world, but it seems to open in Safari because the http/https schemes are handled by Safari by default and there doesn't seem to be a way to override that from what I can tell.

What would need to happen then in this case, is that we would need to have users replace the https:// portion with thunder:// for it to then redirect the user to the app

micahmo commented 7 months ago

Using Universal Links means that we would need to verify the domains we add to the entitlements/info.plist files.

Ah man, that's too bad. I was hoping it would work like Android where the domains are simply disabled by default. But it makes sense from a security perspective why Apple would be a little more restrictive about this. Oh well!

ggichure commented 7 months ago

Also it still just opens to the main settings page (if you did not intend to change that in this PR, that's totally fine!!)

So far I haven't found a way to send the user to the page we need .

ggichure commented 7 months ago

The UI for the new setting still looks a bit funny, with the leading icon misaligned and the trailing very large. It also doesn't have the same stadium border and inkwell as the rest of the settings. Not sure if it's possible to reuse one of the existing setting widgets with a custom action.

Reused the ToggleOption to create a SettingsListTile. Screenshot 2023-10-19 at 11 29 28

ggichure commented 7 months ago

The main issue is that things don't seem to be working very well for me! I'm sure I'm doing something wrong, but here's what I'm seeing. This is with a post on the same instance that I'm logged in with, so shouldn't be a federation issue. (The same thing happens whether Thunder is already open or not.)

I'm unable to reproduce this. Also I'm considering increasing the snackbar duration on link exception. .

hjiangsu commented 7 months ago

Just did a quick code review and everything seems to be fine with me, nothing too much to point out aside from what was already discussed previously (thanks @micahmo for doing a code review as well!)

I unfortunately can't test this myself since I don't have a physical Android device, so I'll leave it up to @micahmo to approve of this if thats okay

hjiangsu commented 7 months ago

Sounds good! Just to confirm, @ggichure is this PR ready to go?

ggichure commented 7 months ago

Sounds good! Just to confirm, @ggichure is this PR ready to go?

Yes it Is.