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

Add a parameter for ContentDialog.ShowAsync() to specify the target window #248

Merged
merged 2 commits into from
Nov 28, 2022
Merged

Add a parameter for ContentDialog.ShowAsync() to specify the target window #248

merged 2 commits into from
Nov 28, 2022

Conversation

TSRBerry
Copy link
Contributor

@TSRBerry TSRBerry commented Nov 25, 2022

We are currently running into Linux specific issues with ContentDialog at Ryujinx, because for some reason our overlay window still has IsActive == false when ShowAsync() is called. On Windows it works fine, but on Linux we couldn't find a way to force IsActive == true even tho we were able to make sure it's true before we call ShowAsync().

This is why I created this method. Using this method we get consistent behavior across all platforms and don't need to worry about IsActive. Usually we would just use a subclass of ContentDialog for this, but since it accesses a lot of private variables, this wasn't really easy to do.

I bet this method also has other usecases, but I figured it might be best to explain why this was needed in the first place.

@TSRBerry
Copy link
Contributor Author

How does that look?

Copy link
Owner

@amwx amwx left a comment

Choose a reason for hiding this comment

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

Minor nit, otherwise looks good, thanks!

Also, I'll do a formal deprecation/removal of ContentDialogPlacement later in a separate PR so I won't worry here about preserving the optional parameter.

FluentAvalonia/UI/Controls/ContentDialog/ContentDialog.cs Outdated Show resolved Hide resolved
@TSRBerry
Copy link
Contributor Author

Could we also backport this to 1.4.4? So we could use this feature with our current version?
If you are okay with that should I create another PR or would this be handled differently?

@amwx amwx merged commit f01f6ec into amwx:master Nov 28, 2022
@amwx
Copy link
Owner

amwx commented Nov 28, 2022

Could we also backport this to 1.4.4? So we could use this feature with our current version? If you are okay with that should I create another PR or would this be handled differently?

This *should* be possible since its a fairly straight-forward change - but master's source is v2/11.0 so I can't just cherry-pick the commit. I'll see what I can do sometime this week.

@TSRBerry
Copy link
Contributor Author

I thought the code for 1.4.4 for ContentDialog is the same as for master right now, so cherry-picking that commit should work, but if it's different and I'm misremembering then I'm not sure what the best approach would be.

Thank you for helping us! :D

@amwx
Copy link
Owner

amwx commented Nov 28, 2022

I thought the code for 1.4.4 for ContentDialog is the same as for master right now, so cherry-picking that commit should work, but if it's different and I'm misremembering then I'm not sure what the best approach would be.

Thank you for helping us! :D

#242 is the most recent change to ContentDialog which are v2 fixes only (theres a couple breaking changes and new APIs that rely on stuff added only in the v2 stuff), and there were a couple changes within ShowAsync that would've conflicted. I'm pretty much full steam ahead on v2/11.0 now, and because there's so many big changes on going most stuff won't be backported unless its a critical bug fix.
Anyway, I've pushed 1.4.5 package with this fix in it so hopefully that helps

@TSRBerry
Copy link
Contributor Author

Thank you very much!

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.

2 participants