3771 mark post as reply #3778

Lakoja · 2023-06-29T08:49:11Z

Fixes #3771 / partly addresses #1469

This shows in a timeline the status info "In reply to < username>" just like the existing "< username> boosted" above a status.

This is a more or less low-hanging fruit as the information is available and the web gui does this (but see review comments here for details).

This is NOT the requested "Show thread" functionality - which would still need to be discussed; i.e. every reply makes a thread?

Lakoja · 2023-06-29T08:50:24Z

app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineTypeMappers.kt

@@ -207,8 +207,8 @@ fun TimelineStatusWithAccount.toViewData(gson: Gson, isDetailed: Boolean = false
            id = status.serverId,
            url = null, // no url for reblogs
            account = this.reblogAccount!!.toAccount(gson),
-            inReplyToId = null,
-            inReplyToAccountId = null,
+            inReplyToId = status.inReplyToId,


Would be consistent (a boost can also be a reply).

But the boost itself is not the reply, right? The actionable status is the reply

What is boosted ("status") can be a reply (too).

This info is "unused".

Ah, I see now, this is here as a hack to populate the inreplyto timeline account of the concrete statusviewdata when it's a boost of a reply.
…but as you say, we're not displaying that information, so it could be reverted to avoid confusing the issue (although the status/actionable thing is already confusing)

.../java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt

app/src/main/res/drawable/ic_reply_24dp.xml

app/src/main/java/com/keylesspalace/tusky/db/TimelineStatusEntity.kt

nikclayton

A couple of low-hanging things I spotted. Later today I'll check this out locally and take a proper look.

.../java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt

app/src/main/java/com/keylesspalace/tusky/db/TimelineStatusEntity.kt

app/src/main/res/values/strings.xml

nikclayton · 2023-07-08T14:49:27Z

I haven't tested this yet, but I've just observed the following:

item_status.xml (and related layouts) have a R.id.status_info view at the top, which is where the "X boosted" message goes.
StatusBaseViewHolder.java has a setIsReply method that's called whenever the status is a reply

So, in StatusViewHolder.java, wouldn't it be simpler to just do this:

protected void setIsReply(boolean isReply) {
    super setIsReply(isReply);
    if (isReply) {
        statusInfo.text = getString(R.string.post_replied_format, ...)
    }
}

?

Also, what should happen if the status was a reply, and it was reblogged? https://chaos.social/@jollysea/110668273086228868 is an example post that is a reply that was reblogged by @mcclure.

The "X boosted" message is more important
The "In reply to Y" message is more important
The message should be "X boosted this reply to Y"
Show two lines of text

Note that that status above is an interesting edge case for this, because it's mcc reblogging a reply to her own post, so although option 3 might be generally good, it would lead to "mcc boosted this reply to mcc". Which might be more mcc than a single line can handle :-)

Lakoja · 2023-07-10T12:32:47Z

So, in StatusViewHolder.java, wouldn't it be simpler to just do this:

protected void setIsReply(boolean isReply) {
    super setIsReply(isReply);
    if (isReply) {
        statusInfo.text = getString(R.string.post_replied_format, ...)
    }
}

?

Instead of what?
At least all the changes before and in (now) setStatusInfoText() would be necessary as they also deal with emojis and a maybe different name.
And at least at the moment the setIsReply() only deals with two different reply cases. The reblog information is orthogonal.

Lakoja · 2023-07-10T12:35:20Z

Also, what should happen if the status was a reply, and it was reblogged? https://chaos.social/@jollysea/110668273086228868 is an example post that is a reply that was reblogged by @mcclure.
3. The message should be "X boosted this reply to Y"

I kind of like option 3.
However that might be too much information? (and will often be too long, names tend to be long...)
2 lines will also be too much, I think.

At the moment (in my code) reblog is more important than reply. So an "if ... else if" logic.

nikclayton · 2023-07-29T18:30:04Z

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

@@ -17,7 +17,7 @@
        android:layout_marginStart="14dp"
        android:layout_marginTop="@dimen/status_reblogged_bar_padding_top"
        android:layout_marginEnd="14dp"
-        android:drawableStart="@drawable/ic_reblog_18dp"
+        android:drawableStart="@drawable/ic_reblog_24dp"


Why is this switching to the bigger icon? It causes the effective left margin of the text to change.

Also, you could copy ic_reply_24dp.xml to ic_reply_18dp.xml`, and then use that for replies.

The size is determined by the component (height of TextView).

Already before: The text behind the drawable is not alligned with the name or the content below. This does not change here.

So I could leave it as 18 but that would not change it graphically.

nikclayton · 2023-07-29T20:56:09Z

@Lakoja Can you take a look at Lakoja#4, and see if you want to merge that in to this PR? I figured that was easier than more back-and-forth.

You'll want to merge develop in to this PR first to minimse the diffs.

Some quick screenshots:

It shows reply and boost, with a bullet between them

If the account isn't known, but mentioned, it uses the @-form of the user name

Long lines are truncated / ellipsised

I'm not 100% convinced that moving the left margin over is a good idea, but as you can see from the screenshots, sometimes the line does get quite long.

WDYT?

Lakoja · 2023-07-29T21:03:30Z

Some quick screenshots:

WDYT?

I find the lines with two information quite hard to parse visually.
(thus) I still wouldn't present two informations there.

nikclayton · 2023-07-30T11:39:01Z

Some quick screenshots:

WDYT?

I find the lines with two information quite hard to parse visually. (thus) I still wouldn't present two informations there.

I think that might be a problem, because with this PR we're trying to use the statusInfo to show two difference pieces of information.

Currently the statusInfo answers the "Why am I seeing this in my timeline?" question. It tells the user "You are seeing this because someone you follow has boosted it".

This PR extends that to also answer the "Can I tap on this post to read a thread?" question.

I'm not sure that only ever showing one bit of information is a good idea, because it means that users will not be able to rely on the information in the statusInfo -- what's shown will vary, and it will be difficult for users to figure out why it's varying.

By which I mean that some replies -- replies from people that the user follows -- will be marked as replies, and the user will know they can tap the post to see the thread.

But other replies -- replies from people they don't follow that have been boosted by someone they do -- will not be marked as replies.

[I know the "reply" icon at the bottom of the post will change as well, but as we see from the bug reports, this is also something that users don't always notice]

If we flip that example around, and decide that showing that it's part of a thread is more important than showing that it's a boost, then we get the problem that a user can see "user-i-dont-follow replied to ..." in the statusInfo, and not know why this post is appearing in their timeline.

The first screenshot in my previous comment is an example of that. I don't follow rain, but I do follow Norm.

Both of those seem like a recipe for user confusion.

I think we can collapse some of the information down. For example, if someone boosts their own post there's no need to show their display name again.

And we could experiment with putting some of the information elsewhere. I'm not sure a two-line statusInfo is a good idea, but maybe the "reply" icon could appear next to the display name of the poster (i.e., the displayname that appears to the right of the avatar)? That would leave the statusInfo for boosts (and later, hashtags).

~~But... that would mean the left-edge of the display names would jump around a bit as the user scrolls through their timeline.~~

[Edit: Putting a reply icon next to the display name is a bad idea -- users could easily add an emoji to their display name that looks like a reply icon (or other), which would be very confusing.]

I'll try some mockups and see what happens.

nikclayton · 2023-07-30T12:38:02Z

Here's the latest variant (I'm still waiting for a self-boost to cross my timeline).

Replying to themselves

nikclayton · 2023-08-03T20:56:44Z

Some of #3883 is relevant to this as well. There's one, possibly two additional values that can be included in the status_info, but they're mutually exclusive with the others (I think). They are:

"Sent you a direct message"
"Replied to your direct message"

or similar. Possibly with "direct message" in that text in bold, or bold tusky_red, or something like that, to make it even more distinctive.

Tak

Doesn't this need a database migration for TimelineAccountEntity?

Tak · 2023-10-05T10:13:17Z

app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineTypeMappers.kt

@@ -207,8 +207,8 @@ fun TimelineStatusWithAccount.toViewData(gson: Gson, isDetailed: Boolean = false
            id = status.serverId,
            url = null, // no url for reblogs
            account = this.reblogAccount!!.toAccount(gson),
-            inReplyToId = null,
-            inReplyToAccountId = null,
+            inReplyToId = status.inReplyToId,


But the boost itself is not the reply, right? The actionable status is the reply

Tak · 2023-10-05T10:15:01Z

app/src/main/java/com/keylesspalace/tusky/adapter/StatusViewHolder.java

            boolean sensitive = !TextUtils.isEmpty(status.getActionable().getSpoilerText());
            boolean expanded = status.isExpanded();

            setupCollapsedState(sensitive, expanded, status, listener);

            Status reblogging = status.getRebloggingStatus();
-            if (reblogging == null || status.getFilterAction() == Filter.Action.WARN) {
+            boolean isReply = status.getStatus().getInReplyToId() != null;


Don't you need to use status.getActionable here, and on line 90?

I was searching for some time for the right location with the right information.
"actionable" was not it if I remember correctly.

It is working and plausible at the moment.

(Would need to look deep(er) into the code if you want me to be 100% certain about this.)

You're right, the inreplyto timeline account is only available on status, but without the timeline type mapper change, the correct thing to check for the inreplyto id is the actionable status. (And if status.getActionable().getInReplyToId() is nonnull, then status.getInReplyToAccount() will be as well.)

What is the meaning of "actionable" in current code?

I see that it only has a value if "reblog" is true (Status.kt:59), so the thinking is probably "the item the action was taken for"?

(The "reply stuff" I have aligned more with the extsting "inReplyToAccountId".)

Yeah, my understanding is that the "outer" status is the boost itself, and the actionable status is the status that was boosted

Lakoja · 2023-10-05T18:16:06Z

Doesn't this need a database migration for TimelineAccountEntity?

There is no change there?

All info here is present (in rare cases the "replied to account info" might be missing and then the name is not show).

mcclure · 2023-10-06T01:44:56Z

What does this patch do if a post is BOTH a reply AND an RT?

Tak · 2023-10-06T09:13:42Z

The current PR code displays this when a reply is boosted

Tak · 2023-10-06T11:37:17Z

Oh, it looks super weird in the thread view - is that intentional?

Tak · 2023-10-06T11:38:17Z

I guess this change should be applied to the notification timeline as well? (Maybe in a second pass after the refactor lands?)

Lakoja · 2023-10-06T18:44:51Z

Oh, it looks super weird in the thread view - is that intentional?

No.
(Super weird: it shows "Replied" above every status.)

I fixed it by not showing the status info in the thread view.

But I guess the introduced flag should/could be part of "StatusDisplayOptions" object?

Lakoja · 2023-10-06T18:45:21Z

I guess this change should be applied to the notification timeline as well? (Maybe in a second pass after the refactor lands?)

Yes, probably and yes.

Lakoja · 2023-10-06T18:46:41Z

gradle/libs.versions.toml

@@ -120,6 +120,7 @@ glide-compiler = { module = "com.github.bumptech.glide:ksp", version.ref = "glid
 glide-core = { module = "com.github.bumptech.glide:glide", version.ref = "glide" }
 glide-okhttp3-integration = { module = "com.github.bumptech.glide:okhttp3-integration", version.ref = "glide" }
 gson = { module = "com.google.code.gson:gson", version.ref = "gson" }
+kapt = { module = "androidx.room:room-compiler", version.ref = "androidx-room" }


Not sure why this is needed now (starting today)?

If I don't have this there will be a build error that the (automatically generated) AppDatabase_Impl does not implement all needed dao factory methods.

This should not be necessary as we migrated Room to ksp. If you get an error, try a clean build.

Tak · 2024-04-26T07:16:30Z

imo the changes look ok, but it will have to be rebased again after #4026 lands (after the 25.0 release) 😐
I'm really sorry that this has been in stasis for almost a year

connyduck · 2024-05-03T12:13:50Z

Should we in turn get rid of the double arrow on the reply button that currently indicates replies? It wouldn't be necessary anymore.

Lakoja · 2024-12-29T10:29:43Z

Replaced by: #4834

Earlier PR: #3778 Fixes: #3771

Lakoja commented Jun 29, 2023

View reviewed changes

Lakoja force-pushed the 3771-mark-post-as-reply branch from 8e37063 to b19cf1e Compare July 7, 2023 11:02

nikclayton suggested changes Jul 7, 2023

View reviewed changes

nikclayton reviewed Jul 29, 2023

View reviewed changes

Lakoja force-pushed the 3771-mark-post-as-reply branch from 3f94d4d to 9e7fcff Compare October 3, 2023 14:32

Lakoja requested a review from Tak October 3, 2023 15:05

Tak reviewed Oct 5, 2023

View reviewed changes

Lakoja commented Oct 6, 2023

View reviewed changes

Lakoja force-pushed the 3771-mark-post-as-reply branch 2 times, most recently from 4e1ac16 to 202968f Compare April 25, 2024 19:14

Lakoja requested a review from Tak April 25, 2024 19:14

Lakoja force-pushed the 3771-mark-post-as-reply branch from 202968f to e6bbf90 Compare April 26, 2024 13:21

Lakoja force-pushed the 3771-mark-post-as-reply branch from e6bbf90 to 82476a3 Compare May 10, 2024 17:53

Lakoja force-pushed the 3771-mark-post-as-reply branch from 82476a3 to d383e77 Compare May 10, 2024 18:12

3771: Add an "in reply to" text to a reply

04d7cd6

Lakoja force-pushed the 3771-mark-post-as-reply branch from d383e77 to 04d7cd6 Compare May 10, 2024 18:20

Lakoja mentioned this pull request Dec 27, 2024

3771: Add "display replied to" functionality #4834

Merged

Lakoja closed this Dec 29, 2024

connyduck pushed a commit that referenced this pull request Jan 6, 2025

3771: Add "display replied to" functionality (#4834)

d6b276d

Earlier PR: #3778 Fixes: #3771

3771 mark post as reply #3778

3771 mark post as reply #3778

Conversation

Lakoja commented Jun 29, 2023 • 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

nikclayton left a comment

Choose a reason for hiding this comment

nikclayton commented Jul 8, 2023

Lakoja commented Jul 10, 2023

Lakoja commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikclayton commented Jul 29, 2023

It shows reply and boost, with a bullet between them

If the account isn't known, but mentioned, it uses the @-form of the user name

Long lines are truncated / ellipsised

Lakoja commented Jul 29, 2023

nikclayton commented Jul 30, 2023 • edited Loading

nikclayton commented Jul 30, 2023

Replying to themselves

nikclayton commented Aug 3, 2023

Tak 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

Lakoja commented Oct 5, 2023

mcclure commented Oct 6, 2023

Tak commented Oct 6, 2023 • edited Loading

Tak commented Oct 6, 2023

Tak commented Oct 6, 2023

Lakoja commented Oct 6, 2023

Lakoja commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tak commented Apr 26, 2024

connyduck commented May 3, 2024

Lakoja commented Dec 29, 2024

Lakoja commented Jun 29, 2023 •

edited

Loading

nikclayton commented Jul 30, 2023 •

edited

Loading

Tak commented Oct 6, 2023 •

edited

Loading