View Issue Details

IDProjectCategoryView StatusLast Update
0017077ScribusUser Interfacepublic2023-12-21 13:45
Reporterale Assigned To 
PrioritynormalSeverityminorReproducibilityN/A
Status newResolutionopen 
Product Version1.7.0.svn 
Summary0017077: scrolling with the mousewheel over the palettes can focus spinoboxes and comboboxes
Descriptionwhen 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?
TagsNo tags attached.
PatchNo

Activities

nitramr

2023-12-15 17:52

developer   ~0050600

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);
scrspinbox_2023-12-15_01.patch (1,255 bytes)   

ale

2023-12-18 20:03

manager   ~0050624

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.

nitramr

2023-12-20 16:08

developer   ~0050645

Maybe I got it wrong. Please, can you rephrase the expected behavior?

ale

2023-12-20 16:27

manager   ~0050646

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.

nitramr

2023-12-20 19:39

developer   ~0050657

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.

nitramr

2023-12-20 20:43

developer   ~0050658

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;
scrspinbox_2023-12-20_01.patch (3,193 bytes)   

ale

2023-12-21 07:04

manager   ~0050671

yeah!

you nailed it!

cbradney

2023-12-21 10:55

administrator   ~0050673

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;

nitramr

2023-12-21 12:19

developer   ~0050674

Last edited: 2023-12-21 12:25

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

ale

2023-12-21 12:32

manager   ~0050675

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.

ale

2023-12-21 13:45

manager   ~0050676

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?

Issue History

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