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