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

Add news messages to crashlog #5722

Closed
DorpsGek opened this issue Aug 24, 2013 · 3 comments
Closed

Add news messages to crashlog #5722

DorpsGek opened this issue Aug 24, 2013 · 3 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

LordAro opened the ticket and wrote:

As per the Todo list, i have added pending news messages to the crashlog

hg patch queue, based on r25739
patch 02_ isn't strictly necessary, but is nice to have :)

However, i would like some feedback regarding patch 03 -
Alberth pointed out to me that converting StringIDs to text after a crash probably isn't very safe.
So, should i store a char* of the message in the NewsItem object, or just list the relevant numbers from the NewsItem? Or something else altogether?

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/5722
@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 6, 2013

frosch wrote:

Does 01 change anything, or does it only move functions?
While a function name "GetReferenceTile" is fine when it is static to a file, it's not exactly suitable for a globally accessible thing.

Wrt. 03:
WriteNewsString definitely needs a "last" parameter to denote the end of the buffer.
"(%i-%i-%i) %s\n" could be "(%i-%02i-%02i) %s\n"
But I agree with Albert. The text message might be troublesome, since news serveral times caused crashes in the past, when they referenced invalid entities from the past. So, maybe print NewsItem members: type, references, stringid, stringparams


This comment was imported from FlySpray: https://bugs.openttd.org/task/5722#comment12658

@DorpsGek
Copy link
Member Author

andythenorth wrote:

frosch123: andythenorth: it's on the offical todo list
LordAro: andythenorth: i think it could probably be finished relatively easily


This comment was imported from FlySpray: https://bugs.openttd.org/task/5722#comment14630

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 7, 2018
@TrueBrain
Copy link
Member

@LordAro : sorry for the 5 year delay, but could you make a PR out of this? :D

@TrueBrain TrueBrain added patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay and removed enhancement from FlySpray labels Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

2 participants