View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0017077 | Scribus | User Interface | public | 2023-12-14 20:50 | 2023-12-21 13:45 |
| Reporter | ale | Assigned To | |||
| Priority | normal | Severity | minor | Reproducibility | N/A |
| Status | new | Resolution | open | ||
| Product Version | 1.7.0.svn | ||||
| Summary | 0017077: scrolling with the mousewheel over the palettes can focus spinoboxes and comboboxes | ||||
| Description | when the properties palette (or other palettes) does not fit vertically, one has to scroll up and down the widget. one comfortable way to do that, is to use the mouse wheel. the problem is that if, while scrolling the palette, the mouse gets over a spinbox or a qcombobox (and derivates), then the values change like crazy. this is a general problem in qt (and qt designer / qt creator shows the same bug: with much bigger consequences!) and it seems not to be easy to solve. personally, i like the idea to be able to modify those values with the mouse wheel, and i also really like the fact that i don't have to click on the input field before scrolling. but when you scroll down a dialog, suddenly you're on a qcombobox, it scrolls down to the last element, and then continues scrolling down the dialog. now, you have changed the values without seeing which values have changed (and no undo is available). so, the damage from the uncontrolled modifications is higher than the comfort. i had a try, and looked for solutions, but could not find a combination of various options that would allow to only allow the scrolling when the widget was already focused. i did some research, and this stackoverflow post has some interesting facts and maybe a solution: https://stackoverflow.com/questions/14065750/how-to-get-focus-on-click-but-keep-it-on-mouse-wheel-in-qt it requires that the focusInEvent() is "patched" to ignore the "click" from the mousewheel. here https://forum.qt.io/topic/25072/how-to-ignore-the-mouse-wheel-event-when-combo-box-is-not-in-focus/2 there is some more information. i've tried it out and without the patching of the focusInEvent, i could not get to a workign combination of magic. so, before i go on with my research and tries: do you agree, that the behavior should be changed? | ||||
| Tags | No tags attached. | ||||
| Patch | No | ||||
|
|
I totally agree, we should avoid unwanted value changes by scrolling over the palettes. I found a solution, see patch file. For regular QSpinBox it doesn't work if I just set setFocusPolicy(Qt::StrongFocus); scrspinbox_2023-12-15_01.patch (1,255 bytes)
Index: scribus/ui/scrspinbox.cpp
===================================================================
--- scribus/ui/scrspinbox.cpp (Revision 25879)
+++ scribus/ui/scrspinbox.cpp (Arbeitskopie)
@@ -28,11 +28,11 @@
ScrSpinBox::ScrSpinBox(QWidget *parent, int unitIndex) : QDoubleSpinBox(parent)
{
init(unitIndex);
+ setFocusPolicy(Qt::StrongFocus);
}
-ScrSpinBox::ScrSpinBox(double minValue, double maxValue, QWidget *pa, int unitIndex) : QDoubleSpinBox(pa)
+ScrSpinBox::ScrSpinBox(double minValue, double maxValue, QWidget *pa, int unitIndex) : ScrSpinBox(pa, unitIndex)
{
- init(unitIndex);
setMinimum(minValue);
setMaximum(maxValue);
}
@@ -333,9 +333,10 @@
if (event->type() == QEvent::Wheel)
{
- //If read only don't spin
- if (isReadOnly())
- return false;
+ // If read only don't spin OR avoid value changes if widget has no focus
+ if (isReadOnly() || !hasFocus())
+ return true;
+
auto* wheelEvent = dynamic_cast<QWheelEvent*>(event);
bool shiftB = wheelEvent->modifiers() & Qt::ShiftModifier;
bool altB = wheelEvent->modifiers() & Qt::AltModifier;
@@ -343,7 +344,7 @@
{
setSingleStep(0.1);
retval = QAbstractSpinBox::event(event);
- }
+ }
else if (!shiftB && altB)
{
setSingleStep(10.0);
|
|
|
hi martin thanks for the patch! sadly, no: that does not work here. it indeed does stop the scrolling when the item is not focused, but, then i can't use the scroll wheel for modifying the value even when it's focused. even worse: it goes one step, but then it's over with scrolling. |
|
|
Maybe I got it wrong. Please, can you rephrase the expected behavior? |
|
|
the problem with your patch (and i had the same problem while i was trying my self to find a solution): when the spinbox is focused, the scroll wheel does one step and then it is not detected anymore. that's not nice. it should not work at all or (better) only change the values when the spinbox is focused. what works: your patch stops the values from changing, when the spinbox is not focused. in short, the expected behavior: - scroll wheel does change the values in the spinbox when the spinbox is focused. - scroll wheel does not affect the spinbox when the spinbox is not focused. |
|
|
I got it. I mainly tested the patch in the dialog for creating new documents, where it works. Unfortunately, as you already wrote, the result in the properties panel is not acceptable. In my opinion the problem is not the implemented logic, but the fact that the spinboxes lose focus after the property change. I'll take a closer look at this again. |
|
|
I think I've fixed it. Every time the property changes, this function is called: void PropertiesPalette::handleSelectionChanged() Every time all panels are unnecessarily set to setEnabled(false) only to be turned on again depending on the item type. All input fields lose their focus. In Scribus 1.5.8 the input fields don't lose focus and I vaguely remember changing this in a previous patch to simplify the logic. Gone stupid. scrspinbox_2023-12-20_01.patch (3,193 bytes)
Index: scribus/ui/propertiespalette.cpp
===================================================================
--- scribus/ui/propertiespalette.cpp (Revision 25886)
+++ scribus/ui/propertiespalette.cpp (Arbeitskopie)
@@ -356,12 +356,13 @@
if (!m_haveDoc || !m_ScMW || m_ScMW->scriptIsRunning())
return;
- enablePalettes(false);
+// enablePalettes(false);
PageItem* currItem = currentItemFromSelection();
if (m_doc->m_Selection->count() > 1)
{
scXYZ->setEnabled(true);
+ scShape->setEnabled(false);
scShadow->setEnabled(true);
scLine->setEnabled(true);
scColor->setEnabled(true);
@@ -377,6 +378,12 @@
case -1:
m_haveItem = false;
colorPalette->showGradient(0);
+ scXYZ->setEnabled(false);
+ scShape->setEnabled(false);
+ scLine->setEnabled(false);
+ scColor->setEnabled(false);
+ scTransparency->setEnabled(false);
+ scShadow->setEnabled(false);
break;
case PageItem::ImageFrame:
case PageItem::LatexFrame:
@@ -386,16 +393,24 @@
scXYZ->setEnabled(true);
scShadow->setEnabled(true);
scShape->setEnabled(true);
+ scLine->setEnabled(false);
scColor->setEnabled(true);
+ scTransparency->setEnabled(false);
}
else
{
- enablePalettes(true);
+ scXYZ->setEnabled(true);
+ scShadow->setEnabled(true);
+ scShape->setEnabled(true);
+ scLine->setEnabled(true);
+ scColor->setEnabled(true);
+ scTransparency->setEnabled(true);
}
break;
case PageItem::Line:
scXYZ->setEnabled(true);
scShadow->setEnabled(true);
+ scShape->setEnabled(false);
scLine->setEnabled(true);
scColor->setEnabled(true);
scTransparency->setEnabled(true);
@@ -412,7 +427,12 @@
case PageItem::PolyLine:
case PageItem::Spiral:
case PageItem::PathText:
- enablePalettes(true);
+ scXYZ->setEnabled(true);
+ scShadow->setEnabled(true);
+ scShape->setEnabled(true);
+ scLine->setEnabled(true);
+ scColor->setEnabled(true);
+ scTransparency->setEnabled(true);
break;
}
}
@@ -424,6 +444,7 @@
{
setCurrentItem(currItem);
}
+
}
void PropertiesPalette::unitChange()
Index: scribus/ui/scrspinbox.cpp
===================================================================
--- scribus/ui/scrspinbox.cpp (Revision 25886)
+++ scribus/ui/scrspinbox.cpp (Arbeitskopie)
@@ -28,11 +28,11 @@
ScrSpinBox::ScrSpinBox(QWidget *parent, int unitIndex) : QDoubleSpinBox(parent)
{
init(unitIndex);
+ setFocusPolicy(Qt::StrongFocus);
}
-ScrSpinBox::ScrSpinBox(double minValue, double maxValue, QWidget *pa, int unitIndex) : QDoubleSpinBox(pa)
+ScrSpinBox::ScrSpinBox(double minValue, double maxValue, QWidget *pa, int unitIndex) : ScrSpinBox(pa, unitIndex)
{
- init(unitIndex);
setMinimum(minValue);
setMaximum(maxValue);
}
@@ -333,9 +335,9 @@
if (event->type() == QEvent::Wheel)
{
- //If read only don't spin
- if (isReadOnly())
- return false;
+ // If read only don't spin OR avoid value changes if widget has no focus
+ if (isReadOnly() || !hasFocus())
+ return true;
auto* wheelEvent = dynamic_cast<QWheelEvent*>(event);
bool shiftB = wheelEvent->modifiers() & Qt::ShiftModifier;
bool altB = wheelEvent->modifiers() & Qt::AltModifier;
|
|
|
yeah! you nailed it! |
|
|
I have not had time to test yet.. but.. why do we go from returning false in one case to returning true? - //If read only don't spin - if (isReadOnly()) - return false; + // If read only don't spin OR avoid value changes if widget has no focus + if (isReadOnly() || !hasFocus()) + return true; |
|
|
Event handling should return true if the event is handled. In our case, we want to block the wheel event if there is no focus or if the widget is read only. Or am I wrong? "In this case, the filter handles the relevant events and returns true to stop them from being processed any further. All other events are ignored, and the filter returns false to allow them to be sent on to the target widget, via any other event filters that are installed on it." Reference: https://doc.qt.io/qt-6/eventsandfilters.html |
|
|
in my early tries, i made the same change. not sure if it's correct, but it think to recall that the mousewheel is not blocked if you return false. |
|
|
Now, another problem begins : - ) We need the other (non scr) spinboxes, and comboboxes to behave the same. Personally, I would prefer if the behaviour gets injected into each widget, but I fear that we willcreate a scribus specific widget that will then extended (like scrspinbox) if we go for the injection: - setting the focus policy should be simple, but repetitive - for the event filter, it should be possible to create a class that is then attached to each widget concerned (https://doc.qt.io/qt-6/qobject.html#installEventFilter) if we prefer a base class: - One issue is that scrspinbox already exist and it does not cover every usage of spinboxes in scribus. Sow we probably need a second scribus specifc class for spinboxes. - We should then find a good prefix for scribus specific widgets (personally, i tend to dislike this scr... could it be a scribus::widget:: namespace?) - We should decide if want to use this for every spinboxes & co or only for those who can be in a scrollable context. On the other side, the way the panels are enabled and disabled could be improved in order to make the intent clear (currently, it looks like a long list with no explicit logic on what gets enabled or disable at what time). Any thoughts? |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2023-12-14 20:50 | ale | New Issue | |
| 2023-12-15 17:52 | nitramr | Note Added: 0050600 | |
| 2023-12-15 17:52 | nitramr | File Added: scrspinbox_2023-12-15_01.patch | |
| 2023-12-18 20:03 | ale | Note Added: 0050624 | |
| 2023-12-20 16:08 | nitramr | Note Added: 0050645 | |
| 2023-12-20 16:27 | ale | Note Added: 0050646 | |
| 2023-12-20 19:39 | nitramr | Note Added: 0050657 | |
| 2023-12-20 20:43 | nitramr | Note Added: 0050658 | |
| 2023-12-20 20:43 | nitramr | File Added: scrspinbox_2023-12-20_01.patch | |
| 2023-12-21 07:04 | ale | Note Added: 0050671 | |
| 2023-12-21 10:55 | cbradney | Note Added: 0050673 | |
| 2023-12-21 12:19 | nitramr | Note Added: 0050674 | |
| 2023-12-21 12:25 | nitramr | Note Edited: 0050674 | |
| 2023-12-21 12:32 | ale | Note Added: 0050675 | |
| 2023-12-21 13:45 | ale | Note Added: 0050676 |
