Replace some GList with STL structs #3042

Marmare314 · 2021-04-16T06:53:12Z

Since I am new to the Xournal++ code I thought I could get started by doing some refactoring :)

ClipboardHandler
- replaced GList by std::set because it is sorted
- added some static_cast's to make clang-tidy happy
RegionSelect
- replaced GList by std::vector since only push_back is needed
- reserve has not been called

Marmare314 · 2021-04-16T07:50:25Z

Unfortunately I can't seem to figure out how to install clang-format-12 on Linux Mint 20.
And apparently clang-format-10 with 'AlignOperands: true' yields a different result.

rolandlo · 2021-04-16T08:11:03Z

Unfortunately I can't seem to figure out how to install clang-format-12 on Linux Mint 20.
And apparently clang-format-10 with 'AlignOperands: true' yields a different result.

I fixed it. Do you have clang-format-11 on Linux Mint 20? That may be enough.

Marmare314 · 2021-04-16T08:32:25Z

It does! clang-format-11 seems to work fine while git-clang-format-11 complains about the AlignOperators Argument.
But I guess that's good enough.

Febbe · 2021-04-18T13:43:30Z

you can install the newest (not-stable) version, which brings meta-packages from here: https://apt.llvm.org/

git-clang-format-11 complains about the AlignOperators Argument

There is a bug (in my opinion); git-clang-format-11 calls the meta-package clang-format, instead of clang-format-11. When you use the canonical provided meta package, you'll have clang-format-10 installed. so git-clang-format-11 will call clang-format which calls clang-format-10.

Installing the llvm provided meta-package will fix everything.

Marmare314 · 2021-04-19T11:53:20Z

I got clang-format-12 running now, however I had no luck with the meta-package.
But git-clang-format --binary clang-format-12 works well.

Also: If I do some similar refactoring, should I create a new pull request, or push it to this one?

rolandlo · 2021-04-19T18:24:38Z

I got clang-format-12 running now, however I had no luck with the meta-package.
But git-clang-format --binary clang-format-12 works well.

Also: If I do some similar refactoring, should I create a new pull request, or push it to this one?

I suggest you push it to this one. Just squash the commits which belong together and keep commits apart, if they belong to different refactoring steps.

Marmare314 · 2021-04-28T10:57:59Z

I think that should be everything worth replacing.

Febbe

Here, a bulk of change requests, thank you for the PR

Febbe · 2021-04-29T21:14:51Z

src/control/ClipboardHandler.cpp

+auto ElementCompareFunc(Element* a, Element* b) -> bool {
    if (a->getY() == b->getY()) {
-        return a->getX() - b->getX();
+        return (a->getX() - b->getX()) > 0;
    }
-    return a->getY() - b->getY();
+    return (a->getY() - b->getY()) > 0;
 }


Either use the less operator here, or create a comparator object with an adequate name like

constexpr auto operator < (Element *lhs, Element *rhs) -> bool{ return std::tie(lhs->y, lhs->x) > std::tie(rhs->y, rhs->x); }

Febbe · 2021-04-29T21:18:52Z

src/control/ClipboardHandler.cpp

@@ -137,23 +138,22 @@ auto ClipboardHandler::copy() -> bool {
    // prepare text contents
    /////////////////////////////////////////////////////////////////

-    GList* textElements = nullptr;
+    std::multiset<Element*, decltype(&ElementCompareFunc)> textElements(ElementCompareFunc);


You know, all objects of the set will be Text* so make them one:

Suggested change

std::multiset<Element*, decltype(&ElementCompareFunc)> textElements(ElementCompareFunc);

std::multiset<Text*, decltype(&ElementCompareFunc)> textElements(ElementCompareFunc);

Febbe · 2021-04-29T21:19:28Z

src/control/ClipboardHandler.cpp

+    for (Element* l: textElements) {
+        Text* e = dynamic_cast<Text*>(l);


Suggested change

for (Element* l: textElements) {

Text* e = dynamic_cast<Text*>(l);

for (Text* e: textElements) {

Febbe · 2021-04-29T21:19:54Z

src/control/ClipboardHandler.cpp


    for (Element* e: *this->selection->getElements()) {
        if (e->getType() == ELEMENT_TEXT) {
-            textElements = g_list_insert_sorted(textElements, e, reinterpret_cast<GCompareFunc>(ElementCompareFunc));
+            textElements.insert(e);


Suggested change

textElements.insert(e);

textElements.insert(static_cast<Text*>(e));

Febbe · 2021-04-29T21:34:28Z

src/control/tools/Selection.cpp

+            if (ax > p.x) {
+                ax = p.x;


Suggested change

if (ax > p.x) {

ax = p.x;

ax = std::min(ax, p.x);

Febbe · 2021-04-29T22:18:50Z

src/control/xml/XmlNode.h

+    XmlNode(const XmlNode& node) = delete;
+    void operator=(const XmlNode& node) = delete;


Since you are using std::unique_ptr those are deleted anyways. But defining them as deleted requires to fulfill the rule of 5. So best would be to remove them. This will implicitly allow to move the object.

Febbe · 2021-04-29T22:26:17Z

src/control/xml/XmlPointNode.h

+    XmlPointNode(const XmlPointNode& node) = delete;
+    void operator=(const XmlPointNode& node) = delete;


Same here, remove or fulfill the rule of 5.

Febbe · 2021-04-29T22:30:00Z

src/control/xml/XmlPointNode.cpp

+    for (auto pointIter = points.begin(); pointIter != points.end(); pointIter++) {
+        if (pointIter != points.begin()) {


I would move the condition out of the loop, also prepend the ++ (increment) in c++, because val++ will create a copy of val before increment. And not every compiler is capable to remove the extra code.

Febbe · 2021-04-29T22:31:41Z

src/control/xml/XmlPointNode.cpp

-}
-
-void XmlPointNode::addPoint(const Point* point) { this->points = g_list_append(this->points, new Point(*point)); }
+void XmlPointNode::addPoint(const Point* point) { points.emplace_back(*point); }


Suggested change

void XmlPointNode::addPoint(const Point* point) { points.emplace_back(*point); }

void XmlPointNode::addPoint(Point point) { points.emplace_back(std::move(point)); }

Febbe · 2021-04-29T22:34:41Z

src/control/xojfile/SaveHandler.cpp

-
-        auto tmpfn = (fs::path(filepath) += ".") += img->getFilepath();
-        if (!gdk_pixbuf_save(img->getPixbuf(), tmpfn.u8string().c_str(), "png", nullptr, nullptr)) {
+    for (BackgroundImage& img: backgroundImages) {


img is only used as const&:

Suggested change

for (BackgroundImage& img: backgroundImages) {

for (BackgroundImage const& img: backgroundImages) {

Febbe · 2021-04-30T19:32:53Z

src/model/eraser/EraseableStrokePart.cpp

-
-        this->points = g_list_delete_link(this->points, link);
-    }
+    points.erase(points.begin() + 1, points.end() - 1);  // deletes everything but endpoints


Suggested change

points.erase(points.begin() + 1, points.end() - 1); // deletes everything but endpoints

assert(points.size() > 1);

points.erase(points.begin() + 1, points.end() - 1); // deletes everything but endpoints

Should assert from <cassert> be used or g_assert?

Febbe · 2021-04-30T19:35:14Z

src/model/eraser/EraseableStrokePart.h


 public:
    void addPoint(Point p);
    double getWidth() const;

-    GList* getPoints();
+    std::vector<Point>& getPoints();


Can you check, if this function can be const and if you can return a const&

In EraseableStroke::erasePart points are erased. A const version could be added though.

Febbe · 2021-04-30T19:41:06Z

src/undo/PageLayerPosEntry.h

-};
+    // TODO: replace with <=> operator
+    static int cmp(const PageLayerPosEntry<T>& a, const PageLayerPosEntry<T>& b) { return a.pos - b.pos; }
+};


Re add the line break ;)

Febbe · 2021-04-30T19:45:48Z

src/undo/PageLayerPosEntry.h

+    // TODO: replace with <=> operator
+    static int cmp(const PageLayerPosEntry<T>& a, const PageLayerPosEntry<T>& b) { return a.pos - b.pos; }


I am also looking forward to c++20 👀 .
I think this function can be constexpr.
Also, I would implement the less operator

Febbe · 2021-04-30T20:00:45Z

src/undo/PageLayerPosEntry.h

+    // TODO: constructor could be removed with C++20
+    PageLayerPosEntry<T>(Layer* layer, T* element, int pos): layer(layer), element(element), pos(pos) {}


Are you sure? I think this is already replaceable since c++14 or at least c++17.

At least vector::emplace does not support it yet.
But I think this is cleaner than dealing with it in the files using it.

Febbe · 2021-04-30T20:35:18Z

src/undo/EraseUndoAction.cpp

+            EraseableStroke* e = entryIter->element->getEraseable();
+            std::vector<std::unique_ptr<Stroke>> strokeList = e->getStroke(entryIter->element);
+            for (auto& stroke: strokeList) {
+                // TODO (Marmare314): should use unique_ptr in layer


Thank you for the todo :); Yes that is a refactoring, which should go into another pr :)

Febbe · 2021-04-30T20:37:15Z

src/undo/DeleteUndoAction.h

+    std::multiset<PageLayerPosEntry<Element>, decltype(&PageLayerPosEntry<Element>::cmp)> elements{
+            PageLayerPosEntry<Element>::cmp};


Like below this violates the Compare Requirement. (Reference below)

Suggested change

std::multiset<PageLayerPosEntry<Element>, decltype(&PageLayerPosEntry<Element>::cmp)> elements{

PageLayerPosEntry<Element>::cmp};

std::multiset<PageLayerPosEntry<Element>> elements{};

Febbe · 2021-04-30T20:42:46Z

src/undo/AddUndoAction.h

+    std::multiset<PageLayerPosEntry<Element>, decltype(&PageLayerPosEntry<Element>::cmp)> elements{
+            PageLayerPosEntry<Element>::cmp};


Febbe · 2021-04-30T20:52:31Z

src/model/eraser/EraseableStroke.cpp

-            s->setLineStyle(original->getLineStyle());
-            s->setWidth(original->getWidth());
-            list = g_list_append(list, s);
+            auto& newStroke = strokeList.emplace_back(std::move(std::make_unique<Stroke>()));


std::make_unique<Stroke>() is already a rvalue-reference, no need to call move here.

Suggested change

auto& newStroke = strokeList.emplace_back(std::move(std::make_unique<Stroke>()));

auto& newStroke = strokeList.emplace_back(std::make_unique<Stroke>());

Febbe · 2021-04-30T20:55:20Z

src/model/eraser/EraseableStroke.cpp

-        GList* points = p->getPoints();
-        if (g_list_length(points) < 2) {
+    for (EraseableStrokePart& part: parts) {
+        std::vector<Point>& points = part.getPoints();


points is only used as const &

Suggested change

std::vector<Point>& points = part.getPoints();

auto const& points = part.getPoints();

Febbe · 2021-04-30T20:58:49Z

remove po/xournalpp.pot from the commit history

arvryna · 2021-06-08T19:52:28Z

@Marmare314 : There are some files you did not change for example:

PopplerGlibPage.cpp,
ToolbarCustomizeDialog.cpp,
MainWindow.cpp,
RecentManager.cpp
DeviceListHelper.cpp

In these place, is there any specific reason why you avoided it ? or is it not worth changing there ?
If you don't want to work on them, maybe i will it up.

Marmare314 · 2021-06-09T11:01:08Z

@Marmare314 : There are some files you did not change for example:

PopplerGlibPage.cpp,
ToolbarCustomizeDialog.cpp,
MainWindow.cpp,
RecentManager.cpp
DeviceListHelper.cpp

In these place, is there any specific reason why you avoided it ? or is it not worth changing there ?
If you don't want to work on them, maybe i will it up.

PopplerGlibPage: its not worth replacing it as poppler_page_find_text returns a glist an it is only iterated over once
MainWindow, DeviceListHelper, ToolbarCustomizeDialog: same thing here
RecentManager: i forgot about this one

It will be a while before I will have time to replace it, so feel free to do it.

Febbe · 2021-06-09T11:54:27Z

With c++20, we will get std::ranges and Sentinels, this will make it simple, to just create a
template <GListLike C> GListView;, without wrapping it. Then we can also get rid of some copies.

rolandlo · 2021-07-23T10:21:05Z

This PR should be rebased on current master, while solving the merge conflicts.

refactor ClipboardHandler, RegionSelect refactor XmlNode replace glist in SaveHandler replace glist in FormatDialog + clang-format refactor EraseableStrokePart refactor EraseableStroke refactor EraseableStroke::getStroke fix everything except clipboardhandler and commented requests

Febbe · 2021-12-01T16:07:09Z

Rebased on current master

refactor ClipboardHandler, RegionSelect refactor XmlNode replace glist in SaveHandler replace glist in FormatDialog + clang-format refactor EraseableStrokePart refactor EraseableStroke refactor EraseableStroke::getStroke fix everything except clipboardhandler and commented requests

Febbe · 2021-12-01T16:57:52Z

Thank you for the PR. I finally got time to merge it.

refactor ClipboardHandler, RegionSelect refactor XmlNode replace glist in SaveHandler replace glist in FormatDialog + clang-format refactor EraseableStrokePart refactor EraseableStroke refactor EraseableStroke::getStroke fix everything except clipboardhandler and commented requests

Febbe added this to the Future major version milestone Apr 18, 2021

Marmare314 force-pushed the move_to_stl branch from e54452d to ebad49d Compare April 19, 2021 22:13

Technius added the refactoring label Apr 20, 2021

Febbe requested changes Apr 29, 2021

View reviewed changes

Febbe requested changes Apr 30, 2021

View reviewed changes

Marmare314 force-pushed the move_to_stl branch from 7f34296 to 67f03f5 Compare May 1, 2021 07:58

rolandlo mentioned this pull request Jun 26, 2021

Known Bugs and Todo's #2354

Open

30 tasks

Febbe modified the milestones: Future major version, 1.2 Jul 19, 2021

Febbe force-pushed the move_to_stl branch from 73553ce to 30a4245 Compare December 1, 2021 16:05

Febbe force-pushed the move_to_stl branch from 30a4245 to f31b443 Compare December 1, 2021 16:39

Febbe force-pushed the move_to_stl branch from f31b443 to cb9b443 Compare December 1, 2021 16:57

Febbe merged commit 0156eb6 into xournalpp:master Dec 1, 2021

This was referenced Dec 6, 2021

Copied texts are pasted in reversed order #3637

Closed

Fix 3637 - properly order copied text in clipboard #3639

Merged

This was referenced Dec 6, 2021

Crash using eraser #3640

Closed

Strokes sometimes disappear when recognized as lines by the recognizer #3642

Closed

	std::multiset<Element*, decltype(&ElementCompareFunc)> textElements(ElementCompareFunc);
	std::multiset<Text*, decltype(&ElementCompareFunc)> textElements(ElementCompareFunc);

		for (Element* l: textElements) {
		Text* e = dynamic_cast<Text*>(l);

	for (Element* l: textElements) {
	Text* e = dynamic_cast<Text*>(l);
	for (Text* e: textElements) {

	textElements.insert(e);
	textElements.insert(static_cast<Text*>(e));

		XmlNode(const XmlNode& node) = delete;
		void operator=(const XmlNode& node) = delete;

		XmlPointNode(const XmlPointNode& node) = delete;
		void operator=(const XmlPointNode& node) = delete;

		for (auto pointIter = points.begin(); pointIter != points.end(); pointIter++) {
		if (pointIter != points.begin()) {

	void XmlPointNode::addPoint(const Point* point) { points.emplace_back(*point); }
	void XmlPointNode::addPoint(Point point) { points.emplace_back(std::move(point)); }

	for (BackgroundImage& img: backgroundImages) {
	for (BackgroundImage const& img: backgroundImages) {

	points.erase(points.begin() + 1, points.end() - 1); // deletes everything but endpoints
	assert(points.size() > 1);
	points.erase(points.begin() + 1, points.end() - 1); // deletes everything but endpoints

		// TODO: replace with <=> operator
		static int cmp(const PageLayerPosEntry<T>& a, const PageLayerPosEntry<T>& b) { return a.pos - b.pos; }

		// TODO: constructor could be removed with C++20
		PageLayerPosEntry<T>(Layer* layer, T* element, int pos): layer(layer), element(element), pos(pos) {}

		std::multiset<PageLayerPosEntry<Element>, decltype(&PageLayerPosEntry<Element>::cmp)> elements{
		PageLayerPosEntry<Element>::cmp};

	auto& newStroke = strokeList.emplace_back(std::move(std::make_unique<Stroke>()));
	auto& newStroke = strokeList.emplace_back(std::make_unique<Stroke>());

	std::vector<Point>& points = part.getPoints();
	auto const& points = part.getPoints();

Replace some GList with STL structs #3042

Replace some GList with STL structs #3042

Conversation

Marmare314 commented Apr 16, 2021

Marmare314 commented Apr 16, 2021

rolandlo commented Apr 16, 2021

Marmare314 commented Apr 16, 2021

Febbe commented Apr 18, 2021

Marmare314 commented Apr 19, 2021

rolandlo commented Apr 19, 2021

Marmare314 commented Apr 28, 2021

Febbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Febbe commented Apr 30, 2021

arvryna commented Jun 8, 2021

Marmare314 commented Jun 9, 2021

Febbe commented Jun 9, 2021

rolandlo commented Jul 23, 2021

Febbe commented Dec 1, 2021

Febbe commented Dec 1, 2021