Add "web+mastodon" protocol handler; prompt for selecting account when action is triggered by external application. #633

remi6397 · 2018-05-06T15:04:54Z

web+mastodon:// test case: https://jsfiddle.net/pvyoof6L/11/
For me it works with Chrome, but not with Firefox.
QS tile and external application Share functionalities will now prompt for account selection.

…icker

charlag

Thank you for doing this, it is a cool feature!
Tbh, I didn't build it yet but I have some questions.
My biggest concern is replies. You see, different servers have different IDs for the same statuses (or maybe the same IDs for different statuses). So when replying we risk to reply to wrong account/status.
I think we should either disallow such things when replying or handle it gracefully.
Maybe I've missed something but this PR doesn't handle this?

charlag · 2018-05-06T16:04:11Z

app/src/main/java/com/keylesspalace/tusky/adapter/AccountListAdapter.kt

+import com.pkmmte.view.CircularImageView
+import com.squareup.picasso.Picasso
+
+class AccountListAdapter(private val accountList: List<AccountEntity>, private val onAccountSelectedListener : OnAccountSelectedListener) : RecyclerView.Adapter<AccountListAdapter.AccountListViewHolder>() {


Please format it as per guidelines

charlag · 2018-05-06T16:07:52Z

app/src/main/java/com/keylesspalace/tusky/AccountActivity.java

@@ -22,6 +23,7 @@
 import android.graphics.Color;
 import android.graphics.drawable.Drawable;
 import android.os.Bundle;
+import android.os.StrictMode;


We don't need that ;)

charlag · 2018-05-06T16:09:42Z

app/src/main/java/com/keylesspalace/tusky/adapter/AccountListAdapter.kt

+        }
+    }
+
+    class AccountListViewHolder(val accountItem: RelativeLayout) : RecyclerView.ViewHolder(accountItem)


I think it doesn't care which layout it's passed, you can remove redundant info

charlag · 2018-05-06T16:10:51Z

app/src/main/java/com/keylesspalace/tusky/adapter/AccountListAdapter.kt

+    override fun onBindViewHolder(holder: AccountListViewHolder, position: Int) {
+        val account = accountList[position]
+
+        val avatar = holder.accountItem.findViewById<CircularImageView>(R.id.account_avatar)


I'm sorry, but that's not how ViewHolder pattern works. The whole point of holders is to avoid expensive view search on every item bind. Look it up, but in general you find views in ViewHolder constructor and just fill in data in onBindViewHolder

It always has the same elements anyway, so… ¯_(ツ)_/¯
Anyway, I'm changing this :Þ

Well, ofc they're the same, what's the point of traversing the view hierarchy every time then?

charlag · 2018-05-06T16:11:29Z

app/src/main/res/layout/account_selection_dialog.xml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="utf-8"?>
+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"


Could it be FrameLayout? Maybe just RecyclerView?

charlag · 2018-05-06T16:12:41Z

app/src/main/java/com/keylesspalace/tusky/ComposeActivity.java

+        if (inReplyToId == null) {
+            composeAvatar.setOnClickListener(v -> showAccountSwitchDialog(true));
+
+            if (!intent.hasExtra(ACCOUNT_PRESET) && !intent.hasExtra(SAVED_TOOT_UID_EXTRA) && (getCallingActivity() == null || !getCallingActivity().getClassName().equals(MainActivity.class.getName()))) {


Super long line 😅
If you are to to check calling activity, maybe you should check that the caller is not ComposeActivity?

charlag · 2018-05-06T16:20:04Z

app/src/main/java/com/keylesspalace/tusky/ComposeActivity.java

+
+        AlertDialog dialog = builder.create();
+        AccountListAdapter adapter = new AccountListAdapter(accountManager.getAllAccountsOrderedByActive(), account -> {
+            accountManager.setActiveAccount(account);


Maybe I'm missing the point, but why do we have to restart activity? What happens after you return from it - all other screens are still for the old account (or flags handle this)?
Maybe we can avoid doing setActiveAccount() by using InstanceSwitchAuthInterceptor? Need to fix search too in this case...
I would ask input of others on how we should do it.
Are you in the Matrix room?

charlag · 2018-05-06T17:39:11Z

Now holder if fine, thank you!

…icker

charlag

I think you're moving in the right direction, I'm not sure yet how you want to implement dialog but using pairs is okay imo - maybe if it becomes too long you need to import class names or make a typealias but it's details

charlag · 2018-05-07T18:59:34Z

app/src/main/res/layout/item_following_account.xml

@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="utf-8"?>
+<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"


Please use ConstraintLayout instead of RelativeLayout whenever you can

You certainly can deal with just one ConstraintLayout instead of all these containers, ask me if you need help

charlag · 2018-05-07T19:00:24Z

app/src/main/java/com/keylesspalace/tusky/adapter/FollowingAccountListAdapter.kt

+import com.pkmmte.view.CircularImageView
+import com.squareup.picasso.Picasso
+
+class FollowingAccountListAdapter(private val accountList: List<Pair<AccountEntity, FollowState>>,


Please format fields by guidelines afterwards

charlag · 2018-05-07T19:01:09Z

app/src/main/java/com/keylesspalace/tusky/adapter/FollowingAccountListAdapter.kt

+    }
+
+    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): FollowingAccountListViewHolder {
+        val view = LayoutInflater.from(parent.context).inflate(R.layout.item_following_account, parent, false) as RelativeLayout


Avoid mentioning concrete layout whenever you can. Holder doesn't care.

charlag · 2018-05-07T19:33:07Z

app/src/main/AndroidManifest.xml


+    <!-- for day/night mode -->


put it back ;)

charlag · 2018-05-07T19:34:49Z

app/src/main/java/com/keylesspalace/tusky/AuthorizeFollowActivity.kt

+    }
+
+    private fun loadAccounts() {
+        for (account in accountManager.getAllAccountsOrderedByActive()) {


excuse me, I don't understand what's happening here

charlag · 2018-05-07T19:38:49Z

app/src/main/java/com/keylesspalace/tusky/AuthorizeFollowActivity.kt

+            }
+        }
+
+        followingAccounts.map { if (it.first == account) Pair(account, followState) else it }


I think using to operator is more idiomatic than "Pair()"

charlag · 2018-05-07T19:39:39Z

app/src/main/java/com/keylesspalace/tusky/adapter/AccountListAdapter.kt

+import com.pkmmte.view.CircularImageView
+import com.squareup.picasso.Picasso
+
+class AccountListAdapter(private val accountList: List<AccountEntity>,


Please format fields by guidelines afterwards

charlag · 2018-05-07T19:44:51Z

app/src/main/java/com/keylesspalace/tusky/network/InstanceSwitchAuthInterceptor.java

@@ -49,8 +49,13 @@ public Response intercept(@NonNull Chain chain) throws IOException {

            Request.Builder builder = originalRequest.newBuilder();

+            String authorizationHeader = originalRequest.header(MastodonApi.TOKEN_HEADER);


As we expect for #517 it is going to be a common case to use another account and because interceptor already knows about accounts maybe it's a good idea to pass account ID instead of token header for every request? We should think about priority it needs to have but I think it should be lower than instance but should override active account

charlag · 2018-05-12T15:44:52Z

app/src/main/java/com/keylesspalace/tusky/AuthorizeFollowActivity.kt

    private fun broadcast(action: String, id: String) {
        val intent = Intent(action)
        intent.putExtra("id", id)
        LocalBroadcastManager.getInstance(applicationContext).sendBroadcast(intent)
    }

-    override fun onFollowingAccountSelected(account: AccountEntity, position: Int) {
+    override fun onFollowingAccountSelected(authorizeFollow: AuthorizeFollow) {
+        (recyclerView.adapter as FollowingAccountListAdapter).updateAccount(authorizeFollow.accountEntity, authorizeFollow.followState, anyPendingTransaction = true)


You've got a field with adapter, why do you fetch it this (unholy) way?

And in a lot of places as well...

What do you mean by unholy there? It's definitely awful, but I'm not sure what to change it to.

In your class you already have field named adapter - no need to fetch it through recycler

charlag

My previous comments still hold (especially about headers).
It's working but need some more love

charlag · 2018-05-12T15:45:50Z

app/src/main/java/com/keylesspalace/tusky/AuthorizeFollowActivity.kt

    private fun broadcast(action: String, id: String) {
        val intent = Intent(action)
        intent.putExtra("id", id)
        LocalBroadcastManager.getInstance(applicationContext).sendBroadcast(intent)
    }

-    override fun onFollowingAccountSelected(account: AccountEntity, position: Int) {
+    override fun onFollowingAccountSelected(authorizeFollow: AuthorizeFollow) {
+        (recyclerView.adapter as FollowingAccountListAdapter).updateAccount(authorizeFollow.accountEntity, authorizeFollow.followState, anyPendingTransaction = true)


And in a lot of places as well...

charlag · 2018-05-12T15:47:55Z

app/src/main/java/com/keylesspalace/tusky/AuthorizeFollowActivity.kt

+            override fun onResponse(call: Call<Relationship>, response: Response<Relationship>) {
+                if (!response.isSuccessful || response.body() == null) {
+                    onNetworkError()
+                    authorizeFollow.followState


This line is just hanging? You wanted to change it maybe

charlag · 2018-05-12T15:50:55Z

app/src/main/java/com/keylesspalace/tusky/entity/AuthorizeFollow.kt

+import com.keylesspalace.tusky.adapter.FollowingAccountListAdapter.FollowState
+import com.keylesspalace.tusky.db.AccountEntity
+
+data class AuthorizeFollow(val accountEntity: AccountEntity,


data class AuthorizeFollow( val accountEntity: AccountEntity, val subjectAccount: Account, var followState: FollowState, var anyPendingTransaction: Boolean )

charlag · 2018-05-12T15:59:26Z

app/src/main/java/com/keylesspalace/tusky/AuthorizeFollowActivity.kt

+        adapter = FollowingAccountListAdapter(mutableListOf(), this)
+        recyclerView.adapter = adapter
+
+        cancelButton.setOnClickListener({ finish() })


someMethod({ ... }) ----> someMethod { }

charlag · 2018-05-12T16:09:33Z

app/src/main/res/layout/item_following_account.xml

@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="utf-8"?>
+<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"


You certainly can deal with just one ConstraintLayout instead of all these containers, ask me if you need help

charlag · 2018-05-12T16:10:33Z

app/src/main/java/com/keylesspalace/tusky/adapter/FollowingAccountListAdapter.kt

+import com.pkmmte.view.CircularImageView
+import com.squareup.picasso.Picasso
+
+class FollowingAccountListAdapter(private val authorizeFollowList: MutableList<AuthorizeFollow>,


class Blahblah ( private val ... , private val ... , ) { .... }

charlag · 2018-05-12T16:20:05Z

It overlaps the name. I would also align nickname and display name with each other. I think it's also important to show full username (with instance) but I'm not sure how to do that - maybe push things closer to edges, make margins and font smaller

charlag · 2018-05-12T16:26:02Z

CI fails with:

:app:kaptGenerateStubsDebugUnitTestKotline: /home/circleci/code/app/src/test/java/com/keylesspalace/tusky/ComposeActivityTest.kt: (88, 24): Unresolved reference: customEmojis
e: /home/circleci/code/app/src/test/java/com/keylesspalace/tusky/ComposeActivityTest.kt: (110, 24): Unresolved reference: instance

connyduck · 2018-05-28T19:07:35Z

It overlaps the name. I would also align nickname and display name with each other. I think it's also important to show full username (with instance) but I'm not sure how to do that - maybe push things closer to edges, make margins and font smaller

Probably best to give it more lines. First line image+button, second line display name, third line full username including instance.

connyduck · 2018-07-28T13:45:38Z

Will you continue working on this or should somebody else take it over? @remi6397

remi6397 · 2018-07-28T16:56:36Z

@connyduck Hi! I'm sorry, but I don't think that I can do that. :( It would be better if someone more experienced would take that over…

connyduck · 2018-07-29T07:53:09Z

ok, no problem, thx for the info

remi6397 added 9 commits May 5, 2018 21:42

Handle web+mastodon scheme

b5455e9

Add account selector in ComposeActivity

a5798f7

Merge remote-tracking branch 'upstream/master' into compose-account-p…

29b72d5

…icker

Do not activate account picker when replying

b61094a

Optimize imports

9c38b3e

Pick account before authorizing follow

4058547

Legal stuff

4a0d7f3

Fix unpredictable behavior of account prompt in ComposeActivity

6bd6658

Optimize imports

215d172

charlag requested changes May 6, 2018

View reviewed changes

remi6397 added 2 commits May 6, 2018 18:44

Code style

1e833f8

Code style: ViewHolder

397b23f

connyduck added this to the Tusky 1.9 milestone May 6, 2018

remi6397 added 5 commits May 6, 2018 20:44

Override account instead of switching globally in ComposeActivity

6c8355e

Override account instead of switching globally in AccountActivity

7d2f9fa

Move follow authorization to separate activity

914000b

Merge remote-tracking branch 'upstream/master' into compose-account-p…

571c7ef

…icker

Start working on popup AuthorizeFollowActivity

8ffeee7

charlag reviewed May 7, 2018

View reviewed changes

remi6397 added 4 commits May 8, 2018 14:06

Fix AuthorizeFollowActivity intent filter

e380f7a

Rewrite AuthorizeFollowActivity

2e27403

Remove follow request handling code

75c4182

Broadcast unfollow intent

356235c

charlag reviewed May 12, 2018

View reviewed changes

Fix a mistake

8349be4

charlag requested changes May 12, 2018

View reviewed changes

Remove unneeded stuff

f4120c4

Code style

09ec1e2

connyduck removed this from the Tusky 1.9 milestone May 28, 2018

connyduck closed this Jul 29, 2018

wiktor-k mentioned this pull request May 28, 2019

Design a standard way of following people / replying to toots rugk/mastodon-simplified-federation#31

Closed

		@@ -0,0 +1,9 @@
		<?xml version="1.0" encoding="utf-8"?>
		<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"

		@@ -0,0 +1,56 @@
		<?xml version="1.0" encoding="utf-8"?>
		<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"

		@@ -49,8 +49,13 @@ public Response intercept(@NonNull Chain chain) throws IOException {

		Request.Builder builder = originalRequest.newBuilder();

		String authorizationHeader = originalRequest.header(MastodonApi.TOKEN_HEADER);

Add "web+mastodon" protocol handler; prompt for selecting account when action is triggered by external application. #633

Add "web+mastodon" protocol handler; prompt for selecting account when action is triggered by external application. #633

Conversation

remi6397 commented May 6, 2018

charlag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remi6397 May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlag commented May 6, 2018

charlag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlag commented May 12, 2018

charlag commented May 12, 2018

connyduck commented May 28, 2018

connyduck commented Jul 28, 2018

remi6397 commented Jul 28, 2018

connyduck commented Jul 29, 2018

remi6397 May 6, 2018 •

edited

Loading