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

List:       koffice-devel
Subject:    Re: Review Request: Add ability to read,
From:       Inge Wallin <inge () lysator ! liu ! se>
Date:       2009-10-21 0:47:36
Message-ID: 200910210247.36195.inge () lysator ! liu ! se
[Download RAW message or body]

On Tuesday 20 October 2009 22:08:58 Thomas Zander wrote:
> On Tuesday 20. October 2009 15.12.12 Inge Wallin wrote:
> > > On 2009-10-20 09:36:18, Thomas Zander wrote:
> > > > trunk/koffice/libs/odf/KoBorder.cpp, line 513
> > > > <http://reviewboard.kde.org/r/1890/diff/3/?file=13030#file13030line51
> > > >3>
> > > >
> > > >     Please don't parse each item 4 times, store it in an intermediate
> > > > variable.
> >
> > Copied from your own code, but I fixed it here.  I suggest you do the
> > same in ParagraphStyle :-)
> 
> Your patch 6 doesn't show this fix.

Yes it does.  The code now uses QString::split instead of parsing.  Can you 
find a spot where it doesn't, then I will change that too, but everywhere I 
looked it's fixed.

> >> >     Please place the curly brace one line up.
> >
> >That will be more difficult to read, but ok
> 
> This doesn't show in your latest patch.

Well, it did, but there was another place where the same happened.  Now fixed 
in both places.
 
> >> On 2009-10-20 09:36:18, Thomas Zander wrote:
> >> > trunk/koffice/libs/widgets/KoPageLayout.h, line 62
> 
> <http://reviewboard.kde.org/r/1890/diff/3/?file=13031#file13031line62>
> 
> >> >     Why did you write this? How is the operator= different from the
> >> > auto generated one for a struct?
> >
> >Good question, but without it it doesn't compile.
> 
> That doesn't make it right  :)
> Better look for the real reason. [looks] Ah, you forgot a const in the copy
> constructor of KoBorder.

unbelievable!  well, fixed now.
_______________________________________________
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]