Fix #312442: Fix palette search shortcut functionality #6802

worldwideweary · 2020-11-02T07:39:41Z

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.

[ x ] I signed CLA
[ x ] I made sure the code in the PR follows the coding rules
[ x ] I made sure the code compiles on my machine
[ x ] I made sure there are no unnecessary changes in the code
[ x ] I made sure the title of the PR reflects the core meaning of the issue you are solving
[ x ] I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
[ x ] I made sure the commit message title starts with "fix #424242:" if there is a related issue
[ ] I created the test (mtest, vtest, script test) to verify the changes I made

dmitrio95 · 2020-11-02T12:49:03Z

#6627 contains several such changes of signal handlers to functions in QML code. Should they also be reverted?

worldwideweary · 2020-11-02T20:31:48Z

#6627 contains several such changes of signal handlers to functions in QML code. Should they also be reverted?

I merely looked this up last night, but my guess is that those which have emit signals should be without the function keyword, but those that have QMetaObject::invokeMethod() code can have the function keyword without a problem (e.g. applyCurrentPaletteElement()).

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.

Jojo-Schmitz · 2020-11-03T07:35:07Z

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: Fix qml runtime warnings exactly following the advice given by that warning, but without really know(ing) what I'm doing here or whether that is still backwards compatible to Qt 5.9.

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...

Jojo-Schmitz · 2020-11-03T08:52:37Z

Hmm, actually I do get warnings, at runtime and tons of them (here only one of each):

Warning: qrc:/qml/palettes/PaletteTree.qml:773:5: QML Connections: Implicitly defined onFoo properties in Connections are deprecated. Use this syntax instead: function onFoo(<arguments>) { ... } (qrc:/qml/palettes/PaletteTree.qml:773, )
Warning: qrc:/qml/palettes/Palette.qml:766:13: QML Connections: Implicitly defined onFoo properties in Connections are deprecated. Use this syntax instead: function onFoo(<arguments>) { ... } (qrc:/qml/palettes/Palette.qml:766, )
Warning: qrc:/qml/palettes/PalettesWidgetHeader.qml:186:5: QML Connections: Implicitly defined onFoo properties in Connections are deprecated. Use this syntax instead: function onFoo(<arguments>) { ... } (qrc:/qml/palettes/PalettesWidgetHeader.qml:186, )
Warning: qrc:/qml/palettes/PalettesWidgetHeader.qml:174:5: QML Connections: Implicitly defined onFoo properties in Connections are deprecated. Use this syntax instead: function onFoo(<arguments>) { ... } (qrc:/qml/palettes/PalettesWidgetHeader.qml:174, )
Warning: file:///C.../qml/QtQuick/Controls/TableViewColumn.qml:66:1: QML TableViewColumn: Accessible must be attached to an Item (file:///.../qml/QtQuick/Controls/TableViewColumn.qml:66, )
Warning: qrc:/qml/palettes/PaletteTree.qml:516: TypeError: Cannot read property 'width' of null (qrc:/qml/palettes/PaletteTree.qml:516, )
Warning: qrc:/qml/palettes/PaletteTree.qml:353:19: QML ItemDelegate: Binding loop detected for property "width" (qrc:/qml/palettes/PaletteTree.qml:353, )
Warning: Using QCharRef with an index pointing outside the valid range of a QString. The corresponding behavior is deprecated, and will be changed in a future version of Qt. (:0, )

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 import QtQml 2.15 too.

worldwideweary · 2020-11-03T17:55:35Z

Hmm, actually I do get warnings, at runtime and tons of them (here only one of each):

... 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 function () syntax. . . since Qt 6 will probably no longer support it

dmitrio95 · 2020-11-03T18:40:49Z

Most probably function() syntax for signal handlers is not yet working in Qt 5.9, and those deprecation warnings are relevant only for newer versions of Qt. If so, we need to revert these changes in 3.x branch but leave them in master.

Jojo-Schmitz · 2020-11-03T18:48:04Z

Yes, exactly

worldwideweary · 2020-11-04T01:36:10Z

Yes, exactly

It looks like onElementDraggedToScoreView is failing also in 3.5.2. To test this, drag a palette item (treble clef for instance) to a score view area without dropping it and notice that the palette area goes blank where it originally was. 3.5.0 doesn't have this issue.

I've figured out a way to get this to be a QML function for the palette searching and have it work, but not sure if i should just leave it all alone in this reversion or actually update this to use the function() mechanism with appropriate code changes to have it work properly. It involves using similar code to void PaletteWidget::applyCurrentPaletteElement() and then redirecting a function in PaletteWidget.qml into PaletteWidgetHeader.qml without using emit or the qml "Connections"

Jojo-Schmitz · 2020-11-04T08:25:53Z

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.

worldwideweary · 2020-11-04T17:26:15Z

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 function() form. That way if anyone in the future wants to convert the others, they may have a lead here as one possible solution. So long as everything is working fine for users, that's of main concern.

Jojo-Schmitz · 2020-11-04T17:32:55Z

You could add those changes as a 3rd commit

worldwideweary · 2020-11-04T17:39:45Z

You could add those changes as a 3rd commit

Should be good to go.

Jojo-Schmitz · 2020-11-04T17:53:23Z

I meant the convert to a proper function() form.

Oh, I see. I though you meant to convert those other 3 (was 4) too

worldwideweary · 2020-11-04T18:01:38Z

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.

Jojo-Schmitz · 2020-11-04T18:40:46Z

Got it

worldwideweary · 2020-11-06T10:43:09Z

mscore/palette/palettewidget.cpp


 void PaletteWidget::activateSearchBox()
      {
+      int delay = isFloating() ? 300 : 0;


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

worldwideweary · 2020-11-06T10:44:22Z

mscore/qmldockwidget.cpp

      {
-      if (_view && !_view->activeFocusItem())
+      if (_view && !_view->activeFocusItem()) {
+            widget()->activateWindow();


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.

worldwideweary · 2020-11-06T10:45:11Z

mscore/musescore.cpp

      {
-      if (currentScoreView())
+      if (currentScoreView()) {
+            currentScoreView()->activateWindow();


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.

worldwideweary · 2020-11-06T10:47:35Z

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.

Fix #312442: Fix palette search shortcut functionality

acefb69

Jojo-Schmitz mentioned this pull request Nov 2, 2020

MuseScore 3.6 Alpha. PRs to be merged and issues to be fixed. #6505

Closed

74 tasks

Jojo-Schmitz approved these changes Nov 2, 2020

View reviewed changes

worldwideweary added 2 commits November 4, 2020 09:14

Change palette searching to use QML function syntax

4c24c59

Revert QML of onHasFocusChanged and onElementDraggedToScoreView

3e38b70

Fix search and escape shortcut for floating palette

a7bab93

worldwideweary commented Nov 6, 2020

View reviewed changes

vpereverzev merged commit 52d4835 into musescore:3.x Nov 14, 2020

worldwideweary mentioned this pull request Dec 3, 2020

Fix #313926: Correct focus-in problem when a dialog box is open #7005

Merged

8 tasks

Fix #312442: Fix palette search shortcut functionality #6802

Fix #312442: Fix palette search shortcut functionality #6802

Uh oh!

Conversation

worldwideweary commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitrio95 commented Nov 2, 2020

Uh oh!

worldwideweary commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jojo-Schmitz commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jojo-Schmitz commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

worldwideweary commented Nov 3, 2020

Uh oh!

dmitrio95 commented Nov 3, 2020

Uh oh!

Jojo-Schmitz commented Nov 3, 2020

Uh oh!

worldwideweary commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jojo-Schmitz commented Nov 4, 2020

Uh oh!

worldwideweary commented Nov 4, 2020

Uh oh!

Jojo-Schmitz commented Nov 4, 2020

Uh oh!

worldwideweary commented Nov 4, 2020

Uh oh!

Jojo-Schmitz commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

worldwideweary commented Nov 4, 2020

Uh oh!

Jojo-Schmitz commented Nov 4, 2020

Uh oh!

worldwideweary Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

worldwideweary Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

worldwideweary Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

worldwideweary commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

worldwideweary commented Nov 2, 2020 •

edited

Loading

worldwideweary commented Nov 2, 2020 •

edited

Loading

Jojo-Schmitz commented Nov 3, 2020 •

edited

Loading

Jojo-Schmitz commented Nov 3, 2020 •

edited

Loading

worldwideweary commented Nov 4, 2020 •

edited

Loading

Jojo-Schmitz commented Nov 4, 2020 •

edited

Loading

worldwideweary Nov 6, 2020 •

edited

Loading