[Psi-devel] Re: vCard dialog

[Francisco Joaquín Rodríguez Prados]

By the way, I have uploaded the new patch with those things fixed :)
http://www.subtrama.net/psi-dev/vcard-photo-interface.diff (the patch
has the layout a: http://www.subtrama.net/psi-dev/psi050413a.png)

On 4/13/05, Remko Troncon <remko.troncon at cs.kuleuven.ac.be> wrote:
> > http://www.subtrama.net/psi-dev/vcard-photo-interface.diff
> 
> Some small remarks:
> - label_photo->setText("Picture not\navailable");
>         should become
>   label_photo->setText(tr("Picture not\navailable"));
>         to make the string translatable. (twice)
> - setPreviewPhoto(QString) -> setPhoto(const QString&), to be consistent
>   with clearPhoto, and to avoid making a needless copy ?
> - Above every method definition, there should be a doxygen style comment
>   in the style of:
>         /*!
>          * Sets the picture of the vcard.
>          * \param path the file to use as a picture
>          */
>         void InfoDlg::setPreviewPhoto(QString path)
>   I think you put your doxygen code _inside_ the method, and you missed some
>   params too ;)
> - The following blurb:
>                 label_photo->setText("Picture not\navailable");
>    should probably be separated into a different (protected) method,
>    called clearDisplayedPhoto() or doClearPhoto() or so, which is called from
>    clearPhoto() and from the initializer. While this may seem silly for a one
>    liner, it sounds bad to me to have duplicated identical strings (Picture
>    not available).
> 
> i think that's more or less it for now for me :)
> 
> cheers,
> Remko
> 


-- 
Francisco Joaquín Rodríguez Prados
Fachbereich Informatik,
Fachhochschule Darmstadt,
Darmstadt, Germany