[WIP/DO NOT MERGE] Implementation of COLOR command

[Luca1991]

This is a WIP PR, it isn't ready for merging yet. I've created it so you can review my code as I write it (since I'll modify quite a bit of code in order to implement this command).

From my initial research, most of the code changes will be in Wnd.cpp/Wnd.h and in DisasmWnd.cpp/DisasmWnd.h.

[Luca1991]

Implemented help bar support (blue with purple text in this example, 0x95):

[Luca1991]

Implemented title bar support (purple with brown text in this example, 0x56):

[Luca1991]

Implemented reverse and bold. In this example: Reverse (eip indicator) -> green with black text (0x20) Bold (jump indicator) -> cyan with red text (0x34)

These two values only affects the disasm window. AFAIK this is the same as in the original SoftICE. Please correct me if I'm wrong.

The only missing color is the "normal" one, that is a bit more complex to implementes because it affects multiple functions.

EDIT: I am aware of a bug that occurs when entering an hex value containing A-F (in Reverse and Bold fields). I'm working on it. FIXED

[Luca1991]

Ok we are almost there! "normal" color is now supported. This is an example with this command: "color b5 35 20 95 56": And this is the same command in the original SoftICE:

As far as I can see, this is working correctly. Well, "almost correctly", here is why:

1. Bug Checker uses another color for "bold" in Regs window. This makes this implementation incompatible with the original SoftICE one. What should I do with the highlighted registers? Should we add a new params to the color command? This would allow us to specify another color, but would make it incompatilbe with the SoftICE "themes".

2. After executing the color command, the effects are visible only after manually execute a command that refresh the disasm/regs window (like "t"). I don't know if there is a way to force a full redraw of the Bug Checker UI.

@vitoplantamura please let me know what you think, any feedback is welcome! :)

EDIT: I still have to document the color command in the README.md (I'll will do this tomorrow).

[vitoplantamura]

hi Luca,

I need to take a better look at your changes, but in the meantime I will try to answer your questions:

1. What happens when you modify the colors in SoftICE? I mean, what color it uses to highlight a changed register value after executing the COLOR command? With the default colors, it should use cyan IIRC (I don't have SoftICE installed so I cannot check out).

2. Try replacing the calls to DrawAll_End/Final with "params.result = CmdParamsResult::RedrawUi;".

Thank you, --vito

[Luca1991]

Hi Vito,

What happens when you modify the colors in SoftICE? I mean, what color it uses to highlight a changed register value after executing the COLOR command? With the default colors, it should use cyan IIRC (I don't have SoftICE installed so I cannot check out).

You are correct, the default color used to highlight the changed registers is cyan:

This is what happens on SoftICE when I set the custom "theme" I'm using for my tests (color b5 35 20 95 56), after some stepping: (it looks very ugly..)

In SoftICE API names in CALL aren't highlighted tho (Validate_VM_Handle in this example): But in BugChecker they are highlighted in yellow.

These same issues apply also to the Code Window.

do you have any thoughts/preference on how to handle these cases? If you need some more testing in SoftICE just let me know :)

Try replacing the calls to DrawAll_End/Final with "params.result = CmdParamsResult::RedrawUi;".

Yeah, this fixed the issue. Thanks! :)

PS: I've added the COLOR command to the command list in README.md

[vitoplantamura]

hi Luca,

For the changed registers color, the code could detect the condition where the color is equal to the new background color, and then use a map of default highlight colors for each background color. This map should be something like this:

New Background Color -> Highlight Color 0 -> 7 1 -> 3 2 -> 9 ... 15 -> 2

NOTE: I used random numbers here for the second value.

The code should use this map only when the new background color == default highlight color.

NOTE2: when you set a breakpoint, that breakpoint is highlighted in the Disassembler window with the cyan color. Maybe we should use this trick here as well.

NOTE3: the BL command also uses custom colors.

NOTE4: maybe we should implement the "RESET" option for the COLOR command, as in the original SoftICE.

NOTE5: in the code, please replace "char current_colors[30];" with "CHAR currentColors[64];".

For the symbol names in the Disassembler window, we can keep them yellow, but we should use the color map described above to change their color if it is equal to the background color.

In the script window, we could modify only the background color in order to match the background color of the other windows.

Thanks! --vito

[Luca1991]

Hi Vito, I'll comment your NOTES in reverse order :)

NOTE5: in the code, please replace "char current_colors[30];" with "CHAR currentColors[64];".

Fixed!

NOTE4: maybe we should implement the "RESET" option for the COLOR command, as in the original SoftICE.

Implemented and documented!

NOTE3: the BL command also uses custom colors.

I've edited the code to use the "normal" color.

NOTE: I used random numbers here for the second value. The code should use this map only when the new background color == default highlight color.

Yeah, good idea. I think that this map should contain the "reverse" of the affected color. This map should be used wherever a custom "highlight" color is used (when this color is also used as "normal" color):

• Register Window
• Disassembler Window
• BL Command

Tomorrow I'll try to implement this map.

Thanks, Luca

[Luca1991]

Hi, I've done some research and I figured out that this should be the color conversion map:

0 (black)         ->   F (white)

1 (blue)          ->   E (yellow)

2 (green)         ->   D (light magenta)

3 (cyan)          ->   C (light red)

4 (red)           ->   B (light cyan)

5 (magenta)       ->   A (light green)

6 (brown)         ->   9 (light blue) 

7 (grey)          ->   8 (dark grey)

8 (dark grey)     ->   7 (grey)

9 (light blue)    ->   6 (brown)

A (light green)   ->   5 (magenta)

B (light cyan)    ->   4 (red)

C (light red)     ->   3 (cyan)

D (light magenta) ->   2 (green)

E (yellow)        ->   1 (blue)

F (white)         ->   0 (black)

Basically every color code is reversed. This should be used if a custom color is the same as the highlight color. I hope to roll out these changes over the weekend :)

[vitoplantamura]

Perfect! :-)

Thanks, --vito

[Luca1991]

Hi @vitoplantamura sorry for the delay, I had a very busy week. So, I've fixed the highlight colors. I think this PR can now be considered complete (apart from the two minor known issues). Please review it carefully bofore to merging!

KNOWN ISSUES:

1. Since the Simbol Epilogue (in DisasmWnd) has 0x07 as its custom color, which is the same as the default normal color, the result is a reversed color:

2. For some reason, the CodeWindow doesn't auto-refresh when changing colors. You have to move the cursor over each line to refresh it.

---

Please let me know if there is anything I need to fix. Thanks a lot, Luca

[vitoplantamura]

hi Luca,

I need to take a better look at your changes, but in the meantime I will try to answer your questions:

1. You should not check for "Wnd::nrmClr != 0x07", but instead you should simply pick the value of "Wnd::nrmClr".

For example, this code:

Wnd::nrmClr != 0x07 ? "07" : Utils::HexToString(Utils::NegateByte(Wnd::nrmClr), sizeof(BYTE))

becomes:

Utils::HexToString(Wnd::nrmClr, sizeof(BYTE))

Does it make sense?

2. in CodeWnd.cpp, please add this method:

VOID CodeWnd::SyntaxColorAll() { for ( ULONG y = 0; y < contents.size (); y ++ ) SyntaxColor( y ); }

and call it before or after "params.result = CmdParamsResult::RedrawUi;" in Cmd_COLOR.cpp.

Thanks, --vito

[Luca1991]

Hi @vitoplantamura I've just pushed the fixes, thanks!

You should not check for "Wnd::nrmClr != 0x07", but instead you should simply pick the value of "Wnd::nrmClr". For example, this code:

Wnd::nrmClr != 0x07 ? "07" : Utils::HexToString(Utils::NegateByte(Wnd::nrmClr), sizeof(BYTE))

becomes:

Utils::HexToString(Wnd::nrmClr, sizeof(BYTE))

Does it make sense?

Yep, that makes sense, I wasn't sure if that's what you were expecting (Simbol Epilogue without highlight), that's why I asked :)

in CodeWnd.cpp, please add this method: VOID CodeWnd::SyntaxColorAll() { for ( ULONG y = 0; y < contents.size (); y ++ ) SyntaxColor( y ); }

and call it before or after "params.result = CmdParamsResult::RedrawUi;" in Cmd_COLOR.cpp.

Fixed, thanks!

I need to take a better look at your changes

Sure! Take your time. This PR has changed quite a bit of code, so please review it carefully before to merge. I'll wait patiently for any other fixes on my side :)

[vitoplantamura]

hi Luca,

here are some considerations/notes:

1. please remove the NegateByte method and replace it with these two methods:

eastl::string Utils::GetColor( BYTE clr ) { return HexToString(clr, sizeof(BYTE)); }

eastl::string Utils::GetColorSpecial( BYTE clr ) { if ( ! ( clr & 0xF0 ) ) clr |= Wnd::nrmClr & 0xF0; else if ( ( clr & 0xF0 ) == ( Wnd::nrmClr & 0xF0 ) ) clr = ( ~clr & 0xF0 ) | ( clr & 0x0F );

if ( clr == Wnd::nrmClr ) clr = ( clr & 0xF0 ) | ( ~clr & 0x0F );

return HexToString(clr, sizeof(BYTE)); }

These methods should be called whenever a color is converted to a string, instead of calling HexToString directly.

For example, "Wnd::nrmClr != 0x04 ? "04" : Utils::HexToString(Utils::NegateByte(Wnd::nrmClr), sizeof(BYTE)))" becomes "Utils::GetColorSpecial(0x04)".

I have no way of testing the GetColorSpecial method. Its functioning is self explanatory: let me know if everything renders correctly.

2. please replace "args[1] == "reset" || args[1] == "RESET"" with a call to Utils::AreStringsEqualI.

3. I don't understand why Cmd::ParseListOfBytesArgs accepts * as an argument.

4. please remove "colors.reserve(5);".

5. please replace "sprintf" with "::sprintf".

6. please replace the calls to eastl::vector::at with eastl::vector::operator[] in cmd_color.cpp.

Thanks, --vito

[Luca1991]

Hi Vito

please remove the NegateByte method and replace it with these two methods: eastl::string Utils::GetColor( BYTE clr ) { return HexToString(clr, sizeof(BYTE)); }

eastl::string Utils::GetColorSpecial( BYTE clr ) { if ( ! ( clr & 0xF0 ) ) clr |= Wnd::nrmClr & 0xF0; else if ( ( clr & 0xF0 ) == ( Wnd::nrmClr & 0xF0 ) ) clr = ( ~clr & 0xF0 ) | ( clr & 0x0F );

if ( clr == Wnd::nrmClr ) clr = ( clr & 0xF0 ) | ( ~clr & 0x0F );

return HexToString(clr, sizeof(BYTE)); }

These methods should be called whenever a color is converted to a string, instead of calling HexToString directly.

For example, "Wnd::nrmClr != 0x04 ? "04" : Utils::HexToString(Utils::NegateByte(Wnd::nrmClr), sizeof(BYTE)))" becomes "Utils::GetColorSpecial(0x04)".

I have no way of testing the GetColorSpecial method. Its functioning is self explanatory: let me know if everything renders correctly.

Yes I understand, but are you sure you want a Wnd reference in Utils? I think it would be better to pass Wnd::nrmClr as a second argument to GetColorSpecial function. Please let me know what do you think about this.

please replace "args[1] == "reset" || args[1] == "RESET"" with a call to Utils::AreStringsEqualI.

please remove "colors.reserve(5);".

please replace "sprintf" with "::sprintf".

please replace the calls to eastl::vector::at with eastl::vector::operator[] in cmd_color.cpp.

Fixed :)

I don't understand why Cmd::ParseListOfBytesArgs accepts * as an argument.

I'm a bit confused here :/ the * argument is the cmdId, no? I tried to keep this function similar to ParseListOfDecsArgs but I may have missed something. Please let me know.

---

I'd like to spend a moment to sincerely apologize for my mistakes and I'm grateful to you for pointing me to the right direction in order to fix them! Thanks!

[vitoplantamura]

hi Luca,

No need to apologize, we are here to enjoy ourselves writing software!

but are you sure you want a Wnd reference in Utils?

you are right! But I don't want to pass in Wnd::nrmClr because it would be redundant.

I would move GetColor and GetColorSpecial to the Wnd class.

I tried to keep this function similar to ParseListOfDecsArgs

ParseListOfDecsArgs is used, for example, in the BD command: "BD *" means "disable all breakpoints".

In the case of the COLOR command, "COLOR *" has no meaning.

Thanks, --vito

[Luca1991]

Hi Vito, I'm experiencing an issue with the GetColorSpecial function. If I don't use the COLOR command, it works correctly (passing 0x0B will return "0x0B"), but once a custom color is set, it produces weird results: for example if nrmClr is set to 0xB5, it will return 0xBB (but it should return 0x0B, since 0xB5 != 0x0B). Do you have any idea why?

Thanks, Luca

[vitoplantamura]

hi Luca,

try this version. The code I added is in bold:

eastl::string GetColorSpecial( BYTE clr ) { if ( ! ( clr & 0xF0 ) ) clr |= Wnd::nrmClr & 0xF0; else if ( ( clr & 0xF0 ) == ( Wnd::nrmClr & 0xF0 ) ) clr = ( ~clr & 0xF0 ) | ( clr & 0x0F );

if ( clr == Wnd::nrmClr || ( clr >> 4 ) == ( clr & 0x0F ) ) clr = ( clr & 0xF0 ) | ( ~clr & 0x0F );

return HexToString(clr, sizeof(BYTE)); }

Thanks, --vito

[Luca1991]

Hi Vito, I think your latest implementation of GetColorSpecial works fine :) I've been testing it for a while without any noticeable issues. I also removed the "*" from ParseListOfBytesArgs. Please let me know if there are any other fixes from my side!

Thanks, Luca

[vitoplantamura]

hi Luca,

one last thing: can you remove the "ULONG size" parameter from ParseListOfBytesArgs (since it is not used anymore)?

thanks, --Vito

[Luca1991]

hi Vito, you are right, it is not used. Removed :)

Thanks, Luca