View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0016191 | Scribus | Internal | public | 2020-07-27 20:30 | 2020-09-14 14:46 |
Reporter | ale | Assigned To | jghali | ||
Priority | normal | Severity | minor | Reproducibility | N/A |
Status | closed | Resolution | fixed | ||
Product Version | 1.5.6.svn | ||||
Fixed in Version | 1.5.6.svn | ||||
Summary | 0016191: In ScribusMainWindow return early in keyPressEvent() and keyReleaseEvent() if there is no document | ||||
Description | if 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() | ||||
Tags | No tags attached. | ||||
Patch | Yes | ||||
|
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())) |
|
Patch is incorrect: you should notice there is code executed at the end of ScribusMainWindow::keyPressEvent() even if no doc is opened. |
|
Same in ScribusMainWindow::keyReleaseEvent(), m_arrowKeyDown may have its value changed if no doc is opened. |
|
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. |
|
I've committed the removal of the duplicate code. Let's test |
|
Closing this issue as ScribusMainWindow keyPressEvent() and keyReleaseEvent() have been reduced to their most simple expression since commit r24003. |
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 |