Closed wake-0 closed 9 years ago
Commit looks good overall. However, ConsoleChangeDisplayedInformationType
should not be public API.
Refactored the code. Pls have a look @trylimits and close the ticket.
I guess you got me wrong :)
The console should not provide API to change the color to debug/error/info. It should provide functions for changing the color.
Or is there any other reason why those functions should be public API?
I did some testing with the implemented feature and I found a bug in the implementation:
Running the ps
command (which uses printf
output) uses the debug color for it's output (SBW = should be white). Imo the color changing is not done correctly. It should be done as follows:
... KernelDebug(..)
{
Change color to green
[...] Print the debug output
Reset color to white
}
Atm we are changing the color but we are missing to reset it back.
I do not want to change the color back because it is a lot of overhead. Normally you chose a KernelDebug, KernelInfo or KernelError to display something, so the color should be set always correctly.
But you are right this change is necessary at the moment. I will commit this change.
This statement is true only within the kernel. User processes share the same console but cannot use Kernel...(..)
functions to output something. User processes have to use printf(..)
which is not completely under our control. Resetting the color in Kernel..(..)
functions is therefore absolutely necessary. The overhead of sending 7 characters is acceptable.
Read commit above.
Output color is set correctly now:
Some minor changes should still be done:
1.) The public API of the console should be ConsoleColor(color_t)
2.) Instead of calling ConsoleDisplayInfo()
for resetting the color we should call ConsoleColor(white)
As we just discussed:
I agree that Kernelnfo(..)
should not print white. Blue would be a nice color for KernelInfo(..)
.
Maybe this would be a better style for displaying the different types (debug, error, info):
I like the output as it is on master branch, i.e. the whole line is colored.
@Blackjack92 What purpose has KernelPrint(..)
? Imo we should refactor this to KernelVerbose(..)
which makes more sense in the context of printing something system-relevant. I'll open a new ticket for that. See #97
I agree this would be a nice feature.
KernelInfo
should however print in the default color.