[PATCH] On Screen Display

[Simon Hausmann]

On Tue, Jan 28, 2003 at 06:18:14PM +0100, Jakob Schroeter wrote:
> What do you think?

Here are some comments about the kdelibs patch :)

+KOSD::~KOSD()
+{

I think KOSD is a bad class name, because it lacks any
descriptivity. Why not go for a verbose name, like KOnScreenDisplay
or something?

+void KOSD::message( QWidget *parent, const QString &text, const int timeout, 
+                    const osdPosition position, const QColor fgColor, 
+                    const QColor bgColor, const bool shadow,
+                    const int sOffset, const int hOffset,
+                    const int vOffset, const QFont font, const bool transparency,
+                    const bool scroll, const osdScrollDir scrollDir, const int scrollSpeed )
+{
+    KOSD *kosd = new KOSD(parent);
+    kosd->setOSDFont(font);
+		kosd->setOSDBgColor(bgColor);
+		kosd->setOSDFgColor(fgColor);
+		kosd->setOSDTransparency(transparency);
+		kosd->setOSDLocation(position, vOffset, hOffset);
+		kosd->setOSDText(text);
+		kosd->setOSDTimeout(timeout);
+		kosd->setOSDShadowOffset(shadow, sOffset);
+		kosd->setOSDScroll(scroll, scrollSpeed, scrollDir);
+		kosd->showOSD();

Who deletes the kosd?

If the idea is to let the parent widget (usually the caller) delete
it, then I think it would make sense to make the method return a
KOSD object to at least give the caller a chance to handle memory
management itself, apart from the parent-deletes-child mechanism.

+void KOSD::message(const QString &text)
+{
+    KOSD *kosd = new KOSD(NULL);
+    kosd->loadOSDConfig();
+    kosd->setOSDText(text);
+    kosd->showOSD();
+}

Same thing.... who's going to delete the kosd object here?

+void KOSD::setOSDFont(QFont qfont)

This should take a 'const QFont &' I believe. But why this method in
the first place? It looks like a wrapper that gives no additional
functionality?

+void KOSD::setOSDFont(const QString fontName, const int fontSize, const QFont::Weight fontWeight)

Why this method in the first place? Convenience? IMHO it's not worth
it. I think it's better to have only one method for setting the
font, the one that QWidget provides. Gives a simpler (less
cluttered) API. And it's not that creating a QFont object is
terribly complex thing, especially given that most of the time one
wants to use one of the font methods of KGlobalSettings anyway and
adjust some properties on the returned font object, instead of doing
QFont( "helvetica" ) or something in the application code, which is
very bad.

+void KOSD::loadOSDConfig()
+{
+    KConfig *c = new KConfig("kosdrc", false, false);
+    {
+      KConfigGroupSaver saver(c, "General");
+
+      setOSDTimeout(c->readUnsignedNumEntry("Timeout", 5));
+      setOSDLocation( (osdPosition) c->readUnsignedNumEntry("Position", 4),
+                      c->readUnsignedNumEntry("VerticalOffset", 0),
+                      c->readUnsignedNumEntry("HorizontalOffset", 0) );
+      setOSDShadowOffset( c->readBoolEntry("TextShadow", true),
+                          c->readUnsignedNumEntry("ShadowSize", 2) );
+      setOSDFgColor(QColor( c->readUnsignedNumEntry( "ForegroundColorRed", 255 ),
+                            c->readUnsignedNumEntry( "ForegroundColorGreen", 255 ),
+                            c->readUnsignedNumEntry( "ForegroundColorBlue", 255 ) ));
+      setOSDBgColor(QColor( c->readUnsignedNumEntry( "BackgroundColorRed", 0 ),
+                            c->readUnsignedNumEntry( "BackgroundColorGreen", 0 ),
+                            c->readUnsignedNumEntry( "BackgroundColorBlue", 0 ) ));
+      setOSDFont( c->readEntry("FontName", "Helvetica"),
+                  c->readUnsignedNumEntry("FontSize", 24),
+                  (c->readBoolEntry("FontBold", true)?(QFont::Bold):(QFont::Normal)) );
+      setOSDTransparency(c->readBoolEntry("TransparentBackground", true));
+			setOSDScroll(c->readBoolEntry("Scroll", false),
+                   c->readUnsignedNumEntry("ScrollSpeed", 50),
+                   (osdScrollDir) c->readUnsignedNumEntry("ScrollDirection", 0) );
+    }
+    delete c;

Why not just write

KConfig c( "kosdrc", ... );

setFoo( c.readBlahEntry( ... ) );

?

Neither the configgroupsaver is necessary nor is there any need to
create the KConfig object on the heap, I believe.

+public:
+    enum osdPosition {
+      POS_TOPLEFT,
+      POS_TOPCENTER,
+      POS_TOPRIGHT,
+      POS_MIDDLELEFT,
+      POS_MIDDLECENTER,
+      POS_MIDDLERIGHT,
+      POS_BOTTOMLEFT,
+      POS_BOTTOMCENTER,
+      POS_BOTTOMRIGHT
+    };
+    enum osdScrollDir {
+      DIR_TOBOTTOM,
+      DIR_TOTOP
+    };

I'm tempted to say that it's common to use studilyCapsForEnumValues
in Qt/kdelibs :)

+    KOSD(QWidget *parent);

I believe this should be

KOSD( QWidget *parent, const char *name = 0 )

for consistency.


+    /**
+     * The most simple way to use this class. Displays a text on the screen.
+     * @param parent The parent widget.
+     * @param text The text that will be displayed.
+     * @param timeout The time the text will be displayed. Default 5 seconds.
+     * @param position The position of the text on the screen.
+     * @param fgColor The foreground (aka text) color. Default is black.
+     * @param bgColor The background color. Default is white.
+     * @param shadow Decides whether the text should get a (black) shadow
+     * @param sOffset The size of the shadow in pixels. Default is 2.
+     * @param hOffset The text gets moved by hOffset pixels to the (left or right).
+     * Depends on @ref position. Default is 0.
+     * @param vOffset Same as @ref hOffset, but moves the text up or down. Depends
+     * on @ref position, too. Default is 0.
+     * @param font The font that should be used.
+		 */
+    static void message( QWidget *parent, const QString &text, const int timeout = 5, 
+                         const osdPosition position = POS_MIDDLECENTER, const QColor fgColor = QColor(255,255,255), 
+                         const QColor bgColor = QColor(0, 0, 0), const bool shadow = true,
+                         const int sOffset = 2, const int hOffset = 0,
+                         const int vOffset = 0, const QFont font = QFont("Helvetica", 24, QFont::Bold),
+                         const bool transparency = true, const bool scroll = true,
+                         const osdScrollDir scrollDir = DIR_TOBOTTOM, const int scrollSpeed = 0 );

I wonder if it's a good idea to provide such a function. One the
first sight it seems like convenience, but I think it does not
really help to keep code readable because of the huuuuge amount of
parameters it takes.

Compare

KOSD *osd = new KOSD( this );
osd->setText( "blah" );
osd->setTimeout( 42 );
osd->setForegroundColor( blue );
osd->setShadow( true );

with

KOSD::message( this, "blah", 42, POS_MIDDLECENTER, blue, black, true );

It's shorter, but it's far less readable. When reading that code two
months later, with the first piece of code it's immediately clear
what each argument corresponds with, with the second piece I'd have
no idea if the blue or the black is the foreground and the other one
the background.

+    void setOSDLocation(const osdPosition position, const int verticalOff = 0, const int horizontalOff = 0);

Why the OSD infix in the setters, btw? It seems redundant to me.

+    void setOSDFgColor(QColor fgColor);

For consistency I think this should be const QColor & .

+bool KNotify::notifyByOSD(const QString &text)
+{
+    KOSD::message(text);
+    return true;
+}

Memory leak? Who deletes the KOSD object?

Simon