D29210: [NestedListHelper] Fix indentation of selection, add tests

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:7012493410ae: [NestedListHelper] Fix indentation of 
selection, add tests (authored by poboiko).
Herald added a project: Frameworks.

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29210?vs=81585&id=81758

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

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

D29210: [NestedListHelper] Fix indentation of selection, add tests

[David Faure]

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

BRANCH
  change-indent (branched from master)

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

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

D29210: [NestedListHelper] Fix indentation of selection, add tests

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> dfaure wrote in nestedlisthelper.cpp:225
> should the opposite happen? delete the list when decreasing indentation?
> 
> Or is that what the blkFmt.setObjectIndex(-1); is about?
> A comment there would be useful...
> 
> To me setObjectIndex(-1) sounds like it could kill any kind of embedded 
> object there but I might be wrong.

We indeed delete it when decreasing, but here `delta > 0`, which corresponds to 
increasing :)

Regarding `setObjectIndex()`: it does indeed look like magic, and I don't 
totally understand it either. It was there, however, and it is also present in 
the official Qt `TextEdit` example, where it's used when item is removed from 
the list.
I've found more explicit way: just remove the block from list and set it's 
indentation to zero, it seems to be working well.

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

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

D29210: [NestedListHelper] Fix indentation of selection, add tests

[Igor Poboiko]

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


  Make the code that removes the block from the list more obvious (without 
`setObjectIndex` magic)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29210?vs=81267&id=81585

BRANCH
  change-indent (branched from master)

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

D29210: [NestedListHelper] Fix indentation of selection, add tests

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> nestedlisthelper.cpp:225
> +if (delta > 0) {
> +// No list, we're increasing indentation -> create a new one
> +listFmt.setStyle(QTextListFormat::ListDisc);

should the opposite happen? delete the list when decreasing indentation?

Or is that what the blkFmt.setObjectIndex(-1); is about?
A comment there would be useful...

To me setObjectIndex(-1) sounds like it could kill any kind of embedded object 
there but I might be wrong.

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

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

D29210: [NestedListHelper] Fix indentation of selection, add tests

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, dfaure, mlaurent.
poboiko requested review of this revision.

REVISION SUMMARY
  When we try to increase / decrease an indentation level of a selection, just 
change
  indentation level of each block in the selection, and then make sure all 
styles
  are consistent via `reformatList`.
  
  This actually fixes indentation of selection: previously all blocks had the 
same indentation after this action.
  
  Add a unit test that covers as much weird selections & indentation cases as I 
could imagine.
  
  Slightly overlaps with D29208: [NestedListHelper] Improve indentation code 
, so this one should be applied after that.

TEST PLAN
  `make && ctest`

BRANCH
  change-indent (branched from master)

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

AFFECTED FILES
  autotests/krichtextedittest.cpp
  autotests/krichtextedittest.h
  src/widgets/krichtextedit.cpp
  src/widgets/nestedlisthelper.cpp
  src/widgets/nestedlisthelper_p.h

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel