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

Made FadingMessageWindow implement all UI logic on the UI thread #1375

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

tombogle
Copy link
Contributor

@tombogle tombogle commented Jan 27, 2025

This fixes crashes like SP-2340
I have marked this as a breaking change because of the removal of protected members, but I know of no derived classes, and in practice, the existing implementation was such that these members would be unlikely to have been meaningful or helpful in a derived class anyway.


This change is Reviewable

@tombogle tombogle requested a review from josephmyers January 27, 2025 22:04
@tombogle tombogle self-assigned this Jan 27, 2025
Copy link

github-actions bot commented Jan 27, 2025

Palaso Tests

     4 files  ±0       4 suites  ±0   17m 4s ⏱️ -18s
 4 880 tests ±0   4 652 ✅ ±0  228 💤 ±0  0 ❌ ±0 
15 890 runs  ±0  15 206 ✅ ±0  684 💤 ±0  0 ❌ ±0 

Results for commit 1f7f59a. ± Comparison against base commit a4215d3.

♻️ This comment has been updated with latest results.

@tombogle tombogle requested a review from hahn-kev February 3, 2025 22:41
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

works for me, though the popup message was not relative to the test app form which was surprising.

@tombogle
Copy link
Contributor Author

tombogle commented Feb 4, 2025

Yeah, I probably could have (and maybe should have) gotten it to pop up in a more sensible place, but for the test app I didn't care. I just wanted to make sure it appeared and faded as expected.

Copy link
Contributor Author

@tombogle tombogle left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @hahn-kev and @josephmyers)

CHANGELOG.md Outdated Show resolved Hide resolved
@tombogle tombogle force-pushed the sp-2340-create-fading-message-on-ui-thread branch from bed9607 to 1f7f59a Compare February 4, 2025 05:15
@tombogle tombogle merged commit d237a40 into master Feb 5, 2025
7 of 8 checks passed
@tombogle tombogle deleted the sp-2340-create-fading-message-on-ui-thread branch February 5, 2025 13:19
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