At a quick glance:

• lxterminal-PROFILE.conf should probably read lxterminal-NAME.conf
• gchar * profile isn't freed (should be NULL by default)
• it is probably easier to set and use arguments->profile to create config_filename

Thanks for your comments.

Agreed on: lxterminal-PROFILE --> lxterminal-NAME. That was an oversight.

[Having written the following description, it's clear to me that the global "profile" should definitely be renamed, perhaps to "profile_string", to make it explicit that it's different than what was passed to the command-line argument.]

The default value of the profile global needs to be the empty string, i.e. "", not NULL. When set, it will contain the profile's name, plus an extra leading '-' character, due to the sprintf at lxterminal.c:1549. This makes the construction of the full config filename, and the full socket name, work easily with just a single sprintf -- e.g., the file name will be either "lxterminal.conf" or "lxterminal-myname.conf", without any extra logic. Using address->profile directly would involve extra conditionals to provide the '-' or not.

Also, because the socket name and file name may be needed repeatedly over the life of the process, I don't think profile should ever be freed. I think the only leak that can occur is if the --profile argument is given more than once. And I believe this is the case for any of the string-valued argments.

paul

The default value of the profile global needs to be the empty string, i.e. "", not NULL.

Of course, but you can't free it that way. Why not using profile_string ? profile_string : ""?

You don't need to add comments to make its role more obvious.

Why would I free it? Losing one byte of data because it's no longer pointed to is really not a big deal.

But this isn't my codebase, so I'm more than happy to change it. :-)

(and yes, I know that the static initializer can't be freed anyway)

We may be talking at cross purposes.

Why would I free it?

Because you are allocating memory with g_strdup_printf().

Instead of profile_string ? profile_string : "", it may be more straightforward to do profile_string = g_strdup("") if --profile isn't specified.

Just because I'm allocating it with g_strdup_printf() doesn't mean it will be freed. As I said earlier, that memory must never be freed, because the string it contains might be needed at any time. (At least, that's how I currently understand the code. Perhaps I've missed something?) But perhaps your concern is that the code will change and someone will then want to free it?

I think I will (after hearing from you) switch to using profile_string = g_strdup(""), since it will clean up the code a bit.

Is there anything else about the pull request that should change? Should I remove the change to lxterminal.1 ? I created it "by hand", so it may not match what the tools would produce, though I think it's syntactically correct. But I don't even know if that placeholder should be modified.

Once you approve, I'd prefer to create a new pull request (or, modify this one?) to collapse my changes into one commit. But I'll defer to your opinion on whether I should or not.

There is only a small diff to your lxterminal.1 after rebuilding from xml:

--- a/man/lxterminal.1
+++ b/man/lxterminal.1
@@ -75,7 +75,7 @@ Set the terminal\*(Aqs working directory\&.
 .PP
 \fB\-\-profile=\fR\fB\fINAME\fR\fR
 .RS 4
-If set, then all instances of lxterminal invoked in the same way will share a config file named lxterminal-NAME.conf\&.
+If set, then all instances of lxterminal invoked in the same way will share a config file named lxterminal\-NAME\&.conf\&.
 .RE
 .SH "AUTHOR"
 .PP
@@ -90,7 +90,7 @@ General Public License, Version 2 any later version published by the Free Softwa
 On Debian systems, the complete text of the GNU General Public License can be found in /usr/share/common\-licenses/GPL\&.
 .SH "FILES"
 .PP
-The configuration file can be found in ~/\&.config/lxterminal/lxterminal\&.conf, or lxterminal-NAME.conf\&.
+The configuration file can be found in ~/\&.config/lxterminal/lxterminal\&.conf, or lxterminal\-NAME\&.conf\&.
 .SH "AUTHOR"
 .PP
 \fBYing\-Chun Liu\fR

And yes, there should be a pre-generated man page.

Apart from that, please wait until I find time to get back to lxterminal.

Please take a look at this.

I think that the profile belongs to the terminal window instead of a global variable, so I put it in the LXTerminal structure where it can be freed after the window is closed. (Unlike the global variable, which is allocated new memory each time it is used.)

It also seems clearer to me to pass the profile as an argument to the setting functions and the socket creation function (even if it makes the commit a bit more complex).

What do you think?

I'm a little confused by this: the profile is shared by all windows
that were started from the same process, correct? So it feels like
there's no reason for it to be copied into every window structure.

It seems to be the only way to get the profile in terminal_preferences_dialog() to save the settings. Or am I missing something?

Although the implementation is a bit different now, thank you very much for your contribution.