Change how 'Disable Formatting' works

[ChrisJefferson]

This PR changes GAP so when formatting is disabled we still indent functions (and other objects), just disable line wrapping.

This also fixes a bug which where SetPrintFormattingStatus wouldn't work if stdout was opened twice (it might be we should also stop that happening, separately).

I'm not saying this should be final (we certainly want a whole bunch of tests), but I'm exploring options. Comments welcome.

[zickgraf]

Thanks for the PR, I can confirm that this makes indentation work regardless of the formatting status and makes SetPrintFormattingStatus work in the REPL again!

I noticed one additional issue which I assume is related to stdout being opened multiple times: If I put

SetPrintFormattingStatus("*stdout*", false);

in a file (or my gaprc), call gap with the filename and then check PrintFormattingStatus("*stdout*") in the REPL, it reports false (the value stored in GAPInfo.FormattingStatusStdout). However, the formatting is actually enabled in the REPL and I have to call SetPrintFormattingStatus("*stdout*", false); again to disable it. But that's a minor issue for me, I just wanted to mention it for the sake of completeness.

[fingolfin]

I have to teach, will comment more later

[fingolfin]

This change is wrong, it should affect stdout, not the top open file, which may be different from stdout.

[ChrisJefferson]

I agree this isn't currently right.

There is currently a problem, whereby we end up with two copies of stdout on the stack. By chucking in some prints I see the stack of Outputs is two "stdout". The old code would change the bottom one, but then the top (currently active) one would not have printing disabled.

I could explicitly search down the stack for the first "stdout" (which would mean file=1), and change the formatting of that, or investigate why we get multiple stdout on the output stack.

[fingolfin]

We get multiple stdout (and stderr) on the stack because that's the whole point of the stack: we start with stdout open and run code; that code opens a file and runs some more code; which opens stdout again. Now we have a stack with three levels: stdout, that file, again stdout. If the currently running code closes its output, the active output should again point at the file -- the bottommost "stdout" should still stay open, though.

The problematic part thus is not that stdout (and stderr) can be multiple times on the stack; rather it is that they do not share data: neither the formatting hints nor the line cache. This already causes issues with printing to stderr: if your terminal is 80 chars wide, and you print 50 chars to stdout, then open stderr and print another 45 chars, GAP fails to realize that a wraparound has occurred, because the stderr output stream is not aware of the 50 chars printed before it. And when stderr is closed, then stdout still thinks that 50 chars were printed to the terminal, when really it was 95. See also issue #1197.

There are several ways to address this. One crude but easy to implement approach would be to detect whenever we open/close stdout or stderr, and add some special treatment: when opening a new one, make sure to copy the content of the TypOutputFile struct of the any previously opened stdout/stderr over (well, at least part of it; maybe pos/hint/indent/...?). Conversely, when closing, copy it back to the previously opened stdout/stderr. Of course variations are possible, e.g. there could be a static TypOutputFile Stdout in io.c, and we simply sync with that resp. use that whenever output goes to stdout or stderr.

[ChrisJefferson]

I agree that might well be a better fix. I've tweaked this PR so it now walks the stack of open Outputs and changes the formatting on each one which is *stdout*. That still doesn't fix the other problems you discuss of course.

[fingolfin]

Since indentation is now always on, this breaks the changes @ChrisJefferson recently made in PR #4458, as those specifically relied on indentation turning off in this case.

We may need a more elaborate way to control the "formatting settings" of output streams than just a single boolean.

That said, SetPrintFormattingStatus explicitly is documented as turning off indentation, so if we change that, then at the very least we should also change the documentation. This may be acceptable, given that apparently in the past SetPrintFormattingStatus didn't reliably turn off indentation (or did it? We are not just talking about stdout here, but also about printing to files). And when did it change, exactly?

Or we leave it as it is, and instead add a way to let the user separately turn off "line wrapping" and "indentation".

[ChrisJefferson]

I believe SetPrintFormattingStatus always worked, except for stdout was it has been broken (at least sometimes).

Personally, I've never seen a good reason to disable indentation, so I'd be happy to just remove the ability to disable it, but I realise some people might have a use case where they want to disable indentation. For that, we could split indenting and line wrapping into two options (I'd personally then suggest we disable line wrapping by default when writing to files, which would be another change of behaviour, but one I think, again, most users would want).

[zickgraf]

Sorry for the late reply, I lost track of the PR over the weekend.

This is much more intricate than I expected, so I thought a bit about what the simplest interface for my use case would be. My use case is: I never want line wrapping (because I prefer to let my terminal wrap lines and do not want "random" backslashes when printing to strings/files) and I always want indentation, regardless of the print target (stdout, file, string). So I think the easiest solution would be to introduce a function SetGlobalLineWrappingStatus which disables (or re-enables) line wrapping completely. With that, I would not need SetPrintFormattingStatus anywhere in my code, so the original issue would be void (for me).