Introduce SetGlobalLineWrappingStatus

[zickgraf]

Description

This is a prototype of my suggestion in #4496.

My setting 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). This PR introduces a function SetGlobalLineWrappingStatus which disables (or re-enables) line wrapping completely (I hope).

If you like the idea, I would add some documentation and tests and fill in the text for release notes below.

Text for release notes

TODO

If this pull request shall not be mentioned in the release notes (to be distributed in the file CHANGES.md), please add the label release notes: not needed.

Otherwise, please proceed in one of the following ways:

• Choose a title that can serve as text for the release notes, and add the label release notes: use title.

• Put the text for the release notes here, that is, between the markers Text for release notes and (End of text for release notes).

The first variant is recommended whenever the text for release notes is suitable as title.

In both cases, please follow the style of the GAP CHANGES.md file in the root directory. In particular, please surround the names of GAP functions with backquotes.

(End of text for release notes)

Further details

If necessary, please provide further details here.

Checklist for pull request reviewers

• [ ] proper formatting

If your code contains kernel C code, run clang-format on it; the simplest way is to use git clang-format, e.g. like this (don't forget to commit the resulting changes):

git clang-format $(git merge-base HEAD master)

• [ ] usage of relevant labels

  1. either release notes: not needed or release notes: to be added
  2. at least one of the labels bug or enhancement or new feature
  3. for changes meant to be backported to stable-4.X add the backport-to-4.X label
  4. consider adding any of the labels build system, documentation, kernel, library, tests

• [ ] runnable tests

• [ ] lines changed in commits are sufficiently covered by the tests

• [ ] adequate pull request title

• [ ] well formulated text for release notes

• [ ] relevant documentation updates

• [ ] sensible comments in the code

[ChrisJefferson]

Let's see what other people have to say before you make further changes, but I wonder if this should be a per-stream option (like the existing PrintFormattingStatus), rather than a global setting.

Personally, I'm happy for it to be a global setting, but it is a bit weird that PrintFormattingStatus (which also effects this) is file-specific.

[zickgraf]

Let's see what other people have to say before you make further changes

Yes, that's why I didn't invest time into adding documentation and tests yet :D

but I wonder if this should be a per-stream option (like the existing PrintFormattingStatus), rather than a global setting.

I thought about this, but then I think we would have the same problems as in #4496 again, that is, how we could synchronize the different stdouts. And with the global setting one can still emulate a per-stream option by changing the global setting before writing to a stream and resetting it afterwards (for remembering the original state we would of course also need a getter GlobalLineWrappingStatus). So I think most applications should be covered by this. But of course I see your point that it's unexpected when compared to PrintFormattingStatus.

[fingolfin]

For me this would be a last resort: For years I've been working hard on eliminating global state in the GAP kernel, so I am reluctant to add more of it. One reason for avoiding global state is that it can have weird and unexpected effects... like using it can break Test and a user may have a hard time figuring out what's going on.

Thus I'd indeed greatly prefer if "line wrapping" and "indentation" could be toggled for each stream separately.

But if we decide to merge this PR anyway (e.g. because it provides a quick solution, now) then at the very least we should also add a way to query the status of this flag so that it can be saved and restored if necessary

[zickgraf]

@fingolfin I see. I am carrying around some downstream patches anyway, so I am happy with simply keeping this patch for my local build for now, hoping for a official solution in the future. Should I close this PR or keep it open for future discussion?

[fingolfin]

Please leave it open

[fingolfin]

Here is what I suggest we do:

1. In the kernel, we split the single boolean format into two: enableLinewrapping and enableIndenting (or so)
2. calling SetPrintFormattingStatus(stream, val) with val equal to true or false sets both of those new settings to the same value

This leaves the question how to set the two new settings independently. Now there is quite some code which saves the current formatting status, sets it to some new value, then restores. To keep this code working, we could do this:

3. also allow passing a record as second argument to SetPrintFormattingStatus, which can have entries linewrapping := true/false and indent := true/false, and doing so will modify the setting for all entries present
4. the return value of PrintFormattingStatus either is true or false (if both settings agree), or else a record as described above.

So turn off just line wrapping for stdout, you could do SetPrintFormattingStatus("*stdout*", rec(linewrapping:=false)).

Of course we could still add additional functions like SetLineWrapping(stream, bool) for convenience.

[zickgraf]

Sounds good, I would only propose one change regarding point 4.: I think it would be nicer if PrintFormattingStatus would always return a record. Code which expects a boolean would have to be adapted anyway if there is the possibility of returning a record, so we can as well always return a record, which would be a cleaner interface in my view.