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

Fixed WebUI crash when a status opened in the media modal is deleted #15701

Merged
merged 5 commits into from
Feb 11, 2021

Conversation

kaias1jp
Copy link
Contributor

Fixed a problem that a compatibility error occurs when the status is deleted while the status image etc. is selected and displayed.

@kaias1jp kaias1jp closed this Feb 10, 2021
@kaias1jp kaias1jp reopened this Feb 10, 2021
@ClearlyClaire ClearlyClaire changed the title Fixed picture in picture compatibility error in WebUI when status is deleted Fixed WebUI crash when a status opened in the pop-out player is deleted Feb 10, 2021
@ClearlyClaire
Copy link
Contributor

Thank you for the contribution! I took the liberty of changing the PR title to reflect that it's not a compatibility error but just a crash.

Your change seem to fix the crash itself, but it would result in the pop-out player still being displayed, although without the status bar. A better approach would probably be to close the pop-out player as soon as the status is deleted.

This could be handled by changing app/javascript/mastodon/reducers/picture_in_picture.js to process status deletion the same way as PICTURE_IN_PICTURE_REMOVE when the deleted status id is the currently-displayed one. Statuses are deleted through a TIMELINE_DELETE action (see app/javascript/mastodon/reducers/statuses.js for example).

@kaias1jp
Copy link
Contributor Author

Thank you. You're right. I will consider how to close the pop-out player, but should this PR be closed once? Since this was my first PR, I don't know what to do in such a case. If you do not mind, please teach me.

@ClearlyClaire
Copy link
Contributor

You can push additional commits in the PR. I'd start by reverting the latest commit (git revert HEAD) and continue working as before.

This will result in a slightly noisy commit history, but that isn't really an issue. If you still want to clean that commit history later on, you can use git rebase -i, but that is completely optional.

@kaias1jp
Copy link
Contributor Author

I reconfirmed where the crash occurred.
What I found was that the WebUI crashed when the status was deleted when the image was displayed modally.
So I want to change the title to "Fixed WebUI crashing when status is deleted when displaying images modally".
As an additional fix, I would like to close the image only if the modally displayed image has a deleted status.
The pop-out player does not crash, but I think it is necessary to close the player by deleting the status. I'm wondering whether to include this in this PR or put it out as another PR.

@ClearlyClaire
Copy link
Contributor

Ah yes, there is indeed a similar crash in the media modal. I'd say it's a similar issue but not exactly the same. I don't have a preference with regards to fixing both issues in the same PR or different ones, do what's easiest for you.

@kaias1jp kaias1jp changed the title Fixed WebUI crash when a status opened in the pop-out player is deleted Fixed WebUI crash when a status opened in the media modal is deleted Feb 11, 2021
@ClearlyClaire
Copy link
Contributor

Looks good to me, thanks!

@Gargron Gargron merged commit 08ae116 into mastodon:main Feb 11, 2021
chrisguida pushed a commit to Start9Labs/mastodon that referenced this pull request Feb 26, 2022
…astodon#15701)

* Fixed picture in picture compatibility error in WebUI when status is deleted

* Revert "Fixed picture in picture compatibility error in WebUI when status is deleted"

This reverts commit f003b7d.

* Close the modal display of the image when status is deleted

* Fixed the case statement before the default statement

* Removed unnecessary parts
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.

3 participants