Review Request 112787: Completion filtering: Abbreviation expansion and "contains" filtering

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112787/#review41156
-----------------------------------------------------------


Very cool feature, but this review request is missing unit tests which are imo essential. Also some kind of benchmarks would be interesting to see how it impacts performance.


part/completion/katecompletionmodel.cpp
<http://git.reviewboard.kde.org/r/112787/#comment30189>

    unrelated change



part/completion/katecompletionmodel.cpp
<http://git.reviewboard.kde.org/r/112787/#comment30191>

    static to give file-local linkage



part/completion/katecompletionmodel.cpp
<http://git.reviewboard.kde.org/r/112787/#comment30198>

    don't use a reference to QChar, you wouldn't do it for int either. QChar is just a wrapper around ushort.



part/completion/katecompletionmodel.cpp
<http://git.reviewboard.kde.org/r/112787/#comment30195>

    also move this block into an external function.



part/completion/katecompletionmodel.cpp
<http://git.reviewboard.kde.org/r/112787/#comment30197>

    here and below: don't use references to QChar, you wouldn't do it for int either. QChar is just a wrapper around a ushort which is smaller than a ptr/ref.



part/completion/katecompletionmodel.cpp
<http://git.reviewboard.kde.org/r/112787/#comment30193>

    if you implement the below, you may not need this check anymore as you'll get it for free from the new function.



part/completion/katecompletionmodel.cpp
<http://git.reviewboard.kde.org/r/112787/#comment30192>

    this is inefficient. rewrite this such that you merge the startsWith into the getAbbrevation call, i.e.:
    
    if (abbrevationMatches(m_nameColumn, match)) {
      matchCompletion = AbbrevationMatch;
    }
    
    Note: Don't pass m_nameColumn.toLower() as that would need a alloc which is costly. Instead, use QChar comparison and it's ::toLower() method which does not need malloc.
    
    Furthermore, this way you only need to go up to the first part of m_name where it's abbreviation deviates from the match.
    
    I.e.:
    
    m_name = fooBarAsdfLaLaLa
    match= fXYZ
    
    your code would need to convert fXYZ to fxyz. Then it iterates over all of m_name and allocates a string holding "fBALLL". Then it does the comparison.
    
    Instead I want no allocs, and only iterate up to, including, fooB of m_name. At that point you can already say that there is no match.



part/completion/katecompletionmodel.cpp
<http://git.reviewboard.kde.org/r/112787/#comment30194>

    this will be rewritten anyways, but in the future always use str.startsWith(match, Qt::CaseInsensitively). instead of match.toLower() which needs an allocation


- Milian Wolff


On Oct. 2, 2013, 10:53 p.m., Sven Brauch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112787/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2013, 10:53 p.m.)
> 
> 
> Review request for Kate and KDevelop.
> 
> 
> Repository: kate
> 
> 
> Description
> -------
> 
> (I'll put this up here for discussion, I think a bit more work is required before submission.)
> 
> This patch implements less restrictive rules for completion list filtering. Currently, an item is only displayed if it starts with the typed text. In addition to that, I'd like to allow:
> 
>  1) Abbreviation expansion; see first screnshot. This matches entries where the entered text matches the (beginning of) the camelCased or under_scored entry name's first letters.
> 
>  2) Match if the entry only contains the word. This probably needs a bit of thinking to not disrupt existing workflows. My current solution only uses this filter if the length of the entered text is 4 or more characters.
> 
> What do you think?
> 
> Do you think this should be configurable?
> 
> 
> Diffs
> -----
> 
>   part/completion/katecompletionmodel.h 088ac19 
>   part/completion/katecompletionmodel.cpp accd5e4 
> 
> Diff: http://git.reviewboard.kde.org/r/112787/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> abbreviation expansion
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/17/matching.png
> "contains" matching (if length > 3)
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/17/matching1.png
> 
> 
> Thanks,
> 
> Sven Brauch
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20131003/9d84095c/attachment-0001.html>