Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

End of Year: Share text #570

Merged
merged 5 commits into from
Nov 16, 2022
Merged

End of Year: Share text #570

merged 5 commits into from
Nov 16, 2022

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Nov 14, 2022

📘 Project: #410

iOS PR: Automattic/pocket-casts-ios#478

Description

Adds text content for each story shared and for the top podcast(s) and longest episode add a link to them.

How the shared content is handled is up to the app. Some apps work fine with images + text, and some do not. E.g. Facebook app only allows sharing an image.

Testing Instructions

  1. Set END_OF_YEAR_ENABLED feature flag to true in base.gradle
  2. Run the app and make sure you're logged in to an account with a few items on Listening History
  3. Check your 2022 EoY stats (either by tapping "View My 2022" on the modal or going to Profile and tapping the card there)
  4. When the first story appears, tap "Share"
  5. Share to Notes or Twitter
  6. ✅ Check the content and the image for the shared story
  7. Repeat these steps for each story, always checking the output (image + text)

Additional Tests

  1. ✅ For the top 1 podcast, check that the URL contained in the text redirects to the podcast
  2. ✅ For the longest episode, check that the URL contained in the text redirects to the episode
  3. ✅ For the top 5 podcasts, check that the URL opens a list containing exactly the 5 top podcasts

Share Text

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@ashiagr ashiagr requested a review from a team as a code owner November 14, 2022 12:56
@ashiagr ashiagr mentioned this pull request Nov 14, 2022
35 tasks
@ashiagr ashiagr marked this pull request as draft November 14, 2022 14:10
@ashiagr ashiagr marked this pull request as ready for review November 14, 2022 14:29
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. The 403 errors I was getting is a bit concerning, but that may just be something weird going on with my setup, so I'll go ahead and approve. Let me know if you have any ideas about what might be causing that for me. 🤔

Comment on lines +32 to +44
try {
listServerManager.createPodcastList(
title = context.resources.getString(
LR.string.end_of_year_story_top_podcasts_share_text,
""
),
description = "",
podcasts = story.topPodcasts.map { it.toPodcast() }
) ?: shortURL
} catch (ex: Exception) {
Timber.e(ex)
shortURL
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This api call failed for me with a 403 everytime I tried to share the top 5 podcasts, so I fell through to the generic PC link each time. Not sure what is causing that. I tried both logged-in and logged-out on the staging and prod server with the same result each time. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm if you can share podcasts and episodes in general (from podcasts or episode fragments)?

I'll merge this PR for now. Based on your response, I'll recheck what might be going wrong with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm if you can share podcasts and episodes in general

Good idea. I'm also getting an error when I try to create a shared list of podcasts from the podcasts page, so I don't think this is an issue with your EOY work (and fwiw, I can share a list with our current production builds, so I must be doing something weird locally).

Comment on lines +146 to +150
val currentState = (state.value as State.Loaded)
mutableState.value = currentState.copy(preparingShareText = true)

val shareTextData = shareableTextProvider.getShareableDataForStory(story)
mutableState.value = currentState.copy(preparingShareText = false)
Copy link
Contributor

@mchowning mchowning Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's any way the state could get mutated while we're waiting for the suspended shareableTextProvicer.getShareableDataForStory call? It doesn't seem likely, but I was able to get a bit of weird behavior by quickly tapping on Story after tapping the Share button (it would jump back to the beginning of the Story sequence). That's probably unrelated to this, but figured I'd mention it.

device-2022-11-14-112608.mp4

Copy link
Contributor Author

@ashiagr ashiagr Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because StoriesViewModel initialized inside the StoriesPage dialog composable is not scoped to it but to the activity/ fragment that contains it. As a workaround, I manually call viewmodel's clear() and start() which doesn't seem to be working well.

This SO answer helped me understand it:

Compose does not offer any mechanism to scope ViewModels to an individual @composable - any ViewModels you create outside of a NavHost's destination is scoped to the activity/fragment that contains your ComposeView/where you call setContent and, thus, lives for the entire lifetime of your Compose hierarchy - that's why you always get the same instance back.

Feature request: https://issuetracker.google.com/issues/165642391

I'll open another PR with a fix or temporary workaround.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that disabling the UI as soon as the Share button is touched might help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to DialogFragment with composable content in #581. I'll shortly make it ready for review and we can test it again if it fixes the issue.

@ashiagr ashiagr force-pushed the task/410-share-text branch from de64293 to 91bbac9 Compare November 15, 2022 06:46
@ashiagr ashiagr mentioned this pull request Nov 15, 2022
8 tasks
Base automatically changed from task/410-epilogue-story-design to main November 15, 2022 13:22
@ashiagr ashiagr merged commit e67a40c into main Nov 16, 2022
@ashiagr ashiagr deleted the task/410-share-text branch November 16, 2022 13:06
@ashiagr ashiagr added this to the 7.27 ❄️ milestone Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants