Handle text completion with block selection mode
Needs RevisionPublic

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


Group Reviewers

Handle text completion with block selection mode

BUG: 359763
FIXED-IN: 5.56

Test Plan

With block selection mode, selecting some text block and typing a word
invoking text completion, the completion text should be inserted on all
the lines in the block selection.

Diff Detail

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

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

mwolff requested changes to this revision.Tue, Feb 19, 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) {
} else {

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.Tue, Feb 19, 12:27 PM