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

List:       koffice-devel
Subject:    Re: Font dialog
From:       David Faure <faure () kde ! org>
Date:       2005-06-17 13:28:48
Message-ID: 200506171528.48967.faure () kde ! org
[Download RAW message or body]

On Monday 06 June 2005 23:06, pierre.stirnweiss_koffice@gadz.org wrote:
> Ok, here it is. What is included:
> 
> - ui files for the tabs of the Font dialog
> - corresponding implementation files (subclassing the ui generated classes)
> - a local copy (modified) of kfontdialog (.h .cc). Needed to add a possibility 
> to hide the font preview in the KFontChooser.

Can you also make a patch for kdelibs' KFontChooser to add that possibility there?
This way we can get rid of our kfontchooser copy later (e.g. after kde4, or whenever
we require kde-3.5).

> - a new class: KoFontDialogPreview. This provides the preview frame for all 
> visual properties of the dialog: font family, style, size. font and 
> background color, shadow, underlining and strikethrough, capitalisation, 
> sub/superscript. The draw function might need some optimisation, hints 
> welcome.
> 
> The following files are touched by the patch:
> - koFontDia (.h, .cc, _p.h). This one is actually completelly changed. The 
> KoFontChooser is gone and the tabs are in separate files.

I see that the new classes are named like "koFontTab" - can you fix that to start \
with an uppercase letter, like all other classes do?

Also, while you're there, please make any new file completely lowercased.
KOffice is very inconsistent in that respect, but I do want to clean this up after
1.4.1 (since it will make merging bugfixes more difficult).

You should change the comment at the top of kfontdialog_local.* to explain why we \
have a copy in koffice.

You can remove koFontDia_p.h, no point in keeping an empty file.
Remove also KoStyleFontTab::resizeEvent().

void koDecorationTab::slotTextColorChanged( const QColor& color )
{
        emit fontColorChanged( color );
}
No need for a slot that sends a signal: you can connect a signal to a signal, to \
forward it directly. connect( textKColorButton, SIGNAL( changed( const QColor& ) ), \
this, SIGNAL( fontColorChanged( const QColor& ) ) );

Last comment: too bad that you use tabs for indenting when most files didn't do so...
It would be fine in new files if they were consistent, but e.g. \
koDecorationTab::shadowDistanceX has two spaces when the rest of the file has tabs \
now... Generally, most of KOffice libs has 4 spaces.

KoFontDiaPreview : all member variables should be private, not protected,
so that we can change them even when binary compatibility must be kept.

/// OK including config.h in public headers is bad practice - to be removed once \
kspell2 is required #include <config.h>
#include <kspell2/broker.h>

That comment is from me, and only makes sense with #ifdef, but all of that can be
cleaned up in trunk. Feel free to remove the first two lines, and to use kspell2 \
unconditionally.

int koLayoutTab::getOffsetFromBaseline()
{
        switch ( positionButtonGroup->selectedId() )
        {
        case 0:
                return 0;
                break;
No need for break; after a return. Return already stops there and returns \
immediately. (You can remove many "breaks" in that file).

> I think that's about it. The stuff compiles and there doesn't seem to be a bug 
> in KWord's behaviour.

Great ;)

> I am enabling the relative text spinbox even when subscript or superscript is 
> enabled to be consistant with OASIS. Offset from baseline is now only enabled 
> when Custom alignment is selected.
OK.

Thanks!

-- 
David Faure, faure@kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

_______________________________________________
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]