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

Fix textStoryToggle visibility when hasSelectedMedia #13948

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Sagar0-0
Copy link
Contributor

First time contributor checklist

Contributor checklist

  • Device A, Android X.Y.Z
  • Device B, Android Z.Y
  • Virtual device W, Android Y.Y.Z
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

Before:

document_6107040231842648805.mp4

After:
Observing to the latest state of selectedMedia count and updating the visibility

Copy link
Contributor

@greyson-signal greyson-signal left a comment

Choose a reason for hiding this comment

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

I'm interested in how you're reproducing this bug, it doesn't personally happen to me.

@@ -233,7 +241,6 @@ class MediaSelectionActivity :
private fun canDisplayStorySwitch(): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is called from multiple places, and now those call sites no longer take into account whether there's selected media. Are those call sites still necesessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are still required. We have to hide on DestinationChange(navigating to preview or gallery), and when we are back to MediaCapture, even when no media was selected, we have to update it back to visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, this might create a problem, as now there are two sources for textStoryToggle visibility state(hasSelectedMedia and DestinationChangedListener) and it might create difficulty in testing and finding bugs. A better approach can be to move the logic to ViewModel and check the combinations there. If destinations are mediaCapture or textStore then handle the logic, else false. This way we will only expose a single LiveData of isTextStoryToggleVisible as a single source of truth.

@Sagar0-0
Copy link
Contributor Author

Sagar0-0 commented Jan 31, 2025

I'm interested in how you're reproducing this bug, it doesn't personally happen to me.

Capture a media, Click anywhere on the Preview screen and wait for 1-2 seconds, press back button(See video). This might not show sometimes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants