Free42 state file switching, export, and import

[Thomas Okken]

Hi Sammy,

Thank you for your feedback!

The platform-independent state files did require a lot of code changes, but that was a feature I've wanted for a long time, and doing it the way I ended up doing wasn't that bad, it took about a day and a half in the end. And it gave me an opportunity to simplify the core/shell interface a bit. I'm pretty happy with the result.

The state file management dialog is a lot more work, because that's all shell code, so it has to be done five times. Three done, two to go! I'll work on the Android version today, that should be a bit less work that the previous three. For the iOS version, I'll probably be able to reuse a lot of the code I wrote for the MacOS version, but that's going to have to wait, there's no way I'll be able to finish both this weekend.

My comments on your comments follow below.

Thomas

(08-18-2019 09:43 AM)SammysHP Wrote:  1. Arch Linux requires the X11 library as well (LIBS += -lX11). Probably all Linux distributions require it.

The X11 library is always required, but what varies is whether you have to tell the linker about it or not. I do my Linux builds in Ubuntu 12.04, and -lX11 is not needed there. I'll have to check what happens when I do add it there; I don't remember whether I left it off out of misguided command-line minimalism or because of a warning.

(08-18-2019 09:43 AM)SammysHP Wrote:  2. At least under Arch Linux -Wno-narrowing is necessary. As you've added it to the FreeBSD and SunOS builds, can you please add it to the normal Linux build as well? I think there are no downsides.

The downsides are error messages from compilers that don't recognize -Wno-narrowing.

(08-18-2019 09:43 AM)SammysHP Wrote:  3. Speaking of compile options: Adding -Wno-missing-braces -Wno-unused-but-set-variable -Wno-unused-variable removes hundreds of warnings.

Argh. In Ubuntu 12.04, there are zero warnings (there are some in the Intel library but none in Free42 itself). Newer compilers warn about more things, and some of those warnings are idiotic in my opinion (like the ones about adding parentheses in expressions like a && b || c && d) and others are about things that used to be common, even unavoidable, in C programming, and for which better alternatives sometimes exist in newer versions of the C and C++ standards. Unfortunately, bringing the source code up to date with those new standards is a big task, especially considering all the regression testing that would be required, so I just end up adding -Wno-annoying-warning-du-jour options in an endless game of whac-a-mole. And what makes this especially annoying is that every revision of gcc/llvm/clang on every distribution and every OS has its own set of warnings, and you can't just turn them all off, because turning off a warning that a compiler doesn't recognize, is an error.
See also: item 2, above.

(08-18-2019 09:43 AM)SammysHP Wrote:  4. The buffer for the path generation in gtk/shell_main.cc is too small [...]
You are trying to put two strings with a length of up to 255 characters in a buffer of only 255 characters. There are several places with this issue.

I don't think it's an issue in practice, because who actually uses directories nested that deep and filenames that long that you end up with absolute paths of more than 255 characters? My main reason for using snprintf is that in the unlikely event of such extremely long paths occurring, the code would at least fail gracefully rather than crash.

Still, it's not exactly elegant, and a few days ago, I went through the Windows version and eliminated all fixed-size strings wherever I could, replacing them with std::string. It turned out to require a ridiculous number of code changes, and to re-test everything because those fixed-length strings are everywhere, and then I would have to repeat that whole miserable exercise in the GTK, MacOS, and iOS versions as well. (The Android shell is Java so no fixed-length strings there, praise be to James Gosling and the 1990s Sun Microsystems managers who recognized the value of his work!) So in the end I backed out those changes. Maybe I'll re-apply them in some indeterminate future, when I'm sufficiently bored and when I feel up to the additional challenge of making the Windows version fully Unicode-compatible at the same time.

Sorry about the rant! TL;DR It's not something to worry about, but the compiler is too stupid to know that, and I'm unable to deal with all warnings myself; that task falls on downstream package maintainers. See also items 2 and 3...

(08-18-2019 09:43 AM)SammysHP Wrote:  5. There are some misleading-indentation warnings

Misleading indentation is misleading! I'll take a look and fix the source where I find them, at least where I agree with the warnings. The indentation in the example you gave is indeed wrong.