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

DIRECTOR: LINGO: Implement traceLogFile Lingo Property #4031

Merged
merged 3 commits into from Jul 2, 2022

Conversation

r41k0u
Copy link
Contributor

@r41k0u r41k0u commented Jun 21, 2022

This change implements traceLogFile and the required function in g_debugger. There is a new wrapper of debugPrintf which will log the stdout and stderr output in the log file specified. Functionality has been tested and us found consistent to that of the original

Copy link
Member

@sev- sev- left a comment

Needs more work

void Debugger::debugLogFile(Common::String logs) {
debugPrintf("%s", logs.c_str());
if (!g_director->_traceLogFile.empty()) {
Common::DumpFile out;
Copy link
Member

@sev- sev- Jun 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. I would move this variable to the class and then open and close it only once per session. Close it in the destructor.

Copy link
Contributor Author

@r41k0u r41k0u Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would present a problem.
What if the filename changes during the session, as in the test movie? In that case, a new log file is created, and both the logfiles exist. Closing the DumpFile object only once per session won't be able to handle change of log file name when the movie is running.

Copy link
Contributor Author

@r41k0u r41k0u Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I was abe to find a workaround which would do what you had advised and also solve the above problem

@@ -1130,7 +1132,10 @@ void Lingo::setTheEntity(int entity, Datum &id, int field, Datum &d) {
g_lingo->_traceLoad = d.asInt();
break;
case kTheTraceLogFile:
setTheEntitySTUB(kTheTraceLogFile);
if (d.asString().size())
g_director->_traceLogFile = ConfMan.get("path") + "/" + d.asString();
Copy link
Member

@sev- sev- Jun 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to write here a test that the path is writeable. Perhaps open the file and seek to the end.

Copy link
Contributor Author

@r41k0u r41k0u Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used FSNode's isWriteable() to test the same now. Is that fine or I shall go by the seeking method?

Copy link
Member

@sev- sev- Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this would work.

@r41k0u r41k0u force-pushed the traceLog branch 2 times, most recently from c07c363 to bbdac02 Compare Jun 22, 2022
@sev-
Copy link
Member

@sev- sev- commented Jun 28, 2022

THis has to be rebased and code formatting fixed.

@sev-
Copy link
Member

@sev- sev- commented Jul 2, 2022

Hello? @r41k0u

@sev-
Copy link
Member

@sev- sev- commented Jul 2, 2022

Merging, however, I will add after this a warning() if the log file is not writeable.

@sev- sev- merged commit ca35334 into scummvm:master Jul 2, 2022
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants