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

feat: customize button text, Close #72 #74

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

meowtec
Copy link
Contributor

@meowtec meowtec commented Jun 12, 2022

Use TaskDialogIndirect to show the message dialog because it supports customizing button texts.

Besides, TaskDialogIndirect has a nicer look than MessageBoxW.

MessageBoxW:
Screenshot 2022-06-12 213009

TaskDialogIndirect:
Screenshot 2022-06-12 212229

While TaskDialogIndirect is only available with comctl32 version 6, but the OS inbox version is stuck at 5.x. (ref microsoft/windows-rs#1294 (comment))
So I added a cargo feature namedcommon-controls-v6 for enabling v6.

The change has been tested on Windows, Linux and macOS, but not on WASM.

@meowtec meowtec force-pushed the custom-message-button-text branch 2 times, most recently from f067714 to e8ee540 Compare June 12, 2022 14:59
Copy link
Owner

@PolyMeilex PolyMeilex left a comment

Choose a reason for hiding this comment

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

Would it make sense to make common-controls-v6 the default feature? I don't care about any OS older than Windows 10 and maybe in extreme cases Windows 8.

EDIT: Or maybe remove the feature all together, I'll leave that for you to decide as I don't have a strong preference.

let res = rfd::MessageDialog::new()
.set_title("Msg!")
.set_description("Description!")
// .set_buttons(rfd::MessageButtons::Ok)
Copy link
Owner

Choose a reason for hiding this comment

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

This comment looks unnecessary

@PolyMeilex
Copy link
Owner

Also WASM CI is complaining about wrong match patterns

@meowtec
Copy link
Contributor Author

meowtec commented Jun 13, 2022

Would it make sense to make common-controls-v6 the default feature? I don't care about any OS older than Windows 10 and maybe in extreme cases Windows 8.

EDIT: Or maybe remove the feature all together, I'll leave that for you to decide as I don't have a strong preference.

When an application use some other API from COMCTL32.dll, will this API be imcompatible? (It originally work well with COMCTL32.dll v5.8, but we force the app to use v6.)

I don't know it because I am not a windows developer. I think it's better to keep the feature and make it default, collect feedback and consider to remove the feature and old API in the feature.

@meowtec meowtec force-pushed the custom-message-button-text branch from e8ee540 to b8a446f Compare June 13, 2022 02:54
@meowtec meowtec force-pushed the custom-message-button-text branch from b8a446f to ebc50a6 Compare June 13, 2022 03:00
Copy link
Owner

@PolyMeilex PolyMeilex left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

@PolyMeilex PolyMeilex merged commit 9b2d38c into PolyMeilex:master Jun 13, 2022
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.

None yet

2 participants