View Issue Details

IDProjectCategoryView StatusLast Update
0016191ScribusInternalpublic2020-09-14 14:46
Reporterale Assigned Tojghali  
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionfixed 
Product Version1.5.6.svn 
Target VersionFixed in Version1.5.6.svn 
Summary0016191: In ScribusMainWindow return early in keyPressEvent() and keyReleaseEvent() if there is no document
Descriptionif there is no document, keyPressEvent() and keyReleaseEvent() do not contain any active code.

it's better to just return at the beginning of the function

i'm not sure if the "m_keyrep = true;" before the early return in keyPressEvent() is needed, but it seems to be what the code is currently doing.

in the switch, i've also removed the breaks after the returns. they are not reachable.

the final is goal is to understand if it's possible to remove the code that is exactly the same as in CanvasMode::commonkeyPressEvent_NormalNodeEdit()
TagsNo tags attached.
PatchYes

Activities

ale

2020-07-27 20:30

manager  

no-doc-return-early.diff (4,488 bytes)
diff --git a/scribus/scribus.cpp b/scribus/scribus.cpp
index eb2e78b5c..97e3d3e88 100644
--- a/scribus/scribus.cpp
+++ b/scribus/scribus.cpp
@@ -1745,17 +1745,18 @@ QVariant ScribusMainWindow::inputMethodQuery ( Qt::InputMethodQuery query ) cons
 //AV -> CanvasMode
 void ScribusMainWindow::keyPressEvent(QKeyEvent *k)
 {
-	QList<QMdiSubWindow *> windows;
-	QMdiSubWindow* w = nullptr;
+	if (!HaveDoc)
+		m_keyrep = true;
+		return;
+
 	int kk = k->key();
-	if (HaveDoc)
+
+	if ((doc->appMode == modeMagnifier) && (kk == Qt::Key_Shift))
 	{
-		if ((doc->appMode == modeMagnifier) && (kk == Qt::Key_Shift))
-		{
-			view->setCursor(IconManager::instance().loadCursor("lupezm.png"));
-			return;
-		}
+		view->setCursor(IconManager::instance().loadCursor("lupezm.png"));
+		return;
 	}
+
 	if (m_keyrep)
 		return;
 	m_keyrep = true;
@@ -1767,7 +1768,7 @@ void ScribusMainWindow::keyPressEvent(QKeyEvent *k)
 	if (k->modifiers() & Qt::AltModifier)
 		keyMod |= Qt::ALT;
 	//User presses escape and we have a doc open, and we have an item selected
-	if ((kk == Qt::Key_Escape) && (HaveDoc))
+	if (kk == Qt::Key_Escape)
 	{
 		m_keyrep = false;
 		PageItem *currItem;
@@ -1831,7 +1832,7 @@ void ScribusMainWindow::keyPressEvent(QKeyEvent *k)
 	}
 	Qt::KeyboardModifiers buttonModifiers = k->modifiers();
 	/**If we have a doc and we are not changing the page or zoom level in the status bar */
-	if ((HaveDoc) && (!zoomSpinBox->hasFocus()) && (!pageSelector->hasFocus()))
+	if (!zoomSpinBox->hasFocus() && !pageSelector->hasFocus())
 	{
 		//Show our context menu
 		if (actionManager->compareKeySeqToShortcut(kk, buttonModifiers, "viewShowContextMenu"))
@@ -1880,7 +1881,6 @@ void ScribusMainWindow::keyPressEvent(QKeyEvent *k)
 				else
 					view->requestMode(modePanning);
 				return;
-				break;
 			case Qt::Key_PageUp:
 				if (doc->masterPageMode() || doc->symbolEditMode() || doc->inlineEditMode())
 					view->scrollBy(0, -m_prefsManager.mouseWheelJump());
@@ -1896,7 +1896,6 @@ void ScribusMainWindow::keyPressEvent(QKeyEvent *k)
 				}
 				m_keyrep = false;
 				return;
-				break;
 			case Qt::Key_PageDown:
 				if (doc->masterPageMode() || doc->symbolEditMode() || doc->inlineEditMode())
 					view->scrollBy(0, m_prefsManager.mouseWheelJump());
@@ -1912,32 +1911,28 @@ void ScribusMainWindow::keyPressEvent(QKeyEvent *k)
 				}
 				m_keyrep = false;
 				return;
-				break;
 			case Qt::Key_Left:
 				view->scrollBy(-wheelVal, 0);
 				m_keyrep = false;
 				return;
-				break;
 			case Qt::Key_Right:
 				view->scrollBy(wheelVal, 0);
 				m_keyrep = false;
 				return;
-				break;
 			case Qt::Key_Up:
 				view->scrollBy(0, -wheelVal);
 				m_keyrep = false;
 				return;
-				break;
 			case Qt::Key_Down:
 				view->scrollBy(0, wheelVal);
 				m_keyrep = false;
 				return;
-				break;
 			case Qt::Key_Tab:
 				if (buttonModifiers == Qt::ControlModifier)
 				{
 					m_keyrep = false;
-					windows = mdiArea->subWindowList();
+					auto windows = mdiArea->subWindowList();
+					QMdiSubWindow* w = nullptr;
 					if (windows.count() > 1)
 					{
 						for (int i = 0; i < static_cast<int>(windows.count()); ++i)
@@ -1976,15 +1971,16 @@ void ScribusMainWindow::keyPressEvent(QKeyEvent *k)
 
 void ScribusMainWindow::keyReleaseEvent(QKeyEvent *k)
 {
+	if (!HaveDoc)
+		return;
+
 	//Exit out of panning mode if Control is release while the right mouse button is pressed
-	if (HaveDoc)
-	{
-		if ((doc->appMode == modePanning) && (k->key() == Qt::Key_Control) && (QApplication::mouseButtons() & Qt::RightButton))
-			view->requestMode(modeNormal);
+	if ((doc->appMode == modePanning) && (k->key() == Qt::Key_Control) && (QApplication::mouseButtons() & Qt::RightButton))
+		view->requestMode(modeNormal);
+
+	if (doc->appMode == modeMagnifier)
+		view->setCursor(IconManager::instance().loadCursor("lupez.png"));
 
-		if (doc->appMode == modeMagnifier)
-			view->setCursor(IconManager::instance().loadCursor("lupez.png"));
-	}
 	if (k->isAutoRepeat() || !m__arrowKeyDown)
 		return;
 	switch (k->key())
@@ -1994,7 +1990,7 @@ void ScribusMainWindow::keyReleaseEvent(QKeyEvent *k)
 		case Qt::Key_Up:
 		case Qt::Key_Down:
 			m__arrowKeyDown = false;
-			if ((HaveDoc) && (!zoomSpinBox->hasFocus()) && (!pageSelector->hasFocus()))
+			if (!zoomSpinBox->hasFocus() && !pageSelector->hasFocus())
 			{
 				int docSelectionCount = doc->m_Selection->count();
 				if ((docSelectionCount != 0) && (doc->appMode == modeEditClip) && (doc->nodeEdit.hasNodeSelected()))
no-doc-return-early.diff (4,488 bytes)

jghali

2020-07-28 18:48

administrator   ~0047882

Patch is incorrect: you should notice there is code executed at the end of ScribusMainWindow::keyPressEvent() even if no doc is opened.

jghali

2020-07-28 19:44

administrator   ~0047884

Same in ScribusMainWindow::keyReleaseEvent(), m_arrowKeyDown may have its value changed if no doc is opened.

ale

2020-07-29 07:06

manager   ~0047887

the code for the key handling looks way too much entangled to be maintainable.

as i wrote yesterday, i'm working on a patch to "linearize" the key handling for the normal (and node) mode and now i know a bit better how it works.

    switch (kk)
    {
        case Qt::Key_Left:
        case Qt::Key_Right:
        case Qt::Key_Up:
        case Qt::Key_Down:
            m_arrowKeyDown = true;
    }
    m_keyRepeat = false;

for m_keyRepeat, it looks like that it's set to true at the beginning and to false when going out if. is it a guard avoiding that new key presses are processed before the handler has finished its work?

"m_arrowKeyDown" = true (with one or two underlines depending on the class where it is used) seems to be asking for actions in the release event, but it's not really clear what those actions are.
only one transaction for all movements until the key is released? that would be a good behavior.
but why do nodes need more code at release time? why can't it be done when the pressed event is managed?

would it make sense to rename the first variable to something like m_keyPressGuard and the second to m_arrowKeyNeedsReleaseEvent.

i'd also need to know why ScribusMainWindow::keyPressEvent(QKeyEvent *k) has duplicated code from CanvasMode, even if andreas has put the comment "//AV -> CanvasMode" on top of it.

cbradney

2020-08-16 16:00

administrator   ~0047942

I've committed the removal of the duplicate code. Let's test

jghali

2020-09-14 14:45

administrator   ~0048055

Closing this issue as ScribusMainWindow keyPressEvent() and keyReleaseEvent() have been reduced to their most simple expression since commit r24003.

Issue History

Date Modified Username Field Change
2020-07-27 20:30 ale New Issue
2020-07-27 20:30 ale File Added: no-doc-return-early.diff
2020-07-28 01:29 jghali Category - => Internal
2020-07-28 18:48 jghali Note Added: 0047882
2020-07-28 19:44 jghali Note Added: 0047884
2020-07-29 07:06 ale Note Added: 0047887
2020-08-16 16:00 cbradney Note Added: 0047942
2020-09-14 14:44 jghali Summary PATCH: in ScribusMainWindow return early in keyPressEvent() and keyReleaseEvent() if there is no document => In ScribusMainWindow return early in keyPressEvent() and keyReleaseEvent() if there is no document
2020-09-14 14:45 jghali Assigned To => jghali
2020-09-14 14:45 jghali Status new => resolved
2020-09-14 14:45 jghali Resolution open => fixed
2020-09-14 14:45 jghali Fixed in Version => 1.5.6.svn
2020-09-14 14:45 jghali Note Added: 0048055
2020-09-14 14:46 jghali Status resolved => closed