[PATCH] On Screen Display

Jakob Schroeter js at camaya.de
Thu Jan 30 12:56:33 GMT 2003


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thursday 30 January 2003 10:39, Simon Hausmann wrote:

> I think KOSD is a bad class name, because it lacks any
> descriptivity. Why not go for a verbose name, like KOnScreenDisplay
> or something?
Agreed, changed it to the verbose.
 
> +		kosd->showOSD();
> Who deletes the kosd?
> [...]
> Same thing.... who's going to delete the kosd object here?
It is now deleteLater()'ed and a pointer is returned.

> +void KOSD::setOSDFont(QFont qfont)
> [...]
> +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.
I guess I was in the mood of creating a lot of new API. But I think you're 
right. Removed.

> +    enum osdPosition {
> [...]
> +    };
removed in favour of Qt::AlignmentFlags as proposed by Aaron.

> +    static void message( QWidget *parent, const QString &text, const int
> [...]
> 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.
I introduced this monster with the preview function of the kcm. I agree that 
it's far easier to understand the customization of an own object than the 
call to this function.
I've kicked it out because I don't feel like keeping track of additional 
functionality within this, too.

> Why the OSD infix in the setters, btw? It seems redundant to me.
Well, IIRC, I introduced them with that setOSDFont() function... ;)

New patch will follow.

Jakob
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE+OSEGRIytGOFpChERAjGKAJ9AI5604F+M2IoioAncN9gZKC9mHgCeOAso
9j5Fmbby1nqnI2GNueq3L6I=
=xXhF
-----END PGP SIGNATURE-----





More information about the kde-core-devel mailing list