D17932: Improvements to completion

2019-02-09 Thread Christoph Cullmann
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R39:7c42ba4a1aed: Improvements to completion (authored by 
thomassc, committed by cullmann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17932?vs=49397=51267

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h

To: thomassc, #ktexteditor, #kdevelop, mwolff, cullmann
Cc: dhaumann, apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, 
kde-frameworks-devel, michaelh, ngraham, bruns, demsking, head7, sars


D17932: Improvements to completion

2019-02-06 Thread Christoph Cullmann
cullmann accepted this revision.
cullmann added a comment.


  Then I will apply this.

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff, cullmann
Cc: dhaumann, apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, 
kde-frameworks-devel, hase, michaelh, ngraham, bruns, demsking, head7, sars


D17932: Improvements to completion

2019-02-04 Thread Milian Wolff
mwolff added a comment.


  I'm in favor!

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: dhaumann, apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, 
kde-frameworks-devel, hase, michaelh, ngraham, bruns, demsking, head7, sars


D17932: Improvements to completion

2019-02-02 Thread Dominik Haumann
dhaumann added a comment.


  Well, giving it a try and merge should be postponed ubtil tomorrow: dfaure 
tags today, so we would then have 1 month of internal testing.

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: dhaumann, apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, 
kde-frameworks-devel, hase, michaelh, ngraham, bruns, demsking, head7, sars


D17932: Improvements to completion

2019-02-02 Thread Christoph Cullmann
cullmann added a comment.


  Should we give this some try and merge it?

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, kde-frameworks-devel, 
hase, michaelh, ngraham, bruns, demsking, head7, sars, dhaumann


D17932: Improvements to completion

2019-01-15 Thread Milian Wolff
mwolff accepted this revision as: mwolff.
mwolff added a comment.


  @brauch, @kfunk what do you say?

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, kde-frameworks-devel, 
hase, michaelh, ngraham, bruns, demsking, head7, sars, dhaumann


D17932: Improvements to completion

2019-01-13 Thread Thomas Schöps
thomassc updated this revision to Diff 49397.
thomassc added a comment.


  Change to a single setter for match_cs and exact_match_cs to avoid 
restriction on call order of functions

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17932?vs=49343=49397

BRANCH
  improvements_to_completion (branched from master)

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, kde-frameworks-devel, 
hase, michaelh, ngraham, bruns, demsking, head7, sars, dhaumann


D17932: Improvements to completion

2019-01-13 Thread Milian Wolff
mwolff added subscribers: brauch, kfunk, apol.
mwolff added a comment.


  In D17932#392060 , @thomassc wrote:
  
  > Thanks for reviewing. Regarding the question about which models would have 
an insensitive exact match, and which ones have sensitive exact matches:
  >
  > - An example for case-insensitive exact matches might be plain text, or a 
hypothetical case-insensitive programming language. For example for plain text, 
one might want to treat words like "Question" and "question" as exact matches, 
which will make the completion widget close itself when it shows one of them 
and the user types the other. This is the current behavior for the word 
completion in KWrite / Kate.
  > - An example for case-sensitive exact matches would be a case-sensitive 
programming language like C++. If the user typed "m_var" but the variable is 
actually called "m_Var", then the completion widget should not hide itself, 
since it might still be useful to replace the typed text with the completion 
item that has different case.
  
  
  please put that information into the commit message - I think it's valuable 
to have
  
  overall, I think I'm in favor of getting this in - but I'd like to get input 
from others. @brauch , @kfunk, @apol what do you have to say to this?

INLINE COMMENTS

> thomassc wrote in katecompletionmodel.h:394
> I added asserts to the setters. One should be aware then though that it 
> restricts the order in which these must be called. For example, if both 
> settings are case-insensitive at first and both should be changed to 
> case-sensitive, then the exact-match-sensitivity must be changed before the 
> match-sensitivity. Alternatively, one could make a single setter function 
> that changes both properties.

either that, or handle it gracefully (with a warning?) wherever it's used?

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, kde-frameworks-devel, 
hase, michaelh, ngraham, bruns, demsking, head7, sars, dhaumann


D17932: Improvements to completion

2019-01-12 Thread Thomas Schöps
thomassc updated this revision to Diff 49343.
thomassc added a comment.


  Update according to Milian's comments

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17932?vs=48574=49343

BRANCH
  improvements_to_completion (branched from master)

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: mwolff, cullmann, kwrite-devel, kde-frameworks-devel, hase, michaelh, 
ngraham, bruns, demsking, head7, kfunk, sars, dhaumann


D17932: Improvements to completion

2019-01-12 Thread Thomas Schöps
thomassc marked 4 inline comments as done.
thomassc added a comment.


  Thanks for reviewing. Regarding the question about which models would have an 
insensitive exact match, and which ones have sensitive exact matches:
  
  - An example for case-insensitive exact matches might be plain text, or a 
hypothetical case-insensitive programming language. For example for plain text, 
one might want to treat words like "Question" and "question" as exact matches, 
which will make the completion widget close itself when it shows one of them 
and the user types the other. This is the current behavior for the word 
completion in KWrite / Kate.
  - An example for case-sensitive exact matches would be a case-sensitive 
programming language like C++. If the user typed "m_var" but the variable is 
actually called "m_Var", then the completion widget should not hide itself, 
since it might still be useful to replace the typed text with the completion 
item that has different case.

INLINE COMMENTS

> mwolff wrote in katecompletionmodel.cpp:2026
> maybe simplify this to:
> 
> if (matchCompletion && m_nameColumn.startsWith(match, 
> model->exactMatchCaseSensitivity())) {
> 
>   matchCompletion = PerfectMatch;
>   m_haveExactMatch = true;
> 
> }

Hmm, I guess the current version might be a tiny bit faster in general since it 
only does another startsWith() check if this would be a stricter check than the 
one done before ...

> mwolff wrote in katecompletionmodel.h:394
> should this comment be asserted in the setters or is it handled gracefully in 
> the logic below?

I added asserts to the setters. One should be aware then though that it 
restricts the order in which these must be called. For example, if both 
settings are case-insensitive at first and both should be changed to 
case-sensitive, then the exact-match-sensitivity must be changed before the 
match-sensitivity. Alternatively, one could make a single setter function that 
changes both properties.

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: mwolff, cullmann, kwrite-devel, kde-frameworks-devel, hase, michaelh, 
ngraham, bruns, demsking, head7, kfunk, sars, dhaumann


D17932: Improvements to completion

2019-01-10 Thread Milian Wolff
mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  Hey there, sorry for the long delay. In general, I think your suggestions are 
very sane - most notably preferring exact case matches over fuzzy matches is a 
good thing to have!
  
  But I wonder: do we really need to make this configurable? Can't we just 
always do this? I.e. can you explain which models would have an insensitive 
exact match, and which ones have sensitive exact matches?
  
  Can you please submit the next patch iteration with context (I suggest you 
use `arc diff` for that)

INLINE COMMENTS

> katecompletionmodel.cpp:1554
>  
> -if(m_unimportant && !rhs.m_unimportant){
> +if (m_unimportant && !rhs.m_unimportant) {
>  return false;

unrelated whitespace changes should be fixed in separate commits

> katecompletionmodel.cpp:1575
> +
> +if( thisStartWithFilter && ! rhsStartsWithFilter ) {
> +return true;

here and below:remove space after `!`

> katecompletionmodel.cpp:1888
>  
> +QChar toLowerIfInsensitive(QChar c, Qt::CaseSensitivity caseSensitive) 
> +{

static or anon namespace

> katecompletionmodel.cpp:2026
>  
>  if (matchCompletion && match.length() == m_nameColumn.length()) {
> +if (model->matchCaseSensitivity() == Qt::CaseInsensitive &&

maybe simplify this to:

if (matchCompletion && m_nameColumn.startsWith(match, 
model->exactMatchCaseSensitivity())) {

  matchCompletion = PerfectMatch;
  m_haveExactMatch = true;

}

> katecompletionmodel.cpp:2029
> +model->exactMatchCaseSensitivity() == Qt::CaseSensitive &&
> +! m_nameColumn.startsWith(match, Qt::CaseSensitive)) {
> +return matchCompletion;

remove whitespace after !

> katecompletionmodel.h:394
> +Qt::CaseSensitivity m_matchCaseSensitivity = Qt::CaseInsensitive;
> +Qt::CaseSensitivity m_exactMatchCaseSensitivity = Qt::CaseInsensitive;  
> // Must be equal to or stricter than m_matchCaseSensitivity.
> +

should this comment be asserted in the setters or is it handled gracefully in 
the logic below?

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: mwolff, cullmann, kwrite-devel, kde-frameworks-devel, hase, michaelh, 
ngraham, bruns, demsking, head7, kfunk, sars, dhaumann


D17932: Improvements to completion

2019-01-08 Thread Christoph Cullmann
cullmann added a reviewer: KDevelop.
cullmann added a comment.


  Perhaps some KDevelop people have feedback, given they use that mostly ;=)

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop
Cc: cullmann, kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, 
bruns, demsking, head7, kfunk, sars, dhaumann


D17932: Improvements to completion

2019-01-03 Thread Thomas Schöps
thomassc created this revision.
thomassc added a reviewer: KTextEditor.
thomassc added a project: KTextEditor.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
thomassc requested review of this revision.

REVISION SUMMARY
  My goal is to fix two issues with code completion in KDevelop:
  
  First, it seems to be mostly insensitive to upper/lowercasing, which is an 
issue with a case-sensitive language such as C++. I'd like to have all 
(case-insensitive) matches in the completion list, but always sort those to the 
top and have them selected by default which match the case of the typed text.
  
  Second, I'd like to have the completion widget hidden when there is a 
case-sensitive exact match, such that the completion widget does not obstruct 
the view in this case. Currently it never seems to get hidden automatically.
  
  This diff is an attempt to implement a first step towards these goals by 
improving some things in KTextEditor. I'd make more changes to ensure that the 
auto-selected completion item tries to match the case of the typed text, and to 
make this usable in KDevelop, but would like to put this up here for discussion 
first before making further changes. Would you be fine with the proposed 
changes in general? Would you choose a different approach? The changes in this 
initial diff are described below.
  
  1. The diff introduces a new setting m_exactMatchCaseSensitivity next to 
m_matchCaseSensitivity. By setting m_matchCaseSensitivity == 
Qt::CaseInsensitive and m_exactMatchCaseSensitivity == Qt::CaseSensitive, it is 
possible to have all case-insensitive matches in the completion list, while 
only allowing case-sensitive matches to be exact matches (that will hide the 
completion widget).
  2. In KateCompletionModel::Item::operator <, the (case-sensitive) comparison 
with the current typed text is moved up to just below the matchCompletion 
comparison. This means that items with correctly matching case will match 
better than items with incorrect case (if they both start with the typed text), 
regardless of inheritance depth and alphabetical ordering.
  3. This comparison is also modified to only return true or false if only one 
of the items matches the typed text. The original code would yield inconsistent 
results if both items match the typed text, since the comparison would return 
true for both the test (a < b) and the test (b < a).
  4. When determining whether a completion item matches the typed text, the 
diff makes matchesAbbreviation() take model->matchCaseSensitivity() into 
account (as the other match tests already do). This fixes the issue that 
without this change, "test" and "TEST" would be considered to match exactly 
with model->matchCaseSensitivity() == Qt::CaseSensitive, since the abbreviation 
matching would treat it as a match.
  
  Regarding point 2, this change will only sort those completion items by 
case-compatibility that start with the typed text, but not the others. An 
alternative would be to determine case-compatibility of the item with the typed 
text when matching (in KateCompletionModel::Item::match()). Then completion 
items with the same MatchType could be sorted by case-compatibility (either in 
binary form, "matches case" vs. "does not match case", or by counting the 
number of letters with differing case).

TEST PLAN
  completion_test still passes. Added four checks to this test which test 
case-sensitive matching with matchesAbbreviation(). Did some manual testing.

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  autotests/src/completion_test.cpp
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h

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