Use std::unique_ptr for two private data members
ClosedPublic

Authored by sander on Sep 5 2018, 7:19 PM.

Details

Summary

This leads to a minor code simplification.

Test Plan

There should be no functional changes caused by this patch.

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
sander created this revision.Sep 5 2018, 7:19 PM
Restricted Application added a project: Okular. · View Herald TranscriptSep 5 2018, 7:19 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
sander requested review of this revision.Sep 5 2018, 7:19 PM
tobiasdeiminger added inline comments.
ui/pageviewannotator.cpp
304

+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.

sander updated this revision to Diff 41114.Sep 6 2018, 3:37 PM

You are right about QPixmap. The updated diff now uses a QPixmap object directly.

sander marked an inline comment as done.Sep 6 2018, 3:37 PM
tobiasdeiminger added inline comments.Sep 8 2018, 8:30 AM
ui/pageviewannotator.cpp
515

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.

sander added inline comments.Sep 10 2018, 7:44 PM
ui/pageviewannotator.cpp
515

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.

tobiasdeiminger accepted this revision.Sep 11 2018, 7:01 AM
This revision is now accepted and ready to land.Sep 11 2018, 7:01 AM
This revision was automatically updated to reflect the committed changes.