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

List:       koffice-devel
Subject:    Re: Review Request: 'Combined characters' support for KWord
From:       "Thomas Zander" <zander () kde ! org>
Date:       2010-04-23 10:00:11
Message-ID: 20100423100011.2296.15437 () localhost
[Download RAW message or body]



> On 2010-04-22 14:27:00, Thomas Zander wrote:
> > Nifty way of solving your issue. I think this requires a unit test as this is one \
> > of those features that will never actually be used so nobody will notice it \
> > getting broken. Please add a loading unit test. The way you load it now looks \
> > like its going to be a formula when you save it again, thus destroying user data \
> > on a load/save cycle.  Any ideas how to solve that? 
> > Added some more comments inline.
> 
> Miroslav Nohaj wrote:
> Yes, now it saves that thing as a formula. One way how to avoid this would be to \
> check for the 'isCombinedCharacters' attribute in the applied style on the part of \
> the text and then output it as a span with style 'combine=characters' instead of \
> letting it to save it as a formula... (which is currently bit out of scope for me).

I'm against having this in koffice if it causes known dataloss. Please consider \
adding that saving code too.


> On 2010-04-22 14:27:00, Thomas Zander wrote:
> > /trunk/koffice/libs/kotext/opendocument/KoTextLoader.cpp, line 135
> > <http://reviewboard.kde.org/r/3701/diff/3/?file=24247#file24247line135>
> > 
> > Can't the variable be local to the method that uses it?
> 
> Miroslav Nohaj wrote:
> Yes, you're right. It could be a local static variable instead (not a normal one \
> because setting the value to true and using it is in a different nesting level of \
> loadSpan()). Would the local static variable be better?

Hmm, no, making it a local static is not better IMO :)  It just means the class still \
takes memory after its not been used anymore (among others). My suggestion to make it \
local only makes sense if its used in the same call to the method; but if you say it \
not then please keep using the class member.


> On 2010-04-22 14:27:00, Thomas Zander wrote:
> > /trunk/koffice/libs/kotext/opendocument/KoTextLoader.cpp, line 975
> > <http://reviewboard.kde.org/r/3701/diff/3/?file=24247#file24247line975>
> > 
> > Getting the width like this will fail for many reasons; please don't set a size \
> > on the frame while loading.
> 
> Miroslav Nohaj wrote:
> I can remove setting of the size here. It currently doesn't work like I wish it \
> would anyway (resizing of KoFormulaShape based on size).

That should be fixed too the, likely needs code in the formula shape.


- Thomas


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


On 2010-04-23 09:38:13, Miroslav Nohaj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3701/
> -----------------------------------------------------------
> 
> (Updated 2010-04-23 09:38:13)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> This code adds displaying of combined characters into KWord using KoFormulaShape by \
> generating appropriate MathML code. This is the 1st version, because currently the \
> KoFormulaShape ignores the width and height attributes and thus the displayed size \
> is always 50pt by 50pt (will have to check the KoFormulaShape deeper) and thus the \
> final size of the combined characters is not as small as a regular character (but \
> the patch as a whole is 95% complete). Please take a look and post comments if you \
> have some. 
> 
> Diffs
> -----
> 
> /trunk/koffice/kformula/flake/KoFormulaShapeFactory.cpp 1117436 
> /trunk/koffice/libs/kotext/opendocument/KoTextLoader.cpp 1117436 
> /trunk/koffice/libs/kotext/styles/KoCharacterStyle.h 1117436 
> /trunk/koffice/libs/kotext/styles/KoCharacterStyle.cpp 1117436 
> 
> Diff: http://reviewboard.kde.org/r/3701/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Miroslav
> 
> 

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