feat: Adds navigationDelegate for handling links #15

Alexander-Ordina · 2022-09-27T13:44:32Z

Adds NavigationDelegate to the chatbox for handling links.

mrcnkoba · 2022-09-27T13:46:16Z

Thank you @Alexander-Ordina for your contribution to our plugin. We will review your PR and get it released.

eteeselink · 2022-09-29T08:57:19Z

Hi @Alexander-Ordina, first off, thanks for contributing this PR!

We're reviewing it, and while the code change is tiny, it may impact architecture changes we got lined up in the near future, so we'll need some time to evaluate it. (note that we'll also need a signed CLA, even for such tiny code changes unfortunately, as discussed in the other PR you made)

Can you confirm that for your own project, you're unblocked since you can use your forked version of the SDK for the time being, until we merge this PR?

Alexander-Ordina · 2022-10-18T12:41:13Z

Hi @eteeselink, I just got confirmation that the CLA is signed and send to your legal department. I'm guessing the architecture change you are talking about is the switch to InAppWebView, to get the same behavior with InAppWebView the shouldOverrideUrlLoading property can be used.

We do not plan to use the fork I made, the fork was only to make is possible to create a Pull Request, so until this is solved we are still blocked for rolling out Talkjs in our App.

eteeselink · 2022-10-19T08:00:25Z

Hi Alexander,

Fantastic! For our understanding: we recently open-sourced the flutter SDK specificallly to make interactions like these possible, so we're very happy with your contributions. Sorry about our delay in addressing them.

That said, another goal we had with open-sourcing the SDK was to unblock customers, ie not keep customers blocked on our development speed for things inside the SDK, if something is missing. I see that you're nevertheless unable to temporarily use your fork until we worked things out, which surprised me. Would you be able to shed some light on the dynamics on your side that prevent you from going live now using your fork, and switching over to our official SDK once we merged your PR? (or addressed the problem it solves in an alternative way)

bugnano · 2022-10-19T08:24:09Z

Hi @eteeselink, I just got confirmation that the CLA is signed and send to your legal department. I'm guessing the architecture change you are talking about is the switch to InAppWebView, to get the same behavior with InAppWebView the shouldOverrideUrlLoading property can be used.

You're totally right about the architecture change being the switch to InAppWebView.
Thank you for pointing us at the shouldOverrideUrlLoading property, and as you can see its interface is slightly different than the NavigationDelegate.
Given that we would like to be able to switch implementation any time we want, can you come up with a solution that exposes the properties that you need in a WebView-implementation independent way for this PR?

Alexander-Ordina · 2022-10-20T15:25:29Z

@bugnano I have added a onUrlNavigation function on the chatbox widget, this property can be used for adding the navigation logic independent of which webview is used. It is still needed to have a webview dependent implementation to translate this function for the type of webview used, for this I added the private function _shouldNavigateToUrl which is used as the NavigationDelegate for the flutter webview. When switching to the InAppWebview this function will have to be rewritten to work with it.

Alexander-Ordina · 2022-10-20T15:43:09Z

Hi @eteeselink, I understand that it might be surprising that we do not want to use the temporary fork. We want to prevent creating unnecessary forks which we have to maintain, we only do this if there is no other option. We try to make sure that we have plenty of time to solve issues with plugins before we release new features either by filing issues or creating pull requests.

We expect that these issues can probably be solved before we go live with chat feature in our app, to make sure that is the case we would like to know when there is a new release for TalksJs plugin planned?

bugnano

Good work, I like it, but before merging this PR, I'd like to have the UrlNavigationRequest class to work with Dart's Uri regarding the url parameter, and to be explicit about the type.

bugnano · 2022-10-24T06:17:15Z

lib/src/chatbox.dart

+enum UrlNavigationAction { allow, deny }
+
+class UrlNavigationRequest {
+  final url;


It would be better to explicitly state the type of the variable here (which should be preferably Dart's Uri)

I Updated the UrlNavigationRequest it now had 3 properties:

final Uri? url; final String rawUrl; final bool isValidUrl;

This is to make sure that if the url is not a valid uri that it still is possible to pass the UrlNavigationRequest without throwing exceptions.

I'm also thinking that it might be better to just pass the url as a String, this way any error handling or conversion to Uri Objects can be handled by the specific app using the plugin.
What do you feel is best?

OK, so in this case let's use String for the url, and the validation is done by the user's code.
I thought that the Uri type could be the more appropriate one, but given that there could be some cases where the url is not a valid Uri, then my previous assumption was wrong.

bugnano

Thank you for the update, we're close to merging this PR, but I still ask you to do one last change.

Given that the policy at TalkJS has been to support APIs forever, and never break compatibility, we can only achieve this if we expose the bare minimum, and we expose the correct types in the first place.

bugnano · 2022-10-27T12:13:21Z

lib/src/chatbox.dart

+
+class UrlNavigationRequest {
+  final Uri? url;
+  final String rawUrl;


I would not expose the variables rawUrl and isValidUrl, because everything that's public needs to be supported forever, so the less we expose, the better.

The problem is that the Uri.tryParse() method returns Null for an invalid url. If the rawUrl is not exposed there is no way for the app that uses plugin to figure out what the original Url was for the link.

It could be possible that a link is send with an invalid url, but in the app we still want to do something with this link. For example:

We want to show an errormessage containing the url

We could have a special url that opens a page in the app, maybe a url like: app://about would navigate to the app's about page. This is not a valid uri, but could be wanted behavior in a App.

I think it would best to make the type for url a String this way the plugin stays simple and the rawUrl and isValidUrl can be removed.

class UrlNavigationRequest { final String url; ...

Yes, let's keep things simple and use String.

I updated the code, I also made the String Nullable to make sure it will work with any future implementation of the webview

bugnano · 2022-10-27T12:18:23Z

lib/src/chatbox.dart

+    this.isValidUrl,
+  ) ;
+
+  factory UrlNavigationRequest(String url){


We should not do the String to Uri conversion in a public API.
A simple constructor with an Uri parameter is the best here.

The whole String to Uri conversion is better to be done in the private _shouldNavigateToUrl function.

… will work with different implementations for the webview

bugnano

Thank you for the changes, I just have one question:
I appreciate very much that you got into the mentality of implementation-independence, but I don't get how a nullable string can achieve that.
Think of the user of the API:
If the url is nullable, then I expect to have an UrlNavigationRequest with a null url.
What does that mean to me, as a user?

Or, to put it another way: we decided that the url is of type String.
This means that whenever we change the implementation, we will convert whatever url type we have to String, I don't get how null can be helpful in this situation.

bugnano · 2022-10-31T06:20:14Z

lib/src/chatbox.dart

+enum UrlNavigationAction { allow, deny }
+
+class UrlNavigationRequest {
+  final String? url;


What's the practical use of an UrlNavigationRequest with a null url?

InAppWebView sometimes returns null for the url so it will not always be a String the result could be null.
I did a little more research on how InAppWebview works with urls and they do use a Uri object this means they parse the Url String to a Uri in their implementation.

This means that invalid urls won't work with there implementation, so creating a special url is only an option if it is also a valid Uri.

More information can be found here: https://inappwebview.dev/docs/webview/in-app-webview/#handle-custom-urls

I think this invalidates my previous comment about why it should be a String looking at this I do feel making it a Nullable Uri should be better. And custom urls can be handled like described in the link above.

What do you think would be best?

If you want we could also setup a meeting to discuss an maybe get this issue resolved a bit quicker?

I believe that a non-nullable string is the best solution here, and here's why:
While not all Strings can be valid Uris, thus resulting in a null Uri, the same is not true for Uris, which are all valid Strings, so it's better to have a non-nullable String, and then let the user decide what to do with that string.

As I see from the documentation of the InAppWebView, if an Uri is invalid, the shouldOverrideUrlLoading method is not called anyway, so it's not a problem.
And the fact that the Uri can be null in the NavigationAction.URLRequest does not affect this functionality, as I'm sure that shouldOverrideUrlLoading will not be called with a null Uri (if you read the docs, a null Uri is equivalent to "about:blank").

Good point, I've now changed it to a non nullable String

zSoNz · 2023-03-01T10:36:09Z

@eteeselink , hey! Seems like in last version you forgot add this functionality :(

vickz84259 · 2023-03-02T11:34:42Z

Hey @zSoNz, I'm a developer at TalkJS. Regarding the feature above, it was an intentional decision to not include it. For versions 0.6.0 and lower of our SDK, links inside the chat would open inside the webview rather than in an external browser. This was the initial motivation for implementing navigationDelegate as highlighted in this issue. This behaviour has been changed since version 0.7.0.

Was there another use case you intended to use navigationDelegate for?

feat: Adds navigationDelegate for handling links

332fa4a

Alexander-Ordina added 2 commits October 20, 2022 16:38

feat: Add custom NavigationHandler

9d73a3e

chore: rename argument for NavigationHandler

5fa4e98

bugnano reviewed Oct 25, 2022

View reviewed changes

feature: Add UrlNavigationRequest url type and checks for url validation

edaafb6

bugnano reviewed Oct 27, 2022

View reviewed changes

Alexander-Ordina added 2 commits October 27, 2022 21:31

feature: change url type to String

ee51f7d

chore: Change url property String to nullable String? to make sure it…

ab08a46

… will work with different implementations for the webview

bugnano reviewed Oct 31, 2022

View reviewed changes

change String? to String for UrlRequest

3f454b0

bugnano merged commit 9dd9e3f into talkjs:main Nov 7, 2022

bugnano mentioned this pull request Apr 12, 2023

Re-enabled the onUrlNavigation callback on the ChatBox #30

Merged

feat: Adds navigationDelegate for handling links #15

feat: Adds navigationDelegate for handling links #15

Uh oh!

Conversation

Alexander-Ordina commented Sep 27, 2022

Uh oh!

mrcnkoba commented Sep 27, 2022

Uh oh!

eteeselink commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alexander-Ordina commented Oct 18, 2022

Uh oh!

eteeselink commented Oct 19, 2022

Uh oh!

bugnano commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alexander-Ordina commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alexander-Ordina commented Oct 20, 2022

Uh oh!

bugnano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bugnano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bugnano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zSoNz commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vickz84259 commented Mar 2, 2023

Uh oh!

Uh oh!

eteeselink commented Sep 29, 2022 •

edited

Loading

bugnano commented Oct 19, 2022 •

edited

Loading

Alexander-Ordina commented Oct 20, 2022 •

edited

Loading

zSoNz commented Mar 1, 2023 •

edited

Loading