Review Request 123341: Optimize reading Sonnet settings by minimizing the cals to save()

[Milian Wolff]

> On May 19, 2015, 3:12 p.m., Vishesh Handa wrote:
> > autotests/test_settings.cpp, line 43
> > <https://git.reviewboard.kde.org/r/123341/diff/4/?file=362129#file362129line43>
> >
> >     Minor thing, which is up to you.
> >     
> >     In this test you're allocating the `Speller` off the heap and having to delete it. In the other test you're allocating it off the stack.
> 
> Kåre Särs wrote:
>     In the first test I allocate the Speller on the heap to be able delete it before the end of test case.  I delete it to test that it does not save the settings automatically on destruction. I'll add comments :)

and better use QScopedPointer


- Milian


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


On May 12, 2015, 6:03 p.m., Kåre Särs wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123341/
> -----------------------------------------------------------
> 
> (Updated May 12, 2015, 6:03 p.m.)
> 
> 
> Review request for KDE Frameworks, Laurent Montel and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: sonnet
> 
> 
> Description
> -------
> 
> The curent implementation can causes ~25 cals to Settings::save() for every created Speller instance. 
> 
> The Settings::restore() function ends with setQuietIgnoreList(...) which would call save() for evey ignore-word.
> 
> This patch removes the save() cals in the setFoo() functions of Settings and replace it by a boolean that indicates if the settign is changed or not. Then in the Speller class save() is called when needed.
> 
> The reason for this patch is that it took Kate ~2 minutes to open a kate session with ~340 files. The implementation in Kate called reload() for every file. There is a RR for Kate to remove that unneeded reload.
> 
> This patch also prepares Sonnet::Settings for being used in the public API to set aplication specific Sonnet settings without saving them in the global settings.
> 
> The Settings class is exported but the header is private. This change is BIC, but since the class is private, my interpretation is that it should not mater.
> 
> If accepted I will also add a Review Request to make the Settings class public to enable application specific settings.
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt 8ade413 
>   autotests/test_settings.h PRE-CREATION 
>   autotests/test_settings.cpp PRE-CREATION 
>   src/core/settings.cpp b5c4198 
>   src/core/settings_p.h 0d48889 
>   src/core/speller.cpp 3903b42 
>   src/ui/configwidget.cpp 02f2a26 
> 
> Diff: https://git.reviewboard.kde.org/r/123341/diff/
> 
> 
> Testing
> -------
> 
> Tested with Kate and with the patch the startup time was back to "normal". 
> Two unit tests added.
> 
> 
> Thanks,
> 
> Kåre Särs
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150519/985d3223/attachment-0001.html>