Differential D15301 Diff 41071 ui/pageviewannotator.cpp

Changeset View

Standalone View

View Options

ui/pageviewannotator.cpp

Context not available.
29 29​
30// system includes 30​// system includes
31#include <math.h> 31​#include <math.h>
32​#include <memory>
32#include <QStandardPaths> 33​#include <QStandardPaths>
33 34​
34// local includes 35​// local includes
Context not available.
47{ 48​{
48 public: 49​ public:
49 PickPointEngine( const QDomElement & engineElement ) 50​ PickPointEngine( const QDomElement & engineElement )
50 : AnnotatorEngine( engineElement ), clicked( false ), pixmap( nullptr ), 51​ : AnnotatorEngine( engineElement ), clicked( false ),
51 xscale( 1.0 ), yscale( 1.0 ) 52​ xscale( 1.0 ), yscale( 1.0 )
52 { 53​ {
53 // parse engine specific attributes 54​ // parse engine specific attributes
Context not available.
64 65​
65 // create engine objects 66​ // create engine objects
66 if ( !hoverIconName.simplified().isEmpty() ) 67​ if ( !hoverIconName.simplified().isEmpty() )
67 pixmap = new QPixmap( GuiUtils::loadStamp( hoverIconName, QSize( size, size ) ) ); 68​ pixmap = std::unique_ptr<QPixmap>( new QPixmap( GuiUtils::loadStamp( hoverIconName, QSize( size, size ) ) ) );
68 } ​
69 ​
70 ~PickPointEngine() override ​
71 { ​
72 delete pixmap; ​
73 } 69​ }
74 70​
75 QRect event( EventType type, Button button, double nX, double nY, double xScale, double yScale, const Okular::Page * page ) override 71​ QRect event( EventType type, Button button, double nX, double nY, double xScale, double yScale, const Okular::Page * page ) override
Context not available.
305 Okular::NormalizedRect rect; 301​ Okular::NormalizedRect rect;
306 Okular::NormalizedPoint startpoint; 302​ Okular::NormalizedPoint startpoint;
307 Okular::NormalizedPoint point; 303​ Okular::NormalizedPoint point;
308 QPixmap * pixmap; 304​ std::unique_ptr<QPixmap> pixmap;
tobiasdeimingerUnsubmitted
Done

+1 for automatic memory management. But is there a reason to not just say

QPixmap pixmap;

? From top of my head, QPixmap is already a small handle to implicitly shared COW pixmap data. So, no need to wrap it again.

tobiasdeiminger: +1 for automatic memory management. But is there a reason to not just say ``` QPixmap pixmap…
309 QString hoverIconName, iconName; 305​ QString hoverIconName, iconName;
310 int size; 306​ int size;
311 double xscale,yscale; 307​ double xscale,yscale;
Context not available.
490{ 486​{
491 public: 487​ public:
492 TextSelectorEngine( const QDomElement & engineElement, PageView * pageView ) 488​ TextSelectorEngine( const QDomElement & engineElement, PageView * pageView )
493 : AnnotatorEngine( engineElement ), m_pageView( pageView ), 489​ : AnnotatorEngine( engineElement ), m_pageView( pageView )
494 selection( nullptr ) ​
495 { 490​ {
496 // parse engine specific attributes 491​ // parse engine specific attributes
497 } 492​ }
498 493​
499 ~TextSelectorEngine() override ​
500 { ​
501 delete selection; ​
502 } ​
503 ​
504 QRect event( EventType type, Button button, double nX, double nY, double xScale, double yScale, const Okular::Page * /*page*/ ) override 494​ QRect event( EventType type, Button button, double nX, double nY, double xScale, double yScale, const Okular::Page * /*page*/ ) override
505 { 495​ {
506 // only proceed if pressing left button 496​ // only proceed if pressing left button
Context not available.
521 { 511​ {
522 const QPoint start( (int)( lastPoint.x * item()->uncroppedWidth() ), (int)( lastPoint.y * item()->uncroppedHeight() ) ); 512​ const QPoint start( (int)( lastPoint.x * item()->uncroppedWidth() ), (int)( lastPoint.y * item()->uncroppedHeight() ) );
523 const QPoint end( (int)( nX * item()->uncroppedWidth() ), (int)( nY * item()->uncroppedHeight() ) ); 513​ const QPoint end( (int)( nX * item()->uncroppedWidth() ), (int)( nY * item()->uncroppedHeight() ) );
524 delete selection; 514​ selection.reset();
525 selection = nullptr; 515​ std::unique_ptr<Okular::RegularAreaRect> newselection( m_pageView->textSelectionForItem( item(), start, end ) );
tobiasdeimingerUnsubmitted
Not Done

This assumes m_pageView->textSelectionForItem transfers ownership for the returned RegularAreaRect*. That's actually true, but we can only tell it by looking at the code inside TextPage::textArea.

Would be nice if we could make ownership explicit throughout the call chain. But I'm afraid it's problematic because TextPage::textArea is part of public interface and we don't want to change it?

If you can't change it, just go on. It's not a problem introduced by your patch but already existed before.

tobiasdeiminger: This assumes `m_pageView->textSelectionForItem` transfers ownership for the returned…
sanderAuthorUnsubmitted
Not Done

Would be nice if we could make ownership explicit throughout the call chain.

Agreed

But I'm afraid it's problematic because TextPage::textArea is part of public interface and we don't want to change it?

Not sure. But I'd rather leave that to a separate diff.

sander: > Would be nice if we could make ownership explicit throughout the call chain. Agreed > But…
526 Okular::RegularAreaRect * newselection = m_pageView->textSelectionForItem( item(), start, end ); ​
527 if ( newselection && !newselection->isEmpty() ) 516​ if ( newselection && !newselection->isEmpty() )
528 { 517​ {
529 const QList<QRect> geom = newselection->geometry( (int)xScale, (int)yScale ); 518​ const QList<QRect> geom = newselection->geometry( (int)xScale, (int)yScale );
Context not available.
536 newrect |= r; 525​ newrect |= r;
537 } 526​ }
538 rect |= newrect; 527​ rect |= newrect;
539 selection = newselection; 528​ selection = std::move(newselection);
540 } ​
541 else ​
542 { ​
543 delete newselection; ​
544 } 529​ }
545 } 530​ }
546 } 531​ }
Context not available.
621 ann = ha; 606​ ann = ha;
622 } 607​ }
623 608​
624 delete selection; 609​ selection.reset();
625 selection = nullptr; ​
626 610​
627 // safety check 611​ // safety check
628 if ( !ann ) 612​ if ( !ann )
Context not available.
647 // data 631​ // data
648 PageView * m_pageView; 632​ PageView * m_pageView;
649 // TODO: support more pages 633​ // TODO: support more pages
650 Okular::RegularAreaRect * selection; 634​ std::unique_ptr<Okular::RegularAreaRect> selection;
651 Okular::NormalizedPoint lastPoint; 635​ Okular::NormalizedPoint lastPoint;
652 QRect rect; 636​ QRect rect;
653}; 637​};
Context not available.