Goodbye KJanusWidget

[Tobias Koenig]

On Sun, Jun 04, 2006 at 11:29:30AM +0200, Thiago Macieira wrote:
> Tobias Koenig wrote:
Hi Thiago,

> Just a few comments before you do:
> 1) kdialog.h includes QDialog. Please change that to QtGui/QDialog or 
> QtGui/qdialog.h. The same applies to the other files:
> 	kpagemodel.h includes QAbstractItemModel. Please add the QtCore/ part.
> 	kpageview.h includes QAbstractItemView and QWidget. Those require the 
> QtGui/ part.
> 	kpagewidgetmodel.h includes QIcon: QtGui/ missing.
Ok, will fix it.

> 2) NoDefault (KDialog::ButtonCode) has a value of 0x80000001. Is that the 
> intended value? You'll be conflicting with the Help value (0x1).
No idea, I just removed some methods from KDialog and stripped down the
ctor, all the other code is left untouched...

What do you think, which value should be used instead?

> 3) ActionButtonStyle makes no sense. Should it be removed or moved to a 
> private part of the class? Or is that one of the items for which docs are 
> missing?
Hmm, it's used for setButtonStyle(int) which is a private method...

The enum should be accessable, since KGlobalSettings::buttonStyle() uses
them, but we could move them to a kdialog_p.h.


> 4) KDialog::button says "Normally you should not use this function". Why 
> is it there, then?
Well, here a list of use cases I found in kdelibs:

  button( Ok )->setFocus();
  button( User1 )->setIcon( ... );
  button( User1 )->setEnabled( false );
  button( ... )->animateClick();

The first three cases could be replaced by accroding setter methods

  setButtonFocus( ButtonCode );
  setButtonIcon( ButtonCode, QIcon );
  setButtonEnabled( ButtonCode, bool );

but the last one seems to be to special IMHO.

Hmm, this code is in a test application anyway, so maybe we should just
drop it here and go with the first solution.


> 5) incInitialSize <--- bad name. Maybe you should be verbose here.
Right, should be incrementInitialSize...

> 6) KDialog::setMainWidget says "reimplemented from QDialog", but QDialog 
> has no member called setMainWidget. Either the docs are misleading, or 
> you may be expecting a behaviour that does not exist anymore.
Right, the doc is misleading, will fix it.

> 7) KPageView has private functions rebuildGui, updateSelection and 
> cleanupPages. Hint: move them to the Private class.
Ok

> 8) Technically, you're not supposed to use Q_PRIVATE_SLOT, but it's too 
> convenient :-)
Well, how to hide the private slots otherwise? Lettings the private
object inherit from QPrivateObject?

Thanks for all the hints, will implement them and commit later these
week.

Ciao,
Tobias
-- 
Separate politics from religion and economy!
The Council of the European Union is an undemocratic and illegal institution!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20060604/9394cfbd/attachment.sig>