D15204: Add typewriter annotation tool

[Tobias Deiminger]

tobiasdeiminger added inline comments.

INLINE COMMENTS

> editannottooldialog.cpp:274
> +        engineElement.setAttribute( QStringLiteral("type"), QStringLiteral("PickPoint") );
> +        engineElement.setAttribute( QStringLiteral("color"), QStringLiteral("#000000") );
> +        engineElement.setAttribute( QStringLiteral("block"), QStringLiteral("true") );

Default the text input popup to some light yellow.

> annotationwidgets.cpp:194
>  {
> +    if ( m_textAnn->inplaceIntent() == Okular::TextAnnotation::TypeWriter )
> +        return;

Better handle this in an override in TextAnnotationWidget.

> annotationwidgets.cpp:204
>      QGridLayout * gridlayout = new QGridLayout( widget );
> -
> -    QLabel * tmplabel = new QLabel( i18n( "&Color:" ), widget );
> -    gridlayout->addWidget( tmplabel, 0, 0, Qt::AlignRight );
> -    m_colorBn = new KColorButton( widget );
> -    m_colorBn->setColor( m_ann->style().color() );
> -    tmplabel->setBuddy( m_colorBn );
> -    gridlayout->addWidget( m_colorBn, 0, 1 );
> -
> -    tmplabel = new QLabel( i18n( "&Opacity:" ), widget );
> -    gridlayout->addWidget( tmplabel, 1, 0, Qt::AlignRight );
> -    m_opacity = new QSpinBox( widget );
> -    m_opacity->setRange( 0, 100 );
> -    m_opacity->setValue( (int)( m_ann->style().opacity() * 100 ) );
> -    m_opacity->setSuffix( i18nc( "Suffix for the opacity level, eg '80 %'", " %" ) );
> -    tmplabel->setBuddy( m_opacity );
> -    gridlayout->addWidget( m_opacity, 1, 1 );
> +    if ( m_textAnn->inplaceIntent() == Okular::TextAnnotation::Unknown )
> +    {

Really only if TextAnnotation::Unknown? What about TextAnnotation::Callout?

> annotationwidgets.cpp:222-223
> +
> +        connect( m_colorBn, &KColorButton::changed, this, &AnnotationWidget::dataChanged );
> +        connect( m_opacity, SIGNAL(valueChanged(int)), this, SIGNAL(dataChanged()) );
> +    }

Those generic signal slot connections must not be dependent on if it's a TextAnnotation.

> annotationwidgets.cpp:289-292
> +        connect( m_fontReq, &KFontRequester::fontSelected, this, &AnnotationWidget::dataChanged );
> +
> +        if ( m_textAnn->inplaceIntent() == Okular::TextAnnotation::TypeWriter )
> +            return widget;

Leave connect where it was before, don't return early and delegate widget setup to private methods createPopupNoteStyleWidget, createInlineNoteStyleWidget, createTypeWriterStyleWidget.

> annotationwidgets.h:95
>      Okular::Annotation * m_ann;
> +    Okular::TextAnnotation * m_textAnn;
>      QWidget * m_appearanceWidget;

It's not nice to have such a special type in the base class. Should better go to TextAnnotationWidget.

> annotationwidgets.h:102
>  
>  class TextAnnotationWidget
>    : public AnnotationWidget

All pointer type members of TextAnnotationWidget should be default initialized to nullptr (unrelated cleanup).

> pageviewannotator.cpp:153
> +            bool resok;
> +            const QString note = QInputDialog::getMultiLineText(nullptr, i18n( "New Text Note" ), prompt, QString(), &resok);
> +            if(resok)

Getting input from user by dialog and adding the annotation are two different things. Divide it into two methods (or just leave QInputDialog::getMultiLineText in PickPointEngine::end).

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15204

To: tobiasdeiminger
Cc: sander, okular-devel, ngraham, aacid
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20180921/16683b57/attachment-0001.html>