Skip to content

Word wrap squashed #518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

Word wrap squashed #518

wants to merge 9 commits into from

Conversation

balthisar
Copy link
Member

Implement word-wrapping in Tidy

  • Give Tidy an internal TY_(tidyWrappedText) and external tidyWrappedText service.
  • Add TidyConsoleWidth to configuration options (--console-width).
  • Implement wrapping for non-report messages.
  • language_en.h updated a bit for changed newlines.
  • Implemented wrapping in the console application strings.
  • tidy.c console application sniffs out terminal width in Win/*nix.

Improved documentation

  • Fully documented tidy.c in Doxygen format.
  • This file contents are re-ordered so that the functions can be easily grouped
    in Doxygen. There's a word_wrap branch in the API website right now that
    shows this documentation. Will update that site after this merge.

Compiler warnings

  • Snuffed out a couple of outstanding compiler warnings on Windows and Linux.

Tested to build and run on:

  • Mac OS X
  • Ubuntu
  • Windows 10

Regression testing:

  • Using the new branch word_wrap, there are zero regressions when tested in
    Mac OS X, Ubuntu, and Windows 10.
  • In word_wrap, all of the config files were updated to use --console-width 80,
    so that regression testing won't be dependent on the testers' console width!
  • Will update the testing branch after merge.

- Give Tidy an internal TY_(tidyWrappedText) and external tidyWrappedText service.
- Add TidyConsoleWidth to configuration options (--console-width).
- Implement wrapping for non-report messages.
- language_en.h updated a bit for changed newlines.
- Implemented wrapping in the console application strings.
- tidy.c console application sniffs out terminal width in Win/*nix.

Improved documentation
- Fully documented tidy.c in Doxygen format.
- This file contents are re-ordered so that the functions can be easily grouped
  in Doxygen. There's a word_wrap branch in the API website right now that
  shows this documentation. Will update that site after this merge.

Compiler warnings
- Snuffed out a couple of outstanding compiler warnings on Windows and Linux.

Tested to build and run on:
- Mac OS X
- Ubuntu
- Windows 10

Regression testing:
- Using the new branch word_wrap, there are zero regressions when tested in
  Mac OS X, Ubuntu, and Windows 10.
- In word_wrap, all of the config files were updated to use --console-width 80,
  so that regression testing won't be dependent on the testers' console width!
- Will update the testing branch after merge.
… case

where the user might specify console-width that's exactly the same as the
actual console width.
@geoffmcl
Copy link
Contributor

@balthisar I have reviewed this - it compiles and runs fine in Windows... thanks...

But as usual it is quite difficult to review a 230K diff file... As suggested previously, it would be much better if done in increments, and keep issues separate... new feature vs formats vs ... but ok, understand when you are on a roll, you are on a roll... ;=))

What exactly is word wrapping? Is this addressing a bug, or a specific Feature Request. Simply, why do we need a new configuration, --console-width?

In reading the TidyConsoleWidth, and some of the code... I think I understand - but in language-en.h I can see you sometimes removed the newline, \n, but in other case kept, or added them... And I can still read output to 78 characters per line here and there... Quite confusing, sort of contrary...

AFAIK the console will wrap a string anyway... in linux and Windows at least... but I was happy that tidy had long ago sort of chosen a wrap at about 70+ chars... because again if I read correctly, if I have redirection in place, such wrapping will now not take place - still to test this - but then not all editors I use either support, or can be configured to do, line wrap, so long lines would go out of view...

And as you point out, this means updating the tidy-tests bases to match, which you mention will happen after the merge... and adding this new config option, already in branch word_wrap...

Not that I am against it, just rather curious about the use case...

Of course the other big part seems to be the Doxygen formatting... although still to carefully read through all the changed wording, aside from the format changes, but what I did read looks good... thanks...

Just a small coloring changed noted in the API docs since 5.4.0... while I consider myself a poor judge of style, I do not like the red on green... seems to lack sufficient contrast, but perhaps that is just my failing sight...

@balthisar
Copy link
Member Author

But as usual it is quite difficult to review a 230K diff file... As suggested previously, it would be much better if done in increments, and keep issues separate... new feature vs formats vs ... but ok, understand when you are on a roll, you are on a roll... ;=))

Yeah, sorry about that. When I was looking through the code to figure out where to wrap text in the console, it seemed like a good opportunity to document it, too. The actual functional changes are pretty small. I'll resist that urge to document in the future (i.e., change existing documentation in a separate step).

What exactly is word wrapping? Is this addressing a bug, or a specific Feature Request. Simply, why do we need a new configuration, --console-width?

Feature request. One that I've had for a while.

language-en.h I can see you sometimes removed the newline, \n, but in other case kept, or added them... And I can still read output to 78 characters per line here and there... Quite confusing, sort of contrary...

I kept the newlines where paragraph breaks should occur, or when wanting to force a line wrap, like in the help text. In some cases it was grouping the newlines, e.g., "\n\n" just to make it easier to manage the paragraph text. Ah, yes, that note about 78 characters per line is still important for East Asian languages, because they very often don't have spaces! Strictly speaking I could borrow code from Unicode people that implements rules for East Asian line breaks, but I think most of that is C++ code. If anyone ever bothers to translate into Chinese and they raise a fuss, I can revisit this issue.

AFAIK the console will wrap a string anyway...

If the console is fewer than 80 characters, it won't wrap at work breaks; it just wraps wherever. But mostly I like to work with wider terminals, and this allows wrapping much longer.

if I have redirection in place, such wrapping will now not take place

Nope. If you have redirection in place, then wrapping will be per the console-width setting, which defaults to 80. We could change this to 78 if desired. In effect, with redirection Tidy behaves like it always has done (other than 80 vs 78 columns).

Just a small coloring changed noted in the API docs since 5.4.0... while I consider myself a poor judge of style, I do not like the red on green... seems to lack sufficient contrast, but perhaps that is just my failing sight...

Yeah, that does look hideous, both with the color theme but also in its own right! I'll make that change in the word_wrap branch.

@geoffmcl
Copy link
Contributor

@balthisar this is nothing about your Doxygen improvements... everything there seems very good... and lots of work... thanks...

Feature request. One that I've had for a while.

Well, we are owners, so I guess in a way, what we would like will prevail... no discussion...

But me, on, wrap text, I would like to discuss this more...

Simply, you have not yet convinced me that we must remove the \n from many messages, and replace it with this console-width option...

I think today everybody uses a console much wider that something like about 80 characters, but I really have no problem that tidy messages wrap at about that length... with a \n embedded in the message stream...

So, no, I do not see the introduction of this new option, as necessary, required, or desired... but maybe I am wrong!

Note, have still to test what happens when one of the outputs is redirected...

Remove this from the PR, and the rest is ok... great even...

If you feel very strongly on wrap text, then open an issue, and let's discuss it... in detail, advantages, disadvantages, reasons, gains, losses, etc... including languages that do not normally have a space wrap...

But right now, I personalty am against this all inclusive PR... sorry...

And very thankful for some color style changes in the API docs... thanks...

@balthisar
Copy link
Member Author

I'll open an issue tomorrow, but the biggest argument is, if you pretend it doesn't exist, then there's zero difference in how Tidy behaves. If you know that it exists, you can use the full width of your console, or when redirecting, eliminate carriage returns from the file output. Perhaps the biggest gain is for message API users; they gain complete strings for UI applications without spurious newlines that upset formatting with proportional fonts or different sized text containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants