There are serious issues with the encapsulation of the EM_SETMARGINS and EM_GETMARGINS messages in TEdit and TRichEdit:
- The implementations of TEdit::SetLeftMargin and TEdit::SetRightMargin are plain wrong — the parameters to MkUint32 are switched, resulting in those calls always setting a margin of 0.
- The implementation of TEdit::SetMarginUseFontInfo is plain wrong — the documentation states that for edit controls the EC_USEFONTINFO must be passed in lParam.
- The functions are marked as "deleted" from TRichEdit implementation with a comment that they are not supported for rich edit controls, but the documentation states they are, even though EC_USEFONTINFO is handled differently.
- The function TEdit::GetMargins currently returns uint32, which the caller has to split into hi and low words. It may be more convenient if it returns a structure in which those are already split.
- TRichEdit::GetMargins is marked as "deleted" and the documentation states that EM_GETMARGINS is not supported by rich edit controls, but my tests indicate that it works and returns the correct values.
Note that issues 1) and 2) are carried over all the way from OWL 5.02, while 3) and 5) were introduced in OWLNext 6.04.
Suggestions:
- Fix 1) and 2) in all LTS versions. Since the functions do not work correctly, most likely noone has ever used them, so no concerns about breaking existing code behavior.
- In the trunk, revamp the API:
- Add a SetMargins function, since both margins can be set at the same time.
- Remove TEdit::SetMarginUseFontInfo and instead make the margin parameters to SetLeftMargin, SetRightMargin and SetMargins to default to EC_USEFONTINFO.
- Make GetMargins return a structure with left and right margin members.
- Remove the "deleted" functions for TRichEdit, including GetMargins as it seems to work correctly.
- Implement SetMarginUseFontInfo for TRichEdit, where it would set both the left and right margins.
@jogybl wrote:
Agree, good catch! I trust your judgement on the other suggestions. And good to see you are preparing the branches for release. Full check-list for release can be found in [Contributing].
PS. For 6.44, if you have the chance and energy to test it works with all supported compilers, consider the suggested C++17 compliance patch suggested by @lfastrup [bugs:#557].
Related
Bugs:
#557Wiki: Contributing
The fixed SetMarginUseFontInfo function works well for Edit controls, but I cannot get it to work properly for Rich Edit controls
If the same code as for Edit is used:
it results in a blank (and seemingly very wide) rich edit control.
While using the code that the documentation states should work:
or even
SendMessage(EM_SETMARGINS, EC_USEFONTINFO, MkUint32(EC_USEFONTINFO, EC_USEFONTINFO));have no effect - no margin is added to the rich edit control.
On the other hand, the SetLeftMargin and SetRightMargin functions work fine for Rich Edit controls.
Diff:
Issues 1 and 2 (implementation bugs in TEdit::SetLeftMargin, SetRightMargin and SetMarginUseFontInfo) have been fixed on the trunk [r6583], and the fixes have been merged into branches 6.30 [r6597], 6.36 [r6598], 6.44 [r6599] and 7.0 [r6600].
I've retargeted the ticket to version 8 for the remaining API improvements.
Related
Commit: [r6583]
Commit: [r6597]
Commit: [r6598]
Commit: [r6599]
Commit: [r6600]
Changes to TEdit and TRichEdit implemented in [r8763] and [r8764].
Related
Commit: [r8763]
Commit: [r8764]