DocumentPrivate: Support auto bracket in block selection mode
ClosedPublic

Authored by loh.tar on Mar 1 2019, 5:23 PM.

Details

Summary

..how it should be by adding brackets to each line of the block

  • Ensure the new added brackets will never be part of the selection
  • Encapsulate in editBegin/End
  • Reduce if/else nesting which make this patch looking bad, sorry

BUG:382213
Fixed in: 5.57

Test Plan
  • Feature or bug? Select in block mode from right->left on a single line after last char a block. The brackets are exchanged )( and placed at the end of the block

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Mar 1 2019, 5:23 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 1 2019, 5:23 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Mar 1 2019, 5:23 PM

Hi,

I think this is an improvement.

Could you update the patch to current master?

For your questions:

  • Feature or bug? Select in block mode from right->left on a single line after last char a block. The brackets are exchanged )( and placed at the end of the block

Hmm, I think this counts as a bug.

  • Code use MovingRange, old(?) similar code fumble around with backup cursors, I hope not for some optimizing but only because its older than MovingRange

I think this was just ancient code.
I think for this action, there is no need to try to micro-optimized, the movingrange solution is more safe.

  • No use of toVirtualColumn or similar. It looks so far good anyway. Only when you mix tabs and spaces the result may sometimes odd. But I think there is no solution in using toVirtualColumn in the cases I had played around

I would have thought that toVirtualColumn solves the issues if you have mixed tabs/spaces. Thought I am not that concerned if that is not perfect, given block selection + mixing tabs/spaces is always a mess.

  • No call of view->slotTextInserted (will only emit signal) can't see any effect and isn't such signal not already emit by the doc itself?

Calls to that should be added. People might rely in the apps using the part that we emit that for "user-inserted" text, as the docs state.
Should be no big issue, or? Just call that after each insertText operation, like before.

  • Autotests looks OK, I may add some for the new feature when patch gets accepted so far

That would be great!
You can count this as accepted after the above pointed out things are altered.

cullmann requested changes to this revision.Mar 31 2019, 10:09 AM
This revision now requires changes to proceed.Mar 31 2019, 10:09 AM
loh.tar updated this revision to Diff 55272.Apr 2 2019, 1:10 PM
  • Rebase on master
loh.tar updated this revision to Diff 55280.Apr 2 2019, 2:18 PM
loh.tar edited the test plan for this revision. (Show Details)
  • Add some auto tests

Feature or bug? Select in block mode from right->left on a single line after last char a block. The brackets are exchanged )( and placed at the end of the block

Hmm, I think this counts as a bug.

Have trouble to reproduce now. Was that fixed somewhere? Qt? Strange.

loh.tar updated this revision to Diff 55284.Apr 2 2019, 2:41 PM
  • QCOMPARE also the selection in tests
loh.tar updated this revision to Diff 55303.Apr 2 2019, 7:32 PM
loh.tar set the repository for this revision to R39 KTextEditor.
  • Set proper start/end column independent from selection direction.
loh.tar updated this revision to Diff 55346.Apr 3 2019, 2:51 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
  • Ensure the new added brackets will not be part of the selection when selection was done from right->left
  • Enhance autotest

There is already toVirtualColumn used by rangeOnLine() and I have no idea how to fix the remaining issues, maybe you.


In the pic was the selection always done from right->left which seems to be the problems to increase. Everything looks good (even it looks odd) except the highlighted case.
The problem occurs when the selection start/end has mixed(different) space/tabs. The block below the highlighted is the same but works fine. So I think rangeOnLine() may need an improved.

KTextEditor::Range KTextEditor::DocumentPrivate::rangeOnLine(KTextEditor::Range range, int line) const
{
    const int col1 = toVirtualColumn(range.start());
    const int col2 = toVirtualColumn(range.end());
    return KTextEditor::Range(line, fromVirtualColumn(line, col1), line, fromVirtualColumn(line, col2));
}
loh.tar updated this revision to Diff 55355.Apr 3 2019, 3:51 PM
loh.tar edited the test plan for this revision. (Show Details)
loh.tar set the repository for this revision to R39 KTextEditor.
  • Bring back call of view->slotTextInserted

I think it's ready for a new review

loh.tar added a comment.EditedApr 4 2019, 1:26 PM

Feature or bug? Select in block mode from right->left on a single line after last char a block. The brackets are exchanged )( and placed at the end of the block

Hmm, I think this counts as a bug.

Have trouble to reproduce now. Was that fixed somewhere? Qt? Strange.

Stupid me, didn't understand my own description. :-/
The behavior still exist and is independent from the selection direction.

Our MovingRange "selectionRange" gets wrong updated in that case. So I think it's out of the scope from this patch.

Edit: The issue is located here...

TextBlock::insertText(...)
...
        // special handling if cursor behind the real line, e.g. non-wrapping cursor in block selection mode
        else if (cursor->m_column < textOfLine.size()) {
            cursor->m_column = textOfLine.size();
        }
...

...where ALL cursors are updated. While this special treatment is correct for our first inserted bracket, the closing one, there will also the start cursor of our range set to that value. Hence when the second bracket is inserted, the open one, it goes to that (now wrong) position.

Ideas?

Edit2: The mentioned code in TextBlock::insertText shouldn't run because there are test before these lines to ignore not affected cursors (before the insert position). Unfortunately are our cursors behind the actual end of the line. Hence they are always treated as after the insert position.

When the first bracket is inserted, this code pad the closing bracket like " )" to achieve the desired position.

DocumentPrivate::editInsertTex(...)
...
    QString s2 = s;
    int col2 = col;
    if (col2 > l->length()) {
        s2 = QString(col2 - l->length(), QLatin1Char(' ')) + s;
        col2 = l->length();
    }
...

Perhaps can this code move to TextBlock::insertText or the count of extra add spaces given as argument to adjust the checked bounds.

Edit: The issue is located here...

TextBlock::insertText(...)
...
        // special handling if cursor behind the real line, e.g. non-wrapping cursor in block selection mode
        else if (cursor->m_column < textOfLine.size()) {
            cursor->m_column = textOfLine.size();
        }
...

Removing these lines solve the issue surprising good. Test are passed too...
But a full solution ist it not. Entering now a char after end of line did not move the cursor.

cullmann accepted this revision.Apr 7 2019, 1:14 PM

The new functionality works ok enough for me.

For the other issues you found with the cursor updates and the range stuff I would like different patches if we want to tackle them.

This revision is now accepted and ready to land.Apr 7 2019, 1:14 PM
This revision was automatically updated to reflect the committed changes.