DlgTrackInfo: apply pending changes also when saving via hotkey #4562
Conversation
ronso0
force-pushed
the
track-info-reset
branch
from
5a51afb
to
f346a30
Compare
src/library/dlgtrackinfo.cpp
Outdated
| if (this == QApplication::activeWindow()) { | ||
| auto focusWidget = QApplication::focusWidget(); | ||
| if (focusWidget) { | ||
| QLineEdit* focusedLineEdit = static_cast<QLineEdit*>(focusWidget); |
There was a problem hiding this comment.
This is undefined behavior if the focused widget is not of type QLineEdit.
There was a problem hiding this comment.
See qobject_cast for the safe cast.
There was a problem hiding this comment.
Is it possible to switch focus to the Apply button and then process all subsequent events before continuing? This would be a more robust solution that is independent of the actual widgets that are used.
There was a problem hiding this comment.
See
qobject_castfor the safe cast.
Yeah, thanks!
This also allows to remove the if (focusWidget) check because qobject_cast returns nullptr if focusWidget is a nullptr
There was a problem hiding this comment.
Is it possible to switch focus to the Apply button and then process all subsequent events before continuing?
The benefit of using hotkeys is that the metadata field remains focused when switching tracks, see the mass-edit example I mentioned.
alternative hacks:
store focus widget, focus Apply button to fire editingFinished, re-focus previous widget, but this is a bit over the top to fix the behavioursend aReturnkey event
requires filtering for non-QPushButton widgetssendingTabthenShift+Tab
that's is even more of a hackhave a list of all used QLineEdits (they're hardcoded here anyway) and insaveTrack()iterate over that to emit editingFinished() for all of them- shortest widget-independent and most straight-forward hack:
auto focusWidget = QApplication::focusWidget();
if (focusWidget) {
focusWidget->clearFocus();
focusWidget->setFocus();
}
: )
There was a problem hiding this comment.
It works, but still it's a hack -- and I thought we prefer 'official ways' over hacks.
I don't care as long as the bug is fixed.
There was a problem hiding this comment.
Leaving the focused widget is what is supposed to happen before hitting Apply. That's the regular flow. I don't like to introduce shortcuts that work differently.
There was a problem hiding this comment.
The focus change variant works for me. Didn't notice that you introduced it here.
There was a problem hiding this comment.
Leaving the focused widget is what is supposed to happen before hitting Apply
...when you use the mouse, sure.
I don't like to introduce shortcuts that work differently
They do work differently: QShortcut makes its QPushButton emit clicked() but it does not force-focus the button.
FWIW #3906 broke the flow for hotkey users, or at least made it more complex by requiring to hit Return.
(no, that's no occassion to post that famous xkcb cartoon... ;)
There was a problem hiding this comment.
The focus change variant works for me. Didn't notice that you introduced it here.
I just did after you asked for it.
ronso0
force-pushed
the
track-info-reset
branch
2 times, most recently
from
1a253fc
to
234005a
Compare
ronso0
force-pushed
the
track-info-reset
branch
from
234005a
to
be68ef6
Compare
This fixes a small usability regression introduced by #3906
It affects my personal workflow when mass-editing metadata (same metadata field for a few tracks in a row).
https://bugs.launchpad.net/mixxx/+bug/1954346
https://bugs.launchpad.net/mixxx/+bug/1954346
The issue is that the track record is only updated when changed QLineEdits send the editingFinished() signal -- which is not the case when I don't press Enter and
Apply/saveTrack() is invoked via hotkey while the changed line is still focused.I will definitely add this to my personal branch and I don't mind if it's rejected because "The user should simply hit Enter, let's not add this hack".