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

Port to Qt 6.3.0 due CMake/Wasm fixes #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Port to Qt 6.3.0 due CMake/Wasm fixes #1

wants to merge 1 commit into from

Conversation

dantti
Copy link
Contributor

@dantti dantti commented Apr 25, 2022

No description provided.

@dantti dantti requested review from winterz and dfaure-kdab April 25, 2022 21:56
@dantti
Copy link
Contributor Author

dantti commented Apr 25, 2022

BTW this drops Qt5, not sure how to import on QML based on that which is why I found it easier to just drop it

Copy link

@krf krf left a comment

Choose a reason for hiding this comment

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

Just reviewed the CMake bits as this PR popped up in my notifications.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@@ -9,16 +9,32 @@

project(KDABBoatDemo)
cmake_minimum_required(VERSION 3.9)
find_package(Qt5 CONFIG REQUIRED COMPONENTS Core Gui Quick)

find_package(Qt6 6.3.0 COMPONENTS Core Quick Core5Compat REQUIRED)

Choose a reason for hiding this comment

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

Random question, do we want to drop Qt 5 support entirely, or keep this code compile in both 5 and 6?

(I don't know if a decision has been made in that regard.)

Of course one can require Qt 6 for a WASM build.


SwipeView {
Background {

Choose a reason for hiding this comment

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

Need to test the changes in here to understand what they're about.

Are they self contained? I guess they could be split in their own commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added another Item to fill the parent, because wasm was not starting at the right size (only after a click it noticed the window size), with this it just works

@@ -21,7 +21,7 @@ Flickable {
flickDeceleration: 350
Image {
id: image
source: "/img/maps/resources/images/maps/shot"+map.numberi
source: "/img/maps/resources/images/maps/shot" + map.numberi + (Qt.platform.os === "wasm" ? ".png" : "")

Choose a reason for hiding this comment

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

This is weird.

Is this a Qt 6 regression? Is there a bug report somewhere? Qt is supposed to detect that one can't load compressed textures and fall back on the .png files gracefully.

Side note: if WASM can't make use of the compressed textures at all (don't really know, need to check), then it's worth splitting the .qrc files and not shipping such textures on this platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me it's a regression, didn't look at bug reports, it certainly would be good to remove them as they probably add ~4MB..

@@ -9,7 +9,7 @@
*/

import QtQuick 2.0
import QtGraphicalEffects 1.0 // for OpacityMask
import Qt5Compat.GraphicalEffects // for OpacityMask

Choose a reason for hiding this comment

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

And this is annoying to keep working across 5 and 6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which is why I just dropped Qt5 :)

@winterz
Copy link

winterz commented Sep 12, 2022

this is a demo. I don't think we need to support qt5 and qt6.
qt6 is good enough

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

Successfully merging this pull request may close these issues.

4 participants