View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0012425ScribusUndo/Redopublic2014-06-23 00:492024-01-02 19:23
ReporterFirasH Assigned To 
PrioritynormalSeveritymajorReproducibilityalways
Status confirmedResolutionopen 
Platformx86_64OSopenSUSEOS Version13.1
Product Version1.5.0svn 
Target Version1.5.5 
Summary0012425: Undo/Redo records wrong entries using the Guide Manager
DescriptionUndo/Redo records wrong entries using the Guide Manager
Steps To Reproduce0) Create a new document and make sure Windows > Action History is opened
1) Go to: Page > Manage Guides...
2) Add a Horizontal Guide (works also with Vertical)
You can notice that in Action History step 2 is not recorded
(there should be "Page X - Add vertical/horizontal guide").

3) Add a second Horizontal Guide
You can notice that a new entry "Page X - Remove horizontal/vertical guide" is added to Action History.

If you double click on one of the entered values to edit its value and keep pressed Tab, each time a new entry is added to Action History (basically "make" the the field editable then deselect it).
-----
I must also say that it is quite "unfriendly" the way guides are added: to apply the guide you need to deselect the field and I don't get why the Add should not do that (instead adds another guide).
Example: when the field is in "edit" mode Add applies the guide (as when the field is deselected), if not adds a new guide.
Additional InformationScribus 1.5.0.svn (19265)

Another bug related to Guide Manager/Action History: 0012308
Tagsguides, HOST-Oman
PatchYes

Relationships

related to 0012308 confirmed Undo/Redo records useless entries deleting guides from Guide Manager 
related to 0015874 new Guides created with the "Guides manager" cannot be undone 
child of 0012500 acknowledged Metabug: Undo/Redo (1.5.x.svn) 
child of 0002935 acknowledgedsubik Metabug: Guide Dialog 

Activities

Kunda

2014-07-19 12:28

updater   ~0032832

Confirmed on 1.5svn r19352
I find that in the "Guide Manager" pressing 'Add' redundantly 2-3 times triggers
'Page X - Remove horizontal/vertical guide' in Action windows

I also agree with FirasH's UX feedback. 'Add' could automagically trigger a new guide without having to deselect the guide that was just added. This way if people are adding several guides they can add a group of them and then individually edit them.

Fahad

2015-10-26 05:16

developer   ~0036935

It seems that guide manager not being integrated well with Undo Manager. There are many UndoTransactions not implemented yet.

Please find my patch to fix this bug. It does the following:

1- add UndoTransactions to following actions:
   - add Vertical guide
   - add Horizontal guide
   - delete Vertical guide
   - delete Horizontal guide
   - Move Vertical guide
   - Move Horizontal guide
2- when addHorButton_clicked() will add a new Horizontal Guide in action history and assign 0 value to it. I apply same behavior to addVerButton_clicked().

3- as requested here, this patch gives you the ability to create many guides that have 0 value, the you can edit them later.

Fahad

2015-10-26 05:16

developer  

12425_patch.diff (5,498 bytes)    
Index: scribus/guidesmodel.cpp
===================================================================
--- scribus/guidesmodel.cpp	(revision 20488)
+++ scribus/guidesmodel.cpp	(working copy)
@@ -121,8 +121,8 @@
 {
 // 	insertRows(rowCount(), 1);
 	beginResetModel();
-	if (m_values.contains(0.0))
-		return;
+//	if (m_values.contains(0.0))
+//		return;
 	m_values.append(0.0);
 	qSort(m_values);
 	endResetModel();
Index: scribus/ui/guidemanager.cpp
===================================================================
--- scribus/ui/guidemanager.cpp	(revision 20488)
+++ scribus/ui/guidemanager.cpp	(working copy)
@@ -294,10 +294,18 @@
 		// see GuidesModel::data()
 		v.append(horizontalModel->data(ix, Qt::EditRole).toDouble());
 
+    UndoTransaction trans;
+    if(UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::DelHGuide, "",
+                                                        Um::IGuides);
 	horizontalModel->removeValues(v);
 
 	currentPage->guides.clearHorizontals(GuideManagerCore::Standard);
 	currentPage->guides.addHorizontals(horizontalModel->values(), GuideManagerCore::Standard);
+    if (trans)
+        trans.commit();
 	drawGuides();
 	m_Doc->changed();
 }
@@ -313,10 +321,18 @@
 		// see GuidesModel::data()
 		v.append(verticalModel->data(ix, Qt::EditRole).toDouble());
 
+    UndoTransaction trans;
+    if(UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::DelVGuide, "",
+                                                        Um::IGuides);
+
 	verticalModel->removeValues(v);
-
 	currentPage->guides.clearVerticals(GuideManagerCore::Standard);
 	currentPage->guides.addVerticals(verticalModel->values(), GuideManagerCore::Standard);
+    if (trans)
+        trans.commit();
 	drawGuides();
 	m_Doc->changed();
 }
@@ -323,16 +339,41 @@
 
 void GuideManager::addHorButton_clicked()
 {
+    UndoTransaction trans;
+    if(UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::AddHGuide, "",
+                                                        Um::IGuides);
 	horizontalModel->insertRow();
-	horizontalView->setCurrentIndex(horizontalModel->index(0, 0, QModelIndex()));
-	horizontalView->edit(horizontalView->currentIndex());
+    currentPage->guides.clearHorizontals(GuideManagerCore::Standard);
+    currentPage->guides.addHorizontal(0.0, GuideManagerCore::Standard);
+
+    if (trans)
+        trans.commit();
+
+    drawGuides();
+    m_Doc->changed();
+//	horizontalView->setCurrentIndex(horizontalModel->index(0, 0, QModelIndex()));
+//	horizontalView->edit(horizontalView->currentIndex());
 }
 
 void GuideManager::addVerButton_clicked()
 {
+    UndoTransaction trans;
+    if(UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::AddVGuide, "",
+                                                        Um::IGuides);
+
 	verticalModel->insertRow();
-	verticalView->setCurrentIndex(verticalModel->index(0, 0, QModelIndex()));
-	verticalView->edit(verticalView->currentIndex());
+    currentPage->guides.clearVerticals(GuideManagerCore::Standard);
+    currentPage->guides.addVertical(0.0, GuideManagerCore::Standard);
+    if(trans)
+        trans.commit();
+//    verticalView->setCurrentIndex(verticalModel->index(0, 0, QModelIndex()));
+//	verticalView->edit(verticalView->currentIndex());
 }
 
 void GuideManager::lockCheck_stateChanged( int )
@@ -626,8 +667,17 @@
 
 void GuideManager::verticalModel_valueChanged()
 {
+    UndoTransaction trans;
+    if (UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::MoveVGuide, "",
+                                                        Um::IGuides);
 	currentPage->guides.clearVerticals(GuideManagerCore::Standard);
 	currentPage->guides.addVerticals(verticalModel->values(), GuideManagerCore::Standard);
+
+    if (trans)
+        trans.commit();
 	drawGuides();
 	m_Doc->changed();
 }
@@ -634,8 +684,17 @@
 
 void GuideManager::horizontalModel_valueChanged()
 {
+    UndoTransaction trans;
+    if (UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::MoveHGuide, "",
+                                                        Um::IGuides);
 	currentPage->guides.clearHorizontals(GuideManagerCore::Standard);
 	currentPage->guides.addHorizontals(horizontalModel->values(), GuideManagerCore::Standard);
+    if (trans)
+        trans.commit();
+
 	drawGuides();
 	m_Doc->changed();
 }
12425_patch.diff (5,498 bytes)   

JLuc

2015-10-26 19:58

developer   ~0036951

Last edited: 2015-10-26 20:01

 Hello Fahad

I tested your patch and redo/undo situation has improved, and the UX is improved also.

But history management (undo/redo) doesnt behave as expected.

Example :
A- create guide (0)
B- set it to 10
C- create second guide
D- Undo
see : 2nd guide has disappeared : OK

E- Undo
see : 2nd guide is back there : wrong !

F- Undo = supposed to be back to original state
see : there is one guide left (the 2nd one, still 0)

Can you improve that ?

Fahad

2015-10-27 11:29

developer   ~0036979

The problem here is in :
void GuideManager::horizontalModel_valueChanged()
void GuideManager::horizontalModel_valueChanged()

I tried to modified valueChanged() signal to catch from & to values then I pass the info to horizontalModel_valueChanged() & horizontalModel_valueChanged() but without success.

I am stuck here & I need help or hints about what I should do.

Fahad

2015-10-27 11:49

developer  

second_try.diff (7,021 bytes)    
Index: scribus/guidesmodel.cpp
===================================================================
--- scribus/guidesmodel.cpp	(revision 20488)
+++ scribus/guidesmodel.cpp	(working copy)
@@ -66,6 +66,7 @@
 	if (role != Qt::EditRole || !index.isValid())
 		return false;
 	bool ok;
+    double from = data(index,role).toDouble();
 	double newVal = value.toDouble(&ok);
 	if (!ok)
 		return false;
@@ -72,7 +73,7 @@
 	m_values[index.row()] = value2pts(newVal, m_docUnitIndex) + rule;
 	qSort(m_values);
 	emit dataChanged(index, index);
-	emit valueChanged();
+    emit valueChanged(from,newVal);
 	return true;
 }
 
@@ -121,8 +122,8 @@
 {
 // 	insertRows(rowCount(), 1);
 	beginResetModel();
-	if (m_values.contains(0.0))
-		return;
+//	if (m_values.contains(0.0))
+//		return;
 	m_values.append(0.0);
 	qSort(m_values);
 	endResetModel();
Index: scribus/guidesmodel.h
===================================================================
--- scribus/guidesmodel.h	(revision 20488)
+++ scribus/guidesmodel.h	(working copy)
@@ -58,8 +58,7 @@
 	signals:
 		/*! \brief Signal emmitted when the user finish the editation of one value.
 		*/
-		void valueChanged();
-
+        void valueChanged(double from, double to);
 	private:
 		Guides m_values;
 		int m_docUnitIndex;
Index: scribus/ui/guidemanager.cpp
===================================================================
--- scribus/ui/guidemanager.cpp	(revision 20488)
+++ scribus/ui/guidemanager.cpp	(working copy)
@@ -97,8 +97,8 @@
 
 	connect(lockCheck, SIGNAL(stateChanged(int)), this, SLOT(lockCheck_stateChanged(int)));
 
-	connect(horizontalModel, SIGNAL(valueChanged()), this, SLOT(horizontalModel_valueChanged()));
-	connect(verticalModel, SIGNAL(valueChanged()), this, SLOT(verticalModel_valueChanged()));
+    connect(horizontalModel, SIGNAL(valueChanged(double, double)), this, SLOT(horizontalModel_valueChanged(double, double)));
+    connect(verticalModel, SIGNAL(valueChanged(double, double)), this, SLOT(verticalModel_valueChanged(double, double)));
 }
 
 void GuideManager::setDoc(ScribusDoc* doc)
@@ -294,10 +294,18 @@
 		// see GuidesModel::data()
 		v.append(horizontalModel->data(ix, Qt::EditRole).toDouble());
 
+    UndoTransaction trans;
+    if(UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::DelHGuide, "",
+                                                        Um::IGuides);
 	horizontalModel->removeValues(v);
 
 	currentPage->guides.clearHorizontals(GuideManagerCore::Standard);
 	currentPage->guides.addHorizontals(horizontalModel->values(), GuideManagerCore::Standard);
+    if (trans)
+        trans.commit();
 	drawGuides();
 	m_Doc->changed();
 }
@@ -313,10 +321,18 @@
 		// see GuidesModel::data()
 		v.append(verticalModel->data(ix, Qt::EditRole).toDouble());
 
+    UndoTransaction trans;
+    if(UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::DelVGuide, "",
+                                                        Um::IGuides);
+
 	verticalModel->removeValues(v);
-
 	currentPage->guides.clearVerticals(GuideManagerCore::Standard);
 	currentPage->guides.addVerticals(verticalModel->values(), GuideManagerCore::Standard);
+    if (trans)
+        trans.commit();
 	drawGuides();
 	m_Doc->changed();
 }
@@ -323,16 +339,41 @@
 
 void GuideManager::addHorButton_clicked()
 {
+    UndoTransaction trans;
+    if(UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::AddHGuide, "",
+                                                        Um::IGuides);
 	horizontalModel->insertRow();
-	horizontalView->setCurrentIndex(horizontalModel->index(0, 0, QModelIndex()));
-	horizontalView->edit(horizontalView->currentIndex());
+    currentPage->guides.clearHorizontals(GuideManagerCore::Standard);
+    currentPage->guides.addHorizontal(0.0, GuideManagerCore::Standard);
+
+    if (trans)
+        trans.commit();
+
+    drawGuides();
+    m_Doc->changed();
+//	horizontalView->setCurrentIndex(horizontalModel->index(0, 0, QModelIndex()));
+//	horizontalView->edit(horizontalView->currentIndex());
 }
 
 void GuideManager::addVerButton_clicked()
 {
+    UndoTransaction trans;
+    if(UndoManager::undoEnabled())
+        trans = UndoManager::instance()->beginTransaction(currentPage->getUName(),
+                                                        currentPage->getUPixmap(),
+                                                        Um::AddVGuide, "",
+                                                        Um::IGuides);
+
 	verticalModel->insertRow();
-	verticalView->setCurrentIndex(verticalModel->index(0, 0, QModelIndex()));
-	verticalView->edit(verticalView->currentIndex());
+    currentPage->guides.clearVerticals(GuideManagerCore::Standard);
+    currentPage->guides.addVertical(0.0, GuideManagerCore::Standard);
+    if(trans)
+        trans.commit();
+//    verticalView->setCurrentIndex(verticalModel->index(0, 0, QModelIndex()));
+//	verticalView->edit(verticalView->currentIndex());
 }
 
 void GuideManager::lockCheck_stateChanged( int )
@@ -624,18 +665,17 @@
 	}
 }
 
-void GuideManager::verticalModel_valueChanged()
+void GuideManager::verticalModel_valueChanged(double from, double to)
 {
-	currentPage->guides.clearVerticals(GuideManagerCore::Standard);
-	currentPage->guides.addVerticals(verticalModel->values(), GuideManagerCore::Standard);
+
+    currentPage->guides.moveVertical(from,to,GuideManagerCore::Standard);
 	drawGuides();
 	m_Doc->changed();
 }
 
-void GuideManager::horizontalModel_valueChanged()
+void GuideManager::horizontalModel_valueChanged(double from, double to)
 {
-	currentPage->guides.clearHorizontals(GuideManagerCore::Standard);
-	currentPage->guides.addHorizontals(horizontalModel->values(), GuideManagerCore::Standard);
+    currentPage->guides.moveHorizontal(from,to,GuideManagerCore::Standard);
 	drawGuides();
 	m_Doc->changed();
 }
Index: scribus/ui/guidemanager.h
===================================================================
--- scribus/ui/guidemanager.h	(revision 20488)
+++ scribus/ui/guidemanager.h	(working copy)
@@ -134,8 +134,8 @@
 private slots:
 	//! Wrapper slot for drawGuides()
 	void forceDrawGuides(const QItemSelection &, const QItemSelection &);
-	void verticalModel_valueChanged();
-	void horizontalModel_valueChanged();
+    void verticalModel_valueChanged(double from, double to);
+    void horizontalModel_valueChanged(double from, double to);
 	void addHorButton_clicked();
 	void delHorButton_clicked();
 	void addVerButton_clicked();
second_try.diff (7,021 bytes)   

Fahad

2015-10-27 11:50

developer   ~0036980

I attached my second try. It will redo whatever recorded but I couldn't figure out how to redraw all guides again in page view yet. I appreciate any help.

JLuc

2015-10-27 13:03

developer   ~0036985

In the source code i read :

if (m_ScMW->doc->appMode != modeNormal)
        m_canvas->setForcedRedraw(true);
m_canvas->repaint();


wouldnt such calls help refresh view ?

JLuc

2015-11-04 19:30

developer   ~0037223

Last edited: 2015-11-04 19:34

 Fahad : i tested the patch and it improves the situation but is still not OK.

To test, i created 3 guides

1) it happens quite often that the guide appearing on top of the list cannot be deleted

2) when undoing, a setting is changed and takes a strange value : it becomes 5.380 when i only did set rounded values.

3) coming back to supposed to be initial state, there still are 2 guides

How is it possible ?
It looks like either you did not test the patch carefully enough, or my source is wrong somewhere.

ale

2015-11-05 10:19

manager   ~0037248

as discussed on irc: you should probably keep track of what happens.

i did not check the code, but you stated that the guide manager is removing all guides and then re-adds all of them.

depending on the place where the undo action is recorded/triggered this approach might very hard to get to work correctly.

you seem to move the recording to a different place. this might work, but might also break other ways of setting the guides.

the guides manager should probably track what is really happening and only apply the real changes to the page.


when you start editing a guide, you should probably find/create an id that helps you recognize which guide on the page has been changed.

removing all guides and recreating them is a good solution is you have no interaction with other subsystems. but as soon as you have one (like the undo) it's probably better to drop this (good and fast but) naive implementation and go for a real management of the resources.

JLuc

2015-11-06 13:55

developer   ~0037282

Fahad PLEASE test your next patch carefully before submiting it.

Issue History

Date Modified Username Field Change
2014-06-23 00:49 FirasH New Issue
2014-07-09 01:17 FirasH Relationship added child of 0012500
2014-07-19 12:28 Kunda Note Added: 0032832
2014-07-19 12:28 Kunda Status new => confirmed
2014-07-19 12:28 Kunda Target Version => 1.5.1
2014-07-19 12:28 Kunda Description Updated
2014-07-19 12:28 Kunda Relationship added related to 0012308
2015-06-21 11:40 FirasH Relationship added child of 0002935
2015-06-21 11:42 FirasH Patch => No
2015-06-21 11:42 FirasH Severity minor => major
2015-10-26 05:16 Fahad Note Added: 0036935
2015-10-26 05:16 Fahad File Added: 12425_patch.diff
2015-10-26 07:53 user5122 Tag Attached: HOST-Oman
2015-10-26 16:27 JLuc Patch No => Yes
2015-10-26 19:58 JLuc Note Added: 0036951
2015-10-26 20:01 JLuc Note Edited: 0036951
2015-10-26 20:01 JLuc Note Edited: 0036951
2015-10-27 11:29 Fahad Note Added: 0036979
2015-10-27 11:49 Fahad File Added: second_try.diff
2015-10-27 11:50 Fahad Note Added: 0036980
2015-10-27 13:03 JLuc Note Added: 0036985
2015-11-04 19:30 JLuc Note Added: 0037223
2015-11-04 19:31 JLuc Note Edited: 0037223
2015-11-04 19:34 JLuc Note Edited: 0037223
2015-11-05 10:19 ale Note Added: 0037248
2015-11-05 12:23 Kunda Sticky Issue No => Yes
2015-11-06 13:55 JLuc Note Added: 0037282
2016-01-23 17:16 cbradney Target Version 1.5.1 => 1.5.3
2016-05-12 13:35 Kunda Tag Attached: guides
2016-12-08 21:30 Kunda Target Version 1.5.3 => 1.5.5
2019-10-25 11:29 ale Relationship added related to 0015874
2024-01-02 19:23 cbradney Sticky Issue Yes => No