D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir added a comment.


  In D18793#437220 , @cullmann wrote:
  
  > 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...

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, mwolff
Cc: loh.tar, kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[loh tar]

loh.tar added a comment.


  In D18793#421367 , @ahmadsamir 
wrote:
  
  > In D18793#420599 , @loh.tar 
wrote:
  >
  > > Fix this patch also Bug 382213 ?
  >
  >
  > No, it doesn't fix it.
  
  
  But this may D19446 

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, mwolff
Cc: loh.tar, kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Christoph Cullmann]

cullmann requested changes to this revision.
cullmann added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, mwolff
Cc: loh.tar, kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir added a subscriber: loh.tar.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, mwolff
Cc: loh.tar, kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir updated this revision to Diff 54150.
ahmadsamir edited the summary of this revision.
ahmadsamir edited the test plan for this revision.
ahmadsamir removed a reviewer: KDevelop.
ahmadsamir removed subscribers: loh.tar, mwolff.
ahmadsamir added a comment.


  Verbatim

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18793?vs=54149=54150

BRANCH
  block-selection-completion-3 (branched from master)

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  autotests/src/completion_test.h
  src/completion/katewordcompletion.cpp
  src/document/katedocument.cpp
  src/document/katedocument.h

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, mwolff, #kdevelop
Cc: kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann, loh.tar

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir updated this revision to Diff 54149.
ahmadsamir added a comment.


  - 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

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18793?vs=52658=54149

BRANCH
  block-selection-completion-3 (branched from master)

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  autotests/src/completion_test.h
  src/completion/katewordcompletion.cpp
  src/document/katedocument.cpp
  src/document/katedocument.h

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: loh.tar, mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir added a comment.


  In D18793#424628 , @cullmann wrote:
  
  > 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.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: loh.tar, mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Christoph Cullmann]

cullmann requested changes to this revision.
cullmann added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: loh.tar, mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir added a comment.


  Do we still need a unit test for this?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: loh.tar, mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir added a comment.


  In D18793#420599 , @loh.tar wrote:
  
  > Fix this patch also Bug 382213 ?
  
  
  No, it doesn't fix it.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: loh.tar, mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[loh tar]

loh.tar added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: loh.tar, mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir updated this revision to Diff 52658.
ahmadsamir added a comment.


  Change replaceText to handle inserting word completion with block selection 
mode

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18793?vs=51051=52658

BRANCH
  block-selection-completion-2 (branched from master)

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

AFFECTED FILES
  src/completion/katewordcompletion.cpp
  src/document/katedocument.cpp

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  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

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir added a comment.


  Ping?

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop
Cc: kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Christoph Cullmann]

cullmann added a reviewer: KDevelop.
cullmann added a comment.


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

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop
Cc: kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D18793: Handle text completion with block selection mode

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: KTextEditor, cullmann, dhaumann.
Herald added projects: Kate, Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  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.

REPOSITORY
  R39 KTextEditor

BRANCH
  block-selection-completion (branched from master)

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

AFFECTED FILES
  src/completion/katewordcompletion.cpp

To: ahmadsamir, #ktexteditor, cullmann, dhaumann
Cc: kde-frameworks-devel, kwrite-devel, #ktexteditor, hase, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann