Handle text completion with block selection mode
Needs RevisionPublic

Authored by ahmadsamir on Feb 6 2019, 7:18 PM.

Details

Reviewers
cullmann
dhaumann
mwolff
Group Reviewers
KTextEditor
Summary

Split out the code responsible for inserting text in multi-line block
selection from typeChars to a new function, insertTextInBlock(), which
can be used in replaceText() to handle text completion when in block
selection mode.

When getting possible text completion matches from the current document,
the code skips the word the cursor is inside; extend that behaviour for
block selection too.

Add a unit test for the new completion behaviour with block selection.

BUG: 359763
FIXED-IN: 5.58

Test Plan

With block selection mode, selecting some text block and typing a word
invoking text completion, the completion text should be inserted on every
line in the block selection, test with remove-tail enabled/disabled.

Diff Detail

Repository
R39 KTextEditor
Branch
block-selection-completion (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7953
Build 7971: arc lint + arc unit
ahmadsamir created this revision.Feb 6 2019, 7:18 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptFeb 6 2019, 7:18 PM
ahmadsamir requested review of this revision.Feb 6 2019, 7:18 PM

KDevelop uses the completion extensively, perhaps there is some opinion about this change.

mwolff requested changes to this revision.Feb 19 2019, 12:27 PM
mwolff added a subscriber: mwolff.

we override execution in our own completion models, so this patch will only change the behavior for the builtin word and keyword completion models in ktexteditor I believe

that said, I think it makes sense to insert the word everywhere in block selection, it shouldn't be different from typing text.

so +1 for the idea, but -1 on the actual implementation:

  • we need to have a unit test for this new behavior
  • we should introduce new helper API to make it easier to opt-in to this new behavior and reduce the if/else depth. This would also make it easier for us in KDevelop to change our behavior accordingly. I believe the code completion execution code should basically be agnostic to the block selection mode. I.e. instead of the proposed
if (completeBlockSelection) {
    removeText
    typeChars
} else {
    replaceText
}

it should always just call "replaceText" with the the block selection range and then internal API should duplicate the text, if the selection is a block selection

as-is, this patch adds too much code duplication

This revision now requires changes to proceed.Feb 19 2019, 12:27 PM
ahmadsamir updated this revision to Diff 52658.Feb 26 2019, 6:40 PM

Change replaceText to handle inserting word completion with block selection mode

I changed the diff to make replaceText handle inserting the word completion text.

Is this a new behaviour? it exactly matches what users expect when they type something in block selection mode.

Also why would this be opt-in? if you type something in block selection is get replicated on all lines, so it follows that word completion should do the same thing...

Also note that the completion-with-remove-tail bits will have to stay in executeCompletion code, because we iterate over each line to find the tail, there won't be a removeText/replaceText that fits all the tails...

As for a unit test, I'll look into that as I haven't written one before.

Fix this patch also Bug 382213 ? - do not do auto-brackets when block mode is enabled
Well, bad title/request. I would expect that each line get the bracket.

Fix this patch also Bug 382213 ?

No, it doesn't fix it.

Do we still need a unit test for this?

cullmann requested changes to this revision.Mar 4 2019, 6:34 PM

I don't like the change to call typeChars.

  1. that needs a valid view, activeView() might be null.
  2. replaceText is not interactive per-default, but typeChars is.

And yes, a test would be wanted I think,too.

This revision now requires changes to proceed.Mar 4 2019, 6:34 PM

I don't like the change to call typeChars.

  1. that needs a valid view, activeView() might be null.
  2. replaceText is not interactive per-default, but typeChars is.

    And yes, a test would be wanted I think,too.

OK, I'll try reworking it.

ahmadsamir updated this revision to Diff 54149.Mar 17 2019, 8:53 PM
  • Split the code responsible for inserting text in block selection from typeChars() to a new function
  • Don't add words where the block selection cursor is inside to the possible completion matches from the document
  • Add unit test
ahmadsamir updated this revision to Diff 54150.Mar 17 2019, 8:54 PM
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir edited the test plan for this revision. (Show Details)
ahmadsamir removed a reviewer: KDevelop.
ahmadsamir removed subscribers: loh.tar, mwolff.

Verbatim

cullmann requested changes to this revision.Mar 24 2019, 1:13 PM

Hi, I still don't like that we do different things in replaceText depending on the selection of the potential activeView, that makes this function harder to use correctly, as it might magically do something the user might not expect.

This revision now requires changes to proceed.Mar 24 2019, 1:13 PM

Fix this patch also Bug 382213 ?

No, it doesn't fix it.

But this may D19446

Hi, I still don't like that we do different things in replaceText depending on the selection of the potential activeView, that makes this function harder to use correctly, as it might magically do something the user might not expect.

If the code is moved to say insertText it'd be the same problem, just further down the line.

I could overload replaceText, with the overload taking a view parameter with the responsibility of deciding which replaceText to use falling on the caller; (which is something KDevelop wants to avoid to ease their integration). The thing is, this piece of code needs a home somewhere.

  • replaceText shouldn't use typeChars because the latter is interactive by design
  • replaceText shouldn't be over-clever and do something different/unexpected depending on the selection in the activeView()

The work of inserting the completion falls on executeCompletionItem, so it should be handled there; also note that executeCompletionItem will become more complicated anyway, because of the remove-tail functionality...