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

List:       koffice-devel
Subject:    Re: Review Request: paint line numbers
From:       "Thomas Zander" <zander () kde ! org>
Date:       2010-05-22 9:59:34
Message-ID: 20100522095934.31970.52531 () localhost
[Download RAW message or body]


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


This looks like a really nice addition but it feels a bit unfinished with too many \
hardcoded values. If you can fix those to be correct for any document size / length \
that would be wonderful.


trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5461>

    This logic is in the wrong place, deciding if document should have its lines \
numbered should not be dependent on KWord concepts.  I suggest just having a bool \
that KWord sets and this code uses so the decision and the concepts of text-type are \
moved to the application.  Maybe just having the lineNumberConfiguration present at \
all can be that boolean.



trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5462>

    page() may return a null pointer so this can crash.



trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5463>

    please remove qDebugs.



trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5464>

    what is that '20' ?



trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5465>

    use m_parent->paintDevice() instead



trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5468>

    y += line.ascent() sounds cheaper.



trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5467>

    Please instead use QPainter::save() / restore()



trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5469>

    Why the 0.5 ?



trunk/koffice/plugins/textshape/Layout.cpp
<http://reviewboard.kde.org/r/4066/#comment5470>

    Please use QString::number() which is much faster.



trunk/koffice/plugins/textshape/TextShape.cpp
<http://reviewboard.kde.org/r/4066/#comment5471>

    This looks weird, why is this here and how does this work when the number of \
digits grows.


- Thomas


On 2010-05-20 11:36:48, Boudewijn Rempt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4066/
> -----------------------------------------------------------
> 
> (Updated 2010-05-20 11:36:48)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> Paint line numbers in the text shape. This does not yet handle all particular types \
> of line numbering, but it's a good start. Future improvements depend on moving to a \
> way of counting line numbers ourselves, instead of depending on Qt. 
> 
> Diffs
> -----
> 
> trunk/koffice/plugins/textshape/Layout.h 1128791 
> trunk/koffice/plugins/textshape/Layout.cpp 1128791 
> trunk/koffice/plugins/textshape/TextShape.cpp 1128791 
> 
> Diff: http://reviewboard.kde.org/r/4066/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Boudewijn
> 
> 

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