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

Deeplink to reviewer closes everything #17662

Merged
merged 1 commit into from
Jan 4, 2025

Conversation

Arthur-Milchior
Copy link
Member

@Arthur-Milchior Arthur-Milchior commented Dec 25, 2024

Currently, the code ensured there was at most one IntentHandler. We
honestly don't care about it. The intended meaning was that there is a
single reviewer.

This bug was probably introduced in #17422 with commit
f26aa95.

I would love to test to avoid another regression. However, it's really
not clear to me how to deal with shortcuts, widget or remainders as
they are outside of the main app.

I discovered that having a single reviewer is still not
enough. Because what is below the reviewer could be nonsense. Instead,
I followed standard practice for deep link. I close everything and
create a fresh stack.

@Arthur-Milchior Arthur-Milchior force-pushed the LaunchType branch 3 times, most recently from 3cbd1fd to c0dc16d Compare January 3, 2025 21:13
@Arthur-Milchior Arthur-Milchior changed the title Ensure there is at most one reviewer Deeplink to reviewer closes everything Jan 3, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Clean!

@david-allison david-allison added the Needs Second Approval Has one approval, one more approval to merge label Jan 3, 2025
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM - proposed commitable comment that could allow this to go immediately via commit-suggestion + squash if it meets with approval

Currently, the code ensured there was at most one IntentHandler. We
honestly don't care about it. The intended meaning was that there is a
single reviewer.

This bug was probably introduced in ankidroid#17422 with commit
f26aa95.

I would love to test to avoid another regression. However, it's really
not clear to me how to deal with shortcuts, widget or remainders as
they are outside of the main app.

I discovered that having a single reviewer is still not
enough. Because what is below the reviewer could be nonsense. Instead,
I followed standard practice for deep link. I close everything and
create a fresh stack.

Bug ankidroid#16956.
Fixes ankidroid#17664.

Co-authored-by: Mike Hardy <github@mikehardy.net>
@david-allison david-allison added this pull request to the merge queue Jan 4, 2025
Merged via the queue into ankidroid:main with commit ea311c9 Jan 4, 2025
9 checks passed
@github-actions github-actions bot added this to the 2.21 release milestone Jan 4, 2025
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Jan 4, 2025
@Arthur-Milchior Arthur-Milchior deleted the LaunchType branch January 4, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After shortcut/widget, the screen to which we return may make not sense
3 participants