Support trending links #3908

nikclayton · 2023-08-01T21:10:32Z

Expand the "trending" support to include trending links.

Main changes:

TrendingActivity can now host multiple trending fragments in a viewpager, and swipe between them
Implement a TrendingLinks suite (Fragment, ViewModel, Adapter, Repository, etc) to get and display the data
Always show "Trending" on the left-nav menu
Support adding trending links as a dedicated tab

Ready for "trendingposts" and "trendinglinks"

…-rename

…TrendingTagsFragment.kt

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/TabPreferenceActivity.kt

nikclayton · 2023-08-01T21:11:36Z

~~This includes the changes in #3906, which are mechanical renames. This PR will be easier to review if you review that one first.~~

Some screenshots:

When added as a tab

As part of the activity

nikclayton · 2023-08-01T21:11:56Z

I'm adding trending posts in another PR.

…-rename

…-rename # Conflicts: # app/src/main/java/com/keylesspalace/tusky/di/FragmentBuildersModule.kt

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/di/FragmentBuildersModule.kt # app/src/main/java/com/keylesspalace/tusky/util/ThrowableExtensions.kt

Lakoja · 2023-08-19T12:24:00Z

(Must this be updated somehow: it still shows the changes from the merged rename PR.)

((But also that PR is not shown under "closed PRs"...))

…-links # Conflicts: # app/src/main/java/com/keylesspalace/tusky/MainActivity.kt # app/src/main/java/com/keylesspalace/tusky/TabData.kt # app/src/main/java/com/keylesspalace/tusky/TabPreferenceActivity.kt # app/src/main/java/com/keylesspalace/tusky/components/trending/TrendingActivity.kt # app/src/main/java/com/keylesspalace/tusky/di/FragmentBuildersModule.kt # app/src/main/java/com/keylesspalace/tusky/di/ViewModelFactory.kt # app/src/main/res/layout/fragment_trending.xml # app/src/main/res/layout/fragment_trending_links.xml # app/src/main/res/layout/fragment_trending_tags.xml

nikclayton · 2023-08-19T16:04:18Z

(Must this be updated somehow: it still shows the changes from the merged rename PR.)

Yes. Done.

((But also that PR is not shown under "closed PRs"...))

Weird. It shows for me.

Note that the list of PRs is default-sorted by "When was the PR opened?", so a PR that was opened several weeks back, but was closed an hour ago, could easily show up on page 2 or 3 of the PR page.

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

app/src/main/java/com/keylesspalace/tusky/components/trending/TrendingLinkViewHolder.kt

Lakoja · 2023-08-20T09:38:58Z

app/src/main/java/com/keylesspalace/tusky/components/trending/TrendingLinkViewHolder.kt

+
+        val cardImageShape = ShapeAppearanceModel.Builder()
+
+        if (statusDisplayOptions.mediaPreviewEnabled && !link.image.isNullOrBlank()) {


What does all this do (comment?)?

It seems to adapt layout dynamically but to what end?

There are two preview layout types.

One puts the preview image above the link title and description (e.g., see the screenshots in the second comment on this PR).

The other puts the preview to the left of the title and description. As the previous comment notes, this is out of StatusBaseViewHolder, and might be moved somewhere common later if it needs reusing again.

This is to follow what I call the rule-of-three for refactoring (see also https://en.wikipedia.org/wiki/Don%27t_repeat_yourself#AHA).

You don't need to refactor code that's only used once into a common function, because you don't have the necessary knowledge to know what parts of the code should be governed by parameters (you have zero examples of reuse).

You don't need to refactor code that's only used twice in to a common function, because you still don't have the necessary knowledge to know what parts of the code should be governed by parameters (you have one example of reuse, which is not enough).

When you've copy pasted the same code in to the three different places... now is the time to start looking at refactoring in to common function, because you've three distinct use cases that you can learn from (you have two examples of reuse, which is starting to become useful).

One way to see the other layout is to disable downloading media, then it looks like this:

There are two preview layout types.

One puts the preview image above the link title and description (e.g., see the screenshots in the second comment on this PR).

The other puts the preview to the left of the title and description. As the previous comment notes, this is out of StatusBaseViewHolder, and might be moved somewhere common later if it needs reusing again.

These might be the code comment(s) I was looking for.

app/src/main/java/com/keylesspalace/tusky/components/trending/TrendingLinksFragment.kt

app/src/main/java/com/keylesspalace/tusky/components/trending/TrendingLinksRepository.kt

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

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

Lakoja · 2023-08-20T09:57:25Z

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

@@ -98,6 +100,12 @@ fun createTabDataFromId(id: String, arguments: List<String> = emptyList()): TabD
            icon = R.drawable.ic_trending_up_24px,
            fragment = { TrendingTagsFragment.newInstance() }
        )
+        TRENDING_LINKS -> TabData(


It would make sense to only have that "trending activity view with tabs" you'll get from the side drawer.

So the (only) tab here would show that as well.

(Consistent and removes clutter from this tab list, this is especially relevant for the following PR when this gets three tabs here.)

That would put a set of tabs (trending tags, links, posts) inside another set of tabs.

That's possible (Material3 calls them "secondary tabs") but it's a departure from how we've done things so far. I also think it's reasonable for a user to only want one or two of them and not all three (e.g., I'd use links and posts, but have no interest in trending hashtags).

Ok...
I see (secondary tabs).

But it would still be (quite) inconsistent:

One location with a combined view (from side drawer)

And one location where I would need to make a choice; and/or add 3 things seperately to get the same thing I get from the side drawer

:beard_scratch:

@Lakoja Yes, so let's think about two typical users.

User A occasionally checks the trending info, but not habitually. Maybe a few times a week to see if there's anything new. They don't have a need to focus on one of the three (soon to be four) types of thing that can trend, so getting to it from the nav menu when they want to, and tapping through the different tabs to see what's new is fine for them.

User B is much more interested in trending links, they want to check regularly as part of their normal Mastodon experience. So much so that they want to make it a tab. They're not interested in posts or hashtags, just links. For them, adding a tab with just trending links is fine.

If they added a tab that contained all the trending items they would have to tap that tab once to choose "trending", and then tap again to choose which trending item they're interested in. That's suboptimal UX for an activity that User B is going to want to do a lot.

That's why I think the UX in this PR is the way to go.

There's a third user, User C, who are like User B, but they want to check all the trending tabs. They're not well served at the moment, because of the 5-tab limit, but I think that should be removed (see discussion in "Tusky General" channel).

nikclayton · 2023-08-20T20:16:31Z

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

@@ -98,6 +100,12 @@ fun createTabDataFromId(id: String, arguments: List<String> = emptyList()): TabD
            icon = R.drawable.ic_trending_up_24px,
            fragment = { TrendingTagsFragment.newInstance() }
        )
+        TRENDING_LINKS -> TabData(


That would put a set of tabs (trending tags, links, posts) inside another set of tabs.

That's possible (Material3 calls them "secondary tabs") but it's a departure from how we've done things so far. I also think it's reasonable for a user to only want one or two of them and not all three (e.g., I'd use links and posts, but have no interest in trending hashtags).

nikclayton · 2023-08-20T20:28:57Z

app/src/main/java/com/keylesspalace/tusky/components/trending/TrendingLinkViewHolder.kt

+
+        val cardImageShape = ShapeAppearanceModel.Builder()
+
+        if (statusDisplayOptions.mediaPreviewEnabled && !link.image.isNullOrBlank()) {


There are two preview layout types.

One puts the preview image above the link title and description (e.g., see the screenshots in the second comment on this PR).

The other puts the preview to the left of the title and description. As the previous comment notes, this is out of StatusBaseViewHolder, and might be moved somewhere common later if it needs reusing again.

This is to follow what I call the rule-of-three for refactoring (see also https://en.wikipedia.org/wiki/Don%27t_repeat_yourself#AHA).

You don't need to refactor code that's only used once into a common function, because you don't have the necessary knowledge to know what parts of the code should be governed by parameters (you have zero examples of reuse).

You don't need to refactor code that's only used twice in to a common function, because you still don't have the necessary knowledge to know what parts of the code should be governed by parameters (you have one example of reuse, which is not enough).

When you've copy pasted the same code in to the three different places... now is the time to start looking at refactoring in to common function, because you've three distinct use cases that you can learn from (you have two examples of reuse, which is starting to become useful).

app/src/main/java/com/keylesspalace/tusky/components/trending/TrendingLinksRepository.kt

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

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

app/src/main/java/com/keylesspalace/tusky/components/trending/TrendingLinkViewHolder.kt

nikclayton · 2023-08-20T21:01:35Z

@Lakoja Please continue to ask questions, but please mark the ones that I've answered and you've understood as "Resolved", so it's easy for me to see whatever's new. Thanks.

Lakoja · 2023-08-22T09:34:27Z

@Lakoja Please continue to ask questions, but please mark the ones that I've answered and you've understood as "Resolved", so it's easy for me to see whatever's new. Thanks.

Unfortunately, I cannot do resolve them (or reopen, non-contributor?). Otherwise I would've done it..

nikclayton · 2023-08-22T12:08:44Z

@Lakoja Please continue to ask questions, but please mark the ones that I've answered and you've understood as "Resolved", so it's easy for me to see whatever's new. Thanks.

Unfortunately, I cannot do resolve them (or reopen, non-contributor?). Otherwise I would've done it..

Huh. Today I learned, I guess. OK, please just add a comment to any of the ones that can be resolved.

Lakoja · 2023-08-22T12:49:05Z

@Lakoja Please continue to ask questions, but please mark the ones that I've answered and you've understood as "Resolved", so it's easy for me to see whatever's new. Thanks.

Unfortunately, I cannot do resolve them (or reopen, non-contributor?). Otherwise I would've done it..

Huh. Today I learned, I guess. OK, please just add a comment to any of the ones that can be resolved.

Yeah. I was wondering about that.
At least my own i would like to resolve or open...

nikclayton · 2023-08-22T16:21:56Z

Yeah. I was wondering about that. At least my own i would like to resolve or open...

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#resolving-conversations says you should be able to resolve comments on the PRs that you have opened.

Add "Trending posts" (statuses) feed. This feed is a good source of interesting accounts to follow and, personally, a sort of "Front page of the Fediverse". Since #3908 and #3910 (which would provide a more thorough, albeit complex, access to trending things) won't get merged, I'd like to address this missing feed by simply adding another tab/feed. ~~If desired, I can move the second commit (fixing lint) to another PR.~~ ## Screenshots ### Tab <img src="https://github.com/tuskyapp/Tusky/assets/1063155/6a71a97e-673e-44c7-b67d-9b1df0bed4f5" width=320 /> <img src="https://github.com/tuskyapp/Tusky/assets/1063155/9bf60b23-d2f3-4dd8-8af6-e96647b02121" width=320 /> ### Activity <img src="https://github.com/tuskyapp/Tusky/assets/1063155/4e07dea3-d97f-42c6-8551-492a3116fcfa" width=320 /> <img src="https://github.com/tuskyapp/Tusky/assets/1063155/ad00a134-d622-43f4-8305-84cfa7fed706" width=320 />

Nik Clayton added 18 commits July 30, 2023 20:57

Rename the trending code to "trendingtags"

e9e65bd

Ready for "trendingposts" and "trendinglinks"

Rename button

9cb1547

Update tab preferences in database

0146d42

Merge remote-tracking branch 'upstream/develop' into 2109-more-trends…

ca6861a

…-rename

Update lint baseline to reflect filename changes

faaeb5b

Update app/src/main/java/com/keylesspalace/tusky/components/trending/…

fba5337

…TrendingTagsFragment.kt

Initial implementation

792d6f5

Lint

7ab15bd

Navigate back through the tabs

1a84adb

Show the message view in case of error

0cdfbf7

Treat a 404 as a non-retryable response

8a1adde

Use 3 columns in landscape mode

b874023

Show refreshing in refreshContent

0874bb8

Actually use the blurhash

fe28468

Lint

22ba311

Delete obsolete comment

8889a84

Two more renames

3a4e5c8

Merge branch '2109-more-trends-rename' into 2109-more-trends-links

9b619e1

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/TabPreferenceActivity.kt

nikclayton requested review from DavidEdwards and Tak August 1, 2023 21:10

nikclayton changed the title ~~Implement support for trending links #1~~ Implement support for trending links Aug 1, 2023

Merge branch '2109-more-trends-timeline' into 2109-more-trends-links

ab95d5d

nikclayton changed the title ~~Implement support for trending links~~ Support trending links Aug 1, 2023

Nik Clayton added 5 commits August 2, 2023 09:05

Merge remote-tracking branch 'upstream/develop' into 2109-more-trends…

dda5c39

…-rename

Merge branch '2109-more-trends-rename' into 2109-more-trends-links

e960dd2

Update Trending -> TrendingTags with a SQL statement

810afd2

Merge branch '2109-more-trends-rename' into 2109-more-trends-links

822df0b

Merge remote-tracking branch 'upstream/develop' into 2109-more-trends…

0438acf

…-rename # Conflicts: # app/src/main/java/com/keylesspalace/tusky/di/FragmentBuildersModule.kt

Merge branch '2109-more-trends-rename' into 2109-more-trends-links

9696075

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/di/FragmentBuildersModule.kt # app/src/main/java/com/keylesspalace/tusky/util/ThrowableExtensions.kt

nikclayton requested a review from Lakoja August 19, 2023 16:02

Lakoja reviewed Aug 19, 2023

View reviewed changes

app/src/main/java/com/keylesspalace/tusky/MainActivity.kt Outdated Show resolved Hide resolved

app/src/main/java/com/keylesspalace/tusky/MainActivity.kt Outdated Show resolved Hide resolved

nikclayton requested a review from Lakoja August 20, 2023 08:56

Lakoja reviewed Aug 20, 2023

View reviewed changes

nikclayton commented Aug 20, 2023

View reviewed changes

Nik Clayton added 2 commits August 20, 2023 22:54

PR feedback

c4da394

Update app/src/main/java/com/keylesspalace/tusky/entity/TrendsLink.kt

4536239

nikclayton requested a review from Lakoja August 20, 2023 21:01

Merge branch 'develop' into 2109-more-trends-links

0c0ac4c

nikclayton closed this by deleting the head repository Aug 27, 2023

tinsukE mentioned this pull request Sep 8, 2023

Add "Trending posts" (statuses) feed #4007

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support trending links #3908

Support trending links #3908

nikclayton commented Aug 1, 2023 •

edited

Loading

nikclayton commented Aug 1, 2023 •

edited

Loading

nikclayton commented Aug 1, 2023

Lakoja commented Aug 19, 2023

nikclayton commented Aug 19, 2023

Lakoja Aug 20, 2023

nikclayton Aug 20, 2023

nikclayton Aug 20, 2023

Lakoja Aug 22, 2023

Lakoja Aug 20, 2023

nikclayton Aug 20, 2023

Lakoja Aug 22, 2023

nikclayton Aug 23, 2023

nikclayton Aug 20, 2023

nikclayton Aug 20, 2023

nikclayton commented Aug 20, 2023

Lakoja commented Aug 22, 2023

nikclayton commented Aug 22, 2023

Lakoja commented Aug 22, 2023

nikclayton commented Aug 22, 2023


		val cardImageShape = ShapeAppearanceModel.Builder()

		if (statusDisplayOptions.mediaPreviewEnabled && !link.image.isNullOrBlank()) {

Support trending links #3908

Support trending links #3908

Conversation

nikclayton commented Aug 1, 2023 • edited Loading

nikclayton commented Aug 1, 2023 • edited Loading

nikclayton commented Aug 1, 2023

Lakoja commented Aug 19, 2023

nikclayton commented Aug 19, 2023

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

nikclayton commented Aug 20, 2023

Lakoja commented Aug 22, 2023

nikclayton commented Aug 22, 2023

Lakoja commented Aug 22, 2023

nikclayton commented Aug 22, 2023

nikclayton commented Aug 1, 2023 •

edited

Loading

nikclayton commented Aug 1, 2023 •

edited

Loading