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

#1653: Create a log file and add it to crash report. #1657

Merged
merged 3 commits into from Jan 17, 2017

Conversation

kduske
Copy link
Collaborator

@kduske kduske commented Jan 16, 2017

Closes #1653. Also touched up the crash dialog a bit.

@kduske kduske self-assigned this Jan 16, 2017
@kduske kduske requested a review from ericwa January 16, 2017 22:23
@kduske kduske changed the title #1653: Create a log fileand add it to crash report. #1653: Create a log file and add it to crash report. Jan 16, 2017
Copy link
Collaborator

@ericwa ericwa left a comment

Choose a reason for hiding this comment

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

Looks good, I'm just wondering if there needs to be some code to enure the parent directory of logFilePath() is created (I tested on macOS only.)


namespace TrenchBroom {
FileLogger::FileLogger(const IO::Path& filePath) :
m_file(std::fopen(filePath.asString().c_str(), "w")) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Throw an exception in the constructor if fopen returns null?

}

FileLogger& FileLogger::instance() {
static FileLogger Instance(IO::SystemPaths::logFilePath());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see where the parent directory of IO::SystemPaths::logFilePath() is created, is that a potential problem?

Testing on macOS, I tried deleting ~/Library/Application Support/TrenchBroom and then launching TB, and the TrenchBroom directory was created and everything worked.

@kduske
Copy link
Collaborator Author

kduske commented Jan 17, 2017

Implemented all suggested changes.

@kduske kduske merged commit a7bd252 into release/v2.0.0 Jan 17, 2017
@kduske kduske deleted the feature/1653 branch November 6, 2017 20:25
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