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.

Sign up for GitHub

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

Jump to bottom

[MU4] Fixed compiler warnings and move to Qt 5.15 #6580

Merged
merged 2 commits into from
Oct 9, 2020
Merged

[MU4] Fixed compiler warnings and move to Qt 5.15 #6580

merged 2 commits into from
Oct 9, 2020

Conversation

RomanPudashkin
Copy link
Contributor

No description provided.

@RomanPudashkin RomanPudashkin changed the title [MU4] Fixed compile warnings [MU4] Fixed compiler warnings Sep 25, 2020
@RomanPudashkin RomanPudashkin force-pushed the warnings_fix branch 2 times, most recently from cd70c1c to 306b436 Compare September 26, 2020 09:16
@RomanPudashkin RomanPudashkin mentioned this pull request Oct 2, 2020
Merged
Jojo-Schmitz
Jojo-Schmitz reviewed Oct 2, 2020
View reviewed changes
mu4/instruments/view/instrumentlistmodel.cpp
@@ -204,7 +204,7 @@ void InstrumentListModel::unselectInstrument(const QString& id)

void InstrumentListModel::swapSelectedInstruments(int firstIndex, int secondIndex)
{
m_selectedInstruments.swap(firstIndex, secondIndex);
m_selectedInstruments.swapItemsAt(firstIndex, secondIndex);
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Requires Qt 5.13 or later, so better protect this with a corresponding #if, like done elsewhere:

void InstrumentListModel::swapSelectedInstruments(int firstIndex, int secondIndex)
{
#if (QT_VERSION >= QT_VERSION_CHECK(5, 13, 0))
    m_selectedInstruments.swapItemsAt(firstIndex, secondIndex);
#else
    m_selectedInstruments.swap(firstIndex, secondIndex);
#endif
    emit selectedInstrumentsChanged();
}

(those other two places wrongly (?) attribute this to Qt 5.14 though, maybe I misread the Qt documentation back when I added those)

Yes, I know, this stuff is a bit of a pain, but as far as I can tell it hasn't yet been finally decided whether MuseScore 4 will use Qt 5.15 or 5.12, last time I heard that it'll be 5.12, 5.15 being too new still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MU4 will use Qt 5.15. This is the final decision :)

Copy link
Contributor

@igorkorsukov igorkorsukov Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@Jojo-Schmitz Could you tell me if you think it makes sense to support builds for Qt 5.12, 5.13 etc?

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

MU4 will use Qt 5.15. This is the final decision :)

Would have been nice it that had been discussed/published in the developers' chat, or did I just miss this? Last word I remember was that 5.15 would be too young and risky.

5.13 may not make much sense (but is the minimum version needed in this case), Qt 5.12 might. Not sure all platforms we support can use Qt 5.15? Some won't even work with Qt 5.12, like macOS 10.10 and 10.11, and I believe we should still be at least able to build for those. Those need Qt 5.9 (or below). Not sure how many more OS versions we loose when jumping to Qt 5.15?

Copy link
Contributor

@igorkorsukov igorkorsukov Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

And what could be the reasons for support the Qt 5.12 build?

Copy link
Contributor

Choose a reason for hiding this comment

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

Quite a few of those #ifdefs are in the code already.
What new featured are we using then?

Copy link
Contributor

Choose a reason for hiding this comment

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

image
https://gs.statcounter.com/macos-version-market-share/desktop/worldwide
These are not MuseScore statistics, but statistics on the use of the OS in general

MacOS 10.10 - 2.43%
MacOS 10.11 - 3.82%
MacOS 10.12 - 4.8%

When MuseScore 4 is released (not earlier than half a year), the use of these versions will be even less.

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Loosing ~11% of the Mac users is quite a number

Copy link
Contributor

Choose a reason for hiding this comment

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

But we don't lose them :)
They will have MuseScore 3.6

Copy link
Contributor

Choose a reason for hiding this comment

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

Tell them and take the heat ;-)

Jojo-Schmitz
Jojo-Schmitz reviewed Oct 2, 2020
View reviewed changes
mu4/plugins/view/pluginsmodel.cpp
@@ -176,7 +176,7 @@ QStringList PluginsModel::categories() const
result << plugin.category;
}

return result.toList();
return result.values();
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This requires Qt 5.15 as far as I can tell (although I couldn't find hard and fast proof for this in the Qt documentation, but IIRC with Qt 5.12 it didn't work), so better protect this with a corresponding #if, like done in some other places:

        result << plugin.category;
    }

#if (QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)) // ??
    return result.values();
#else
    return result.toList();
#endif
}

void PluginsModel::updatePlugin(const PluginInfo& plugin)

Jojo-Schmitz
Jojo-Schmitz reviewed Oct 2, 2020
View reviewed changes
mu4/userscores/view/templatesmodel.cpp
@@ -31,7 +31,7 @@ void TemplatesModel::load()

QStringList TemplatesModel::categoriesTitles() const
{
return m_visibleCategoriesTitles.toList();
return m_visibleCategoriesTitles.values();
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

Jojo-Schmitz
Jojo-Schmitz reviewed Oct 2, 2020
View reviewed changes
mu4/inspector/models/inspectorlistmodel.cpp
= AbstractInspectorModel::supportedElementTypesBySectionType(model->sectionType()).toSet();
auto supportedElementTypes = AbstractInspectorModel::supportedElementTypesBySectionType(model->sectionType());
QSet<Ms::ElementType> supportedElementTypesSet(supportedElementTypes.begin(), supportedElementTypes.end());
supportedElementTypesSet.intersect(newElementTypeSet);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this for Qt 5.12 too? Or would in deeded to get protected by some #if?

Jojo-Schmitz
Jojo-Schmitz reviewed Oct 2, 2020
View reviewed changes
mu4/inspector/models/inspectorlistmodel.cpp

for (Ms::Element* element: m_notation->interaction()->selection()->elements()) {
elements << element;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@igorkorsukov igorkorsukov changed the title [MU4] Fixed compiler warnings [MU4] Fixed compiler warnings and move to Qt 5.15 Oct 2, 2020
@Jojo-Schmitz
Copy link
Contributor

See also #6588

@igorkorsukov igorkorsukov merged commit 72ca0c3 into musescore:master Oct 9, 2020
igorkorsukov added a commit to igorkorsukov/MuseScore that referenced this pull request Feb 8, 2021
@igorkorsukov
ported musescore#6580, musescore#6541, musescore#7004, musescore#7192, …
1db4458 
…musescore#7212
@igorkorsukov igorkorsukov mentioned this pull request Feb 8, 2021
Merged
vpereverzev pushed a commit that referenced this pull request Feb 8, 2021
@igorkorsukov @vpereverzev
ported #6580, #6541, #7004, #7192, #7212
9fc9146
@RomanPudashkin RomanPudashkin deleted the warnings_fix branch February 3, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jojo-Schmitz Jojo-Schmitz Jojo-Schmitz left review comments

@igorkorsukov igorkorsukov igorkorsukov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@RomanPudashkin @Jojo-Schmitz @igorkorsukov @Eism