D29208: [NestedListHelper] Improve indentation code
This revision was automatically updated to reflect the committed changes. Closed by commit R310:18a2371fe394: [NestedListHelper] Improve indentation code (authored by poboiko). REPOSITORY R310 KTextWidgets CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29208?vs=81738=81743 REVISION DETAIL https://phabricator.kde.org/D29208 AFFECTED FILES 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
D29208: [NestedListHelper] Improve indentation code
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R310 KTextWidgets BRANCH unused (branched from master) REVISION DETAIL https://phabricator.kde.org/D29208 To: poboiko, #frameworks, dfaure, mlaurent Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29208: [NestedListHelper] Improve indentation code
poboiko updated this revision to Diff 81738. poboiko added a comment. Improve readability as suggested, also const'ify REPOSITORY R310 KTextWidgets CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29208?vs=81256=81738 BRANCH unused (branched from master) REVISION DETAIL https://phabricator.kde.org/D29208 AFFECTED FILES 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
D29208: [NestedListHelper] Improve indentation code
dfaure added inline comments.
INLINE COMMENTS
> poboiko wrote in nestedlisthelper.cpp:87
> That's the current block being checked, not the next one. I've just checked
> to be sure, last block can be unindented :)
>
> TBH, I don't really know if it's even possible for the current block to be
> invalid (that would probably mean that `textCursor()` returned by `QTextEdit`
> is invalid?).
> I've just borrowed this particular check from the old code...
Oh. I see. Well, it would be much more readable to move that if() *before* the
line that declares and sets nextBlock.
And yes I can't really see how this can happen, either.
> nestedlisthelper.cpp:93
> }
> -
> -QTextCursor cursor = textEdit->textCursor();
> -bool handled = false;
> -
> -if (!cursor.hasSelection() && cursor.currentList()) {
> -
> -// Check if we're on the last list item.
> -// itemNumber is zero indexed
> -QTextBlock currentBlock = cursor.block();
> -if (cursor.currentList()->count() ==
> cursor.currentList()->itemNumber(currentBlock) + 1) {
> -// Last block in this list, but may have just gained another
> list below.
> -if (currentBlock.next().textList()) {
> -reformatList();
> -}
> -
> -// No need to reformatList in this case. reformatList is slow.
> -if ((event->key() == Qt::Key_Return) || (event->key() ==
> Qt::Key_Backspace)) {
> -handled = true;
> -}
> -} else {
> -reformatList();
> -}
> +if (!nextBlock.textList()) {
> +return true;
nextBlock is only used here so I would move its definition to just before this
line.
(same thing in the previous method, about prevBlock)
REPOSITORY
R310 KTextWidgets
REVISION DETAIL
https://phabricator.kde.org/D29208
To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29208: [NestedListHelper] Improve indentation code
poboiko added inline comments. INLINE COMMENTS > dfaure wrote in nestedlisthelper.cpp:87 > So the last block cannot be unindented? How come? That's the current block being checked, not the next one. I've just checked to be sure, last block can be unindented :) TBH, I don't really know if it's even possible for the current block to be invalid (that would probably mean that `textCursor()` returned by `QTextEdit` is invalid?). I've just borrowed this particular check from the old code... REPOSITORY R310 KTextWidgets REVISION DETAIL https://phabricator.kde.org/D29208 To: poboiko, #frameworks, dfaure, mlaurent Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29208: [NestedListHelper] Improve indentation code
dfaure added inline comments.
INLINE COMMENTS
> nestedlisthelper.cpp:87
> +QTextBlock nextBlock = block.next();
> +if (!block.isValid()) {
> +return false;
So the last block cannot be unindented? How come?
REPOSITORY
R310 KTextWidgets
REVISION DETAIL
https://phabricator.kde.org/D29208
To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29208: [NestedListHelper] Improve indentation code
poboiko created this revision. poboiko added reviewers: Frameworks, dfaure, mlaurent. Herald added a project: Frameworks. poboiko requested review of this revision. REVISION SUMMARY The patch includes following improvements: 1. `handleAfterKeyPressEvent` was only used to adjust margins. We don't do it anymore, so get rid of it. 2. Add support for Tab key to increase indentation level (either at the beginning of a list item or with multiple lines selected) 3. Fix `canIndent / canDedent` logic when cursor has a selection. `canIndent` should work with `topOfSelection`, and `canDedent` --- with `bottomOfSelection`. 4. Enclose `handleOnIndentMore / Less` in `beginEditBlock / endEditBlock`, so they appear as a single event in Undo stack. 5. A Return on an empty list element decreases the indentation (so double-Return terminates the list) TEST PLAN 1. Tab / Return keys now work as explained 2. IndentMore / Less are undoable with single `Ctrl+Z` (this leads to cursor jumping around, which is probably beyond the scope of this patch) 3. Increase / Decrease Indent actions now become enabled when they should if the selection is present (the behavior of `handleOnIndentMore / Less` when selection is present is slightly broken, which is also beyond the scope) REPOSITORY R310 KTextWidgets BRANCH unused (branched from master) REVISION DETAIL https://phabricator.kde.org/D29208 AFFECTED FILES 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
