[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Fix some memory leaks in the Fonts Installer KCM
From:       "Craig Drummond" <craig () kde ! org>
Date:       2009-09-11 20:19:20
Message-ID: 20090911201920.17829.18314 () localhost
[Download RAW message or body]


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


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 :-)


svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/kcmfontinst/FontFilterProxyStyle.cpp
 <http://reviewboard.kde.org/r/1567/#comment1625>

    Please just pass 'parent' to the QStyle constructor.



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/lib/FcEngine.h
 <http://reviewboard.kde.org/r/1567/#comment1626>

    Please, use the same coding standard as the rest of the file. In this case it \
would be 'itsFcConfig'



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/lib/FcEngine.cpp
 <http://reviewboard.kde.org/r/1567/#comment1623>

    Again, adhere to the coding style of the file you are editing. In this case \
single line 'if' is not followed by brackets.



svn://anonsvn.kde.org/home/kde/trunk/KDE/kdebase/workspace/kcontrol/kfontinst/lib/FcEngine.cpp
 <http://reviewboard.kde.org/r/1567/#comment1624>

    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.


- Craig


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
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]