View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0016181ScribusStory Editor / Text Framespublic2020-07-20 19:192020-08-19 11:56
Reporterale Assigned To 
PrioritynormalSeverityfeatureReproducibilityN/A
Status newResolutionopen 
Product Version1.5.6.svn 
Summary0016181: PATCH: refactor the anchor mark out of scribus.h
Description- create a marks directory for the whole code for the marks.
- refactor the anchor out of ScribusMainWindow::insertMarkDialog().
- as soon as the code is reviewed and merged, i plan to also refactor the other types of marks and move them to the marks directory.
- the CMakeLists in marks is inspired by the one in styles/.
- ScribusMainWindow::editMarkDlg() should also be refactored out. i first wait for the concept to be accepted.
- instead of the inheritance to a main Marks class, i'm using a utils static class: most of the code in there is not specific to the marks and it might be useful to see it moved to ScribusDoc or ScribusView once the whole refactoring is finished.
- in the future more code should move to specific directories and i hope that one day scribus.cpp (and the other main classes) will get a bit slimmer...

https://gitlab.com/scribus/scribus/-/merge_requests/23
TagsNo tags attached.
PatchYes

Activities

ale

2020-07-20 19:19

manager  

anchor-refactoring.diff (10,638 bytes)    
diff --git a/scribus/CMakeLists.txt b/scribus/CMakeLists.txt
index 33c757c1581ca747cb9ec97c6cf2ef38302cac92..d4c6ace2b3e2a42e36e09d8e76755a9939639a16 100644
--- a/scribus/CMakeLists.txt
+++ b/scribus/CMakeLists.txt
@@ -34,6 +34,7 @@ add_subdirectory(dtd)
 add_subdirectory(colormgmt)
 add_subdirectory(desaxe)
 add_subdirectory(fonts)
+add_subdirectory(marks)
 add_subdirectory(styles)
 add_subdirectory(text)
 add_subdirectory(ui/qml)
@@ -1127,6 +1128,7 @@ link_directories(
 	${CMAKE_CURRENT_BINARY_DIR}/colormgmt
 	${CMAKE_CURRENT_BINARY_DIR}/desaxe
 	${CMAKE_CURRENT_BINARY_DIR}/fonts
+	${CMAKE_CURRENT_BINARY_DIR}/marks
 	${CMAKE_CURRENT_BINARY_DIR}/styles
 	${CMAKE_CURRENT_BINARY_DIR}/text
 	${CMAKE_CURRENT_BINARY_DIR}/third_party/wpg
@@ -1156,6 +1158,7 @@ if(WITH_TESTS)
 else()
 	set(SCRIBUS_TESTS_LIB "")
 endif()
+set(SCRIBUS_MARKS_LIB "scribus_marks_lib")
 set(SCRIBUS_STYLES_LIB "scribus_styles_lib")
 set(SCRIBUS_FONTS_LIB "scribus_fonts_lib")
 set(SCRIBUS_DESAXE_LIB "scribus_desaxe_lib")
@@ -1225,6 +1228,7 @@ if(HAVE_OSG)
 		${SCRIBUS_MAIN_CPP}
 		${SCRIBUS_UI_SOURCES}
 		${SCRIBUS_TEXT_SOURCES}
+		${SCRIBUS_MARKS_SOURCES}
 		${SCRIBUS_STYLES_SOURCES}
 		${SCRIBUS_COLORMGMT_SOURCES}
 		${SCRIBUS_FONTS_SOURCES}
@@ -1240,6 +1244,7 @@ else()
 		${SCRIBUS_MAIN_CPP}
 		${SCRIBUS_UI_SOURCES}
 		${SCRIBUS_TEXT_SOURCES}
+		${SCRIBUS_MARKS_SOURCES}
 		${SCRIBUS_STYLES_SOURCES}
 		${SCRIBUS_COLORMGMT_SOURCES}
 		${SCRIBUS_FONTS_SOURCES}
@@ -1356,6 +1361,7 @@ target_link_libraries(${EXE_NAME}
 	${SCRIBUS_COLORMGMT_LIB}
 	${SCRIBUS_DESAXE_LIB}
 	${SCRIBUS_TEXT_LIB}
+	${SCRIBUS_MARKS_LIB}
 	${SCRIBUS_STYLES_LIB}
 	${SCRIBUS_FONTS_LIB}
 	${SCRIBUS_WPG_LIB}
diff --git a/scribus/actionmanager.cpp b/scribus/actionmanager.cpp
index 13328224ada0795d77f972e61a9dc9b0bda7bb36..bad1a36bbfa4728730bc944d0dcceb82baf1a6ab 100644
--- a/scribus/actionmanager.cpp
+++ b/scribus/actionmanager.cpp
@@ -31,6 +31,8 @@ for which a new license (GPL+exception) is in place.
 #include "urllauncher.h"
 #include "iconmanager.h"
 
+#include "marks/anchor.h"
+
 QMap<QString, QKeySequence> ActionManager::defKeys;
 QVector< QPair<QString, QStringList> > ActionManager::defMenuNames;
 QVector< QPair<QString, QStringList> > ActionManager::defMenus;
@@ -582,7 +584,7 @@ void ActionManager::initInsertMenuActions()
 	connect( (*scrActions)["insertSampleText"], SIGNAL(triggered()), mainWindow, SLOT(insertSampleText()) );
 	connect( (*scrActions)["stickyTools"], SIGNAL(triggered()), mainWindow, SLOT(ToggleStickyTools()) );
 
-	connect( (*scrActions)["insertMarkAnchor"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkAnchor()) );
+	connect((*scrActions)["insertMarkAnchor"], &ScrAction::triggered, [](){ Marks::Anchor::insertSlot(); });
 	connect( (*scrActions)["insertMarkVariableText"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkVariableText()) );
 	connect( (*scrActions)["insertMarkItem"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkItem()) );
 	connect( (*scrActions)["insertMark2Mark"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMark2Mark()) );
diff --git a/scribus/marks/CMakeLists.txt b/scribus/marks/CMakeLists.txt
new file mode 100644
index 0000000000000000000000000000000000000000..f8f9b69f2e7588e3ad215148b26ad754841c7bd3
--- /dev/null
+++ b/scribus/marks/CMakeLists.txt
@@ -0,0 +1,36 @@
+include_directories(
+	${CMAKE_SOURCE_DIR}
+	${CMAKE_SOURCE_DIR}/scribus
+	${FREETYPE_INCLUDE_DIRS}
+)
+
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_FPIC}")
+
+set(SCRIBUS_MARKS_MOC_CLASSES
+	anchor.h
+	anchordialog.h
+)
+
+set(SCRIBUS_MARKS_UI_SRC
+	anchordialog.ui
+)
+
+set(SCRIBUS_MARKS_LIB_SOURCES
+	utils.cpp
+	anchor.cpp
+	anchordialog.cpp
+)
+
+QT5_WRAP_CPP(SCRIBUS_MARKS_MOC_SOURCES ${SCRIBUS_MARKS_MOC_CLASSES})
+QT5_WRAP_UI(SCRIBUS_MARKS_UI_SOURCES ${SCRIBUS_MARKS_UI_SRC})
+
+set(SCRIBUS_MARKS_LIB "scribus_marks_lib")
+add_library(${SCRIBUS_MARKS_LIB} STATIC ${SCRIBUS_MARKS_LIB_SOURCES} ${SCRIBUS_MARKS_MOC_SOURCES} ${SCRIBUS_MARKS_UI_SOURCES})
+
+# This is a convenience library that for linkage purposes is part of Scribus's
+# main API.
+set_target_properties(${SCRIBUS_MARKS_LIB}
+	PROPERTIES
+	COMPILE_FLAGS -DCOMPILE_SCRIBUS_MAIN_APP
+)
+
diff --git a/scribus/marks/anchordialog.cpp b/scribus/marks/anchordialog.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..805b83f1349c6e3408c77e57b0da6ca5c7aecfb2
--- /dev/null
+++ b/scribus/marks/anchordialog.cpp
@@ -0,0 +1,24 @@
+#include "marks/anchordialog.h"
+#include "marks/utils.h"
+
+namespace Marks {
+
+AnchorDialog::AnchorDialog(QWidget *parent) :
+	QDialog(parent),
+	ui{new Ui::AnchorDialog}
+{
+	ui->setupUi(this);
+}
+
+QString AnchorDialog::getLabel()
+{
+	auto label = ui->labelEdit->text();
+	return label.isEmpty() ? tr("Anchor mark") : label;
+}
+
+void AnchorDialog::setLabel(const QString& label)
+{
+	ui->labelEdit->setText(label);
+}
+
+}
diff --git a/scribus/marks/anchordialog.h b/scribus/marks/anchordialog.h
new file mode 100644
index 0000000000000000000000000000000000000000..042e1ea47b498d9858d3aebf6d0a9d28a03ab33e
--- /dev/null
+++ b/scribus/marks/anchordialog.h
@@ -0,0 +1,31 @@
+#ifndef MARKSANCHORDIALOG_H
+#define MARKSANCHORDIALOG_H
+
+#include <QDialog>
+#include "scribusapi.h"
+#include "marks/ui_anchordialog.h"
+
+
+namespace Marks {
+
+namespace Ui { class AnchorDialog; }
+
+
+class SCRIBUS_API AnchorDialog : public QDialog
+{
+    Q_OBJECT
+
+public:
+	explicit AnchorDialog(QWidget *parent = nullptr);
+	~AnchorDialog() {}
+
+	QString getLabel();
+	void setLabel(const QString& label);
+
+private:
+	Ui::AnchorDialog *ui;
+};
+
+}
+
+#endif // MARKSANCHORDIALOG_H
diff --git a/scribus/marks/anchordialog.ui b/scribus/marks/anchordialog.ui
new file mode 100644
index 0000000000000000000000000000000000000000..4913b08bb3035ef1fad3f753fa6a73dcaceaf6b2
--- /dev/null
+++ b/scribus/marks/anchordialog.ui
@@ -0,0 +1,92 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ui version="4.0">
+ <class>Marks::AnchorDialog</class>
+ <widget class="QDialog" name="Marks::AnchorDialog">
+  <property name="geometry">
+   <rect>
+    <x>0</x>
+    <y>0</y>
+    <width>400</width>
+    <height>81</height>
+   </rect>
+  </property>
+  <property name="sizePolicy">
+   <sizepolicy hsizetype="Preferred" vsizetype="Fixed">
+    <horstretch>0</horstretch>
+    <verstretch>0</verstretch>
+   </sizepolicy>
+  </property>
+  <property name="minimumSize">
+   <size>
+    <width>0</width>
+    <height>81</height>
+   </size>
+  </property>
+  <property name="maximumSize">
+   <size>
+    <width>16777215</width>
+    <height>81</height>
+   </size>
+  </property>
+  <property name="windowTitle">
+   <string>Insert new Anchor</string>
+  </property>
+  <layout class="QGridLayout" name="gridLayout">
+   <item row="0" column="0">
+    <widget class="QLabel" name="label">
+     <property name="text">
+      <string>Label:</string>
+     </property>
+    </widget>
+   </item>
+   <item row="0" column="1">
+    <widget class="QLineEdit" name="labelEdit"/>
+   </item>
+   <item row="1" column="1">
+    <widget class="QDialogButtonBox" name="buttonBox">
+     <property name="orientation">
+      <enum>Qt::Horizontal</enum>
+     </property>
+     <property name="standardButtons">
+      <set>QDialogButtonBox::Cancel|QDialogButtonBox::Ok</set>
+     </property>
+    </widget>
+   </item>
+  </layout>
+ </widget>
+ <resources/>
+ <connections>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>accepted()</signal>
+   <receiver>Marks::AnchorDialog</receiver>
+   <slot>accept()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>248</x>
+     <y>254</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>157</x>
+     <y>274</y>
+    </hint>
+   </hints>
+  </connection>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>rejected()</signal>
+   <receiver>Marks::AnchorDialog</receiver>
+   <slot>reject()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>316</x>
+     <y>260</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>286</x>
+     <y>274</y>
+    </hint>
+   </hints>
+  </connection>
+ </connections>
+</ui>
diff --git a/scribus/marks/utils.cpp b/scribus/marks/utils.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..c1df04656151aabf4f148993a59ad79d24c975fc
--- /dev/null
+++ b/scribus/marks/utils.cpp
@@ -0,0 +1,45 @@
+#include "utils.h"
+
+#include "scribuscore.h"
+#include "scribus.h"
+#include "scribusdoc.h"
+#include "pageitem.h"
+#include "pageitem_textframe.h"
+#include "selection.h"
+#include "scribusview.h"
+
+namespace Marks {
+
+PageItem_TextFrame* Utils::getTextFrame(ScribusDoc* doc)
+{
+	if (doc->m_Selection->count() != 1)
+		return nullptr;
+	if  (doc->appMode != modeEdit)
+		return nullptr;
+	auto currItem = doc->m_Selection->itemAt(0);
+	if (!currItem->isTextFrame())
+		return nullptr;
+	return currItem->asTextFrame();
+}
+
+PageItem_TextFrame* Utils::getTextFrameFromPage(ScribusDoc* doc)
+{
+	if (doc->masterPageMode())
+		return nullptr;
+	return getTextFrame(doc);
+}
+
+void Utils::refreshView(PageItem_TextFrame* textFrame)
+{
+	auto doc = textFrame->doc();
+	auto view = ScCore->primaryMainWindow()->view;
+
+	view->updatesOn(false);
+	textFrame->invalidateLayout();
+	textFrame->layout();
+	doc->changed();
+	view->updatesOn(true);
+	view->DrawNew();
+}
+
+}
diff --git a/scribus/marks/utils.h b/scribus/marks/utils.h
new file mode 100644
index 0000000000000000000000000000000000000000..3a4dcb7c95fa1eb09f3b2b6aac25d27703be97a8
--- /dev/null
+++ b/scribus/marks/utils.h
@@ -0,0 +1,22 @@
+#ifndef MARKSUTIL_H
+#define MARKSUTIL_H 
+
+class PageItem_TextFrame;
+class ScribusDoc;
+
+namespace Marks {
+
+class Utils
+{
+public:
+	/** @brief return the currently selected text frame on the page or master page*/
+	static PageItem_TextFrame* getTextFrame(ScribusDoc* doc);
+	/** @brief return the currently selected text frame on the page */
+	static PageItem_TextFrame* getTextFrameFromPage(ScribusDoc* doc);
+	/** @brief refresh the layout */
+	static void refreshView(PageItem_TextFrame* textFrame);
+};
+
+}
+
+#endif // Marks
diff --git a/scribus/scribus.h b/scribus/scribus.h
index 71cee3f2ba1c243606c81840f09d78a74dfaf8cc..45c90bbd16c43a2100df6007b1fe70689baa7c89 100644
--- a/scribus/scribus.h
+++ b/scribus/scribus.h
@@ -565,7 +565,6 @@ public slots:
 
 	//inserting marks
 	void slotInsertMark2Mark() { insertMark(MARK2MarkType); }
-	void slotInsertMarkAnchor() { insertMark(MARKAnchorType); }
 	void slotInsertMarkVariableText() { insertMark(MARKVariableTextType); }
 	void slotInsertMarkItem() { insertMark(MARK2ItemType); }
 	void slotInsertMarkNote();
anchor-refactoring.diff (10,638 bytes)   

jghali

2020-07-20 21:34

administrator   ~0047860

I cannot review this patch as it is incomplete. The marks/anchor.h file and probably others are missing.

jghali

2020-07-20 21:43

administrator   ~0047862

>> Instead of the inheritance to a main Marks class, i'm using a utils static class

Hmmm, when I'm reading this I become pretty skeptic.

ale

2020-07-21 07:25

manager   ~0047863

Last edited: 2020-07-21 07:38

 ooops, yes, i haven't uploaded the two files and it was too late to wait for the CI to finish... sorry.

here is the complete diff.

anchor-refactoring-02.diff (13,547 bytes)    
diff --git a/scribus/CMakeLists.txt b/scribus/CMakeLists.txt
index 33c757c1581ca747cb9ec97c6cf2ef38302cac92..d4c6ace2b3e2a42e36e09d8e76755a9939639a16 100644
--- a/scribus/CMakeLists.txt
+++ b/scribus/CMakeLists.txt
@@ -34,6 +34,7 @@ add_subdirectory(dtd)
 add_subdirectory(colormgmt)
 add_subdirectory(desaxe)
 add_subdirectory(fonts)
+add_subdirectory(marks)
 add_subdirectory(styles)
 add_subdirectory(text)
 add_subdirectory(ui/qml)
@@ -1127,6 +1128,7 @@ link_directories(
 	${CMAKE_CURRENT_BINARY_DIR}/colormgmt
 	${CMAKE_CURRENT_BINARY_DIR}/desaxe
 	${CMAKE_CURRENT_BINARY_DIR}/fonts
+	${CMAKE_CURRENT_BINARY_DIR}/marks
 	${CMAKE_CURRENT_BINARY_DIR}/styles
 	${CMAKE_CURRENT_BINARY_DIR}/text
 	${CMAKE_CURRENT_BINARY_DIR}/third_party/wpg
@@ -1156,6 +1158,7 @@ if(WITH_TESTS)
 else()
 	set(SCRIBUS_TESTS_LIB "")
 endif()
+set(SCRIBUS_MARKS_LIB "scribus_marks_lib")
 set(SCRIBUS_STYLES_LIB "scribus_styles_lib")
 set(SCRIBUS_FONTS_LIB "scribus_fonts_lib")
 set(SCRIBUS_DESAXE_LIB "scribus_desaxe_lib")
@@ -1225,6 +1228,7 @@ if(HAVE_OSG)
 		${SCRIBUS_MAIN_CPP}
 		${SCRIBUS_UI_SOURCES}
 		${SCRIBUS_TEXT_SOURCES}
+		${SCRIBUS_MARKS_SOURCES}
 		${SCRIBUS_STYLES_SOURCES}
 		${SCRIBUS_COLORMGMT_SOURCES}
 		${SCRIBUS_FONTS_SOURCES}
@@ -1240,6 +1244,7 @@ else()
 		${SCRIBUS_MAIN_CPP}
 		${SCRIBUS_UI_SOURCES}
 		${SCRIBUS_TEXT_SOURCES}
+		${SCRIBUS_MARKS_SOURCES}
 		${SCRIBUS_STYLES_SOURCES}
 		${SCRIBUS_COLORMGMT_SOURCES}
 		${SCRIBUS_FONTS_SOURCES}
@@ -1356,6 +1361,7 @@ target_link_libraries(${EXE_NAME}
 	${SCRIBUS_COLORMGMT_LIB}
 	${SCRIBUS_DESAXE_LIB}
 	${SCRIBUS_TEXT_LIB}
+	${SCRIBUS_MARKS_LIB}
 	${SCRIBUS_STYLES_LIB}
 	${SCRIBUS_FONTS_LIB}
 	${SCRIBUS_WPG_LIB}
diff --git a/scribus/actionmanager.cpp b/scribus/actionmanager.cpp
index 13328224ada0795d77f972e61a9dc9b0bda7bb36..bad1a36bbfa4728730bc944d0dcceb82baf1a6ab 100644
--- a/scribus/actionmanager.cpp
+++ b/scribus/actionmanager.cpp
@@ -31,6 +31,8 @@ for which a new license (GPL+exception) is in place.
 #include "urllauncher.h"
 #include "iconmanager.h"
 
+#include "marks/anchor.h"
+
 QMap<QString, QKeySequence> ActionManager::defKeys;
 QVector< QPair<QString, QStringList> > ActionManager::defMenuNames;
 QVector< QPair<QString, QStringList> > ActionManager::defMenus;
@@ -582,7 +584,7 @@ void ActionManager::initInsertMenuActions()
 	connect( (*scrActions)["insertSampleText"], SIGNAL(triggered()), mainWindow, SLOT(insertSampleText()) );
 	connect( (*scrActions)["stickyTools"], SIGNAL(triggered()), mainWindow, SLOT(ToggleStickyTools()) );
 
-	connect( (*scrActions)["insertMarkAnchor"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkAnchor()) );
+	connect((*scrActions)["insertMarkAnchor"], &ScrAction::triggered, [](){ Marks::Anchor::insertSlot(); });
 	connect( (*scrActions)["insertMarkVariableText"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkVariableText()) );
 	connect( (*scrActions)["insertMarkItem"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkItem()) );
 	connect( (*scrActions)["insertMark2Mark"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMark2Mark()) );
diff --git a/scribus/marks/CMakeLists.txt b/scribus/marks/CMakeLists.txt
new file mode 100644
index 0000000000000000000000000000000000000000..f8f9b69f2e7588e3ad215148b26ad754841c7bd3
--- /dev/null
+++ b/scribus/marks/CMakeLists.txt
@@ -0,0 +1,36 @@
+include_directories(
+	${CMAKE_SOURCE_DIR}
+	${CMAKE_SOURCE_DIR}/scribus
+	${FREETYPE_INCLUDE_DIRS}
+)
+
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_FPIC}")
+
+set(SCRIBUS_MARKS_MOC_CLASSES
+	anchor.h
+	anchordialog.h
+)
+
+set(SCRIBUS_MARKS_UI_SRC
+	anchordialog.ui
+)
+
+set(SCRIBUS_MARKS_LIB_SOURCES
+	utils.cpp
+	anchor.cpp
+	anchordialog.cpp
+)
+
+QT5_WRAP_CPP(SCRIBUS_MARKS_MOC_SOURCES ${SCRIBUS_MARKS_MOC_CLASSES})
+QT5_WRAP_UI(SCRIBUS_MARKS_UI_SOURCES ${SCRIBUS_MARKS_UI_SRC})
+
+set(SCRIBUS_MARKS_LIB "scribus_marks_lib")
+add_library(${SCRIBUS_MARKS_LIB} STATIC ${SCRIBUS_MARKS_LIB_SOURCES} ${SCRIBUS_MARKS_MOC_SOURCES} ${SCRIBUS_MARKS_UI_SOURCES})
+
+# This is a convenience library that for linkage purposes is part of Scribus's
+# main API.
+set_target_properties(${SCRIBUS_MARKS_LIB}
+	PROPERTIES
+	COMPILE_FLAGS -DCOMPILE_SCRIBUS_MAIN_APP
+)
+
diff --git a/scribus/marks/anchor.cpp b/scribus/marks/anchor.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..aa5f2c6cb1d83aec611a7419cf252373156000f0
--- /dev/null
+++ b/scribus/marks/anchor.cpp
@@ -0,0 +1,78 @@
+#include "marks/anchor.h"
+
+#include "scribuscore.h"
+#include "scribus.h"
+#include "scribusdoc.h"
+#include "undomanager.h"
+#include "pageitem_textframe.h"
+#include "util.h"
+
+#include "marks/utils.h"
+#include "marks/anchordialog.h"
+
+namespace Marks {
+
+void Anchor::insert(PageItem_TextFrame* textFrame, QString& label)
+{
+	auto doc = textFrame->doc();
+	auto undoManager = UndoManager::instance();
+
+	auto trans = undoManager->beginTransaction();
+	// TODO: if there is a selection use it as the label, do not delete it
+	if (textFrame->HasSel)
+	{
+		trans = undoManager->beginTransaction(Um::Selection, Um::IDelete, Um::Delete, QString(), Um::IDelete);
+		textFrame->deleteSelectedTextFromFrame();
+	}
+
+	MarkData markdata;
+	markdata.itemName = textFrame->itemName();
+	markdata.itemPtr = textFrame;
+
+	Mark* mrk = doc->newMark();
+	mrk->setValues(label, textFrame->OwnPage, MARKAnchorType, markdata);
+	// TODO: remove selection, insert mark, restore the selection
+	textFrame->itemText.insertMark(mrk);
+	mrk->OwnPage = textFrame->OwnPage;
+
+	if (undoManager->undoEnabled())
+	{
+		ScItemsState* is = new ScItemsState(undoManager->InsertMark);
+		is->set("ETEA", mrk->label);
+		is->set("label", mrk->label);
+		is->set("type", (int) mrk->getType());
+		is->set("MARK", QString("new"));
+		is->set("strtxt", mrk->getString());
+		is->set("at", textFrame->itemText.cursorPosition() -1);
+		is->insertItem("inItem", textFrame);
+		undoManager->action(doc, is);
+
+		trans.commit();
+	}
+}
+
+void Anchor::insertSlot()
+{
+	auto win = ScCore->primaryMainWindow();
+	if  (!win->HaveDoc)
+		return;
+
+	ScribusDoc* doc = win->doc;
+
+	auto currItem = Marks::Utils::getTextFrameFromPage(doc);
+	if (!currItem)
+		return;
+
+	Marks::AnchorDialog dialog(win);
+	if (!dialog.exec())
+		return;
+
+	QString label = dialog.getLabel();
+	getUniqueName(label, doc->marksLabelsList(MARKAnchorType), "_");
+
+	insert(currItem, label);
+
+	Marks::Utils::refreshView(currItem);
+}
+
+}
diff --git a/scribus/marks/anchor.h b/scribus/marks/anchor.h
new file mode 100644
index 0000000000000000000000000000000000000000..4b761566ca976bd5ae93b64e55adb818db91a373
--- /dev/null
+++ b/scribus/marks/anchor.h
@@ -0,0 +1,28 @@
+#ifndef MARKSANCHOR_H
+#define MARKSANCHOR_H
+
+#include <QObject>
+#include <QString>
+#include "scribusapi.h"
+
+class PageItem_TextFrame;
+
+namespace Marks {
+
+class SCRIBUS_API Anchor : public QObject
+{
+    Q_OBJECT
+
+private:
+	explicit Anchor() = default;
+	~Anchor() = default;
+
+public:
+	static void insert(PageItem_TextFrame* pageItem, QString& label);
+public slots:
+	static void insertSlot();
+};
+
+}
+
+#endif // MARKSANCHOR_H
diff --git a/scribus/marks/anchordialog.cpp b/scribus/marks/anchordialog.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..805b83f1349c6e3408c77e57b0da6ca5c7aecfb2
--- /dev/null
+++ b/scribus/marks/anchordialog.cpp
@@ -0,0 +1,24 @@
+#include "marks/anchordialog.h"
+#include "marks/utils.h"
+
+namespace Marks {
+
+AnchorDialog::AnchorDialog(QWidget *parent) :
+	QDialog(parent),
+	ui{new Ui::AnchorDialog}
+{
+	ui->setupUi(this);
+}
+
+QString AnchorDialog::getLabel()
+{
+	auto label = ui->labelEdit->text();
+	return label.isEmpty() ? tr("Anchor mark") : label;
+}
+
+void AnchorDialog::setLabel(const QString& label)
+{
+	ui->labelEdit->setText(label);
+}
+
+}
diff --git a/scribus/marks/anchordialog.h b/scribus/marks/anchordialog.h
new file mode 100644
index 0000000000000000000000000000000000000000..042e1ea47b498d9858d3aebf6d0a9d28a03ab33e
--- /dev/null
+++ b/scribus/marks/anchordialog.h
@@ -0,0 +1,31 @@
+#ifndef MARKSANCHORDIALOG_H
+#define MARKSANCHORDIALOG_H
+
+#include <QDialog>
+#include "scribusapi.h"
+#include "marks/ui_anchordialog.h"
+
+
+namespace Marks {
+
+namespace Ui { class AnchorDialog; }
+
+
+class SCRIBUS_API AnchorDialog : public QDialog
+{
+    Q_OBJECT
+
+public:
+	explicit AnchorDialog(QWidget *parent = nullptr);
+	~AnchorDialog() {}
+
+	QString getLabel();
+	void setLabel(const QString& label);
+
+private:
+	Ui::AnchorDialog *ui;
+};
+
+}
+
+#endif // MARKSANCHORDIALOG_H
diff --git a/scribus/marks/anchordialog.ui b/scribus/marks/anchordialog.ui
new file mode 100644
index 0000000000000000000000000000000000000000..4913b08bb3035ef1fad3f753fa6a73dcaceaf6b2
--- /dev/null
+++ b/scribus/marks/anchordialog.ui
@@ -0,0 +1,92 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ui version="4.0">
+ <class>Marks::AnchorDialog</class>
+ <widget class="QDialog" name="Marks::AnchorDialog">
+  <property name="geometry">
+   <rect>
+    <x>0</x>
+    <y>0</y>
+    <width>400</width>
+    <height>81</height>
+   </rect>
+  </property>
+  <property name="sizePolicy">
+   <sizepolicy hsizetype="Preferred" vsizetype="Fixed">
+    <horstretch>0</horstretch>
+    <verstretch>0</verstretch>
+   </sizepolicy>
+  </property>
+  <property name="minimumSize">
+   <size>
+    <width>0</width>
+    <height>81</height>
+   </size>
+  </property>
+  <property name="maximumSize">
+   <size>
+    <width>16777215</width>
+    <height>81</height>
+   </size>
+  </property>
+  <property name="windowTitle">
+   <string>Insert new Anchor</string>
+  </property>
+  <layout class="QGridLayout" name="gridLayout">
+   <item row="0" column="0">
+    <widget class="QLabel" name="label">
+     <property name="text">
+      <string>Label:</string>
+     </property>
+    </widget>
+   </item>
+   <item row="0" column="1">
+    <widget class="QLineEdit" name="labelEdit"/>
+   </item>
+   <item row="1" column="1">
+    <widget class="QDialogButtonBox" name="buttonBox">
+     <property name="orientation">
+      <enum>Qt::Horizontal</enum>
+     </property>
+     <property name="standardButtons">
+      <set>QDialogButtonBox::Cancel|QDialogButtonBox::Ok</set>
+     </property>
+    </widget>
+   </item>
+  </layout>
+ </widget>
+ <resources/>
+ <connections>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>accepted()</signal>
+   <receiver>Marks::AnchorDialog</receiver>
+   <slot>accept()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>248</x>
+     <y>254</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>157</x>
+     <y>274</y>
+    </hint>
+   </hints>
+  </connection>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>rejected()</signal>
+   <receiver>Marks::AnchorDialog</receiver>
+   <slot>reject()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>316</x>
+     <y>260</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>286</x>
+     <y>274</y>
+    </hint>
+   </hints>
+  </connection>
+ </connections>
+</ui>
diff --git a/scribus/marks/utils.cpp b/scribus/marks/utils.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..c1df04656151aabf4f148993a59ad79d24c975fc
--- /dev/null
+++ b/scribus/marks/utils.cpp
@@ -0,0 +1,45 @@
+#include "utils.h"
+
+#include "scribuscore.h"
+#include "scribus.h"
+#include "scribusdoc.h"
+#include "pageitem.h"
+#include "pageitem_textframe.h"
+#include "selection.h"
+#include "scribusview.h"
+
+namespace Marks {
+
+PageItem_TextFrame* Utils::getTextFrame(ScribusDoc* doc)
+{
+	if (doc->m_Selection->count() != 1)
+		return nullptr;
+	if  (doc->appMode != modeEdit)
+		return nullptr;
+	auto currItem = doc->m_Selection->itemAt(0);
+	if (!currItem->isTextFrame())
+		return nullptr;
+	return currItem->asTextFrame();
+}
+
+PageItem_TextFrame* Utils::getTextFrameFromPage(ScribusDoc* doc)
+{
+	if (doc->masterPageMode())
+		return nullptr;
+	return getTextFrame(doc);
+}
+
+void Utils::refreshView(PageItem_TextFrame* textFrame)
+{
+	auto doc = textFrame->doc();
+	auto view = ScCore->primaryMainWindow()->view;
+
+	view->updatesOn(false);
+	textFrame->invalidateLayout();
+	textFrame->layout();
+	doc->changed();
+	view->updatesOn(true);
+	view->DrawNew();
+}
+
+}
diff --git a/scribus/marks/utils.h b/scribus/marks/utils.h
new file mode 100644
index 0000000000000000000000000000000000000000..3a4dcb7c95fa1eb09f3b2b6aac25d27703be97a8
--- /dev/null
+++ b/scribus/marks/utils.h
@@ -0,0 +1,22 @@
+#ifndef MARKSUTIL_H
+#define MARKSUTIL_H 
+
+class PageItem_TextFrame;
+class ScribusDoc;
+
+namespace Marks {
+
+class Utils
+{
+public:
+	/** @brief return the currently selected text frame on the page or master page*/
+	static PageItem_TextFrame* getTextFrame(ScribusDoc* doc);
+	/** @brief return the currently selected text frame on the page */
+	static PageItem_TextFrame* getTextFrameFromPage(ScribusDoc* doc);
+	/** @brief refresh the layout */
+	static void refreshView(PageItem_TextFrame* textFrame);
+};
+
+}
+
+#endif // Marks
diff --git a/scribus/scribus.h b/scribus/scribus.h
index 71cee3f2ba1c243606c81840f09d78a74dfaf8cc..45c90bbd16c43a2100df6007b1fe70689baa7c89 100644
--- a/scribus/scribus.h
+++ b/scribus/scribus.h
@@ -565,7 +565,6 @@ public slots:
 
 	//inserting marks
 	void slotInsertMark2Mark() { insertMark(MARK2MarkType); }
-	void slotInsertMarkAnchor() { insertMark(MARKAnchorType); }
 	void slotInsertMarkVariableText() { insertMark(MARKVariableTextType); }
 	void slotInsertMarkItem() { insertMark(MARK2ItemType); }
 	void slotInsertMarkNote();
anchor-refactoring-02.diff (13,547 bytes)   

ale

2020-07-21 09:23

manager   ~0047866

concerning the static functions:

currently two of them should go to scribusdoc (get a frame if some conditions are met; i have a few ideas to be discussed and tested on how to make them more general) and the third one to pageitem (refresh the document around a page item).
and, of course, they should not be static there.

those are boilerplate that is repeated over and over in scribus and imo distract from the actual logic of the code.
at the end there might be no Utils class at all (and i plan to rename it to Marks::Marks anyway).

ale

2020-07-21 15:13

manager   ~0047867

new patch that takes into consideration craig's concern and a few other things i've noticed or have been discussed.

please see the commits in the gitlab merge request for the details.
marks-refactoring-03.diff (13,698 bytes)    
diff --git a/scribus/CMakeLists.txt b/scribus/CMakeLists.txt
index 33c757c1581ca747cb9ec97c6cf2ef38302cac92..d4c6ace2b3e2a42e36e09d8e76755a9939639a16 100644
--- a/scribus/CMakeLists.txt
+++ b/scribus/CMakeLists.txt
@@ -34,6 +34,7 @@ add_subdirectory(dtd)
 add_subdirectory(colormgmt)
 add_subdirectory(desaxe)
 add_subdirectory(fonts)
+add_subdirectory(marks)
 add_subdirectory(styles)
 add_subdirectory(text)
 add_subdirectory(ui/qml)
@@ -1127,6 +1128,7 @@ link_directories(
 	${CMAKE_CURRENT_BINARY_DIR}/colormgmt
 	${CMAKE_CURRENT_BINARY_DIR}/desaxe
 	${CMAKE_CURRENT_BINARY_DIR}/fonts
+	${CMAKE_CURRENT_BINARY_DIR}/marks
 	${CMAKE_CURRENT_BINARY_DIR}/styles
 	${CMAKE_CURRENT_BINARY_DIR}/text
 	${CMAKE_CURRENT_BINARY_DIR}/third_party/wpg
@@ -1156,6 +1158,7 @@ if(WITH_TESTS)
 else()
 	set(SCRIBUS_TESTS_LIB "")
 endif()
+set(SCRIBUS_MARKS_LIB "scribus_marks_lib")
 set(SCRIBUS_STYLES_LIB "scribus_styles_lib")
 set(SCRIBUS_FONTS_LIB "scribus_fonts_lib")
 set(SCRIBUS_DESAXE_LIB "scribus_desaxe_lib")
@@ -1225,6 +1228,7 @@ if(HAVE_OSG)
 		${SCRIBUS_MAIN_CPP}
 		${SCRIBUS_UI_SOURCES}
 		${SCRIBUS_TEXT_SOURCES}
+		${SCRIBUS_MARKS_SOURCES}
 		${SCRIBUS_STYLES_SOURCES}
 		${SCRIBUS_COLORMGMT_SOURCES}
 		${SCRIBUS_FONTS_SOURCES}
@@ -1240,6 +1244,7 @@ else()
 		${SCRIBUS_MAIN_CPP}
 		${SCRIBUS_UI_SOURCES}
 		${SCRIBUS_TEXT_SOURCES}
+		${SCRIBUS_MARKS_SOURCES}
 		${SCRIBUS_STYLES_SOURCES}
 		${SCRIBUS_COLORMGMT_SOURCES}
 		${SCRIBUS_FONTS_SOURCES}
@@ -1356,6 +1361,7 @@ target_link_libraries(${EXE_NAME}
 	${SCRIBUS_COLORMGMT_LIB}
 	${SCRIBUS_DESAXE_LIB}
 	${SCRIBUS_TEXT_LIB}
+	${SCRIBUS_MARKS_LIB}
 	${SCRIBUS_STYLES_LIB}
 	${SCRIBUS_FONTS_LIB}
 	${SCRIBUS_WPG_LIB}
diff --git a/scribus/actionmanager.cpp b/scribus/actionmanager.cpp
index 13328224ada0795d77f972e61a9dc9b0bda7bb36..bad1a36bbfa4728730bc944d0dcceb82baf1a6ab 100644
--- a/scribus/actionmanager.cpp
+++ b/scribus/actionmanager.cpp
@@ -31,6 +31,8 @@ for which a new license (GPL+exception) is in place.
 #include "urllauncher.h"
 #include "iconmanager.h"
 
+#include "marks/anchor.h"
+
 QMap<QString, QKeySequence> ActionManager::defKeys;
 QVector< QPair<QString, QStringList> > ActionManager::defMenuNames;
 QVector< QPair<QString, QStringList> > ActionManager::defMenus;
@@ -582,7 +584,7 @@ void ActionManager::initInsertMenuActions()
 	connect( (*scrActions)["insertSampleText"], SIGNAL(triggered()), mainWindow, SLOT(insertSampleText()) );
 	connect( (*scrActions)["stickyTools"], SIGNAL(triggered()), mainWindow, SLOT(ToggleStickyTools()) );
 
-	connect( (*scrActions)["insertMarkAnchor"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkAnchor()) );
+	connect((*scrActions)["insertMarkAnchor"], &ScrAction::triggered, [](){ Marks::Anchor::insertSlot(); });
 	connect( (*scrActions)["insertMarkVariableText"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkVariableText()) );
 	connect( (*scrActions)["insertMarkItem"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMarkItem()) );
 	connect( (*scrActions)["insertMark2Mark"], SIGNAL(triggered()), mainWindow, SLOT(slotInsertMark2Mark()) );
diff --git a/scribus/marks/CMakeLists.txt b/scribus/marks/CMakeLists.txt
new file mode 100644
index 0000000000000000000000000000000000000000..ebf06a1f91a7ee7803e725d89ed646e5cdb4b25a
--- /dev/null
+++ b/scribus/marks/CMakeLists.txt
@@ -0,0 +1,36 @@
+include_directories(
+	${CMAKE_SOURCE_DIR}
+	${CMAKE_SOURCE_DIR}/scribus
+	${FREETYPE_INCLUDE_DIRS}
+)
+
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_FPIC}")
+
+set(SCRIBUS_MARKS_MOC_CLASSES
+	anchor.h
+	anchordialog.h
+)
+
+set(SCRIBUS_MARKS_UI_SRC
+	anchordialog.ui
+)
+
+set(SCRIBUS_MARKS_LIB_SOURCES
+	marks.cpp
+	anchor.cpp
+	anchordialog.cpp
+)
+
+QT5_WRAP_CPP(SCRIBUS_MARKS_MOC_SOURCES ${SCRIBUS_MARKS_MOC_CLASSES})
+QT5_WRAP_UI(SCRIBUS_MARKS_UI_SOURCES ${SCRIBUS_MARKS_UI_SRC})
+
+set(SCRIBUS_MARKS_LIB "scribus_marks_lib")
+add_library(${SCRIBUS_MARKS_LIB} STATIC ${SCRIBUS_MARKS_LIB_SOURCES} ${SCRIBUS_MARKS_MOC_SOURCES} ${SCRIBUS_MARKS_UI_SOURCES})
+
+# This is a convenience library that for linkage purposes is part of Scribus's
+# main API.
+set_target_properties(${SCRIBUS_MARKS_LIB}
+	PROPERTIES
+	COMPILE_FLAGS -DCOMPILE_SCRIBUS_MAIN_APP
+)
+
diff --git a/scribus/marks/anchor.cpp b/scribus/marks/anchor.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..5f7223e01ac9a8bc3fb027b277163075590620c4
--- /dev/null
+++ b/scribus/marks/anchor.cpp
@@ -0,0 +1,80 @@
+#include "marks/anchor.h"
+
+#include "scribuscore.h"
+#include "scribus.h"
+#include "scribusdoc.h"
+#include "undomanager.h"
+#include "pageitem_textframe.h"
+#include "util.h"
+
+#include "marks/marks.h"
+#include "marks/anchordialog.h"
+
+namespace Marks {
+
+bool Anchor::insert(PageItem_TextFrame* textFrame, QString& label)
+{
+	auto doc = textFrame->doc();
+	auto undoManager = UndoManager::instance();
+
+	// there cannot be bookmarks on master pages
+	if (doc->masterPageMode())
+		return false;
+
+	auto trans = undoManager->beginTransaction();
+	// TODO: if there is a selection use it as the label, do not delete it
+	if (textFrame->HasSel)
+	{
+		trans = undoManager->beginTransaction(Um::Selection, Um::IDelete, Um::Delete, QString(), Um::IDelete);
+		textFrame->deleteSelectedTextFromFrame();
+	}
+
+	MarkData markdata;
+	markdata.itemName = textFrame->itemName();
+	markdata.itemPtr = textFrame;
+
+	Mark* mrk = doc->newMark();
+	mrk->setValues(label, textFrame->OwnPage, MARKAnchorType, markdata);
+	// TODO: remove selection, insert mark, restore the selection
+	textFrame->itemText.insertMark(mrk);
+	mrk->OwnPage = textFrame->OwnPage;
+
+	if (undoManager->undoEnabled())
+	{
+		ScItemsState* is = new ScItemsState(undoManager->InsertMark);
+		is->set("ETEA", mrk->label);
+		is->set("label", mrk->label);
+		is->set("type", (int) mrk->getType());
+		is->set("MARK", QString("new"));
+		is->set("strtxt", mrk->getString());
+		is->set("at", textFrame->itemText.cursorPosition() -1);
+		is->insertItem("inItem", textFrame);
+		undoManager->action(doc, is);
+
+		trans.commit();
+	}
+	return true;
+}
+
+void Anchor::insertSlot()
+{
+	auto win = ScCore->primaryMainWindow();
+	if  (!win->HaveDoc)
+		return;
+
+	auto currItem = Marks::getTextFrameFromPage(win->doc);
+	if (!currItem)
+		return;
+
+	AnchorDialog dialog(win);
+	if (!dialog.exec())
+		return;
+
+	QString label = dialog.getLabel();
+	getUniqueName(label, win->doc->marksLabelsList(MARKAnchorType), "_");
+
+	if (insert(currItem, label))
+		Marks::refreshView(currItem);
+}
+
+}
diff --git a/scribus/marks/anchor.h b/scribus/marks/anchor.h
new file mode 100644
index 0000000000000000000000000000000000000000..a6709fb8da79ba4b735ea7875a0ce21bf3ab0edb
--- /dev/null
+++ b/scribus/marks/anchor.h
@@ -0,0 +1,29 @@
+#ifndef MARKSANCHOR_H
+#define MARKSANCHOR_H
+
+#include <QObject>
+#include <QString>
+#include "scribusapi.h"
+
+class PageItem_TextFrame;
+
+namespace Marks {
+
+class SCRIBUS_API Anchor : public QObject
+{
+    Q_OBJECT
+
+private:
+	Anchor() = default;
+	~Anchor() = default;
+
+public:
+	/** \return true if the mark was insterted */
+	static bool insert(PageItem_TextFrame* pageItem, QString& label);
+public slots:
+	static void insertSlot();
+};
+
+}
+
+#endif // MARKSANCHOR_H
diff --git a/scribus/marks/anchordialog.cpp b/scribus/marks/anchordialog.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..c418ca2c97cc7e077d368148d469aa2d84050eb9
--- /dev/null
+++ b/scribus/marks/anchordialog.cpp
@@ -0,0 +1,29 @@
+#include "marks/anchordialog.h"
+#include "marks/marks.h"
+
+namespace Marks {
+
+AnchorDialog::AnchorDialog(QWidget *parent) :
+	QDialog(parent),
+	ui{new Ui::AnchorDialog}
+{
+	ui->setupUi(this);
+}
+
+AnchorDialog::~AnchorDialog()
+{
+	delete ui;
+}
+
+QString AnchorDialog::getLabel()
+{
+	auto label = ui->labelEdit->text();
+	return label.isEmpty() ? tr("Anchor mark") : label;
+}
+
+void AnchorDialog::setLabel(const QString& label)
+{
+	ui->labelEdit->setText(label);
+}
+
+}
diff --git a/scribus/marks/anchordialog.h b/scribus/marks/anchordialog.h
new file mode 100644
index 0000000000000000000000000000000000000000..7558f30fd0b5433d4a63710a8c4bbf9e999f58a1
--- /dev/null
+++ b/scribus/marks/anchordialog.h
@@ -0,0 +1,31 @@
+#ifndef MARKSANCHORDIALOG_H
+#define MARKSANCHORDIALOG_H
+
+#include <QDialog>
+#include "scribusapi.h"
+#include "marks/ui_anchordialog.h"
+
+
+namespace Marks {
+
+namespace Ui { class AnchorDialog; }
+
+
+class SCRIBUS_API AnchorDialog : public QDialog
+{
+    Q_OBJECT
+
+public:
+	AnchorDialog(QWidget *parent = nullptr);
+	~AnchorDialog();
+
+	QString getLabel();
+	void setLabel(const QString& label);
+
+private:
+	Ui::AnchorDialog *ui;
+};
+
+}
+
+#endif // MARKSANCHORDIALOG_H
diff --git a/scribus/marks/anchordialog.ui b/scribus/marks/anchordialog.ui
new file mode 100644
index 0000000000000000000000000000000000000000..4913b08bb3035ef1fad3f753fa6a73dcaceaf6b2
--- /dev/null
+++ b/scribus/marks/anchordialog.ui
@@ -0,0 +1,92 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ui version="4.0">
+ <class>Marks::AnchorDialog</class>
+ <widget class="QDialog" name="Marks::AnchorDialog">
+  <property name="geometry">
+   <rect>
+    <x>0</x>
+    <y>0</y>
+    <width>400</width>
+    <height>81</height>
+   </rect>
+  </property>
+  <property name="sizePolicy">
+   <sizepolicy hsizetype="Preferred" vsizetype="Fixed">
+    <horstretch>0</horstretch>
+    <verstretch>0</verstretch>
+   </sizepolicy>
+  </property>
+  <property name="minimumSize">
+   <size>
+    <width>0</width>
+    <height>81</height>
+   </size>
+  </property>
+  <property name="maximumSize">
+   <size>
+    <width>16777215</width>
+    <height>81</height>
+   </size>
+  </property>
+  <property name="windowTitle">
+   <string>Insert new Anchor</string>
+  </property>
+  <layout class="QGridLayout" name="gridLayout">
+   <item row="0" column="0">
+    <widget class="QLabel" name="label">
+     <property name="text">
+      <string>Label:</string>
+     </property>
+    </widget>
+   </item>
+   <item row="0" column="1">
+    <widget class="QLineEdit" name="labelEdit"/>
+   </item>
+   <item row="1" column="1">
+    <widget class="QDialogButtonBox" name="buttonBox">
+     <property name="orientation">
+      <enum>Qt::Horizontal</enum>
+     </property>
+     <property name="standardButtons">
+      <set>QDialogButtonBox::Cancel|QDialogButtonBox::Ok</set>
+     </property>
+    </widget>
+   </item>
+  </layout>
+ </widget>
+ <resources/>
+ <connections>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>accepted()</signal>
+   <receiver>Marks::AnchorDialog</receiver>
+   <slot>accept()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>248</x>
+     <y>254</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>157</x>
+     <y>274</y>
+    </hint>
+   </hints>
+  </connection>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>rejected()</signal>
+   <receiver>Marks::AnchorDialog</receiver>
+   <slot>reject()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>316</x>
+     <y>260</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>286</x>
+     <y>274</y>
+    </hint>
+   </hints>
+  </connection>
+ </connections>
+</ui>
diff --git a/scribus/marks/marks.cpp b/scribus/marks/marks.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..de35775c3cde39903f378ade0f48bba65f54b46e
--- /dev/null
+++ b/scribus/marks/marks.cpp
@@ -0,0 +1,45 @@
+#include "marks.h"
+
+#include "scribuscore.h"
+#include "scribus.h"
+#include "scribusdoc.h"
+#include "pageitem.h"
+#include "pageitem_textframe.h"
+#include "selection.h"
+#include "scribusview.h"
+
+namespace Marks {
+
+PageItem_TextFrame* Marks::getTextFrame(ScribusDoc* doc)
+{
+	if (doc->m_Selection->count() != 1)
+		return nullptr;
+	if  (doc->appMode != modeEdit)
+		return nullptr;
+	auto currItem = doc->m_Selection->itemAt(0);
+	if (!currItem->isTextFrame())
+		return nullptr;
+	return currItem->asTextFrame();
+}
+
+PageItem_TextFrame* Marks::getTextFrameFromPage(ScribusDoc* doc)
+{
+	if (doc->masterPageMode())
+		return nullptr;
+	return getTextFrame(doc);
+}
+
+void Marks::refreshView(PageItem_TextFrame* textFrame)
+{
+	auto doc = textFrame->doc();
+	auto view = ScCore->primaryMainWindow()->view;
+
+	view->updatesOn(false);
+	textFrame->invalidateLayout();
+	textFrame->layout();
+	doc->changed();
+	view->updatesOn(true);
+	view->DrawNew();
+}
+
+}
diff --git a/scribus/marks/marks.h b/scribus/marks/marks.h
new file mode 100644
index 0000000000000000000000000000000000000000..020c7561713dc5c39340586fff88add918cae006
--- /dev/null
+++ b/scribus/marks/marks.h
@@ -0,0 +1,22 @@
+#ifndef MARKSMARKS_H
+#define MARKSMARKS_H
+
+class PageItem_TextFrame;
+class ScribusDoc;
+
+namespace Marks {
+
+class Marks
+{
+public:
+	/** @brief return the currently selected text frame on the page or master page*/
+	static PageItem_TextFrame* getTextFrame(ScribusDoc* doc);
+	/** @brief return the currently selected text frame on the page */
+	static PageItem_TextFrame* getTextFrameFromPage(ScribusDoc* doc);
+	/** @brief refresh the layout */
+	static void refreshView(PageItem_TextFrame* textFrame);
+};
+
+}
+
+#endif // Marks
diff --git a/scribus/scribus.h b/scribus/scribus.h
index 71cee3f2ba1c243606c81840f09d78a74dfaf8cc..45c90bbd16c43a2100df6007b1fe70689baa7c89 100644
--- a/scribus/scribus.h
+++ b/scribus/scribus.h
@@ -565,7 +565,6 @@ public slots:
 
 	//inserting marks
 	void slotInsertMark2Mark() { insertMark(MARK2MarkType); }
-	void slotInsertMarkAnchor() { insertMark(MARKAnchorType); }
 	void slotInsertMarkVariableText() { insertMark(MARKVariableTextType); }
 	void slotInsertMarkItem() { insertMark(MARK2ItemType); }
 	void slotInsertMarkNote();
marks-refactoring-03.diff (13,698 bytes)   

cbradney

2020-08-11 20:55

administrator   ~0047923

This seems to replace just the code for Anchors. Are you planning on something similar for VariableText/Item/Notes?

ale

2020-08-12 07:13

manager   ~0047924

obviously yes, @cbradney . this is how i plan to do it for all marks.

but before going on i need an ok on the way it is done and, if possible the code being merged so that i can continue my work on the pdf bookmarks without fearing big merge conflicts.

jghali

2020-08-18 16:37

administrator   ~0047963

Last edited: 2020-08-18 16:38

 1) please remove "namespace Marks" stuff, it's useless
2) I don't like api such as "bool Anchor::insert(PageItem_TextFrame* textFrame, QString& label)", it is the Anchor which is inserted into text frame so should be something like bool PageItem_TextFrame::insert(Anchor* anchor, QString& label)
3) The undo stuff in Anchor::insert() is incorrect as any call of UndoManager::beginTransaction() must be balanced by a call to UndoManager::commit(). The first call to beginTransaction() in particular should receive as arguments the user description of the undo action. The second call to beginTransaction() in the "if (textFrame->HasSel)" block is then useless. Move trans.commit() out of if (undoManager->undoEnabled()) block and replace by "if (trans) trans.commit()".

ale

2020-08-18 17:10

manager   ~0047966

Last edited: 2020-08-18 17:13

 1/ of course, namespaces are basically useless.
    but i would welcome it, if every class that is in a directory uses the namespaces as a prefix.
   in this way, one knows that Marks/Anchor is to be found in marks/anchor.h
   the alternative is to rename the class to MarksAnchor as it's otherwise done in scribus. but is it really better?

2/ on the one side i agree with you. it's about the text frame.
     on the other side the code in insert is very specific to the anchor mark.
    and one of the main ideas behind having a directory dedicated to the marks is to separate it from the rest of the code.
   as an example it might allow creating a new type of mark and being and easily all or most of the spots to be changed.
   another beneficial effect of this approach, is that some files with lot of code can be made a bit slimmer.

3/ ok, i'll try to fix it.
  since the undo manager seams to be a part of the code where many error happens, it would be nice to have a documentation on it.
  as an example: how should the user description be composed (if it's not there, i fear that it was not in the original code).

cbradney

2020-08-18 18:31

administrator   ~0047967

I see where Jean is coming from with this comment:
2) I don't like api such as "bool Anchor::insert(PageItem_TextFrame* textFrame, QString& label)", it is the Anchor which is inserted into text frame so should be something like bool PageItem_TextFrame::insert(Anchor* anchor, QString& label)

However, I would prefer that all the Marks code is in that one directory.

jghali

2020-08-18 18:55

administrator   ~0047968

4) Do not re-use the marks.h/cpp file names, there is already files with such names in Scribus/scribus directory. That will prevent compilation with MSVC.

ale

2020-08-18 19:28

manager   ~0047970

jean, are you sure that msvc cannot compile files with the same name but in different directories?

that would remove much of the benefit of using namespaces!
namespaces exist to avoid clashes in the class names but, i think, also for file names.

and it also reduces a small bit the advantage of having multiple sub directories.

anyway, this is just a question of principle, since at the end of the refactoring scribus/marks.h/.cpp will disappear and go into marks/ .

p.s.: is it this problem?
https://stackoverflow.com/questions/9351132/two-files-of-the-same-name-give-linker-error-in-visual-studio

cbradney

2020-08-18 20:42

administrator   ~0047971

MSVC is daft

jghali

2020-08-18 20:59

administrator   ~0047972

>> jean, are you sure that msvc cannot compile files with the same name but in different directories?

Yes.

>> namespaces exist to avoid clashes in the class names but, i think, also for file names.

No, namespaces do not prevent in any way clashes of file names.

>> anyway, this is just a question of principle, since at the end of the refactoring scribus/marks.h/.cpp will disappear and go into marks/ .

I think your Marks class is badly named anyway as it does not represent any kind of mark. MarkHelper or MarksHelper would probably be better. Then your marks.h/cpp would have to be renamed mark(s)helper.h/.cpp and problems goes away. Same for your Anchor class btw, I think AnchorHelper would be more suited.

jghali

2020-08-18 21:03

administrator   ~0047973

>> as an example: how should the user description be composed (if it's not there, i fear that it was not in the original code)

You have plenty of example of use of UndoManager::beginTransaction() in our source code. You may have to add proper Action string(s) to UndoManager class.

jghali

2020-08-18 21:35

administrator   ~0047974

Btw, with that Anchor to AnchorHelper renaming, my point 2. would become moot.

ale

2020-08-19 05:55

manager   ~0047976

https://en.wikipedia.org/wiki/Helper_class

are you sure?

the main goal of the Anchor class is to implement the Anchor functionality.

jean, i'm not trying to nitpick but i'd lke to find a solution that can be used in future code of scribus to improve the readability of the code and help new contributors improve existing features and add new ones.
by having sets of files that implement a feature, in their own directories, with a manageable length (and possibly a doc at their beginning or a README file in there).

concerning the file names in msvc: i tried an internet search for the file name conflict, but i could not find that many references (very few indeed).
if there was no solution, i guess that there should be many more results!
i've pasted above a stackoverflow question and answer that could describe the error you're facing and i'd like to know if this is the issue and if you have tried the solution suggested in the post.

this must be a common issue when you pull in external modules... every programmer creates a utility.h file...
i can't think that there is no solution.

(and yes namespaces help also go get nicer file names: it's a common pattern to use the class name as a file name and the directory name for the namespace: MarksAnchor.cpp becomes Marks/Anchor.cpp ... which is nicer than Marks/MarksAnchor.cpp if you want to use directories and unique filenames / class names)

jghali

2020-08-19 11:56

administrator   ~0047978

>> are you sure?

Yes, I'm sure. Your Anchor class does not represent an anchor as it does not have any data which would make it so. It has only functions. An Anchor is already represented by a Mark object of type MARKAnchorType.

Issue History

Date Modified Username Field Change
2020-07-20 19:19 ale New Issue
2020-07-20 19:19 ale File Added: anchor-refactoring.diff
2020-07-20 21:34 jghali Note Added: 0047860
2020-07-20 21:43 jghali Note Added: 0047862
2020-07-21 07:25 ale File Added: anchor-refactoring-02.diff
2020-07-21 07:25 ale Note Added: 0047863
2020-07-21 07:38 ale Note Edited: 0047863
2020-07-21 09:23 ale Note Added: 0047866
2020-07-21 15:13 ale File Added: marks-refactoring-03.diff
2020-07-21 15:13 ale Note Added: 0047867
2020-08-11 20:55 cbradney Note Added: 0047923
2020-08-12 07:13 ale Note Added: 0047924
2020-08-18 16:37 jghali Note Added: 0047963
2020-08-18 16:37 jghali Note Edited: 0047963
2020-08-18 16:38 jghali Note Edited: 0047963
2020-08-18 17:10 ale Note Added: 0047966
2020-08-18 17:11 ale Note Edited: 0047966
2020-08-18 17:13 ale Note Edited: 0047966
2020-08-18 18:31 cbradney Note Added: 0047967
2020-08-18 18:55 jghali Note Added: 0047968
2020-08-18 19:28 ale Note Added: 0047970
2020-08-18 20:42 cbradney Note Added: 0047971
2020-08-18 20:59 jghali Note Added: 0047972
2020-08-18 21:03 jghali Note Added: 0047973
2020-08-18 21:35 jghali Note Added: 0047974
2020-08-19 05:55 ale Note Added: 0047976
2020-08-19 11:56 jghali Note Added: 0047978