Improvements to completion
ClosedPublic

Authored by thomassc on Jan 2 2019, 10:42 PM.

Details

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.

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
thomassc created this revision.Jan 2 2019, 10:42 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptJan 2 2019, 10:42 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
thomassc requested review of this revision.Jan 2 2019, 10:42 PM
cullmann added a subscriber: cullmann.

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

mwolff requested changes to this revision.Jan 10 2019, 10:56 AM
mwolff added a subscriber: mwolff.

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)

src/completion/katecompletionmodel.cpp
1554

unrelated whitespace changes should be fixed in separate commits

1575

here and below:remove space after !

1888

static or anon namespace

2026

maybe simplify this to:

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

matchCompletion = PerfectMatch;
m_haveExactMatch = true;

}

2029

remove whitespace after !

src/completion/katecompletionmodel.h
394

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

This revision now requires changes to proceed.Jan 10 2019, 10:56 AM
thomassc marked 4 inline comments as done.Jan 12 2019, 6:04 PM

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.
src/completion/katecompletionmodel.cpp
2026

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

src/completion/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.

thomassc updated this revision to Diff 49343.Jan 12 2019, 6:05 PM

Update according to Milian's comments

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?

src/completion/katecompletionmodel.h
394

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

thomassc updated this revision to Diff 49397.Jan 13 2019, 4:21 PM

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

mwolff accepted this revision as: mwolff.Jan 15 2019, 7:26 PM

@brauch, @kfunk what do you say?

Should we give this some try and merge it?

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.

mwolff added a comment.Feb 5 2019, 7:30 AM

I'm in favor!

cullmann accepted this revision.Feb 6 2019, 6:20 PM

Then I will apply this.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 9 2019, 4:05 PM
Closed by commit R39:7c42ba4a1aed: Improvements to completion (authored by thomassc, committed by cullmann). · Explain Why
This revision was automatically updated to reflect the committed changes.