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.

Sign up for GitHub

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

Jump to bottom

Fix quicklook dismiss on rotate #756

Merged
merged 6 commits into from
Nov 14, 2023
Merged

Fix quicklook dismiss on rotate #756

merged 6 commits into from
Nov 14, 2023

Conversation

boscojwho
Copy link
Contributor

Checklist

Pull Request Information

About this Pull Request

  1. Fixes issue where rotating device while QuickLookUI is presented causes that UI to be dismissed.
  2. Adds a progress view overlay on top of CachedImage while QuickLookUI is preparing to open, since sometimes QuickLookUI doesn't immediately open on tap gesture.

Screenshots and Videos

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-10.at.17.03.21.mp4

Additional Context

Rotation bug might not be reproducible in iOS simulator.

@boscojwho
- Remove unused import.
adebcc9
@boscojwho
- Move QuickLookUI sheet to app root view.
57fb605 
- Add QuickLookState to enable presentation from any view.
- Pass quickLookState as env value in navigation destination so pushed views have access.
@boscojwho
- Add progress view when opening QuickLookUI.
94b2d55
@boscojwho boscojwho changed the title Bosco/fix quicklook dismiss on rotate Fix quicklook dismiss on rotate Nov 11, 2023
@boscojwho boscojwho requested a review from a team as a code owner November 11, 2023 01:51
@boscojwho boscojwho requested review from JakeShirley and EricBAndrews and removed request for a team November 11, 2023 01:51
EricBAndrews
EricBAndrews requested changes Nov 11, 2023
View reviewed changes
Copy link
Member

@EricBAndrews EricBAndrews left a comment

Choose a reason for hiding this comment

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

This PR appears to break the "mark read on view image" functionality.

Repro:

  • Tap an image in feed on an unread post
  • Close the QuickLook preview

Expected behavior: post is marked read
Actual behavior: post is not marked read

Some very preliminary investigation indicates that the dismissCallback is getting called 2-4 times per dismissal, though why that causes the post to not be marked read at all is a bit of a mystery

@boscojwho
Copy link
Contributor Author

This PR appears to break the "mark read on view image" functionality.

Repro:

  • Tap an image in feed on an unread post
  • Close the QuickLook preview

Expected behavior: post is marked read Actual behavior: post is not marked read

Some very preliminary investigation indicates that the dismissCallback is getting called 2-4 times per dismissal, though why that causes the post to not be marked read at all is a bit of a mystery

Ah, I thought that was just a simulator issue.

Yeah, odd, because callback gets called. Will investigate.

@boscojwho
Copy link
Contributor Author

Self note:
On fresh launch, mark as read works on the first opened photo post. After that, it fails.

@boscojwho
- Call dismissCallback immediately when we acquire url for quick look.
534d63b 
- Set isPresenting = false in error branch.
- Don't observe quickLookState.url, no longer needed.
@boscojwho
- Rename dismissCallback to onTapCallback, reflecting change in A…
659335a 
…PI behaviour.
@boscojwho
Copy link
Contributor Author

Resolved mark post as read issue:

  • Tested in iOS simulator on lemmy.world instance in compact, headline, and large posts setup.

EricBAndrews
EricBAndrews approved these changes Nov 12, 2023
View reviewed changes
Copy link
Member

@EricBAndrews EricBAndrews left a comment

Choose a reason for hiding this comment

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

Nice 👍

@boscojwho boscojwho merged commit cdaddfb into dev Nov 14, 2023
4 checks passed
@boscojwho boscojwho deleted the bosco/fix-quicklook-dismiss-on-rotate branch November 14, 2023 01:21
boscojwho added a commit that referenced this pull request Nov 23, 2023
@boscojwho
Fix quicklook dismiss on rotate (#756)
b9b669a
@EricBAndrews EricBAndrews mentioned this pull request Dec 1, 2023
Closed
EricBAndrews pushed a commit that referenced this pull request Dec 18, 2023
@boscojwho @EricBAndrews
Fix quicklook dismiss on rotate (#756)
9798d5c
EricBAndrews pushed a commit that referenced this pull request Dec 18, 2023
@boscojwho @EricBAndrews
Fix quicklook dismiss on rotate (#756)
a33ce67
EricBAndrews pushed a commit that referenced this pull request Dec 18, 2023
@boscojwho @EricBAndrews
Fix quicklook dismiss on rotate (#756)
794e9c1
EricBAndrews pushed a commit that referenced this pull request Dec 18, 2023
@boscojwho @EricBAndrews
Fix quicklook dismiss on rotate (#756)
1d98bbe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EricBAndrews EricBAndrews EricBAndrews approved these changes

@JakeShirley JakeShirley Awaiting requested review from JakeShirley JakeShirley is a code owner automatically assigned from mlemgroup/mlem-dev-team

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@boscojwho @EricBAndrews