View Issue Details

IDProjectCategoryView StatusLast Update
0016180ScribusStory Editor / Text Framespublic2020-07-21 08:03
Reporterale Assigned To 
PrioritynormalSeverityminorReproducibilityN/A
Status newResolutionopen 
Product Version1.5.6.svn 
Summary0016180: PATCH: marks: fail early if it's a master page (and no variable text)
Descriptiondon't wait for the dialog to almost show up...
TagsNo tags attached.
PatchYes

Activities

ale

2020-07-20 10:27

manager  

mark-master-page.diff (852 bytes)   
diff --git a/scribus/scribus.cpp b/scribus/scribus.cpp
index d06e7ba8a..de5d92d0a 100644
--- a/scribus/scribus.cpp
+++ b/scribus/scribus.cpp
@@ -9616,6 +9616,9 @@ void ScribusMainWindow::insertMark(MarkType mType)
 {
 	if (!HaveDoc)
 		return;
+	if (doc->masterPageMode() && (mType != MARKVariableTextType))
+		// only variable text can go on master pages
+		return;
 	if (doc->m_Selection->count() != 1)
 		return;
 	if  (doc->appMode != modeEdit)
@@ -9759,10 +9762,6 @@ void ScribusMainWindow::slotInsertMarkNote()
 
 bool ScribusMainWindow::insertMarkDialog(PageItem_TextFrame* currItem, MarkType mrkType, ScItemsState* &is)
 {
-	if (doc->masterPageMode() && (mrkType != MARKVariableTextType))
-		//avoid inserting in master pages other marks than Variable Text
-		return false;
-	
 	QScopedPointer<MarkInsert> insertMDialog;
 	switch (mrkType)
 	{
mark-master-page.diff (852 bytes)   

jghali

2020-07-20 13:05

administrator   ~0047857

The insertMarkDialog() function itself must return in these cases, so I do not see why these checks should be removed from this function.

ale

2020-07-20 13:27

manager   ~0047859

because you do not even need to call the insertMarkDialog() in those cases.

except if i did not understand something.
then it's really necessary to document the corner cases!

as far as i understand it, if the user is on a master page and wants to insert a mark that is not a variable text, she just can't.
why bother to get that far?

and there is only one function calling insertMarkDialog in the whole scribus code base.

imo, this is a good example of code that does not respect the separation of concerns.

jghali

2020-07-20 21:35

administrator   ~0047861

Last edited: 2020-07-20 21:39

>> because you do not even need to call the insertMarkDialog() in those cases.

Then that's one more reason for those checks to stay. Nothing prevents to add these checks to insertMark() tho.

ale

2020-07-21 08:02

manager   ~0047864

Last edited: 2020-07-21 08:03

it's definitively too late to check for it there, but if you think that the function could be used in a different context checking that the text frame is not on a master page makes sense.

/me would wish that the pre/post conditions camp had won the race...

if the refactoring of the marks is accepted this ticket can be closed and i will take into consideration that the text frame must be checked again.

Issue History

Date Modified Username Field Change
2020-07-20 10:27 ale New Issue
2020-07-20 10:27 ale File Added: mark-master-page.diff
2020-07-20 10:39 ale Summary marks: fail early if it's a master page (and no variable text) => PATCH: marks: fail early if it's a master page (and no variable text)
2020-07-20 10:39 ale Patch No => Yes
2020-07-20 13:05 jghali Note Added: 0047857
2020-07-20 13:27 ale Note Added: 0047859
2020-07-20 21:35 jghali Note Added: 0047861
2020-07-20 21:39 jghali Note Edited: 0047861
2020-07-21 08:02 ale Note Added: 0047864
2020-07-21 08:03 ale Note Edited: 0047864