-
Notifications
You must be signed in to change notification settings - Fork 224
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
Build: iOS: Use Qt6 and bump build environment #3065
Conversation
@ngocdh as you initially fixed the iOS GUI (I think), do you have any insights on what could be the issue with Qt 6? |
Hi @ann0see I only worked on iOS audio code and maybe some UI related issues but not iOS related if I recall correctly. I did try to fix some mobile UI issues, but not knowing Qt, I made it even worse so eventually gave up. |
Ok. Then someone else fixed them. |
#1450 maybe? |
Ah yes. Maybe something is not set correctly in the conditional or we have the multiple window support enabled. |
@@ -53,6 +53,11 @@ CClientSettingsDlg::CClientSettingsDlg ( CClient* pNCliP, CClientSettings* pNSet | |||
layout()->setMenuBar ( pMenu ); | |||
#endif |
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.
Smells like avoidable duplication.
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.
Do we live with this for now as it's not directly related?
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.
Unrelated, but worth an issue
Ok. I think the chat dialog needs to be fixed (which crashes if I tap on close...) still - but that was the correct direction... |
@@ -148,14 +148,14 @@ CConnectDlg::CConnectDlg ( CClientSettings* pNSetP, const bool bNewShowCompleteR | |||
// set a placeholder text to explain how to filter occupied servers (#397) | |||
edtFilter->setPlaceholderText ( tr ( "Filter text, or # for occupied servers" ) ); | |||
|
|||
// setup timers |
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.
I'm not sure if the order we set the maximized state matters. At least in the version which kind of worked - I first needed to rotate the screen to allow it getting the correct sizing - but that's way better than it was.
Ok. At least connecting works - servers with welcome message bring issues due to the crash if close is tapped (this doesn't happen if I'm not connected to a server but the chat window is open) Hmm. Maybe that's wrong. The crash log gives
Probably we dereference a null pointer. Edit: Also changing to the Android way of the close button in the edit window doesn't change anything. |
da0dab7
to
5c11ddc
Compare
The iOS log says at the start of the iOS app
I don't think that's related but interesting to know. I'll downgrade the SDK. Maybe that fixes it? I don't really understand the issue. To me it seems like we dereference a NULL pointer.
Is probably the place where the error occurs.
seems to be where the system logs the error. |
Now a linking error... I'll try to downgrade to Qt5 and then raise another PR if that works. |
Ok. It could be that just adding the flags doesn't solve the problem. We might need to load some plugins manually https://stackoverflow.com/questions/45508043/qt-ios-linker-error-entry-point-main-undefined |
The build seems to also produce binaries for the iOS simulator - so the IPA would also run on the x86 mac simulator - which is not what we want. I think deploy_ios.sh needs to be tweaked too... https://forum.qt.io/topic/140559/qmake-cmake-build-apk-and-ios-app-via-terminal/5 |
I believe the change in deploy_ios.sh removed the simulator as target. This needs further investigation (might be related to the issue not needing the macOS workaround). I believe this PR should be considered as in development now. The Qt5 one should be ready. |
Crash-fix by @danryu. Co-authored-by: Dan G <dan.garton@gmail.com> Fixes: jamulussoftware#2711 Fixes: jamulussoftware#2939
* Enforce fullscreen mode on mobile OS * Unify iOS and Android logic
087128a
to
f097dc4
Compare
The warning sounds bad here. Xcode would like us to use Putting https://stackoverflow.com/questions/65943369/what-is-a-headermap-in-c-or-c-or-objective-c here Edit: Maybe it's not the case: https://developer.apple.com/documentation/xcode-release-notes/build-system-release-notes-for-xcode-10 Also it doesn't find lrelease... |
So no change there. The only thing C++ seems to do is drop the |
## Builds an ipa file for iOS. Should be run from the repo-root | ||
|
||
# Create Xcode file and build | ||
qmake -spec macx-xcode Jamulus.pro | ||
eval "${qmake_path} Jamulus.pro" |
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.
I don't like eval here
Ok. Could get another run on the simulator and got a crash:
|
It could also be a race condition. But that would be out of scope of Jamulus and a Qt bug? |
hide() instead of close() works. So tat would be an option. |
We've already got issues on Android where the application gets hidden without going through close and not saving settings. I'd beware of hiding rather than closing when the user asks to exit until we're catching all OS signals indicating we should save state (there's a Qt signal, IIRC). |
I know. But it's hard to debug. I have the feeling that Qt6 (Widgets) isn't ready at all. |
Any progress on this or can it be dropped from 3.10.0 (with #2711)? |
I think this can be dropped. I think it could also be a Qt bug which we can't really fix. |
Unfortunately I've lost track on this. Qt 6 UI bugs also show up on bigger devices but that's the main issue. |
Is there any way to split the Jamulus application changes out from the build tool chain changes? Can one be made without the other or must they go in together? |
Potentially yes. As I said, This needs to be recreated anyway. |
Adds a Crash-fix by @danryu for a new ios Qt 6 build. Unfortunately, Qt6 makes the app basically unusable on small devices. This should be fixed after this PR got in:
Short description of changes
Updates iOS to use Qt6 (coded by @hoffie)
CHANGELOG: Update iOS build to use Qt6. For now, only larger devices are usable. Please compile with Qt5 if you use small devices.
Context: Fixes an issue?
Fixes: #2711
Fixes: #2939
Does this change need documentation? What needs to be documented and how?
Status of this Pull Request
Ready for review
What is missing until this pull request can be merged?
Nothing. However, a follow up for the GUI fix (?) should come in after the merge of this.
Checklist