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

Standardise between User32.SendMessageW(this, ...) and this.SendMessage(...) #2300

Closed
hughbe opened this issue Nov 6, 2019 · 9 comments
Closed
Labels
code cleanup cleanup code for unused apis/properties/comments - no functional changes. 🚧 work in progress Work that is current in progress help wanted Good issue for external contributors

Comments

@hughbe
Copy link
Contributor

hughbe commented Nov 6, 2019

There are multiple different ways of sending a message to a control:

                SendMessage((int)ComCtl32.PBM.SETRANGE32, _minimum, _maximum);
                SendMessage((int)ComCtl32.PBM.SETSTEP, _step, 0);
                SendMessage((int)ComCtl32.PBM.SETPOS, _value, 0);
                User32.SendMessageW(this, (User32.WindowMessage)ComCtl32.PBM.SETBKCOLOR, IntPtr.Zero, (IntPtr)ColorTranslator.ToWin32(BackColor));
                User32.SendMessageW(this, (User32.WindowMessage)ComCtl32.PBM.SETBARCOLOR, IntPtr.Zero, (IntPtr)ColorTranslator.ToWin32(ForeColor));

These two (User32.SendMessageW and SendMessage) are functionally the same but we're not consistent in how we use them

What should we standardise on?
/cc @JeremyKuhne

@JeremyKuhne
Copy link
Member

The thought was to put everything in User32.WindowMessage, even if there are duplicates for given numeric values. That gives the best discoverability and tersest code (in particular with switch statements).

User32.SendMessageW(this, (User32.WindowMessage)ComCtl32.PBM.SETBKCOLOR, IntPtr.Zero, (IntPtr)ColorTranslator.ToWin32(BackColor));
User32.SendMessageW(this, (User32.WindowMessage)ComCtl32.PBM.SETBARCOLOR, IntPtr.Zero, (IntPtr)ColorTranslator.ToWin32(ForeColor));

Becomes:

User32.SendMessageW(this, User32.WindowMessage.PBM_SETBKCOLOR, IntPtr.Zero, (IntPtr)ColorTranslator.ToWin32(BackColor));
User32.SendMessageW(this, User32.WindowMessage.PBM_SETBARCOLOR, IntPtr.Zero, (IntPtr)ColorTranslator.ToWin32(ForeColor));

I'm also keen on us structing out some of the defines, which would make these calls simpler and safer. In particular I want to create an LPARAM and WPARAM struct that can have safe explicit/implicit casts to/from common usage types (int, uint, WindowMessage, Color, etc.). It is easy to mess up when casting to IntPtr as it is effectively checked.

(On a somewhat related note, we may get native ints in 5.0)

@hughbe
Copy link
Contributor Author

hughbe commented Nov 6, 2019

I thought we were deliberately moving against putting things in WindowMessage enum and making our own enums

@JeremyKuhne
Copy link
Member

It isn't the way we did it in the designer. There is real value in having them all in the same enum (reduces casting, improves discoverability), I don't know that there are any positives to having them separated out that would outweigh that.

cc: @RussKie

@RussKie RussKie added api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation design-discussion Ongoing discussion about design without consensus labels Nov 7, 2019
@weltkante
Copy link
Contributor

weltkante commented Nov 7, 2019

Control specific message are reusing the same numeric values in the WM_USER range, having conflicting definitions in the same enum can cause confusion during debugging, when the debugger picks an arbitrary unrelated WindowMessage for display just for having the same value.

Personally I'd leave the WM_USER/WM_APP range (0x0400 to 0xBFFF) out of the central enum because you can't figure out the "right" meaning of the message without knowing which control you use. Also the "discoverability" of having control specific messages in the same enum is risking sending an inappropriate message to a control.

And (just to keep in mind) as an edge case we've seen with RichTextBox that there can be different numeric values associated with the same name.

@merriemcgaw merriemcgaw added the 🚧 work in progress Work that is current in progress label Mar 6, 2020
@merriemcgaw merriemcgaw added this to the Future milestone Mar 6, 2020
@RussKie RussKie modified the milestones: Future, 7.0 Aug 27, 2021
@dreddy-work dreddy-work added help wanted Good issue for external contributors and removed api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation labels Feb 28, 2022
@dreddy-work
Copy link
Member

dreddy-work commented Feb 28, 2022

Thanks @hughbe for pointing this out. This is ongoing cleanup work and would welcome community contributions here. We can go this iteratively (may be file by file or control by control).

@dreddy-work dreddy-work added code cleanup cleanup code for unused apis/properties/comments - no functional changes. and removed design-discussion Ongoing discussion about design without consensus labels Feb 28, 2022
@dreddy-work dreddy-work modified the milestones: .NET 7.0, Future Feb 28, 2022
@RussKie RussKie added up-for-grabs-temp help wanted Good issue for external contributors and removed help wanted Good issue for external contributors up-for-grabs-temp labels May 5, 2022
@ghost ghost modified the milestones: Future, Up-for-grabs May 5, 2022
@ghost
Copy link

ghost commented May 5, 2022

This issue is now marked as "up for grabs", and we’re looking for a community volunteer to work on this issue. If we receive no interest in 120 days, we will close the issue. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

@elachlan
Copy link
Contributor

elachlan commented Dec 9, 2022

Does the move to CsWin32 make this resolved?

#7445

@elachlan
Copy link
Contributor

@dreddy-work as far as I can tell the changes to using CsWin32 have resolved this issue. We have standardised to PInvoke.SendMessage.

@dreddy-work
Copy link
Member

Agree. If any remaining will be done soon.

@ghost ghost removed this from the Help wanted milestone Jan 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
code cleanup cleanup code for unused apis/properties/comments - no functional changes. 🚧 work in progress Work that is current in progress help wanted Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

7 participants