#199 Ability to change the text background color in TRichEditView

[Ognyan Chernokozhev]

TRichEditView provides built-in menu items to change the text font and color, but there is no function to change the background color.

See mailing list discussion.

[Ognyan Chernokozhev]

Uncovered a bug in TCharFormat::SetBkColor: [bugs:#529].

[Ognyan Chernokozhev]

Implemented in 6.44 [r5837], 7.0 [r5839] and trunk [r5838].

[Vidar Hasfjord]

Hi Ognyan, good work! Here are are some comments after a quick code review:

The tip text (after '\n' in the texts below) for CM_FORMATBKCOLOR is the same as for CM_FORMATCOLOR:

CM_FORMATCOLOR,     "Formats the selection with color\nColor"
CM_FORMATBKCOLOR,   "Formats the selection with background color\nColor"

Change to "Background" or "Background color"?

Edit: This issue was fixed in [r5880] on the trunk, and the fix has been merged into 6.44 [r5881] and 7.0 [r5882], in which the tip text was set to "Background".

Also, if TColor::None is passed to TCharFormat::SetBkColor, you now set CFE_AUTOBACKCOLOR. Makes sense. Should we also handle TColor::Transparent in some way, e.g. clear CFM_BACKCOLOR? Or ignore it? Or throw an exception? Or just use a PRECONDITION to make it explicit that it is forbidden? Looking at the code, perhaps it would be more logical if TColor::None clears CFM_BACKCOLOR and TColor::Transparent sets CFE_AUTOBACKCOLOR? In any case, documentation should be added.

[Ognyan Chernokozhev]

I think the simplest would be to set CFE_AUTOBACKCOLOR for both TColor::None and TColor::Transparent. Same for SetTextColor.

If a user needs more fine-grained control over the dwMask and dwEffects members - well, they are public, so they can be set any way the user wants.

[Vidar Hasfjord]

@jogybl wrote:

I think the simplest would be to set CFE_AUTOBACKCOLOR for both TColor::None and TColor::Transparent.

Yeah, unfortunately, changing the behaviour for TColor::None would not match current behaviour in SetTextColor (although I think it would have been nicer to have it clear the mask, as if no colour was set).

In addition, passing TColor::SysWindow should set CFE_AUTOBACKCOLOR, rather than immediately convert that symbolic colour to RGB like now. After all, CFE_AUTOBACKCOLOR means using the system background colour, regardless of any later changes to the current setting of that system colour. So it makes more sense to interpret TColor::SysWindow symbolically.

Same for SetTextColor.

For the same reason argued above, passing TColor::SysWindowText to SetTextColor should set CFE_AUTOCOLOR (like TColor::None currently does).

However, it makes no sense for TColor::Transparent to do so. Instead, I propose TColor::Transparent is replaced by the current value of TColor::SysWindow, if passed. That will give the appearance of transparency at least.

I've implemented these changes in [r5841], including improved documentation.

PS. Since, you have updated the release branches, remember to update the version number and set the OWL_PRERELEASE flag.

[Vidar Hasfjord]

This feature has been targeted towards and implemented in 6.44, but is it ABI compatible? The implementation changes the definition of some command constants:

-#define CM_PARAGRAPHLEFT    24378
-#define CM_PARAGRAPHCENTER  24379
-#define CM_PARAGRAPHRIGHT   24380
-#define CM_PARAGRAPHBULLET  24381
+#define CM_PARAGRAPHLEFT    24379
+#define CM_PARAGRAPHCENTER  24380
+#define CM_PARAGRAPHRIGHT   24381
+#define CM_PARAGRAPHBULLET  24382

But I don't see why the renumbering is needed.

[Ognyan Chernokozhev]

Because of this:

//for menu CM_PARAGRAPHLEFT-1
#define CM_PARAGRAPHLEFT    24379

I recall there was some logic for submenus to use the ID of the first item minus one for enabling/disabling the parent menu item.

But, yeah, that renumbering is risky, I can try to see what happens if an old dynamic build of the RichEditor example is started with a new DLL.

Perhaps it will be better if I change CM_FORMATBKCOLOR to use some higher value after the CM_PARAGRAPH* ones.

[Vidar Hasfjord]

@jogybl wrote:

Because [the need to avoid collision with CM_PARAGRAPHLEFT-1]

Ah, I see. I thought so, but I was confused by "all_idds.rh", where there is a typo: Here CM_FORMATBKCOLOR is defined as 24372 (same as CM_FORMATCOLOR), unlike in "richedv.rh", where it is correctly defined as 24377, which as you point out requires room to be made to uphold sequential numbering.

#define CM_FORMATFONT       24371
#define CM_FORMATCOLOR      24372
#define CM_FORMATBOLD       24373
#define CM_FORMATITALIC     24374
#define CM_FORMATUNDERLINE  24375
#define CM_FORMATSTRIKEOUT  24376
+#define CM_FORMATBKCOLOR    24372

Note that "all_idds.rh" has been eliminated in the 7 series.

Perhaps it will be better if I change CM_FORMATBKCOLOR to use some higher value after the CM_PARAGRAPH* ones.

Yes, that should circumvent any issue by leaving the current definitions alone. Except for readability, the assigned constant shouldn't matter, as long as it does not clash with the other commands or submenus (which as pointed out gets an ID one less than the ID of the first command).

Edit: The old commands have now had their IDs reverted in 6.44 [r5864], trunk [r5865] and 7.0 [r5866].

[Vidar Hasfjord]

I can try to see what happens if an old dynamic build of the RichEditor example is started with a new DLL.

Please do, even if leaving the old constants alone. There is a possibility that the new functions that have been added somehow changes the ABI. In particular, I'm thinking about changes in the exported function ordinals in the library files (although I vaguely recall that we may have tested this before and found no issue when adding functions).

[Ognyan Chernokozhev]

I think that the standard way of accessing C++ DLLs is through generated import libraries, that refer to exported functions by name and not by ordinals.