Skip to content

Adds Mac OS branch for the rotating log file opened at startup - #354

Merged
atsju merged 1 commit into
githubdoe:masterfrom
Lucassifoni:fix-macos-log-path
Aug 4, 2026
Merged

Adds Mac OS branch for the rotating log file opened at startup#354
atsju merged 1 commit into
githubdoe:masterfrom
Lucassifoni:fix-macos-log-path

Conversation

@Lucassifoni

Copy link
Copy Markdown
Contributor

Related to #353

I think we could remove the branching and standardize on getting a writable directory through QStandardPaths::writableLocation, what do you think ?

@gr5

gr5 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

I prefer the way you did it where it only uses that function for mac. But @atsju could probably convince me otherwise. He wrote that code originally.

I approve this change but want @atsju to look at this before merging.

@atsju
atsju self-requested a review August 4, 2026 06:19

@atsju atsju left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree with @gr5 I prefer it this way too. Single location is easyer to understand by end users even if it's old school.

On windows, the logs would end in C:/Users/<USER>/AppData/Roaming/<APPNAME>. A better place would probably be HomeLocation but I dislike when applications do that too.

@Lucassifoni

Copy link
Copy Markdown
Contributor Author

I understand how the generic writable directory handed by the OS can be annoying for users who want to actually access the log. I'm fine with keeping the conditional then.

@atsju

atsju commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

As this change does not affect any functionality and has no risk on windows I'm merging it.

@atsju
atsju merged commit 26049df into githubdoe:master Aug 4, 2026
13 of 14 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

Development

Successfully merging this pull request may close these issues.

3 participants