Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preferences: use helper for input widget scroll safeguard #11663

Merged
merged 3 commits into from
Jun 30, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jun 17, 2023

This moves the existing safeguards (I started with #2941) to helpers in DlgPreferencePage and uses it in the other pages as well.

Btw, am I the only who's annoyed by this hover scroll issue? or is just that we have too many big pref pages that require scrolling even on large screens so the issue is relevant at all?

The helper iterates over all dialog children and install the safeguard for input widgets.
There's also a per-widget helper if a page only has a few widgets.

Unfortunately, this doesn't work for QGroupBoxes.
But before I found a way to deal with this, I already started reworking DlgPrefVinyl to store the input widgets in lists (and simplified the entire page code, and fixed an init bug¹, and added proper tabstops)... so this page is pretty much rewritten with widget lists. There are now quite some lines to review in that page. I didn't want to throw away that work so I finished it. If no one wants to review that I can move that commit to a separate PR (and maybe isolate the init bugfix, too).

¹on show, the lead-in time for non existing config values was set to 0 instead of the default time that is picked when selecting a VC type.

@ronso0 ronso0 added this to the 2.4.0 milestone Jun 17, 2023
@github-actions github-actions bot added the ui label Jun 17, 2023
@ronso0 ronso0 force-pushed the pref-scroll-safeguard branch 2 times, most recently from b7c2aa0 to e1a5dce Compare June 18, 2023 21:54
@ronso0 ronso0 marked this pull request as ready for review June 19, 2023 13:23
@ronso0
Copy link
Member Author

ronso0 commented Jun 19, 2023

This is ready for review.

ronso0 added 3 commits June 20, 2023 11:12
Avoid undesired value changes when scrolling a preferences page while
the pointer is above an input widget (QSpinBox, QComboBox, QSlider):
 * set the focus policy to Qt::StrongFocus (focusable by click & tab key)
 * forward wheel events to the top-level layout
@ronso0 ronso0 force-pushed the pref-scroll-safeguard branch from e1a5dce to 3348692 Compare June 20, 2023 09:12
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I have added some comments

src/preferences/dialog/dlgprefcolors.cpp Show resolved Hide resolved
continue;
}

QComboBox* combo = qobject_cast<QComboBox*>(ch);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use a p prefix for pointers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -47,6 +47,8 @@ DlgPrefBeats::DlgPrefBeats(QWidget* parent, UserSettingsPointer pConfig)
&QCheckBox::stateChanged,
this,
&DlgPrefBeats::slotReanalyzeImportedChanged);

setScrollSafeGuard(comboBoxBeatPlugin);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we effort to use here setScrollSafeGuardForAllInputWidgets(this); to not forget new elements if we add any?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume setting the scroll guard per widget is faster than using setScrollSafeGuardForAllInputWidgets which iterates over all dialog children, do an object cast etc.
To keep preferences construction (startup) fast I chose the per-widget function if there are only a few affected input widgets.
I didn't measure the impact though, maybe the difference is negligible.

For this page it probably doesn't make a big difference since there are only a few widgets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion here. It can stay like this if you like.

@ronso0
Copy link
Member Author

ronso0 commented Jun 29, 2023

I'll add the p prefix and remove obsolete this arguments, and pack that as fixup commit.

Let me know when you intend to start reviewing DlgPrefVinyl, so I can squash that beforehand.

@ronso0
Copy link
Member Author

ronso0 commented Jun 30, 2023

We use a p prefix for pointers.

fixed and rebased.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and works good, Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants