Review EM_* message correctness#1234
Conversation
|
/cc: @hughbe |
|
I'm so sorry you had to do this! |
|
Apologies accepted 🤝 😃 It would be great if you could strive for smaller and targeted PRs in the future. |
|
wow, its really unfortunate that the windows header files You might want to add some comments to the definition class to make sure these mistakes are not repeated |
Please feel free to add comments where you feel appropriate. |
|
@RussKie sorry having to ask, but how do you want me to do that? Comment on the PR with the "suggested change" function? Separate PR after this is merged? Some other way to add changes to a not-yet-merged PR I don't know of? |
That will work. Everyone is welcome to review and comment on all submissions. |
|
@Olina-Zhang I've run the same code as provided in #994 and it appears to be working: The different value is probably due to the default font change (#656). |
JeremyKuhne
left a comment
There was a problem hiding this comment.
Given the current schedule I don't think you need to address my comment for 3.0 as I'm only 99% sure. :)
There was a problem hiding this comment.
These first four EM_GETLIMITTEXT , EM_POSFROMCHAR, EM_CHARFROMPOS, and EM_SCROLLCARET are a little odd. The "normal" definitions should work with the RichEdit control from what the headers/docs seem to indicate. It may be safer and less confusing to remove them.
There was a problem hiding this comment.
🤔 These existed in the original definition:
Their values (as defined in richedit.h) are different from those defined in winuser.h (thanks @weltkante for the pointers). Though looking at the definitions in richedit.h the messages are conditionally defined:
#ifndef EM_GETLIMITTEXT
#define EM_GETLIMITTEXT (WM_USER + 37)
#endif
#ifndef EM_POSFROMCHAR
#define EM_POSFROMCHAR (WM_USER + 38)
#define EM_CHARFROMPOS (WM_USER + 39)
#endif
#ifndef EM_SCROLLCARET
#define EM_SCROLLCARET (WM_USER + 49)
#endifDo you think it is safe to remove these four messages?
How can we test (I'm outside my depth here)?
There was a problem hiding this comment.
The #ifndef is probably just there to suppress macro redefinition warnings. Depending on which version is imported first the RTF messages may be visible to C/C++ code. However I don't know if its even possible to import richedit.h without winuser.h - when I try to import richedit.h as the only header VS intellisense still says its using the version from winuser.h - so it may be the case that those particular conflicting messages never actually are used in C/C++ code. In this case it would be safe to remove I think.
I'll have another look and try to verify the #include order to make sure whether it is possible to use the RTF version at all in C/C++
There was a problem hiding this comment.
I can verify that its impossible to include richedit.h before winuser.h in the windows 10 headers because richedit.h includes winuser.h indirectly.
1>------ Rebuild All started: Project: TestProject, Configuration: Debug Win32 ------
1>test.cpp
1>Note: including file: C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\Richedit.h
1>Note: including file: C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\shared\wtypes.h
1>Note: including file: c:\program files (x86)\windows kits\10\include\10.0.17763.0\shared\rpc.h
1>Note: including file: C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\windows.h
1>Note: including file: C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\winuser.h
Therefore C/C++ code will never see the RTF-specific conflicting messages due to the #ifndef never being true.
I think its safe to drop these constants from WinForms if native code never can use them short of redefining the constant under another name.
There was a problem hiding this comment.
Awesome, thanks heaps for the investigation!
As part the dotnet#791 refactor EM_* messages got mixed with RTB.EM_* messages that led to an issue described in dotnet#994. * Restore correct EM_ messagest * Separate RTB.EM_ messages * Update RichTextBox to use RTB.EM_* messages Fixes dotnet#994
| internal static partial class Interop | ||
| { | ||
| /// <summary> | ||
| /// RichTextBox Control Messages. Note that some messages have the same name but different value compared to normal Edit Control Messages. |
There was a problem hiding this comment.
Nit: the second part of the comment isn't relevant now that you've removed the dupes. :)
There was a problem hiding this comment.
I think it is still relevant, since there dups in richedit.h; if someone decides to copy them they may be (more) aware.
Proposed changes
As part the Add Message tests #791 refactor
EM_*messages got mixed withRTB.EM_*messages that led to an issue described in TextBox and MaskedTextBox.GetPositionFromCharIndex() and MaskedTextBox.GetCharIndexFromPosition() methods return the incorrect values #994.Fixes #994