search

GrandOrgue / grandorgue

GrandOrgue software
Other
175 stars 43 forks source link

Fixed saving of Settings->Paths https://github.com/GrandOrgue/grandorgue/issues/1907 #1991

Closed oleg68 closed 2 months ago

oleg68 commented 3 months ago

Resolves: #1907

The reason why the path settings may not be saved was that the directory did not exist.

This PR adds making the directory.

larspalo commented 2 months ago

I have nothing per se against the functionality of this PR, but why are you changing the naming pattern for (one of) the global functions like this?

oleg68 commented 2 months ago

I have nothing per se against the functionality of this PR, but why are you changing the naming pattern for (one of) the global functions like this?

Current naming conventions:

larspalo commented 2 months ago

global or local static functions: lowcase with underscores.

This is a new one. No previous case for it in the code. Normally that style was only used for local variables. But it really doesn't matter at all in any practical way... It just looks strange to me to have a function named in that manner.

larspalo commented 2 months ago

Also, such a style is used for naming a few of the files...

oleg68 commented 2 months ago

global or local static functions: lowcase with underscores.

This is a new one. No previous case for it in the code. Normally that style was only used for local variables. But it really doesn't matter at all in any practical way... It just looks strange to me to have a function named in that manner.

But I use this style in lots of other files. For example, https://github.com/GrandOrgue/grandorgue/blob/d087875cf03f8803049ac9f0e83852cc813a614f/src/grandorgue/config/GOConfig.cpp#L247

larspalo commented 2 months ago

But I use this style in lots of other files.

Though, here it would have a go_ prefix that indeed would suggest a global function (for the app), and that has not been done before in this particular way. But I think this naming pattern - convention or not - is rather un-important in the bigger picture, so I won't argue about it per se.

However, as there are other functions in the same file that follow another naming pattern I think it would have been a better approach to just fix the issue without any re-naming in this PR, and in another separate PR change the naming of all the functions in the same file so that they share a coherent naming pattern (at least within that same file).

oleg68 commented 2 months ago

However, as there are other functions in the same file that follow another naming pattern I think it would have been a better approach to just fix the issue without any re-naming in this PR, and in another separate PR change the naming of all the functions in the same file so that they share a coherent naming pattern (at least within that same file).

I've done it.

This PR only fixes the bug, and another PR #1998 renames the global functions in this file.