Review Request: Fix some memory leaks in the Fonts Installer KCM

[Darío Andrés]

> On 2009-09-11 20:19:31, Craig Drummond wrote:
> > Changes are good - except:
> > 1. Use QStyle(parent) - and not setParent() - matches the rest of the code better.
> > 2. No need for fcConfig variable just change FcInitLoadConfigAndFonts() to FcInitReinitialize()
> > 
> > I'll make these changes to trunk and the KDE4.3 branch.
> > 
> > ps. this kcm does still have a maintainer - me :-)

Good to know it has a maintainer :). I asked for you on kde-devel at freenode but I didn't got any answer: so I decided to paste it here so anyone could check it. 

Can you commit the changes?

Thanks for the review. :)


> On 2009-09-11 20:19:31, Craig Drummond wrote:
> > svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/kcmfontinst/FontFilterProxyStyle.cpp, line 32
> > <http://reviewboard.kde.org/r/1567/diff/2/?file=11227#file11227line32>
> >
> >     Please just pass 'parent' to the QStyle constructor.

QStyle construct doesn't not accept parameters. (I got an error the first time, and now I verified it in the Qt docs...) May be I'm missing something.


> On 2009-09-11 20:19:31, Craig Drummond wrote:
> > svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/lib/FcEngine.cpp, line 1581
> > <http://reviewboard.kde.org/r/1567/diff/2/?file=11229#file11229line1581>
> >
> >     By looking at fontconfig's code (specifically) fcinit.c - it would appear the this class should be just calling FcInitReinitialize()
> >     
> >     FcInitReinitialize() internally calls FcInitLoadConfigAndFonts() and sets the returned config to be the default one.
> >     
> >     Therefore, I think FcInitReinitialize() is what should be used - and then there is no need for the fcConfig variable.

I was storing fcConfig to call "FcConfigDestroy(fcConfig);" later (when updating or in class destroy). Otherwise a memory leak from the FC calls will appear in Valgrind...
Anyways, I don't know FontConfig code nor how it works, so I can't really tell which function should be used. I just only added the function I though it should avoid the mem leak (and it worked, according to Valgrind)


- Darío


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


On 2009-09-11 00:47:15, Darío Andrés wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1567/
> -----------------------------------------------------------
> 
> (Updated 2009-09-11 00:47:15)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> I'm not confident about this one (about the Fc* function calls), and as I don't know if this KCM has a current maintainer to ping, I'm leaving the patch here to discuss. 
> 
> 
> Diffs
> -----
> 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/lib/FcEngine.h 1021986 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/lib/FcEngine.cpp 1021986 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/viewpart/FontPreview.cpp 1021986 
>   svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/kcmfontinst/FontFilterProxyStyle.cpp 1021986 
> 
> Diff: http://reviewboard.kde.org/r/1567/diff
> 
> 
> Testing
> -------
> 
> It fixes several memory leaks easily found running the KCM using valgrind. 
> 
> 
> Thanks,
> 
> Darío
> 
>