-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix #312442: Fix palette search shortcut functionality #6802
Fix #312442: Fix palette search shortcut functionality #6802
Conversation
#6627 contains several such changes of signal handlers to |
I merely looked this up last night, but my guess is that those which have emit signals should be without the All the related changes in #6627 have "emit" signals so they should probably be reverted to not using the function keyword, supposing there aren't any corresponding warnings. I haven't personally verified whether the code is executing correctly or not for the dragging/focusChanged signals. |
To be honest I don't even know why I changed those. Building now with these changes reverted, let's see... I guess they came in here because I cherry-picked from #5616. But I don't remember why those changes made it into that one either... we may need to revert them there too? Actually in b7b3564's commit message I stated: I don't get any warnings in 3.x (and when building that against Qt 5.15, in MinGW and MSVC) when reverting those changes, so please go ahead and revert them. Whether or not to revert them in master too might still need to get determined... |
Hmm, actually I do get warnings, at runtime and tons of them (here only one of each):
The last 3 not being dealt with so far, the others might be OK with Qt 5.15 and so in master? See also https://stackoverflow.com/questions/62297192/qml-connections-implicitly-defined-onfoo-properties-in-connections-are-deprecat, seems that changed syntax came in with Qt 5.14, so we definitely don't want it in 3.x, but most probably do want it in master, but there, as per https://doc.qt.io/qt-5/qml-qtqml-connections.html, we may need to change to |
... Sounds like we're going to need to change something else with the way the code is structured or something in C++ since this isn't working with the simple |
Most probably |
Yes, exactly |
It looks like I've figured out a way to get this to be a |
Either that or just revert all 4 of those, the easy way out. But fixing it your way may help esp. Linux distributions that use a new Qt version. |
Ok. What I think I'll do is revert them all in this PR and update the title, but then also have a separate commit within this PR that "converts" the palette searching mechanism into |
You could add those changes as a 3rd commit |
Should be good to go. |
I meant the convert to a proper Oh, I see. I though you meant to convert those other 3 (was 4) too |
I think there are only 4 in total, including the palette search. I've only "converted" the palette search here and "reverted" the other three. |
Got it |
@@ -119,10 +120,13 @@ void PaletteWidget::setupStyle() | |||
|
|||
void PaletteWidget::activateSearchBox() | |||
{ | |||
int delay = isFloating() ? 300 : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May seem strange, but when palette is floating, the focus in event ends up occuring "after" the focus in event of the searchText and ends up then removing the preparation for search string. Adding a slight delay to the request only when floating clears this up
@@ -211,8 +211,10 @@ QQuickView* QmlDockWidget::getView() | |||
|
|||
void QmlDockWidget::ensureQmlViewFocused() | |||
{ | |||
if (_view && !_view->activeFocusItem()) | |||
if (_view && !_view->activeFocusItem()) { | |||
widget()->activateWindow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up allowing using the search shortcut while floating. Unfortunately took quite some time for me to figure this out, even though it's only one function call extra.
@@ -4032,8 +4032,10 @@ bool MuseScoreApplication::event(QEvent* event) | |||
|
|||
void MuseScore::focusScoreView() | |||
{ | |||
if (currentScoreView()) | |||
if (currentScoreView()) { | |||
currentScoreView()->activateWindow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, this allows for pressing ESCAPE and from within the search string while floating and have the score resume focus. Unfortunately doesn't work without this PR/Commit.
Alright. Also got floating palettes working with search shortcut + escaping working. This wasn't working ever since like 3.2.3 when searching was first implemented I think it was. |
Resolves: https://musescore.org/en/node/312442
From 3.5.1 to 3.5.2, there was a mishap for palette search functionality coming from fixing warnings for Qt 5.15 - #6627
Update: As per conversation, this PR reverts the changes as mentioned in #6627 related to the function() keyword, but also in separate commit reforms the palette search functionality to make use of the function() keyword.
Update: "While I'm up," I've decided to revisit palette search functionality since when the palette search widget is "floating", the keyboard shortcut doesn't work (wasn't a reversion... it just never worked appropriately). After trial/error, I learned that somehow the main paletteWidget "steals" focus after the focus upon the searchText in QML while floating, but it doesn't when docked. Adding a timer takes care of this. There may be a better way, but for now to have this actually working is what counts. There's the additional problem that while floating, the ESCAPE key doesn't actually exit from the palette widget and resume focus back upon the score! These two problems I have resolved and am adding one additional/separate commit within this PR.