ViewPrivate: Make 'Apply Word Wrap' more comfortable
ClosedPublic

Authored by loh.tar on Jan 3 2019, 8:34 PM.

Details

Summary

This patch address not all aspects but should be a step in the right direction.

  • Don't wrap entire document when nothing is selected but wrap current line
  • Join paragraph before wrap to avoid odd result
  • Add new function DocumentPrivate::wrapParagraph for that task
  • Update 'What's This' hint to reflect new behaviour
  • Update 'What's This' hint of dyn wrap too to be more precise

BUG:381985

Old Hints


New Hints

Test Plan

My arbitrary chosen test data.

1aaaaa aaaa aaaa aaaaaaa
2aa aaaaaaa aaaaaa aaaaaaa aaa aaaa aaaa aaaaaaa aa
3aaaaaaaa aaaaaaa aaaaaaa aaaaaa aaaaaaa aaaaaaaaa aaaaaaaa aaaaaaaa aaaaa aaaaa aaaa aaaa aaaaaaa aa
4aaaaa aaaa aaaa aaaaaaa aa
5aaa aaaa aaaa aaaaaaa aa aaaa aaaaaaa aa aaaaaaa

1----- ---- ---- -------
2-- ------- ------ ------- --- ---- ---- ------- --
3-------- ------- ------- ------ ------- --------- -------- -------- ----- ----- ---- ---- ------- --
4----- ---- ---- ------- --
5--- ---- ---- ------- -- ---- ------- -- -------

1xxxxx xxxx xxxx xxxxxxx
2xx xxxxxxx xxxxxx xxxxxxx xxx xxxx xxxx xxxxxxx xx
3xxxxxxxx xxxxxxx xxxxxxx xxxxxx xxxxxxx xxxxxxxxx xxxxxxxx xxxxxxxx xxxxx xxxxx xxxx xxxx xxxxxxx xx
4xxxxx xxxx xxxx xxxxxxx xx
5xxx xxxx xxxx xxxxxxx xx xxxx xxxxxxx xx xxxxxxx

Before


With Patch

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Jan 3 2019, 8:34 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptJan 3 2019, 8:34 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Jan 3 2019, 8:34 PM
mwolff requested changes to this revision.Jan 15 2019, 2:02 PM
mwolff added a subscriber: mwolff.

I like what I'm seeing in the screenshot, but please add proper tests for this functionality

src/view/kateview.cpp
2355 ↗(On Diff #48639)

store in a std::unique_ptr and remove the manual delete further down below

2359 ↗(On Diff #48639)

bool shouldWrap = true;

2366 ↗(On Diff #48639)

shouldWrap = false;
break;

2370 ↗(On Diff #48639)
if (!shouldWrap) {
    continue;
}
This revision now requires changes to proceed.Jan 15 2019, 2:02 PM

please add proper tests for this functionality

No idea how

Because there are a couple of bug reports where often the response was like "No, can't be done" need this a closer look from @dhaumann and @cullmann

src/view/kateview.cpp
2355 ↗(On Diff #48639)

fine, will do it

2359 ↗(On Diff #48639)

I dislike that. I hope someone else like my goto way

@loh.tar Slightly related: You may want to also have a look at this: https://phabricator.kde.org/source/ktexteditor/browse/master/src/script/data/commands/utils.js$271
In short, there is a command line command (F7) called 'rewrap' that rewraps the selection in a "smart" way. My idea when writing this once was:

  1. there is a target wrap column, say 80
  2. there is a soft violation wrap column, say 82
  3. there is a minimum wrap column, say 70

Now, lets take the following example:

         1         2         3         4         5         6         7         8
1234567890123456789012345678901234567890123456789012345678901234567890123456789012345
the quick brown fox jumps of the lazy dog. The bla bla bla bla bla aRatherLongWord

Now, usually, aRatherLongWord would wrap to the next line, since it is longer that column 80. However, it leaves a "big gap" starting at column <70 (cf. 3. soft wrap above) in the end of the line, which is visually not so appealing ;) So in this case, we allow violation of the hard-wrap at 80, and instead allow a violation up to column 82 (cf. 2. above).

With this, you can for instance nicely rewrap a TeX document so that you can nicely read it without too big gaps at the end of a line.

However, to be honest, I don't even know whether I ever finished the implementation, so maybe rewrap even does something different...

Interesting. Didn't know that such function exist. Well, it may help to read the handbook...
Is there no GUI way to access such stuff? "Read the handbook" lala...

Well, word/text wrapping is a field which has many room for improvements.

  • Adjust as block by inserting spaces at "smart" places to avoid ugly holes in the overall appearance
  • Wrap/split the word, not only text at a space. IIRC Is that somewhere done in Kate, or was there a function in Sonnet which offer that(?)
  • Your smart solution to allow in rare cases to extend the desired width
  • Probably some more I didn't know
  • Keep the indent: https://bugs.kde.org/show_bug.cgi?id=135737
  • Keep the indent and the "semantic", e.g. "Is comment" or "Is citation (like in e-mails)" https://bugs.kde.org/show_bug.cgi?id=369049

As some of you know, I talk often about the latter two issues and I'm willing to fix that.
The first three are really nice but more like the candy after the necessary lunch.

loh.tar updated this revision to Diff 50082.Jan 22 2019, 8:13 PM
loh.tar retitled this revision from ViewPrivate: Make applyWordWrap() more comfortable to ViewPrivate: Make 'Apply Word Wrap' more comfortable.
  • Use std::unique_ptr for the cursor
  • Oops!? Use 'if' instead of unneeded 'while' loop which also avoid 'goto'

Regarding autotest point me to some blue print and I will try to do it.

Should you love it as it is and intend to commit, I would update the summary section before and look at that "What this" hint.

loh.tar updated this revision to Diff 50104.Jan 23 2019, 2:39 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar set the repository for this revision to R39 KTextEditor.
  • Update 'What's This' hint to reflect new behaviour and to be more precise
  • Update 'What's This' hint of dyn wrap too to be more precise
  • Update/clear Summary
mwolff requested changes to this revision.Jan 24 2019, 3:39 PM

please look at the existing tests and expand that. There's e.g. KateDocumentTest::testWordWrap in ktexteditor/autotests/src/katedocument_test.cpp

also don't change the behavior of wordwrapping that is used by the view compared to what we would get by calling wordwrap on the document directly

src/view/kateview.cpp
2346 ↗(On Diff #48639)

all of this functionality needs to go into the document, it shouldn't depend on the view.

This revision now requires changes to proceed.Jan 24 2019, 3:39 PM

also don't change the behavior of wordwrapping that is used by the view compared to what we would get by calling wordwrap on the document directly

It's somehow not the case. The document has no such function. It's similar to ViewPrivate::smartNewline() or ViewPrivate::killLine().

ViewPrivate::applyWordWrap() is (now) like a higher level functionality based on two lower level functions.

When everyone want this into the document, I can try to do it. Just give advice how exactly.
My only idea is to add a new function (e.g. wrapParagraph, reformatLines), which will result in an almost empty ViewPrivate::applyWordWrap().

loh.tar updated this revision to Diff 50648.Feb 1 2019, 2:46 PM
  • Move logic into document
  • Tiny docu fix + cosmettic
cullmann added a comment.EditedFeb 12 2019, 7:21 PM

I am ok with the change, but I would like to have one test case in

https://cgit.kde.org/ktexteditor.git/tree/autotests/src/katedocument_test.cpp

It must be nothing complex, just fill a document with a bit of text and apply the wrapping and check the result. The case should include the kind of input that profits by this change.

As it is a document function, nothing more fancy must be done than calling it with the right line pair.

loh.tar updated this revision to Diff 51680.Feb 14 2019, 2:31 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
loh.tar set the repository for this revision to R39 KTextEditor.
  • Add autotest
  • Don't wrap twice when static wrap is set to avoid bad result
loh.tar edited the summary of this revision. (Show Details)Feb 28 2019, 3:46 AM
cullmann accepted this revision.Mar 4 2019, 6:11 PM

Given we have a test, the code is documented and nobody raises large objections since long over the latest implementation, I think this shall go in :=)

This revision was not accepted when it landed; it landed in state Needs Review.Mar 4 2019, 6:12 PM
This revision was automatically updated to reflect the committed changes.