Ui scroll wheel events

[akapar2016]

0001214: Scroll wheel events modifies keyboard focus
Uses eventFilter to disable scrollwheel for comboBox, Slider, and Spin box when adding sources.

[dodgepong]

Does this also disable mouse wheel events for changing volume? It looks like it only affects elements in properties windows.

Granted, some people might like that you can change volume with the mouse wheel, but we also frequently have complaints about scrolling down a list of meters causing volumes to change.

[akapar2016]

It only affects the elements in properties windows. I could look into editing the ones for volume. Should that be a different issue or should it be working on it in the same issue? This was only for the properties window.

[dodgepong]

Since the Mantis issue doesn't specify anything about the properties window specifically, I think including it in this PR would be appropriate.

[SuslikV]

It should be written directly at the sub-classing code (not where the sub-classed widgets are used).

[akapar2016]

@SuslikV I am a bit confused. Would this mean adding event handlers inside the qslider.h and all other widget files itself? I am a bit new to QT so trying to get my head around it.

[SuslikV]

I'm reread my message - and you are right, probably I'm wrote something stupid there. I just want to say that the mentioned (in the bug report) possible method is via sub-classing widgets may look like this:

// example from:
// https://stackoverflow.com/questions/5821802/qspinbox-inside-a-qscrollarea-how-to-prevent-spin-box-from-stealing-focus-when

class MySpinBox : public QSpinBox {

    Q_OBJECT

public:

    MySpinBox(QWidget *parent = 0) : QSpinBox(parent) {
        setFocusPolicy(Qt::StrongFocus);
    }

protected:

    virtual void wheelEvent(QWheelEvent *event) {
        if (!hasFocus()) {
            event->ignore();
        } else {
            QSpinBox::wheelEvent(event);
        }
    }
};

and then using them in OBS instead of QSpinBox *spin = new QSpinBox();
the QSpinBox *spin = new MySpinBox();

here:

obs-studio/UI/properties-view.cpp

Lines 323 to 325 in a0ead6f

	int val = (int)obs_data_get_int(settings, name);
	QSpinBox *spin = new QSpinBox();

and in some other places if needed.

It gives (in the future) the ability to sub-class the objects differently (per platform) and give to them different focus strategy depending on platform, because something may not work for mac/linux or will be solved by Qt exclusively for one platform and so on. In general, keep control over all widgets in one place.

[jp9000]

It should only apply to properties. The behavior to use middle-mouse scroll with widgets is normal operating system behavior. For properties, the reason why this is useful is because it's annoying if you want to scroll through the view itself.

[akapar2016]

I am attempting to create a subclass like @SuslikV stated. I am running into issues creating the Qt MOC files I think. I stated it on the discord server. After I get the subclassing to work, I will remove the eventfilter and implement the subclass to the properties

[jp9000]

No, your pull request was fine as-is, outside of the formatting and commit issues. I don't want you to remove scroll wheel events except for how you did it with the properties window.

[jp9000]

@SuslikV scroll wheel events in the background are a normal part of operating system behavior. This happens with other programs too.

What @akapar2016 actually did, which was disable wheel events for properties within a properties window in favor of being able to scroll the actual full contents of the properties, was actually a more ideal as a change I'd like to have.

[akapar2016]

@jp9000 I ended up switching to QSlider subclass since it allows for more customization. Now the Sliders are disabled in properties, but if you were to click on the Slider, the wheel works again. I set a focus policy to do this like the snippet @SuslikV posted above. I will continue to do the same for combobox and spinbox after I get some feedback on the subclassing. I kept a copy of the event filter way as well just to look at both ways. Sorry I didn't see your comment earlier about keeping it the way it is.

This is the kind of trait I would guess @dodgepong was talking about for volume. Using the wheel scroll is still useful, but I think it makes sense that this is only active when the Slider is focused. I would think that should be a different issue since it is working in a different section. This issue would only cover properties for now.

[jp9000]

Okay, I think that sounds good. I'll do some more testing. I think I misunderstood the situation a bit.

[WizardCM]

I agree subclassing with a focus policy that enables the scroll wheel is a good idea.

[Fenrirthviti]

I prefer the subclass method from the UX standpoint. Disabling the scroll wheel entirely is not ideal, as many users (myself included) use the scroll wheel to change property values. The issue is more that it's too easy to accidentally change it when scrolling the properties window itself. Scroll when focused is the correct solution, IMO.

[SuslikV]

Just a note. Event filter allows same behavior, simply it was implemented not the best way in the first place. And I promote sub-classing for different reason.

As soon as platforms were mentioned I should to clarify my thoughts.
The behavior difference between platforms, as far as I understand it from the Qt bug tracker, is that on some systems if widget is focused (clicked) and mouse pointer moved away of the widget (there is different widget under the pointer right now) then mouse wheel still affects focused widget (that was clicked), but the wheel doesn't switches the focus itself. For other platforms, if mouse pointer is not covers the focused widget - the wheel ignored. In other words, you may click at volume slider and move the mouse away and the wheel wouldn't work, because under the mouse pointer there is no slider that was clicked before (but for some systems it is normal if it will work until you click other widget, other slider). I may wrong in this last statement, of course.

[akapar2016]

Testing it on Win10 64, When I click to focus on the slider, I can adjust the slider using wheel scroll as long as my mouse stays above the slider. Once the mouse moves away, the slider is no longer in focus and the scroll goes back to scrolling the full page. I have to go back and click the slider to gain focus again.

[akapar2016]

I am going to start implementing the same solution to Spinbox and Combobox today.

[SuslikV]

virtual vs override?

[akapar2016]

I made subclasses of combobox and spinbox and implemented them to the properties.
I plan to make changes to the volume in a different PR as it deals with another area.
Does this implementation seem alright?

[SuslikV]

I think, obs uses next semantics (almost everywhere in its code):

virtual void wheelEvent(QWheelEvent *event) override;

(both virtual and override mentioned) for overridden virtual method derived from the base class (for example, the mentioned wheelEvent itself overridden virtual protected method of QAbstractSpinBox).
Maybe not a big deal, but @jp9000 likes when all strings are written accurate.

[akapar2016]

@jp9000 what do think of this implementation. I would like to have this PR merged and closed. Thank you all for your feedback!

[SuslikV]

You will be asked to remove unneeded parentheses when if ... else statement has single statement in its body.

	if (!hasFocus()) {
		event->ignore();
	} else {
		QComboBox::wheelEvent(event);
	}

to

	if (!hasFocus())
		event->ignore();
	else
		QComboBox::wheelEvent(event);

[akapar2016]

I refactored the if else statements.

[akapar2016]

@jp9000 @SuslikV anything else that seems like it should be added prior to merging?

[SuslikV]

I'm not merging anything to obs. As for commits follow the https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst#commit-guidelines
I think, the co-authors should be added at commits to appear at contributing and thus at the Authors list.

Then wait. From few days to few years...

[akapar2016]

I am a bit confused on what you mean

[akapar2016]

Ran into some issues with git to remove the authors commit. I think its fine now. All changes are squashed into one commit and the Authors changes are removed.

[SuslikV]

You know, contributing guidelines were updated... (capital letter in the commit's title after the module name is now mentioned, de-facto it was long time like this)