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

List:       koffice-devel
Subject:    Re: end lines
From:       Thomas Zander <zander () kde ! org>
Date:       2009-10-31 10:14:48
Message-ID: 200910311114.49508.zander () kde ! org
[Download RAW message or body]

Thanks for these patches, as your mail was empty I'm going with the 
assumption that you want a patch review :)
I read through your patch and noticed some issues, I'll write them below. 
Some are issues that apply in general so I didn't specify all places where 
its an issue, it would be good if you read through the patch to see what is 
going on.

After you fixed these, please send us another patch for a second round :)
I don't have time today to actually apply the patch and test it, maybe 
someone else can ? Otherwise it will be when your updated patch comes in.

ok, here some things;

I noticed your added your name to the copyright lines for at least one file 
without an email address, please add your email too.

The patch shows one "\ No newline at end of file" Please add that line end.

In plugins/dockers/shapeendlines/KoEndLinesDockerFactory.h
How is it possible that a new file has only one copyright holder, Jan 
Hambrechts, that is not you ?
The file has a comment that does not belong in this file, please add your own 
copyright and change or remove the class-comment.

I see various kDebug() statements that do not have a section, they should 
get an argument that makes it possible to turn off the debugs in the 
kdebugdialog.

The new file plugins/dockers/shapeendlines/KoShapeEndLinesDocker.cpp 
looks like its copied and modified.
Please make sure only required includes are there, I have the impression the 
list is too long for this short file.
This seems to be the case for most new files, please check this for all new 
files.

+void KoShapeEndLinesDocker::applyChanges()
+{
+    KoCanvasController* canvasController = 
 KoToolManager::instance()->activeCanvasController();

Instead of using the singleton, please make the docker also inherit from 
KoCanvasObserver

in libs/flake/KoPathShape.h you added #define KoPathShapeEndLineSize 15
since this header file is installed and used by everyone I'd like to know why 
you added this define there.

in KoPathShape I see;
+    KoPathPoint* point = m_subpaths.first()->first();
you probably want to check that those 'first()' don't assert because there is 
nothing there.

Also, new code in koffice/libs should follow the coding style;  
http://techbase.kde.org/Policies/Kdelibs_Coding_Style The above pasted line 
should be adjusted. Please check your patch to conform to the style.

Thanks!
-- 
Thomas Zander
_______________________________________________
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]