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

Partially revert "Enable application log" to "Log to file" #2118

Merged
merged 9 commits into from
Feb 3, 2025

Conversation

shenlebantongying
Copy link
Collaborator

@shenlebantongying shenlebantongying commented Feb 2, 2025

Partially revert #2097

Problem: This is only useful for Windows where Console is a none-trivial thing for the user to use.

But for Linux, users are generally knows how to use terminal, and app writing messages to stdio is common.

The change in #2097 effectively disabled logging in terminal by default, which is confusing to Linux users.

src/main.cc Outdated
// Install message handler
qInstallMessageHandler( gdMessageHandler );
// Log to file enabled through command line or preference
if ( gdcl.logFile || cfg.preferences.enableApplicationLog ) {
Copy link
Owner

@xiaoyifang xiaoyifang Feb 3, 2025

Choose a reason for hiding this comment

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

If user set the option () through the preference , this line will never get the chance to run.
eg:
cfg.preferences.enableApplicationLog=false ,
then user set it through preference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes?

If the option is not enabled, the default message handler will be working (printing to stdio/console).

If the option is enabled, the custom message handler that prints to gd_log.txt will starts to work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logFilePtr is useless if the option is not enabled.

Copy link
Owner

Choose a reason for hiding this comment

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

which means the option in the preference will only work in the next time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I know what you mean now, and I have decided to move this thing out of main.cc

@shenlebantongying shenlebantongying enabled auto-merge (squash) February 3, 2025 07:28
@shenlebantongying shenlebantongying merged commit 0aedff4 into xiaoyifang:staged Feb 3, 2025
7 checks passed
Copy link

sonarqubecloud bot commented Feb 3, 2025

@xiaoyifang
Copy link
Owner

image

before this commit ,
If user start the application in Windows Console, the log will be output in the console.

This function does not work now. maybe related with this commit .

@shenlebantongying
Copy link
Collaborator Author

before this commit ,
If user start the application in Windows Console, the log will be output in the console.

This is not what happened before this commit.

Before this commit, there is no way to get console output.

Now you just need to disable log-to-file for logging to console again.

@xiaoyifang
Copy link
Owner

ok, seems introduced by me. :(

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.

2 participants