View links to statuses inside Tusky #568

Tak · Apr 10, 2018

When opening a link, check whether it's a link to a Mastodon status, and if so, open it in Tusky's thread view instead of the external browser.

Hi! I understand your desire but... web UI doesn't do it, right? They probably have a reason. And the reason is that we don't know if a link leads to a status or not. We can ask server to resolve the link but we cannot stop everything until it happens, right?

charlag · Apr 10, 2018

Hi! I understand your desire but... web UI doesn't do it, right? They probably have a reason. And the reason is that we don't know if a link leads to a status or not. We can ask server to resolve the link but we cannot stop everything until it happens, right?

The web ui doesn't do it, but it's not so jarring to have a browser tab created as it is to be shoved into a completely different app. Additionally, having the thread opened in Tusky allows you to interact with the content (favorite/follow/block) like normal.

The link resolution is completely asynchronous, if that's the concern.

Tak · Apr 10, 2018

The web ui doesn't do it, but it's not so jarring to have a browser tab created as it is to be shoved into a completely different app. Additionally, having the thread opened in Tusky allows you to interact with the content (favorite/follow/block) like normal.

The link resolution is completely asynchronous, if that's the concern.

First of all, my apologies: I somehow missed that this is a PR and not just an issue.
I am yet to try it out but my concern is the same: we make user wait for the server search before opening a link. We're sacrificing experience of the most common case for the uncommon.
I believe that if this thing is to be done it should be done on the server side or prior to interacting with a link

charlag · Apr 10, 2018

First of all, my apologies: I somehow missed that this is a PR and not just an issue.
I am yet to try it out but my concern is the same: we make user wait for the server search before opening a link. We're sacrificing experience of the most common case for the uncommon.
I believe that if this thing is to be done it should be done on the server side or prior to interacting with a link

I don't feel that the current implementation detracts from the user experience. However, we could put it behind a preference if you think it would be contentious...

Tak · Apr 10, 2018

I don't feel that the current implementation detracts from the user experience. However, we could put it behind a preference if you think it would be contentious...

I would ask for the @connyduck input here but he seems to be busy now so I cannot guarantee fast responses 😅

charlag · Apr 10, 2018

I would ask for the @connyduck input here but he seems to be busy now so I cannot guarantee fast responses 😅

We could also try a heuristic on the url, and only search urls that look like they could be mastodon statuses

Tak · Apr 10, 2018

We could also try a heuristic on the url, and only search urls that look like they could be mastodon statuses

I think we cant justify a network request for every click on a link :/
What about the Mastalab way: offering to open all links having an @ as the first character of the path, e.g. https://domain.xy/@test

see here: https://github.com/stom79/mastalab/blob/41d65e91206839f597b816211250a4b13241640b/app/src/main/AndroidManifest.xml#L96

This would have the advantage of making all Mastodon links openable in the app (even from Browser or other apps) & not needing an network request but the drawback of maybe opening non-mastodon links.

connyduck · Apr 10, 2018

I think we cant justify a network request for every click on a link :/
What about the Mastalab way: offering to open all links having an @ as the first character of the path, e.g. https://domain.xy/@test

see here: https://github.com/stom79/mastalab/blob/41d65e91206839f597b816211250a4b13241640b/app/src/main/AndroidManifest.xml#L96

This would have the advantage of making all Mastodon links openable in the app (even from Browser or other apps) & not needing an network request but the drawback of maybe opening non-mastodon links.

How about this: we do the search, but only on "probably mastodon" links.
(FWIW, I think pleroma uses a different layout without the @ user chunk in the path, but the check could be relaxed later if desirable.)

If we added the path prefix for the url in the manifest, we would still have to perform the search for all statuses not originating in the current instance, and we would get a bunch of external false positives (e.g. medium.com often has paths that begin with /@contributor)

Tak · Apr 11, 2018

How about this: we do the search, but only on "probably mastodon" links.
(FWIW, I think pleroma uses a different layout without the @ user chunk in the path, but the check could be relaxed later if desirable.)

If we added the path prefix for the url in the manifest, we would still have to perform the search for all statuses not originating in the current instance, and we would get a bunch of external false positives (e.g. medium.com often has paths that begin with /@contributor)

This seems really reasonable 🤔
I am now focused on getting Tusky 1.6 out, but I definitely want this feature in 1.7.

connyduck · Apr 11, 2018

This seems really reasonable 🤔
I am now focused on getting Tusky 1.6 out, but I definitely want this feature in 1.7.

We should consider release branches? Good for hotfixds too

charlag · Apr 11, 2018

We should consider release branches? Good for hotfixds too

Yes I will make one for 1.6, good point!

connyduck · Apr 11, 2018

Yes I will make one for 1.6, good point!

Ok, as I stated, I really like this feature, but we need to get this right before we ship it.
There are several problems with the implementation in this PR:

Users expect an immeditate reaction to a click on a link. But when they have a slow internet connection, nothing happens for a while. Users could click other links in the meantime and then suddenly TWO links open. This is very bad user experience.
In case Users have no internet connection, the link opens in the browser. But if they press back, they see an empty ViewThreadActivity. Very awkward.
We should not only handle links to threads but also to user profiles.
I think the check if a link looks like a Mastodon one should not be made in LinkHelper.setClickableText but when a link is actually clicked (better perf)

One possible solution could be: Upon clicking a link, that might be a status/profile, open a bottom sheet with text "opening link" and/or a progress bar. Bottom sheets are non intrusive (not like dialogs), meaning the user sees something is going on, but can still use the rest of the ui. Once we resolved the status, we can open it or the browser. If the user clicked something else in the meantime, we need to cancel the resolving. I am not sure if thats the ideal solution though.

connyduck · Apr 16, 2018

Ok, as I stated, I really like this feature, but we need to get this right before we ship it.
There are several problems with the implementation in this PR:

Users expect an immeditate reaction to a click on a link. But when they have a slow internet connection, nothing happens for a while. Users could click other links in the meantime and then suddenly TWO links open. This is very bad user experience.
In case Users have no internet connection, the link opens in the browser. But if they press back, they see an empty ViewThreadActivity. Very awkward.
We should not only handle links to threads but also to user profiles.
I think the check if a link looks like a Mastodon one should not be made in LinkHelper.setClickableText but when a link is actually clicked (better perf)

One possible solution could be: Upon clicking a link, that might be a status/profile, open a bottom sheet with text "opening link" and/or a progress bar. Bottom sheets are non intrusive (not like dialogs), meaning the user sees something is going on, but can still use the rest of the ui. Once we resolved the status, we can open it or the browser. If the user clicked something else in the meantime, we need to cancel the resolving. I am not sure if thats the ideal solution though.

Ok. I previously investigated a bit around popping up a busy spinner, but didn't come up with anything that seemed workable (I'm not a hardcore android ui developer 😉 ).
I'll look into addressing these.

Tak · Apr 16, 2018

Ok. I previously investigated a bit around popping up a busy spinner, but didn't come up with anything that seemed workable (I'm not a hardcore android ui developer 😉 ).
I'll look into addressing these.

Moved the "could be a mastodon status" check to the click handler
Added a bottom sheet with a status message, search is canceled when it's dismissed
Suppressed opening the thread view activity while searching

Tak · Apr 19, 2018

Moved the "could be a mastodon status" check to the click handler
Added a bottom sheet with a status message, search is canceled when it's dismissed
Suppressed opening the thread view activity while searching

charlag · Apr 19, 2018

This seems really hacky to me. Just fire a listener and let it decide if it wants to act upon it or not. You don't want your ViewHolder to know in which activity it is placed.

Hm, I'll see if I can propagate this out to the views.

Thank you for your changes, it looks much better now
The only think I still don't understand: why it's activity which is responsible for sheet/searching and not a fragment? If I understand correctly, this functionality only work in SFragments but because you delegate it to activity you had to add this sheet to every activity. Maybe I'm missing some reason for this but to me it seems strange that something which manages timeline things and can be encapsulated has to delegate some specific function to its container.
I would rather add the sheet one time in SFragment and kept it's logic there.

I may be missing something so I would love to hear your opinion on this.

charlag · Apr 20, 2018

Thank you for your changes, it looks much better now
The only think I still don't understand: why it's activity which is responsible for sheet/searching and not a fragment? If I understand correctly, this functionality only work in SFragments but because you delegate it to activity you had to add this sheet to every activity. Maybe I'm missing some reason for this but to me it seems strange that something which manages timeline things and can be encapsulated has to delegate some specific function to its container.
I would rather add the sheet one time in SFragment and kept it's logic there.

I may be missing something so I would love to hear your opinion on this.

charlag · Apr 22, 2018

charlag reviewed Apr 22, 2018

View changes

Okay, this is really cool! Thank you for listening and making changes! It is much, much better code now.

Can I just ask if there's a way to override the search while it's in progress and just open the link? Like a button on the sheet.

I'm really asking for everyone's opinion here (ping @connyduck ). Am I over-thinking? I just think that it may be annoying with poor connectivity to wait for the server round-trip when you want to open the link.

Please don't have me for my bragging!
❤️

I think it could be more efficient to smash them all into one regex but I won't be able to do such wizardry nor I do I ask you, just saying

I could make them a single regex, but I thought this was more readable/debuggable. The time taken to match against all the regexes in the worst case is still tiny compared to the actual network request (either for the search or for the browser).

Yeah, you're right.

Why RelativeLayout here?
I would either:

Make it a FrameLayout
Make it a ConstraintLayout
Make the CoordinatorLayout a parent and put SwipeRefreshLayout inside alongside that include (probably the best choice)

Why RelativeLayout here?
I would either:

Make it a FrameLayout
Make it a ConstraintLayout
Make the CoordinatorLayout a parent and put SwipeRefreshLayout inside alongside that include (probably the best choice)

I guess because I don't have strong feelings about it - I can adapt it to whichever of the above seems best.

I'm kinda late for this but why get prefix? Kotlin is the only reason I can think of but it's private anyway

Good point, this naming is just left over from earlier iterations.

Why do we need this overridden?

Good point, leftover

And this one?

And this one

The same thing here with layout

This sounds a little tech-ish too me, maybe something more friendly like "looking it up?" Not sure what is easier to translate too.
Somebody ask people please, I'm not good at this
Also there's a symbol for ellipsys: … :3

I have no strong feelings; it can be changed to whatever seems best.
(We're not using the ellipsis character in "Sending Toot..." 😉 )

Awesome, thank you!
I have no more complains about the code, I guess 😅

charlag · Apr 24, 2018

Awesome, thank you!
I have no more complains about the code, I guess 😅

Ok I just tried it out and it is perfect 👌
We may need to do some tuning to the message, and maybe it makes sense to delay the bottom sheet a bit so it doesnt show up on fast connections, but lets wait for user feedback.

Mergy-merge?

connyduck · Apr 24, 2018

Ok I just tried it out and it is perfect 👌
We may need to do some tuning to the message, and maybe it makes sense to delay the bottom sheet a bit so it doesnt show up on fast connections, but lets wait for user feedback.

Mergy-merge?

I still want button on the sheet to stop search but I think we can add it later in another PR.
Would be cool to make a build with this and collect some feedback, huh?
Let's merge!

charlag · Apr 24, 2018

I still want button on the sheet to stop search but I think we can add it later in another PR.
Would be cool to make a build with this and collect some feedback, huh?
Let's merge!

Lets see if we get any feedback: https://mastodon.social/@ConnyDuck/99916304078357678

connyduck · Apr 24, 2018

Lets see if we get any feedback: https://mastodon.social/@ConnyDuck/99916304078357678

It does stop the search if you swipe the sheet away...maybe that's not obvious UX?

Tak · Apr 24, 2018

It does stop the search if you swipe the sheet away...maybe that's not obvious UX?

But does it open the status after swiping the sheet away or it will abort the whole process? Cannot really test it

charlag · Apr 24, 2018

But does it open the status after swiping the sheet away or it will abort the whole process? Cannot really test it

It cancels the search and does not open the status.

Tak · Apr 25, 2018

It cancels the search and does not open the status.

View links to statuses inside Tusky

bed7b81

Only attempt to open links that look like mastodon statuses

12f9e99

connyduck added this to the Tusky 1.7 milestone Apr 11, 2018

Add support for pleroma statuses

39f10f3

Tak added some commits Apr 18, 2018

Merge remote-tracking branch 'upstream/master' into open-toot-links

4bd187b

Move "smells like mastodon" url check to click handler

f17777a

Add bottom sheet to notify users of post query status

f14e4b7

Improve architecture for managing search status

7d10d9e

Tak added some commits Apr 23, 2018

Apply PR feedback

a66ee33

Migrate bottom sheet wrappers to CoordinatorLayout

b266d7f

connyduck merged commit 76eae44 into tuskyapp:master Apr 25, 2018
1 check passed

1 check passed

ci/bitrise/55b2f0c77c4bba74/pr Passed - Tusky
Details

@@ -1,13 +1,22 @@
          <?xml version="1.0" encoding="utf-8"?>
-        -<android.support.v4.widget.SwipeRefreshLayout
+        +<RelativeLayout

@@ -379,6 +376,11 @@ public void onViewThread(int position) {
                  super.viewThread(notification.getStatus());
              }
+        +    @Override
+        +    public void onViewURL(String url) {

@@ -455,6 +453,11 @@ public void onViewThread(int position) {
                  super.viewThread(statuses.get(position).getAsRight());
              }
+        +    @Override
+        +    public void onViewURL(String url) {

@@ -260,6 +257,11 @@ public void onViewThread(int position) {
                  super.viewThread(status);
              }
+        +    @Override
+        +    public void onViewURL(String url) {

-        -    android:id="@+id/swipe_refresh_layout"
-        -    android:layout_width="match_parent"
-        -    android:layout_height="match_parent">
+        +<RelativeLayout

tuskyapp/Tusky

Join GitHub today

View links to statuses inside Tusky #568

Conversation

Tak commented Apr 10, 2018

This comment has been minimized.

charlag commented Apr 10, 2018

This comment has been minimized.

Tak commented Apr 10, 2018

This comment has been minimized.

charlag commented Apr 10, 2018

This comment has been minimized.

Tak commented Apr 10, 2018

This comment has been minimized.

charlag commented Apr 10, 2018

This comment has been minimized.

Tak commented Apr 10, 2018

This comment has been minimized.

connyduck commented Apr 10, 2018

This comment has been minimized.

Tak commented Apr 11, 2018

This comment has been minimized.

connyduck commented Apr 11, 2018 • edited Edited 1 time connyduck edited Apr 11, 2018 (most recent)

This comment has been minimized.

charlag commented Apr 11, 2018

This comment has been minimized.

connyduck commented Apr 11, 2018

connyduck added this to the Tusky 1.7 milestone Apr 11, 2018

This comment has been minimized.

connyduck commented Apr 16, 2018

This comment has been minimized.

Tak commented Apr 16, 2018 • edited Edited 1 time Tak edited Apr 16, 2018 (most recent)

Tak added some commits Apr 18, 2018

This comment has been minimized.

Tak commented Apr 19, 2018

This comment has been minimized.

charlag commented on app/src/main/java/com/keylesspalace/tusky/adapter/StatusBaseViewHolder.java in f14e4b7 Apr 19, 2018

This comment has been minimized.

Tak replied Apr 20, 2018

This comment has been minimized.

charlag commented Apr 20, 2018

Tak added some commits Apr 22, 2018

charlag reviewed Apr 22, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Tak added some commits Apr 23, 2018

This comment has been minimized.

charlag commented Apr 24, 2018

This comment has been minimized.

connyduck commented Apr 24, 2018

This comment has been minimized.

charlag commented Apr 24, 2018

This comment has been minimized.

connyduck commented Apr 24, 2018

This comment has been minimized.

Tak commented Apr 24, 2018

This comment has been minimized.

charlag commented Apr 24, 2018

This comment has been minimized.

Tak commented Apr 25, 2018

Hide details View details connyduck merged commit 76eae44 into tuskyapp:master Apr 25, 2018 1 check passed

1 check passed

connyduck commented Apr 11, 2018 •

edited

Edited 1 time

connyduck edited Apr 11, 2018 (most recent)

Tak commented Apr 16, 2018 •

edited

Edited 1 time

Tak edited Apr 16, 2018 (most recent)

charlag commented on `app/src/main/java/com/keylesspalace/tusky/adapter/StatusBaseViewHolder.java` in `f14e4b7` Apr 19, 2018

charlag reviewed Apr 22, 2018

View changes

connyduck merged commit `76eae44` into tuskyapp:master Apr 25, 2018
1 check passed