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

Conversation

@rpwjanzen
Copy link

@rpwjanzen rpwjanzen commented Mar 26, 2021

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
Loading
src/console.cpp Outdated Show resolved Hide resolved
Loading
src/console.cpp Outdated Show resolved Hide resolved
Loading
@LordAro
Copy link
Member

@LordAro 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

Loading

@rpwjanzen
Copy link
Author

@rpwjanzen rpwjanzen 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

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.

Loading

@rpwjanzen rpwjanzen force-pushed the replace-iconsoleprint-cc-error-with-iconsoleerror branch from e4eeff8 to dfebc88 Mar 26, 2021
@rpwjanzen rpwjanzen requested a review from LordAro Mar 26, 2021
src/console.cpp Show resolved Hide resolved
Loading
@rpwjanzen rpwjanzen force-pushed the replace-iconsoleprint-cc-error-with-iconsoleerror branch 3 times, most recently from a35e0b9 to 0ff5503 Mar 28, 2021
@rpwjanzen rpwjanzen requested a review from LordAro Mar 28, 2021
@rpwjanzen rpwjanzen force-pushed the replace-iconsoleprint-cc-error-with-iconsoleerror branch from 0ff5503 to ef0e24b Mar 29, 2021
@rpwjanzen
Copy link
Author

@rpwjanzen 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.

Loading

@LordAro
Copy link
Member

@LordAro 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.

Loading

@rpwjanzen
Copy link
Author

@rpwjanzen rpwjanzen commented Apr 6, 2021

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.

Loading

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

Successfully merging this pull request may close these issues.

None yet

2 participants