Comment fields in ConversationEntity related to expanding and collapsing content #3886
Conversation
…tails in all Status-representing classes.
|
At Conny's request in Matrix, I also put comments in the other Status-representing classes. I also added comments explaining the relationships between the Status classes/files. I am only 95% sure about some of these comments (For example, ConversationEntity). I attempted to match format in the file, so some of these comments use @return and some don't. |
|
I have just realized TimelineStatusEntity and ConversationStatusEntity are different. Some of these comments are wrong. |
| @@ -29,6 +29,7 @@ import com.keylesspalace.tusky.entity.TimelineAccount | |||
| import com.keylesspalace.tusky.viewdata.StatusViewData | |||
| import java.util.Date | |||
|
|
|||
| /** A reply-tree of statuses which have been saved to disk. */ | |||
There was a problem hiding this comment.
It's not a tree. The API for conversations returns the most recent (last) status in the thread. You can see this in Tusky if you open the DMs tab; each DM (conversation) is represented by the last status in the message thread. You have to tap on that message to see the full thread.
"saved to disk" is an implementation detail.
For commenting these I've found it helpful to link directly to the relevant Mastodon API documentation. For this one, it's a https://docs.joinmastodon.org/entities/Conversation/
| @@ -72,6 +74,7 @@ data class ConversationAccountEntity( | |||
| } | |||
| } | |||
|
|
|||
| /** A previously-displayed status which has been saved to disk. */ | |||
There was a problem hiding this comment.
@connyduck In the Mastodon API the Converstation entity contains a regular Status entity.
Why is it a different type here? And can that reasoning please be added to the comment.
| * Class to represent data required to display either a notification or a placeholder. | ||
| * Class to represent data required to display either a status or a placeholder ("Load More" bar). | ||
| * It is either a [StatusViewData.Concrete] or a [StatusViewData.Placeholder]. | ||
| * Can be created either from a ConversationStatusEntity, or by helpers in ViewDataUtils. |
There was a problem hiding this comment.
Would you be OK with holding off on adding the comments here, and adding them as suggestions to https://github.com/tuskyapp/Tusky/pull/3436/files#diff-eb35dc96f92845f7f47b51230940e1c57928b5337d164e64567f3511b19b9647 instead? It'll make syncing that PR with head easier, and that PR removes the .Placeholder type, so there's probably no point documenting it here.
That PR also includes some of the same comments that you're making here.
There was a problem hiding this comment.
I guess my question is, is PR #3436 on track for v24 inclusion? If so, then I agree it would make more sense to glom onto 3436. If not, I think it would be better to do a comments-only patch now. I could do the patch-3436 merge conflict resolution myself if that would help.
There was a problem hiding this comment.
I guess my question is, is PR #3436 on track for v24 inclusion?
¯\(ツ)/¯
Needs a code review and more people to try the APK.
|
Given that #3436 is closed, is this mergable as-is, or does it need more work? |
|
I think this is more likely to be mergeable without #3436, however, as of last update, the comments in this are still wrong (it turns out Conversation refers only to a DM thread, and there are some other similar classes that need to gain comments in addition to Conversation). Without needing to wait on 3436 I can resume work however… |
I found myself in ConversationEntity.kt and found it confusing to distinguish the difference between sensitive, muted, collapsed, expanded, and "isShowingContent". We could refactor this, but a good first step is simply to comment what the fields mean. These comments are based on a comment in Matrix by Nik Clayton, which was 👍ed by Conny.