Review Request 128290: More flexible API to let language plugins when and how quickly to re-parse documents

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128290/#review96880
-----------------------------------------------------------



+1 in general. this should also fix the issue where adding a newline does not trigger a reparse in clang after triggering the "implement function" helper. thanks!


language/backgroundparser/backgroundparser.h (line 177)
<https://git.reviewboard.kde.org/r/128290/#comment65445>

    please add apidox for this param and the delay param. Make sure to mention that the delay needs to be specified in ms. Also document the difference between passing 0 and -1.
    
    In general, I'm not a big fan of "untyped" times, but introducing std::chrono won't give me many fans either. So maybe at rename the variables to msDelay or delayMS or delay_ms ?



language/backgroundparser/backgroundparser.h (line 178)
<https://git.reviewboard.kde.org/r/128290/#comment65448>

    also, looking below, what if delay is -2? could the special values leak here?



language/backgroundparser/documentchangetracker.cpp (line 136)
<https://git.reviewboard.kde.org/r/128290/#comment65446>

    :S
    
    Not opening an issue, but do you have any idea how to cleanup this API? Maybe move the delay forward so we can keep more methods as default? I think we may run into more such issues more often in the future... It's a pity we don't have named arguments in C++, nor  C's designated initializers :-/



language/backgroundparser/documentchangetracker.cpp (line 154)
<https://git.reviewboard.kde.org/r/128290/#comment65447>

    Hm using the max is pretty arbitrary. It could just as well be the min? In both cases you don't actually need a container though. For max e.g., just use
    
        auto delay = 0;
        for (...) {
            delay = max(delay, lang->...);
        }
        return delay;
    
    I know it shouldn't happen, but your version would also crash if no language controller would be found for the doc.


- Milian Wolff


On June 26, 2016, 2:43 p.m., Sven Brauch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128290/
> -----------------------------------------------------------
> 
> (Updated June 26, 2016, 2:43 p.m.)
> 
> 
> Review request for KDevelop and Kevin Funk.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> This replaces the whitespaceSensitivity() API which allowed languages to decide whether they are sensitive to whitespace changes, and instead introduces a function
> 
>     virtual int suggestedReparseDelayForChange(KTextEditor::Document* doc, const KTextEditor::Range& changedRange,
>                                                const QString& changedText, bool removal) const;
> 
> in ILanguageSupport which can be overriden. Languages can choose that the signalled change does not require an update at all (white-space in some languages), that an update should happen with the default delay, or give a custom delay in ms. The default implementation indicates an update with the default delay if the change was not whitespace-only or was whitespace but adjacent to a word.
> 
> In kdev-clang, this enables us to do something like
> 
>     int ClangSupport::suggestedReparseDelayForChange(KTextEditor::Document* /*doc*/, const KTextEditor::Range& /*changedRange*/,
>                                                      const QString& changedText, bool /*removal*/) const
>     {
>         if ( changedText.contains(QLatin1Char('\n')) || changedText.contains(QLatin1Char(';')) ) {
>             return ILanguageSupport::DefaultDelay;
>         }
>         return 3000;
>     }
>     
> (will submit separately), which makes completion feel much more snappy (Kevin knows ;)). I actually think I will do something similar in kdev-python, namely recommend an instant re-parse when a newline is added.
> 
> 
> Diffs
> -----
> 
>   language/backgroundparser/backgroundparser.h bf22d38 
>   language/backgroundparser/backgroundparser.cpp 84e4011 
>   language/backgroundparser/documentchangetracker.h 079feab 
>   language/backgroundparser/documentchangetracker.cpp 4ddded5 
>   language/interfaces/ilanguagesupport.h 5c3fc30 
>   language/interfaces/ilanguagesupport.cpp 67aab36 
> 
> Diff: https://git.reviewboard.kde.org/r/128290/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sven Brauch
> 
>

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