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

List:       koffice-devel
Subject:    Re: a tool for editing paragraph spacing
From:       Florian Merz <FlorianMerz () gmx ! de>
Date:       2008-02-29 16:08:30
Message-ID: 200802291708.31064.FlorianMerz () gmx ! de
[Download RAW message or body]

Hello Thorsten,

> Hello Florian,
>
> > Hi KOffice developers,
> >
> > I've been working on a KoTool for the TextShape which tries to allow
> > editing of paragraph spacing and indentation in a more visual and
> > intuitive way. Please keep in mind, that right now this is not more
> > than a proof of concept. There are lots of bugs and corner cases which
> > need to be taken care of before this is ready for some real use. It
> > also needs a few more features to be truly useful, such as line
> > spacing, QTextListFormat support and QTextTableFormat support.
> >
> >
> > What do you think about it?
>
> It is quite easy to use and looks quite good.

Thanks.

> > Any suggestions or ideas?
>
> How about make it possible to move the lines freely by pressing shift to
> disable the snapping to the predefined steps.

Something like that was already on my todo list, but it's good to know that 
shift is the proper key for this.

> > If this is considered
> > useful for koffice I'd be thankful for a code review. I'm not too sure
> > I used the koffice libraries properly...
>
> looks quite good. Here some remarks

Thanks, I fixed it all.

> +++ plugins/textshape/ParagraphSpacingToolFactory.cpp
>
> > + * Copyright (C) 2006 Florian Merz <florianmerz@web.de>
>
> Did you really do this 2 years ago :-)

Obviously not, but I copied the license stuff from some other file. Must 
have forgotten to change the year for those two files :)

> +++ plugins/textshape/HRuler.h
>
> > +#ifndef HARROWCONTROL_H
>
> the includeguard should match the filename. This i also true for some
> other classes.

It did, but then I changed the name of the class. I must have forgotten to 
change those, too.

> +++ plugins/textshape/HRuler.cpp
>
> > +#include <stdio.h>
>
> looks like this is no longer needed. The same is true for the one in
> VRuler.cpp.

You're right. I was lazy and used printf for debugging. I should start using 
kDebug() for this.

> +++ plugins/textshape/Ruler.h   (Revision 0)
>
> > +    void setBaseline(QPointF position, qreal lineLength);
>
> classes like QPointF should be passed as const reference.

Done.

> +++ plugins/textshape/ParagraphSpacingTool.h
>
> > +#include "TextShape.h"
> >
> > +#include <KoParagraphStyle.h>
> > +#include <KoCanvasBase.h>
>
> this should be moved to ParagraphSpacingTool.cpp and a forward
> declaration used.

Done.

> > +    Ruler *m_firstIndentRuler,
> > +          *m_followingIndentRuler,
> > +          *m_rightMarginRuler,
> > +          *m_topMarginRuler,
> > +          *m_bottomMarginRuler;
>
> Is there a special reason for creating them on the heap and not on the
> stack?

That's because I wasn't sure who has ownership of a QObject which is part of 
the QObject tree and on the stack at the same time. Stupid me. The rulers 
are now members of the tool class. The code is a little bit cleaner now.

> When testing in kpresenter I found a problem with displaying the rulers
> when the space in the shape is no longer big enough to hold all the text.
> See http://www.zagge.de/files/paragraph_spacing.png .

I'l have a look at that. There are still a lot of problems like that which 
need to be fixed.

> Looks very nice.
>
> Thorsten

I'll fix some other bugs and send a new patch as soon as I'm done.

Thanks again,
 Florian
_______________________________________________
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]