Differential D21332 Diff 58625 ui/annotationwidgets.cpp

Changeset View

Standalone View

View Options

ui/annotationwidgets.cpp

Context not available.
494 gridlay2->addWidget( m_spinSize, 0, 1 ); 494​ gridlay2->addWidget( m_spinSize, 0, 1 );
495 tmplabel2->setBuddy( m_spinSize ); 495​ tmplabel2->setBuddy( m_spinSize );
496 496​
497 if ( m_lineType == 1 ) 497​ if ( m_lineType == 1 ) //Polygon
498 { 498​ {
499 m_useColor = new QCheckBox( i18n( "Inner color:" ), gb2 ); 499​ m_useColor = new QCheckBox( i18n( "Inner color:" ), gb2 );
tobiasdeimingerUnsubmitted
Done

Checking a polygons Inner color checkbox now causes segfault. It's because LineAnnotationWidget::applyChanges accesses m_startStyleCombo and m_endStyleCombo unconditionally. But in case of m_lineType == 1 that QComboBoxes have never been constructed. Access should be guarded by if ( m_lineType == 0 ), and probably an additional nullptr check.

tobiasdeiminger: Checking a polygons Inner color checkbox now causes segfault. It's because…
knambiarAuthorUnsubmitted
Done

Is the nullptr check really necessary?

knambiar: Is the `nullptr` check really necessary?
tobiasdeimingerUnsubmitted
Done

No, I'd accept anyways. I just think the implication "object pointer != nullptr" => "object valid" is more obvious to new contributors than "some member == some magic number" => "object valid".

How about adding a Q_ASSERTas we did it in TextAnnotationWidget::applyChanges? If you go for it, please also initialize members (as done for TextAnnotationWidgetin annotationwidgets.h).

tobiasdeiminger: No, I'd accept anyways. I just think the implication "object pointer != nullptr" => "object…
500 gridlay2->addWidget( m_useColor, 1, 0 ); 500​ gridlay2->addWidget( m_useColor, 1, 0 );
501 m_innerColor = new KColorButton( gb2 ); 501​ m_innerColor = new KColorButton( gb2 );
502 gridlay2->addWidget( m_innerColor, 1, 1 ); 502​ gridlay2->addWidget( m_innerColor, 1, 1 );
503 } ​
504 ​
505 if ( m_lineType == 0 ) ​
506 { ​
507 m_spinLL->setRange( -500, 500 ); ​
508 m_spinLL->setValue( m_lineAnn->lineLeadingForwardPoint() ); ​
509 m_spinLLE->setRange( 0, 500 ); ​
510 m_spinLLE->setValue( m_lineAnn->lineLeadingBackwardPoint() ); ​
511 } ​
512 else if ( m_lineType == 1 ) ​
513 { ​
514 m_innerColor->setColor( m_lineAnn->lineInnerColor() ); 503​ m_innerColor->setColor( m_lineAnn->lineInnerColor() );
515 if ( m_lineAnn->lineInnerColor().isValid() ) 504​ if ( m_lineAnn->lineInnerColor().isValid() )
516 { 505​ {
Context not available.
521 m_innerColor->setEnabled( false ); 510​ m_innerColor->setEnabled( false );
522 } 511​ }
523 } 512​ }
513​
514​ if ( m_lineType == 0 ) //Straight line
515​ {
516​ m_spinLL->setRange( -500, 500 );
517​ m_spinLL->setValue( m_lineAnn->lineLeadingForwardPoint() );
518​ m_spinLLE->setRange( 0, 500 );
519​ m_spinLLE->setValue( m_lineAnn->lineLeadingBackwardPoint() );
520​
521​ //Line Term Styles
522​ QLabel * tmplabel3 = new QLabel( i18n( "Line Start:" ), widget );
523​ QLabel * tmplabel4 = new QLabel( i18n( "Line End:" ), widget );
524​ gridlay2->addWidget( tmplabel3, 1, 0, Qt::AlignRight );
525​ gridlay2->addWidget( tmplabel4, 2, 0, Qt::AlignRight );
526​ m_startStyleCombo = new QComboBox( widget );
527​ m_endStyleCombo = new QComboBox( widget );
528​ tmplabel3->setBuddy( m_startStyleCombo );
tobiasdeimingerUnsubmitted
Done

Are the leading whitespaces in " Square", " Circle", and so on, intentional?

tobiasdeiminger: Are the leading whitespaces in " Square", " Circle", and so on, intentional?
knambiarAuthorUnsubmitted
Not Done

No, it was an oversight from previous local patch. Fixed now.

knambiar: No, it was an oversight from previous local patch. Fixed now.
529​ tmplabel4->setBuddy( m_endStyleCombo );
530​ gridlay2->addWidget( m_startStyleCombo, 1, 1, Qt::AlignLeft );
531​ gridlay2->addWidget( m_endStyleCombo, 2, 1, Qt::AlignLeft );
532​ tmplabel3->setToolTip( i18n("Only for PDF documents"));
533​ tmplabel4->setToolTip( i18n("Only for PDF documents"));
534​ m_startStyleCombo->setToolTip( i18n("Only for PDF documents"));
535​ m_endStyleCombo->setToolTip( i18n("Only for PDF documents"));
536​
537​ for ( const QString &i: { i18n( "Square" ), i18n( "Circle" ), i18n( "Diamond" ), i18n( "Open Arrow" ), i18n( "Closed Arrow" ),
538​ i18n( "None" ), i18n( "Butt" ), i18n( "Right Open Arrow" ), i18n( "Right Closed Arrow" ), i18n( "Slash" ) })
539​ {
540​ m_startStyleCombo->addItem(i);
541​ m_endStyleCombo->addItem(i);
542​ }
tobiasdeimingerUnsubmitted
Done

Is this else if ( m_lineType == 1 ) branch really necessary? Looks like the code within could just be moved to the previous if ( m_lineType == 1 ) at L. 497.

tobiasdeiminger: Is this `else if ( m_lineType == 1 )` branch really necessary? Looks like the code within could…
543​
544​ m_startStyleCombo->setCurrentIndex( m_lineAnn->lineStartStyle() );
545​ m_endStyleCombo->setCurrentIndex( m_lineAnn->lineEndStyle() );
546​ connect( m_startStyleCombo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &LineAnnotationWidget::dataChanged );
547​ connect( m_endStyleCombo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &LineAnnotationWidget::dataChanged );
548​
549​ }
550​
524 m_spinSize->setRange( 1, 100 ); 551​ m_spinSize->setRange( 1, 100 );
525 m_spinSize->setValue( m_lineAnn->style().width() ); 552​ m_spinSize->setValue( m_lineAnn->style().width() );
526 553​
Context not available.
537 } 564​ }
538 connect( m_spinSize, QOverload<double>::of(&QDoubleSpinBox::valueChanged), this, &LineAnnotationWidget::dataChanged ); 565​ connect( m_spinSize, QOverload<double>::of(&QDoubleSpinBox::valueChanged), this, &LineAnnotationWidget::dataChanged );
539 566​
540 //Line Term Styles ​
541 QLabel * tmplabel3 = new QLabel( i18n( "Line Start:" ), widget ); ​
542 QLabel * tmplabel4 = new QLabel( i18n( "Line End:" ), widget ); ​
543 gridlay2->addWidget( tmplabel3, 1, 0, Qt::AlignRight ); ​
544 gridlay2->addWidget( tmplabel4, 2, 0, Qt::AlignRight ); ​
545 m_startStyleCombo = new QComboBox( widget ); ​
546 m_endStyleCombo = new QComboBox( widget ); ​
547 tmplabel3->setBuddy( m_startStyleCombo ); ​
548 tmplabel4->setBuddy( m_endStyleCombo ); ​
549 gridlay2->addWidget( m_startStyleCombo, 1, 1, Qt::AlignLeft ); ​
550 gridlay2->addWidget( m_endStyleCombo, 2, 1, Qt::AlignLeft ); ​
551 tmplabel3->setToolTip( i18n("Only for PDF documents") ); ​
552 tmplabel4->setToolTip( i18n("Only for PDF documents") ); ​
553 m_startStyleCombo->setToolTip( i18n("Only for PDF documents")); ​
554 m_endStyleCombo->setToolTip( i18n("Only for PDF documents")); ​
555 ​
556 for ( const QString &i: { i18n( " Square" ), i18n( " Circle" ), i18n( " Diamond" ), i18n( " Open Arrow" ), i18n( " Closed Arrow" ), ​
557 i18n( " None" ), i18n( " Butt" ), i18n( " Right Open Arrow" ), i18n( " Right Closed Arrow" ), i18n( "Slash" ) } ) ​
558 { ​
559 m_startStyleCombo->addItem(i); ​
560 m_endStyleCombo->addItem(i); ​
561 } ​
562 ​
563 m_startStyleCombo->setCurrentIndex( m_lineAnn->lineStartStyle() ); ​
564 m_endStyleCombo->setCurrentIndex( m_lineAnn->lineEndStyle() ); ​
565 connect( m_startStyleCombo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &LineAnnotationWidget::dataChanged ); ​
566 connect( m_endStyleCombo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &LineAnnotationWidget::dataChanged ); ​
567 ​
568 return widget; 567​ return widget;
569} 568​}
570 569​
Context not available.
573 AnnotationWidget::applyChanges(); 572​ AnnotationWidget::applyChanges();
574 if ( m_lineType == 0 ) 573​ if ( m_lineType == 0 )
575 { 574​ {
575​ Q_ASSERT(m_spinLL && m_spinLLE && m_startStyleCombo && m_endStyleCombo);
576 m_lineAnn->setLineLeadingForwardPoint( m_spinLL->value() ); 576​ m_lineAnn->setLineLeadingForwardPoint( m_spinLL->value() );
577 m_lineAnn->setLineLeadingBackwardPoint( m_spinLLE->value() ); 577​ m_lineAnn->setLineLeadingBackwardPoint( m_spinLLE->value() );
578​ m_lineAnn->setLineStartStyle( (Okular::LineAnnotation::TermStyle)m_startStyleCombo->currentIndex() );
579​ m_lineAnn->setLineEndStyle( (Okular::LineAnnotation::TermStyle)m_endStyleCombo->currentIndex() );
578 } 580​ }
579 else if ( m_lineType == 1 ) 581​ else if ( m_lineType == 1 )
580 { 582​ {
583​ Q_ASSERT(m_useColor && m_innerColor);
581 if ( !m_useColor->isChecked() ) 584​ if ( !m_useColor->isChecked() )
582 { 585​ {
583 m_lineAnn->setLineInnerColor( QColor() ); 586​ m_lineAnn->setLineInnerColor( QColor() );
Context not available.
587 m_lineAnn->setLineInnerColor( m_innerColor->color() ); 590​ m_lineAnn->setLineInnerColor( m_innerColor->color() );
588 } 591​ }
589 } 592​ }
593​ Q_ASSERT(m_spinSize);
590 m_lineAnn->style().setWidth( m_spinSize->value() ); 594​ m_lineAnn->style().setWidth( m_spinSize->value() );
591 m_lineAnn->setLineStartStyle( (Okular::LineAnnotation::TermStyle)m_startStyleCombo->currentIndex() ); ​
592 m_lineAnn->setLineEndStyle( (Okular::LineAnnotation::TermStyle)m_endStyleCombo->currentIndex() ); ​
593} 595​}
594 596​
595 597​
Context not available.