Removed using of raw pointers for QGraphicsItem in photo editor.

Now all items are wrapped in the shared_ptr,
and the Scene loses ownership of all items before being destroyed.

Telegram/SourceFiles/editor/editor_paint.cpp

@@ -12,7 +12,7 @@ https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL
 #include "editor/scene_item_canvas.h"
 #include "editor/scene_item_sticker.h"
 #include "editor/controllers.h"
-#include "base/event_filter.h"
+#include "lottie/lottie_single_player.h"
 
 #include <QGraphicsView>
 
@@ -38,6 +38,8 @@ std::shared_ptr<Scene> EnsureScene(
 
 } // namespace
 
+using ItemPtr = Scene::ItemPtr;
+
 Paint::Paint(
 	not_null<Ui::RpWidget*> parent,
 	PhotoModifications &modifications,
@@ -71,7 +73,7 @@ Paint::Paint(
 			? Qt::DescendingOrder
 			: Qt::AscendingOrder);
 
-		auto proj = [&](QGraphicsItem *i) {
+		auto proj = [&](const ItemPtr &i) {
 			return isUndo ? i->isVisible() : isItemHidden(i);
 		};
 		const auto it = ranges::find_if(filtered, std::move(proj));
@@ -108,7 +110,7 @@ Paint::Paint(
 			const auto size = std::min(s.width(), s.height()) / 2;
 			const auto x = s.width() / 2;
 			const auto y = s.height() / 2;
-			const auto item = new ItemSticker(
+			const auto item = std::make_shared<ItemSticker>(
 				document,
 				_zoom.value(),
 				_lastZ,
@@ -169,13 +171,13 @@ void Paint::cancel() {
 		return;
 	}
 
-	for (const auto &group : filtered) {
+	for (const auto &item : filtered) {
 		const auto it = ranges::find(
 			_previousItems,
-			group,
+			item,
 			&SavedItem::item);
 		if (it == end(_previousItems)) {
-			_scene->removeItem(group);
+			_scene->removeItem(item);
 		} else {
 			it->item->setVisible(!it->undid);
 		}
@@ -188,11 +190,12 @@ void Paint::keepResult() {
 	for (const auto &item : _itemsToRemove) {
 		_scene->removeItem(item);
 	}
+	_itemsToRemove.clear();
 
 	const auto items = _scene->items();
 	_previousItems = ranges::views::all(
 		items
-	) | ranges::views::transform([=](QGraphicsItem *i) -> SavedItem {
+	) | ranges::views::transform([=](ItemPtr i) -> SavedItem {
 		return { i, !i->isVisible() };
 	}) | ranges::to_vector;
 }
@@ -204,7 +207,7 @@ bool Paint::hasUndo() const {
 bool Paint::hasRedo() const {
 	return ranges::any_of(
 		_scene->items(),
-		[=](QGraphicsItem *i) { return isItemHidden(i); });
+		[=](const ItemPtr &i) { return isItemHidden(i); });
 }
 
 void Paint::clearRedoList() {
@@ -212,10 +215,10 @@ void Paint::clearRedoList() {
 	auto &&filtered = ranges::views::all(
 		items
 	) | ranges::views::filter(
-		[=](QGraphicsItem *i) { return isItemHidden(i); }
+		[=](const ItemPtr &i) { return isItemHidden(i); }
 	);
 
-	ranges::for_each(std::move(filtered), [&](QGraphicsItem *item) {
+	ranges::for_each(std::move(filtered), [&](ItemPtr item) {
 		item->hide();
 		_itemsToRemove.push_back(item);
 	});
@@ -223,12 +226,12 @@ void Paint::clearRedoList() {
 	_hasRedo = false;
 }
 
-bool Paint::isItemHidden(not_null<QGraphicsItem*> item) const {
+bool Paint::isItemHidden(const ItemPtr &item) const {
 	return !item->isVisible() && !isItemToRemove(item);
 }
 
-bool Paint::isItemToRemove(not_null<QGraphicsItem*> item) const {
-	return ranges::contains(_itemsToRemove, item.get());
+bool Paint::isItemToRemove(const ItemPtr &item) const {
+	return ranges::contains(_itemsToRemove, item);
 }
 
 void Paint::updateUndoState() {

Telegram/SourceFiles/editor/editor_paint.h

@@ -39,7 +39,7 @@ public:
 
 private:
 	struct SavedItem {
-		QGraphicsItem *item;
+		std::shared_ptr<QGraphicsItem> item;
 		bool undid = false;
 	};
 
@@ -47,8 +47,8 @@ private:
 	bool hasRedo() const;
 	void clearRedoList();
 
-	bool isItemToRemove(not_null<QGraphicsItem*> item) const;
-	bool isItemHidden(not_null<QGraphicsItem*> item) const;
+	bool isItemToRemove(const std::shared_ptr<QGraphicsItem> &item) const;
+	bool isItemHidden(const std::shared_ptr<QGraphicsItem> &item) const;
 
 	const std::shared_ptr<float64> _lastZ;
 	const std::shared_ptr<Scene> _scene;
@@ -56,7 +56,7 @@ private:
 	const QSize _imageSize;
 
 	std::vector<SavedItem> _previousItems;
-	QList<QGraphicsItem*> _itemsToRemove;
+	std::vector<std::shared_ptr<QGraphicsItem>> _itemsToRemove;
 
 	rpl::variable<bool> _hasUndo = true;
 	rpl::variable<bool> _hasRedo = true;

Telegram/SourceFiles/editor/scene.cpp

@@ -17,6 +17,8 @@ https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL
 namespace Editor {
 namespace {
 
+using ItemPtr = Scene::ItemPtr;
+
 bool SkipMouseEvent(not_null<QGraphicsSceneMouseEvent*> event) {
 	return event->isAccepted() || (event->button() == Qt::RightButton);
 }
@@ -25,25 +27,44 @@ bool SkipMouseEvent(not_null<QGraphicsSceneMouseEvent*> event) {
 
 Scene::Scene(const QRectF &rect)
 : QGraphicsScene(rect)
-, _canvas(new ItemCanvas) {
-	QGraphicsScene::addItem(_canvas);
+, _canvas(std::make_shared<ItemCanvas>()) {
+	QGraphicsScene::addItem(_canvas.get());
 	_canvas->clearPixmap();
 
 	_canvas->grabContentRequests(
 	) | rpl::start_with_next([=](ItemCanvas::Content &&content) {
-		const auto item = new ItemLine(std::move(content.pixmap));
+		const auto item = std::make_shared<ItemLine>(
+			std::move(content.pixmap));
 		item->setPos(content.position);
 		addItem(item);
 		_canvas->setZValue(++_lastLineZ);
 	}, _lifetime);
 }
 
-void Scene::addItem(not_null<NumberedItem*> item) {
+void Scene::addItem(std::shared_ptr<NumberedItem> item) {
+	if (!item) {
+		return;
+	}
 	item->setNumber(_itemNumber++);
-	QGraphicsScene::addItem(item);
+	QGraphicsScene::addItem(item.get());
+	_items.push_back(std::move(item));
 	_addsItem.fire({});
 }
 
+void Scene::removeItem(not_null<QGraphicsItem*> item) {
+	const auto it = ranges::find_if(_items, [&](const ItemPtr &i) {
+		return i.get() == item;
+	});
+	if (it == end(_items)) {
+		return;
+	}
+	removeItem(*it);
+}
+
+void Scene::removeItem(const ItemPtr &item) {
+	_items.erase(ranges::remove(_items, item), end(_items));
+}
+
 void Scene::mousePressEvent(QGraphicsSceneMouseEvent *event) {
 	QGraphicsScene::mousePressEvent(event);
 	if (SkipMouseEvent(event)) {
@@ -81,23 +102,19 @@ rpl::producer<> Scene::addsItem() const {
 	return _addsItem.events();
 }
 
-std::vector<QGraphicsItem*> Scene::items(Qt::SortOrder order) const {
-	using Item = QGraphicsItem;
-	auto rawItems = QGraphicsScene::items();
+std::vector<ItemPtr> Scene::items(
+		Qt::SortOrder order) const {
+	auto copyItems = _items;
 
-	auto filteredItems = ranges::views::all(
-		rawItems
-	) | ranges::views::filter([](Item *i) {
-		return i->type() != ItemCanvas::Type;
-	}) | ranges::to_vector;
-
-	ranges::sort(filteredItems, [&](not_null<Item*> a, not_null<Item*> b) {
-		const auto numA = qgraphicsitem_cast<NumberedItem*>(a)->number();
-		const auto numB = qgraphicsitem_cast<NumberedItem*>(b)->number();
+	ranges::sort(copyItems, [&](ItemPtr a, ItemPtr b) {
+		const auto numA = qgraphicsitem_cast<NumberedItem*>(
+			a.get())->number();
+		const auto numB = qgraphicsitem_cast<NumberedItem*>(
+			b.get())->number();
 		return (order == Qt::AscendingOrder) ? (numA < numB) : (numA > numB);
 	});
 
-	return filteredItems;
+	return copyItems;
 }
 
 std::vector<MTPInputDocument> Scene::attachedStickers() const {
@@ -105,14 +122,20 @@ std::vector<MTPInputDocument> Scene::attachedStickers() const {
 
 	return ranges::views::all(
 		allItems
-	) | ranges::views::filter([](QGraphicsItem *i) {
+	) | ranges::views::filter([](const ItemPtr &i) {
 		return i->isVisible() && (i->type() == ItemSticker::Type);
-	}) | ranges::views::transform([](QGraphicsItem *i) {
-		return qgraphicsitem_cast<ItemSticker*>(i)->sticker();
+	}) | ranges::views::transform([](const ItemPtr &i) {
+		return qgraphicsitem_cast<ItemSticker*>(i.get())->sticker();
 	}) | ranges::to_vector;
 }
 
 Scene::~Scene() {
+	// Prevent destroying by scene of all items.
+	QGraphicsScene::removeItem(_canvas.get());
+	for (const auto &item : items()) {
+		// Scene loses ownership of an item.
+		QGraphicsScene::removeItem(item.get());
+	}
 }
 
 } // namespace Editor

Telegram/SourceFiles/editor/scene.h

@@ -24,13 +24,17 @@ class NumberedItem;
 
 class Scene final : public QGraphicsScene {
 public:
+	using ItemPtr = std::shared_ptr<QGraphicsItem>;
+
 	Scene(const QRectF &rect);
 	~Scene();
 	void applyBrush(const QColor &color, float size);
 
-	[[nodiscard]] std::vector<QGraphicsItem*> items(
+	[[nodiscard]] std::vector<ItemPtr> items(
 		Qt::SortOrder order = Qt::DescendingOrder) const;
-	void addItem(not_null<NumberedItem*> item);
+	void addItem(std::shared_ptr<NumberedItem> item);
+	void removeItem(not_null<QGraphicsItem*> item);
+	void removeItem(const ItemPtr &item);
 	[[nodiscard]] rpl::producer<> addsItem() const;
 	[[nodiscard]] rpl::producer<> mousePresses() const;
 
@@ -40,7 +44,9 @@ protected:
 	void mouseReleaseEvent(QGraphicsSceneMouseEvent *event) override;
 	void mouseMoveEvent(QGraphicsSceneMouseEvent *event) override;
 private:
-	const not_null<ItemCanvas*> _canvas;
+	const std::shared_ptr<ItemCanvas> _canvas;
+
+	std::vector<ItemPtr> _items;
 
 	float64 _lastLineZ = 0.;
 	int _itemNumber = 0;

Telegram/SourceFiles/editor/scene_item_base.cpp

@@ -7,6 +7,7 @@ https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL
 */
 #include "editor/scene_item_base.h"
 
+#include "editor/scene.h"
 #include "lang/lang_keys.h"
 #include "ui/widgets/popup_menu.h"
 #include "styles/style_editor.h"
@@ -179,9 +180,8 @@ void ItemBase::contextMenuEvent(QGraphicsSceneContextMenuEvent *event) {
 
 	_menu = base::make_unique_q<Ui::PopupMenu>(nullptr);
 	_menu->addAction(tr::lng_selected_delete(tr::now), [=] {
-		if (scene()) {
-			scene()->removeItem(this); // Scene loses ownership of item.
-			delete this;
+		if (const auto s = static_cast<Scene*>(scene())) {
+			s->removeItem(this);
 		}
 	});