D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko closed this revision.

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: abstractdevelop, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  I see, thanks for the explanations.
  
  I don't mind 6 levels if that's what HTML does.

REPOSITORY
  R310 KTextWidgets

BRANCH
  textwidget-heading (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: abstractdevelop, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Laurent Montel]

mlaurent added inline comments.

INLINE COMMENTS

> poboiko wrote in krichtextedit.cpp:346
> Yes, that's true. I agree it's a bit confusing, but that's the case with HTML 
> too, which I had in mind (see screenshot, it's Chrome)
> F8241667: scr1.png 
> 
> For markdown, it seems like there are only 5 levels, 5th having the same size 
> as normal text (6th level gets ignored, that's Okular)
> F8241670: scr2.png 
> 
> I guess we probably can follow the Markdown way and bound it with 5, just to 
> avoid confusion.

But if you insert html text from html with title6 how it will work if it just 
supports h5 ?

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: abstractdevelop, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko marked 3 inline comments as done.
poboiko added a comment.


  In D28854#649189 , 
@abstractdevelop wrote:
  
  > Hey, thanks for this @poboiko  I used to have to implement this myself, so 
this will be very useful in my app, O20.Word. ;)
  
  
  Sure, you're welcome! I like doing useful stuff :)

INLINE COMMENTS

> dfaure wrote in krichtextedit.cpp:346
> If boundedLevel is 6, the size adjustement will be -1?
> Does this mean a heading that's smaller than normal text?

Yes, that's true. I agree it's a bit confusing, but that's the case with HTML 
too, which I had in mind (see screenshot, it's Chrome)
F8241667: scr1.png 

For markdown, it seems like there are only 5 levels, 5th having the same size 
as normal text (6th level gets ignored, that's Okular)
F8241670: scr2.png 

I guess we probably can follow the Markdown way and bound it with 5, just to 
avoid confusion.

> dfaure wrote in krichtextedit.cpp:375
> This seems to duplicate what happened with selectCursor already. Why are two 
> cursors necessary to change the style of one paragraph?

I used `selectCursor` to select a line (block) under cursor and then change the 
style of the selection with `mergeCharFormat` .

`cursor` is for actual users cursor, which also has to change the style, so 
newly typed text will have the same style and heading level too (I use 
`mergeBlockCharFormat` for that reason - if I use `mergeCharFormat` here, the 
cursor will remain big after I press `Enter` after a title. However, it will 
change its size to the smaller one immediately after I start typing). But I 
don't want to mess with users selection, that's why I'm keeping both.

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: abstractdevelop, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Abstract Software]

abstractdevelop added a comment.


  Hey, thanks for this @poboiko  I used to have to implement this myself, so 
this will be very useful in my app, O20.Word. ;)

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: abstractdevelop, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> krichtextedit.cpp:346
> +// level=2 look the same
> +int sizeAdjustment = boundedLevel > 0 ? 5 - boundedLevel: 0;
> +

If boundedLevel is 6, the size adjustement will be -1?
Does this mean a heading that's smaller than normal text?

> krichtextedit.cpp:375
> +
> +cursor.mergeBlockCharFormat(chrfmt);
> +setTextCursor(cursor);

This seems to duplicate what happened with selectCursor already. Why are two 
cursors necessary to change the style of one paragraph?

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Laurent Montel]

mlaurent accepted this revision.
mlaurent added a comment.
This revision is now accepted and ready to land.


  Thanks

REPOSITORY
  R310 KTextWidgets

BRANCH
  textwidget-heading (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko updated this revision to Diff 80202.
poboiko added a comment.


  Added a unit-test testing for everything I could come up with 
  (at least 4 "behavior nuances" from the commit message)
  
  Address other feedback as well (const'ify, @since version)

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28854?vs=80193&id=80202

BRANCH
  textwidget-heading (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D28854

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp
  src/widgets/krichtextedit.h
  src/widgets/krichtextwidget.cpp
  src/widgets/krichtextwidget.h

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Laurent Montel]

mlaurent requested changes to this revision.
mlaurent added a comment.
This revision now requires changes to proceed.


  Can you improve autotest KRichTextEditTest ?

INLINE COMMENTS

> krichtextedit.cpp:346
> +// level=2 look the same
> +int sizeAdjustment = boundedLevel > 0 ? 5 - boundedLevel: 0;
> +

const int

> krichtextedit.h:351
> + *
> + * @since FIXME
> + */

since @5.70

> krichtextwidget.cpp:499
> +d->action_heading_level = new KSelectAction(i18nc("@title:menu", 
> "Heading Level"), this);
> +QStringList listStyles = {i18nc("@item:inmenu no heading",   
>   "Basic text"),
> +  i18nc("@item:inmenu heading level 1 
> (largest)",  "Title"),

const QStringList

> krichtextwidget.h:233
> + *
> + * @since FIXME
> + */

@since 5.70

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko edited the test plan for this revision.

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28854

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28854: [KRichTextWidget] Add support for headings

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, mlaurent, ahmadsamir, dfaure.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  This patch adds support of different headings (essentially, HTML h1..h6 tags).
  Those might be pretty useful when formatting a rich text (I'm having KJots - 
note taking app - in mind)
  
  `QTextBlockFormat` supports `headingLevel` since Qt 5.12; however, as stated 
in the documentation,
  `setHeadingLevel` does not adjust style (font size, etc), so we have to take 
care of it on our own.
  
  It also adds a `KSelectAction` to choose between different headings.
  
  There are few behavior nuances:
  
  1. If user presses `Enter` at the end of heading line, cursor should switch 
to `basic text`
  2. If user presses `Enter` in the middle of the heading, line just breaks 
into two headings
  3. If user presses `Backspace` at the beginning of a line after the heading, 
the line is merged with the previous one (and thus becomes heading itself)
  4. The same with `Delete` at the end of heading line: the next line should 
become heading too.

TEST PLAN
  See the video of `KRichTextEditor` from `KXmlGui` (I had to add 
`format_heading_level` to rc-file).

REPOSITORY
  R310 KTextWidgets

BRANCH
  textwidget-heading (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D28854

AFFECTED FILES
  src/widgets/krichtextedit.cpp
  src/widgets/krichtextedit.h
  src/widgets/krichtextwidget.cpp
  src/widgets/krichtextwidget.h

To: poboiko, #frameworks, mlaurent, ahmadsamir, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns