Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace IConsolePrint*(F)( calls with specific console level calls #8894

Closed
wants to merge 7 commits into from
Closed

Replace IConsolePrint*(F)( calls with specific console level calls #8894

wants to merge 7 commits into from

Conversation

rpwjanzen
Copy link

Motivation / Problem

Implements changes mentioned in #8853 with a few added methods for consistency

Description

Added IConsoleF( and IConsole( methods to make console logging more consistent
Updated calls to IConsoleF( and IConsole( to new methods

Limitations

Formatting of messages with log level text was inconsistent in the existing code. I chose to not modify the current output contents to prefix with the log level. In some places the logged level did not match the apparent severity of the message. I did not update the function calls to match what I though the apparent severity was.

I chose to have the IConsoleDebugF( method signature match the other IConsole%LogLevel%F methods instead of the IConsoleDebug method.

There is at least one place where I did not update the IConsolePrint(CC_WARNING method to use IConsoleWarning( as the message may not have been output depending on the GUI developer setting.

Can be squashed to one commit and reworded if requested.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

src/console.cpp Outdated Show resolved Hide resolved
src/console.cpp Outdated Show resolved Hide resolved
src/console.cpp Outdated Show resolved Hide resolved
@LordAro
Copy link
Member

LordAro commented Mar 26, 2021

Ah yes, I hadn't considered that va_args are a pain to deal with. ...perhaps C++ variadic templates would be nicer? Quickly getting more complicated though, can certainly understand if don't want to go with that

e.g. https://stackoverflow.com/a/46254539/995325

@rpwjanzen
Copy link
Author

Ah yes, I hadn't considered that va_args are a pain to deal with. ...perhaps C++ variadic templates would be nicer? Quickly getting more complicated though, can certainly understand if don't want to go with that

e.g. https://stackoverflow.com/a/46254539/995325

I think that using variadic templates requires moving the now-templated function definitions into the console_func.h header file. The methods refer to the ClientSettings _settings_client that is defined in settings.cpp and not "exported". I don't think we want to make that change. I've never done C++ development before and this is new to me.

@rpwjanzen rpwjanzen requested a review from LordAro March 26, 2021 17:47
@rpwjanzen rpwjanzen requested a review from LordAro March 28, 2021 15:37
@rpwjanzen
Copy link
Author

rpwjanzen commented Mar 31, 2021

@LordAro I force-pushed an cannot see the "requested changes". I believe I addressed all of them. I'm used to using another system that does not "lose" comments when a force push occurs.

@LordAro
Copy link
Member

LordAro commented Apr 6, 2021

I'm not really sure about this PR, as is.

It adds a lot of extra functions, and doesn't even fix the original issue (inconsistent message prefix)

In its current form, the various IConsole*F functions are probably undesirable - They're only setting the CC_ code, which can be done just as easily by the caller of the function. On the other hand, if they also set the correct prefix, then they would be helpful.

Then all of the IConsole* functions (with just a string parameter) aren't really wanted at all - just call the F variant directly.

@rpwjanzen
Copy link
Author

I would have appreciated it if you would have mentioned this up front, almost two weeks ago, instead of now. I do not see any mention of the " inconsistent message prefix" problem in the related issue's comments. If something is undesirable or unwanted please mention this up front. At this time, I do not plan to put any more effort into this PR. Feel free to drop it or improve it. I may take a look at it again in a week or two depending on my priorities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants