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

List:       kde-commits
Subject:    KDE/kdegraphics/kolourpaint
From:       Clarence Dang <dang () kde ! org>
Date:       2007-10-26 22:46:41
Message-ID: 1193438801.296592.6738.nullmailer () svn ! kde ! org
[Download RAW message or body]

SVN commit 729779 by dang:

Code review (just comment changes)


 M  +3 -0      commands/kpCommandHistory.h  
 M  +2 -1      commands/kpCommandHistoryBase.h  
 M  +4 -4      imagelib/kpColor.h  
 M  +2 -4      imagelib/kpPainter.h  
 M  +1 -1      kpViewScrollableContainer.h  
 M  +3 -0      mainWindow/kpMainWindow_Edit.cpp  
 M  +3 -0      mainWindow/kpMainWindow_File.cpp  
 M  +0 -1      mainWindow/kpMainWindow_Text.cpp  
 M  +7 -2      pixmapfx/kpPixmapFX.h  
 M  +3 -0      tools/kpTool.h  
 M  +1 -1      tools/kpTool_Drawing.cpp  
 M  +3 -1      views/manager/kpViewManager.h  


--- trunk/KDE/kdegraphics/kolourpaint/commands/kpCommandHistory.h #729778:729779
@@ -87,6 +87,9 @@
     //
     // If <cmd> creates a selection that is not just a border, this
     // method has the same effect as addCommand().
+    //
+    // REFACTOR: Why not just override addCommand() and test if it was given a
+    //           kpToolSelectionCreateCommand?
     void addCreateSelectionCommand (kpToolSelectionCreateCommand *cmd,
         bool execute = true);
 
--- trunk/KDE/kdegraphics/kolourpaint/commands/kpCommandHistoryBase.h #729778:729779
@@ -50,7 +50,8 @@
 class kpViewManager;
 
 
-// Clone of KCommandHistory with features required by KolourPaint:
+// Clone of KCommandHistory with features required by KolourPaint but which
+// could also be useful for other apps:
 // - nextUndoCommand()/nextRedoCommand()
 // - undo/redo history limited by both number and size
 //
--- trunk/KDE/kdegraphics/kolourpaint/imagelib/kpColor.h #729778:729779
@@ -37,10 +37,10 @@
 
 
 //
-// kpColor is an object-oriented abstraction of QRgb, with the additional
-// restriction of enforcing the KolourPaint convention of only supporting
-// totally transparent and totally opaque colors.  Eventually, this
-// restriction will be dropped.  In the future, other color models such as
+// kpColor is an object-oriented abstraction of QRgb, for document image data,
+// with the additional restriction of enforcing the KolourPaint convention of
+// only supporting totally transparent and totally opaque colors.  Eventually,
+// this restriction will be dropped.  In the future, other color models such as
 // 8-bit indexed will be supported.  It also provides better error handling,
 // reporting (noisy kError()'s) and recovery compared to Qt.  This abstraction
 // will allow us to eventually dump the Qt paint routines.
--- trunk/KDE/kdegraphics/kolourpaint/imagelib/kpPainter.h #729778:729779
@@ -43,13 +43,11 @@
 // you should probably move it into kpPainter to avoid the overhead of
 // passing around this state (e.g. color, line width) and for reuse.
 //
-// Therefore, it should not be used as an on-screen replacement for
-// QPainter since kpImage is not supposed to have any relationship with
-// QPaintDevice in the future.
+// kpPainter is to kpImage as QPainter is to QPixmap.
 //
 // This encapsulates the set of functionality used by all of KolourPaint's
 // document drawing functions and nothing more, permitting rewriting of
-// the graphics backend.  Currently uses QPainter/kpPixmapFX as the backend.
+// the image library.  Currently uses QPainter/kpPixmapFX as the image library.
 //
 
 struct kpPainterPrivate;
--- trunk/KDE/kdegraphics/kolourpaint/kpViewScrollableContainer.h #729778:729779
@@ -51,7 +51,7 @@
 class kpMainWindow;
 
 
-// TODO: refactor by sharing iface's with kpTool
+// REFACTOR: refactor by sharing iface's with kpTool
 class kpGrip : public QLabel
 {
 Q_OBJECT
--- trunk/KDE/kdegraphics/kolourpaint/mainWindow/kpMainWindow_Edit.cpp #729778:729779
@@ -761,6 +761,9 @@
             i18n ("Cannot Paste"));
 
         // TODO: PROPAGATE: interprocess
+        // TODO: Is this loop safe since a KMainWindow later along in the list,
+        //       could be closed as the code in the body almost certainly re-enters
+        //       the event loop?  Problem for KDE 3 as well, I think.
         foreach (KMainWindow *kmw, KMainWindow::memberList ())
         {
             Q_ASSERT (dynamic_cast <kpMainWindow *> (kmw));
--- trunk/KDE/kdegraphics/kolourpaint/mainWindow/kpMainWindow_File.cpp #729778:729779
@@ -215,6 +215,9 @@
 
 
     // TODO: PROPAGATE: interprocess
+    // TODO: Is this loop safe since a KMainWindow later along in the list,
+    //       could be closed as the code in the body almost certainly re-enters
+    //       the event loop?  Problem for KDE 3 as well, I think.
     foreach (KMainWindow *kmw, KMainWindow::memberList ())
     {
         Q_ASSERT (dynamic_cast <kpMainWindow *> (kmw));
--- trunk/KDE/kdegraphics/kolourpaint/mainWindow/kpMainWindow_Text.cpp #729778:729779
@@ -56,7 +56,6 @@
 {
     KActionCollection *ac = actionCollection ();
 
-    // COMPAT: Changing font does not seem to work.
     d->actionTextFontFamily = ac->add<KFontAction> ("text_font_family");
     d->actionTextFontFamily->setText (i18n ("Font Family"));
     connect (d->actionTextFontFamily, SIGNAL (triggered (const QString &)),
--- trunk/KDE/kdegraphics/kolourpaint/pixmapfx/kpPixmapFX.h #729778:729779
@@ -490,8 +490,13 @@
     // Note that this method is slow so you should avoid calling it, if
     // possible.
     //
-    // Generally, the only time you need to call it is when you get a pixmap
-    // from a foreign source e.g. file or clipboard.
+    // Generally, the only time you need to call it is when you get a QImage
+    // from a foreign source (e.g. from a file or clipboard, not originally
+    // from a QPixmap) and need to convert it to a QPixmap.  You don't need
+    // to call it anywhere else since internally, all KolourPaint code is
+    // supposed to maintain the QPixmap-has-no-alpha-channel invariant.
+    // However, convertToPixmap() and convertToPixmapAsLosslessAsPossible()
+    // call this method for you anyway.
     //
     // It is invalid to use a QPainter directly on a QPixmap, which
     // usually introduces an alpha channel, and then to call this method to
--- trunk/KDE/kdegraphics/kolourpaint/tools/kpTool.h #729778:729779
@@ -267,6 +267,9 @@
 
     virtual void endDraw (const QPoint &thisPoint, const QRect &normalizedRect);
 
+    // TODO: I think reimplementations of this should be calling this base
+    //       implementation, so that endDraw() happens before the custom
+    //       endShape() logic of the reimplementation.
     virtual void endShape (const QPoint &thisPoint = QPoint (),
                            const QRect &normalizedRect = QRect ())
     {
--- trunk/KDE/kdegraphics/kolourpaint/tools/kpTool_Drawing.cpp #729778:729779
@@ -381,7 +381,7 @@
     //
     //       Note that the invariant has usually already been inadvertently
     //       restored by kpPixmapFX::draw() (which does mask tricks), which
-    //       is called via most tools calling kpDocument::setPixmapAt() in
+    //       is called via most tools calling kpDocument::setPixmapAt(), in
     //       draw() and/or endDraw().  So the below code is really just in
     //       case this has not happened.
     kpImage *docImage = document ()->imagePointer ();
--- trunk/KDE/kdegraphics/kolourpaint/views/manager/kpViewManager.h #729778:729779
@@ -216,7 +216,9 @@
     // Slow: Let Qt buffer paint events via QWidget::update().
     //       Results in less flicker.  Paint events are probably merged
     //       so long-term efficiency is increased at the expense of
-    //       reduced responsiveness (default).
+    //       reduced responsiveness (default).  Generally, the paint
+    //       event happens a while later -- when you return to the event
+    //       loop.
     // Fast: Force Qt to redraw immediately.  No paint events
     //       are merged so there is great potential for flicker,
     //       if used inappropriately.  Use this when the redraw
[prev in list] [next in list] [prev in thread] [next in thread]