Differential D21332 Diff 58673 ui/annotationwidgets.cpp

Changeset View

Standalone View

View Options

ui/annotationwidgets.cpp

Show First 20 Lines • Show All 488 Lines • ▼ Show 20 Line(s)465{
489 gb2->setTitle( i18n( "Style" ) ); 489​ gb2->setTitle( i18n( "Style" ) );
490 QGridLayout * gridlay2 = new QGridLayout( gb2 ); 490​ QGridLayout * gridlay2 = new QGridLayout( gb2 );
491 QLabel * tmplabel2 = new QLabel( i18n( "&Size:" ), gb2 ); 491​ QLabel * tmplabel2 = new QLabel( i18n( "&Size:" ), gb2 );
492 gridlay2->addWidget( tmplabel2, 0, 0, Qt::AlignRight ); 492​ gridlay2->addWidget( tmplabel2, 0, 0, Qt::AlignRight );
493 m_spinSize = new QDoubleSpinBox( gb2 ); 493​ m_spinSize = new QDoubleSpinBox( gb2 );
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​ {
517 m_useColor->setChecked( true ); 506​ m_useColor->setChecked( true );
518 } 507​ }
519 else 508​ else
520 { 509​ {
521 m_innerColor->setEnabled( false ); 510​ m_innerColor->setEnabled( false );
522 } 511​ }
523 } 512​ }
524 m_spinSize->setRange( 1, 100 ); ​
525 m_spinSize->setValue( m_lineAnn->style().width() ); ​
526 513​
527 if ( m_lineType == 0 ) 514​ if ( m_lineType == 0 ) //Straight line
528 { ​
529 connect( m_spinLL, SIGNAL(valueChanged(double)), this, SIGNAL(dataChanged()) ); ​
530 connect( m_spinLLE, SIGNAL(valueChanged(double)), this, SIGNAL(dataChanged()) ); ​
531 } ​
532 else if ( m_lineType == 1 ) ​
533 { 515​ {
534 connect( m_innerColor, &KColorButton::changed, this, &AnnotationWidget::dataChanged ); 516​ m_spinLL->setRange( -500, 500 );
535 connect( m_useColor, &QAbstractButton::toggled, this, &AnnotationWidget::dataChanged ); 517​ m_spinLL->setValue( m_lineAnn->lineLeadingForwardPoint() );
536 connect(m_useColor, &QCheckBox::toggled, m_innerColor, &KColorButton::setEnabled); 518​ m_spinLLE->setRange( 0, 500 );
537 } 519​ m_spinLLE->setValue( m_lineAnn->lineLeadingBackwardPoint() );
538 connect( m_spinSize, QOverload<double>::of(&QDoubleSpinBox::valueChanged), this, &LineAnnotationWidget::dataChanged ); ​
539 520​
540 //Line Term Styles 521​ //Line Term Styles
541 QLabel * tmplabel3 = new QLabel( i18n( "Line Start:" ), widget ); 522​ QLabel * tmplabel3 = new QLabel( i18n( "Line Start:" ), widget );
542 QLabel * tmplabel4 = new QLabel( i18n( "Line End:" ), widget ); 523​ QLabel * tmplabel4 = new QLabel( i18n( "Line End:" ), widget );
543 gridlay2->addWidget( tmplabel3, 1, 0, Qt::AlignRight ); 524​ gridlay2->addWidget( tmplabel3, 1, 0, Qt::AlignRight );
544 gridlay2->addWidget( tmplabel4, 2, 0, Qt::AlignRight ); 525​ gridlay2->addWidget( tmplabel4, 2, 0, Qt::AlignRight );
545 m_startStyleCombo = new QComboBox( widget ); 526​ m_startStyleCombo = new QComboBox( widget );
546 m_endStyleCombo = new QComboBox( widget ); 527​ m_endStyleCombo = new QComboBox( widget );
547 tmplabel3->setBuddy( m_startStyleCombo ); 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.
548 tmplabel4->setBuddy( m_endStyleCombo ); 529​ tmplabel4->setBuddy( m_endStyleCombo );
549 gridlay2->addWidget( m_startStyleCombo, 1, 1, Qt::AlignLeft ); 530​ gridlay2->addWidget( m_startStyleCombo, 1, 1, Qt::AlignLeft );
550 gridlay2->addWidget( m_endStyleCombo, 2, 1, Qt::AlignLeft ); 531​ gridlay2->addWidget( m_endStyleCombo, 2, 1, Qt::AlignLeft );
551 tmplabel3->setToolTip( i18n("Only for PDF documents") ); 532​ tmplabel3->setToolTip( i18n("Only for PDF documents") );
552 tmplabel4->setToolTip( i18n("Only for PDF documents") ); 533​ tmplabel4->setToolTip( i18n("Only for PDF documents") );
553 m_startStyleCombo->setToolTip( i18n("Only for PDF documents")); 534​ m_startStyleCombo->setToolTip( i18n("Only for PDF documents") );
554 m_endStyleCombo->setToolTip( i18n("Only for PDF documents")); 535​ m_endStyleCombo->setToolTip( i18n("Only for PDF documents") );
555 536​
556 for ( const QString &i: { i18n( " Square" ), i18n( " Circle" ), i18n( " Diamond" ), i18n( " Open Arrow" ), i18n( " Closed Arrow" ), 537​ 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" ) } ) 538​ i18n( "None" ), i18n( "Butt" ), i18n( "Right Open Arrow" ), i18n( "Right Closed Arrow" ), i18n( "Slash" ) } )
558 { 539​ {
559 m_startStyleCombo->addItem(i); 540​ m_startStyleCombo->addItem( i );
560 m_endStyleCombo->addItem(i); 541​ m_endStyleCombo->addItem( i );
561 } 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…
562 543​
563 m_startStyleCombo->setCurrentIndex( m_lineAnn->lineStartStyle() ); 544​ m_startStyleCombo->setCurrentIndex( m_lineAnn->lineStartStyle() );
564 m_endStyleCombo->setCurrentIndex( m_lineAnn->lineEndStyle() ); 545​ m_endStyleCombo->setCurrentIndex( m_lineAnn->lineEndStyle() );
565 connect( m_startStyleCombo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &LineAnnotationWidget::dataChanged ); 546​ connect( m_startStyleCombo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &LineAnnotationWidget::dataChanged );
566 connect( m_endStyleCombo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &LineAnnotationWidget::dataChanged ); 547​ connect( m_endStyleCombo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &LineAnnotationWidget::dataChanged );
567 548​
549​ }
550​
551​ m_spinSize->setRange( 1, 100 );
552​ m_spinSize->setValue( m_lineAnn->style().width() );
553​
554​ if ( m_lineType == 0 )
555​ {
556​ connect( m_spinLL, SIGNAL(valueChanged(double)), this, SIGNAL(dataChanged()) );
557​ connect( m_spinLLE, SIGNAL(valueChanged(double)), this, SIGNAL(dataChanged()) );
558​ }
559​ else if ( m_lineType == 1 )
560​ {
561​ connect( m_innerColor, &KColorButton::changed, this, &AnnotationWidget::dataChanged );
562​ connect( m_useColor, &QAbstractButton::toggled, this, &AnnotationWidget::dataChanged );
563​ connect( m_useColor, &QCheckBox::toggled, m_innerColor, &KColorButton::setEnabled );
564​ }
565​ connect( m_spinSize, QOverload<double>::of(&QDoubleSpinBox::valueChanged), this, &LineAnnotationWidget::dataChanged );
566​
568 return widget; 567​ return widget;
569} 568​}
570 569​
571void LineAnnotationWidget::applyChanges() 570​void LineAnnotationWidget::applyChanges()
572{ 571​{
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() );
584 } 587​ }
585 else 588​ else
586 { 589​ {
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​
596 598​
597InkAnnotationWidget::InkAnnotationWidget( Okular::Annotation * ann ) 599​InkAnnotationWidget::InkAnnotationWidget( Okular::Annotation * ann )
598 : AnnotationWidget( ann ) 600​ : AnnotationWidget( ann )
599{ 601​{
600 m_inkAnn = static_cast< Okular::InkAnnotation * >( ann ); 602​ m_inkAnn = static_cast< Okular::InkAnnotation * >( ann );
▲ Show 20 Lines • Show All 278 Lines • Show Last 20 Lines