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

List:       koffice-devel
Subject:    Re: Review Request: Add struct KoFontFace
From:       "Inge Wallin" <inge () lysator ! liu ! se>
Date:       2009-11-28 12:57:19
Message-ID: 20091128125719.22821.26303 () localhost
[Download RAW message or body]



> On 2009-11-26 04:51:54, Thorsten Zachmann wrote:
> > trunk/koffice/libs/odf/KoGenStyles.cpp, line 320
> > <http://reviewboard.kde.org/r/2257/diff/2/?file=15087#file15087line320>
> > 
> > As you already pointed out this will result in a memory leek as nobody will \
> > delete the KoFontFace objects. How about storing the KoFontFace in the fontFaces \
> > map as a object instead of a pointer like QMap<QString, KoFontFace>. Not sure if \
> > you need it again to be modified which should be avoided in my opinion. If so you \
> > can go the same way as KoGenStyles::styleForModification

Actually, it only looked like a memory leak, since KoGenStyles takes over ownership \
and will later delete it.

As for the other comment, won't that lead to very inefficient code?  The class is not \
implicitly shared and it will lead to a lot of copies when you access the list.  My \
suggestion is that we allow this patch to commit now and if there are other issues \
they will show themselves and we can change it afterwards.


- Inge


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


On 2009-11-25 13:07:11, Inge Wallin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2257/
> -----------------------------------------------------------
> 
> (Updated 2009-11-25 13:07:11)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> Here is the first patch in a small patchset that will add filters for the MS 2007 \
> XML file formats. Note that I am only the middle man here. 
> This patch adds the struct KoFontFace that is a public data structure representing \
> a font style. It is saved as a style:font-face element. Before this patch, only the \
> font name was available, which was saved as a style:name attribute of \
> style:font-face. Also, svg:font-family was saved as a copy. 
> Now the old behaviour is still available but in addition, new attributes can be \
> specified in order to make the description of a font more accurate  \
> (style:font-family-generic, svg:font-style, style:font-pitch). These are needed for \
> increasing quality of import/export filters and is already used in the MSOOXML \
> filters. The ODF specification offers even more attributes. 
> The patch also contains fix for use of m_mainStyles->addFontFace() in two places, \
> both in KWord. 
> 
> Diffs
> -----
> 
> trunk/koffice/libs/odf/CMakeLists.txt 1053329 
> trunk/koffice/libs/odf/KoFontFace.h PRE-CREATION 
> trunk/koffice/libs/odf/KoFontFace.cpp PRE-CREATION 
> trunk/koffice/libs/odf/KoGenStyles.h 1053329 
> trunk/koffice/libs/odf/KoGenStyles.cpp 1053329 
> 
> Diff: http://reviewboard.kde.org/r/2257/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Inge
> 
> 

_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel


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