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

fix build with qt6 #16

Merged
merged 1 commit into from Aug 23, 2023
Merged

fix build with qt6 #16

merged 1 commit into from Aug 23, 2023

Conversation

jmlich
Copy link
Contributor

@jmlich jmlich commented Jul 14, 2023

No description provided.

@Tofee
Copy link
Contributor

Tofee commented Aug 3, 2023

@jmlich I see a reference to the -qt5 suffix still here: https://github.com/jmlich/sensorfw/blob/master/core/loader.cpp#L58 , is that a mistake?

@jmlich
Copy link
Contributor Author

jmlich commented Aug 4, 2023

Thanks for spotting of the issue and patch. I must admit that I haven't done much tests.

@mlehtima
Copy link
Contributor

mlehtima commented Aug 4, 2023

Could you squash the commits into one and use commit message that matches the existing commits so something like "[sensorfw] Fix build with Qt6". Also make sure your fork has all tags from this repo so it's easier for me to test the builds without forking your branch.

@jmlich
Copy link
Contributor Author

jmlich commented Aug 5, 2023

I am not sure if you know, that it is possible to squash commits during merge
image

@jmlich jmlich force-pushed the master branch 2 times, most recently from cd6ba9d to fc84fcc Compare August 8, 2023 05:20
@jmlich
Copy link
Contributor Author

jmlich commented Aug 8, 2023

I have also changed mkspec path same as in sailfishos/libiodata#5

qt-api/abstractsensor_i.cpp Outdated Show resolved Hide resolved
@jmlich jmlich force-pushed the master branch 2 times, most recently from 240f158 to 94ad475 Compare August 9, 2023 05:16
Copy link

@Thaodan Thaodan left a comment

Choose a reason for hiding this comment

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

See comments.

I wonder if we can't just define Qt::SkipEmptyParts before Qt 5.14.

examples/sensorplugin/sampleplugin.cpp Outdated Show resolved Hide resolved
sensors/alssensor/alsplugin.cpp Show resolved Hide resolved
sensors/compasssensor/compassplugin.cpp Show resolved Hide resolved
sensors/contextplugin/contextplugin.cpp Show resolved Hide resolved
sensors/tapsensor/tapplugin.cpp Show resolved Hide resolved
tests/benchmark/benchmarktest/signaldump.h Show resolved Hide resolved
tests/benchmark/benchmarktest/signaldump.h Show resolved Hide resolved
tests/testutils/datafaker/main.cpp Show resolved Hide resolved
@Herrie82
Copy link
Contributor

Qt::SkipEmptyParts

I came across something like this: goldendict/goldendict@c4dcd4d

I'm sure there are other solutions too

@Thaodan
Copy link

Thaodan commented Aug 10, 2023 via email

@Herrie82
Copy link
Contributor

Herrie82 commented Aug 10, 2023

Qt::SkipEmptyParts

All depends on how plain vanilla your Qt is or you used a patched version. I guess I know the answer. An additional patch wouldn't hurt in that case.

[edit]
Though patching QtBase would make it SFOS specific and would break others using Qt5 and sensorfw (not sure if there are any though)
[/edit]

@spiiroin
Copy link
Contributor

I'm sure there are other solutions too

As the split args are always the same, using internal to sensorfwd helper function comes to mind. That would leave the API level differentation a) directly observable without need to know/find/ponder about some nonstandard custom values and b) still contained in just one place. Something like ...

QStringList needsSensibleName(const QString &string)
{
#if QT_VERSION >= QT_VERSION_CHECK(x, y, z)
    return string.split(":", Qt::SkipEmptyParts);
#else
    return string.split(":", QString::SkipEmptyParts);
#endif
}

Co-authored-by: Christophe Chapuis <chris.chapuis@gmail.com>
@jmlich
Copy link
Contributor Author

jmlich commented Aug 11, 2023

I personally prefer using the way is it is currently in pull request

@spiiroin
Copy link
Contributor

I personally prefer using the way is it is currently in pull request

IMO it also is just fine as it is. It makes the sw buildable with qt6 which is a clear improvement. Any further cleanup/refactoring/whatnot can be done separately by somebody else too.

@Thaodan do you still have some objections?

@Herrie82
Copy link
Contributor

FWIW I'm impartial to either solution. There are pro's and cons for both to be honest. Most important is that the code works in both Qt5 and Qt6 :)

@jmlich jmlich requested a review from Thaodan August 22, 2023 07:58
@rainemak
Copy link
Member

FWIW I'm impartial to either solution. There are pro's and cons for both to be honest. Most important is that the code works in both Qt5 and Qt6 :)

Yes, I agree with that and changes in general look good now. Thank you @jmlich !

Copy link
Member

@rainemak rainemak left a comment

Choose a reason for hiding this comment

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

Approving as well.

@rainemak rainemak merged commit c2d825c into sailfishos:master Aug 23, 2023
@Thaodan
Copy link

Thaodan commented Aug 28, 2023

I personally prefer using the way is it is currently in pull request

IMO it also is just fine as it is. It makes the sw buildable with qt6 which is a clear improvement. Any further cleanup/refactoring/whatnot can be done separately by somebody else too.

@Thaodan do you still have some objections?

No issues, fine by me just no time to reply.

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