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 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 (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 (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 ("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