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

[Sven Brauch]

> On June 27, 2016, 8:36 a.m., Milian Wolff wrote:
> > language/backgroundparser/backgroundparser.h, line 178
> > <https://git.reviewboard.kde.org/r/128290/diff/1/?file=469961#file469961line178>
> >
> >     also, looking below, what if delay is -2? could the special values leak here?

No; no update will be scheduled by DocumentChangeTracker when the delay is ILanguageSupport::NoUpdateRequired. I also replaced the other magic values by this enum, sorry for not doing that earlier.


> On June 27, 2016, 8:36 a.m., Milian Wolff wrote:
> > language/backgroundparser/documentchangetracker.cpp, line 161
> > <https://git.reviewboard.kde.org/r/128290/diff/1/?file=469964#file469964line161>
> >
> >     :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 :-/

We could introduce a struct DocumentParseRequest { IndexedString url; int priority; ParseJob::SequentialProcessingFlags flags; int delay_ms; }, with sensible default values. That would at least make it easier to read ... but yes, C++ not having named arguments is a realy pity :(


> On June 27, 2016, 8:36 a.m., Milian Wolff wrote:
> > language/backgroundparser/documentchangetracker.cpp, line 179
> > <https://git.reviewboard.kde.org/r/128290/diff/1/?file=469964#file469964line179>
> >
> >     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.

Well, it seems quite unlikely that we will ever have multiple languages in a single document and they disagree on this ... and if that happens, the case the max handles correctly at least is when one language says "I don't need an update" and the other wants one.


- Sven


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


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/cb633ddd/attachment-0001.html>