add support for qt6 qml resources #50

AIIX · 2023-02-07T23:59:51Z

Add support for qt6 qml resources for the migration phase, this adds a check on returning the resource file based on the qt version targeted:
An image / setup based on newer distributions shipping Qt version 6 can add the qt_version: 6 flag to the GUI config which will force all qml resources to load from the "ui6" folder instead of the currently spec'd "ui" folder which is used by Qt5

codecov · 2023-02-08T00:04:25Z

Codecov Report

❗ No coverage uploaded for pull request base (dev@a43d887). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff          @@
##             dev     #50   +/-   ##
=====================================
  Coverage       ?   0.00%           
=====================================
  Files          ?      30           
  Lines          ?    2735           
  Branches       ?       0           
=====================================
  Hits           ?       0           
  Misses         ?    2735           
  Partials       ?       0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

JarbasAl · 2023-02-08T03:39:09Z

ovos_workshop/resource_files.py

+        # TODO: Remove this when we drop Qt5 support
+        if qt_version_target == 6:
+            if "ui/" in str(file_path):
+                file_path = Path(str(file_path).replace("ui/", "ui6/"))


this will messup some file names and paths, the check needs to be for full words, maybe something like if "ui" in file_path.split("/") is enough

words = [w for w in file_path.split("/") if w != "ui" else "ui6"] file_path = "/".join(words)

this can not be done, i've seen skills that load stuff such as pictures from the ui folder, this will also mess their paths. at minimum this should have a check if file exists before changing the path, but this may still mess things up if there are duplicate files

JarbasAl · 2023-02-08T03:40:01Z

ovos_workshop/resource_files.py

@@ -268,6 +268,10 @@ def _read(self) -> str:
 class QmlFile(ResourceFile):
    def _locate(self):
        """ QML files are special because we do not want to walk the directory """
+        config = Configuration()
+        enclosure_config = config.get("gui") or {}
+        qt_version_target = enclosure_config.get("qt_version", 5)


can this be auto detected? if missing from config instead of assuming 5 we could do something to get a default value

JarbasAl · 2023-02-08T13:55:00Z

I'd prefer for skills to indicate qt versions some other way instead of renaming this folder, can we just stick to the regular ui folder and add some new mechanism?

with remote instances I also see skills potentially serving qml to several clients each on a different qt version, it seems to me the QT version should be session data per request, I will give this a think and come up with an alternate suggestion

AIIX · 2023-02-08T14:08:03Z

I'd prefer for skills to indicate qt versions some other way instead of renaming this folder, can we just stick to the regular ui folder and add some new mechanism?

with remote instances I also see skills potentially serving qml to several clients each on a different qt version, it seems to me the QT version should be session data per request, I will give this a think and come up with an alternate suggestion

QML files serving QT5 and QT6 cannot live in the same folder, QML does not work like that, unless you want to follow the Mycroft Mark-2 example.

AIIX · 2023-02-08T14:10:53Z

Whatever mechanism the UI folder uses is defined by the QT Framework, including the selection of file selectors, One can either replace the skill UI files to support QT6 or one can stick to QT5, QML files import locally from each other, they cannot be mixed and matched between the different versions.

JarbasAl · 2023-02-08T14:26:22Z

My idea is that after X core version we declare we moved to QT6, somewhere around same time mycroft-gui and ovos-shell move the main branch, then the regular folder would be assumed to be QT6

But we want to keep QT5 support around for clients in server like use cases, so this has a few layers:

a doc describing how to migrate and keep QT5 support around is a must for skill devs
add "default_qt_version" to mycroft.conf (default 6)
runtime requirements object can add some new keys for this, once qt6 is default then we just need a supports_qt5 (default False) and supports_qt6 (default True)
QT version support is per request in a server-client model, this needs some thinking about how to implement but would basically be message.context data and skills can access it like we do with self.lang
gui service would check this message.context when serving QML files
qml files for qt5 compat could move to a subfolder under ui or use some naming convention like name.qt5.qml and be parsed at load time

all gui clients should be compatible with this with no changes, but will need to start sending info about what qt version they require, if missing we maybe should assume qt5 for compat (all exisinting clients), then make all clients start requesting QT6 once they implement it. this is just an extra field in every bus emit and should not be hard to do

I think can send PRs for all this over the next weeks

AIIX · 2023-02-08T15:10:41Z

What you are failing to understand is QT5 and QT6 files cannot live in the same folder a very simple example is:

A skill has 2 QML imports from local UI folder (Homescreen has 10's):

A QML file Import and how they work:

A UI Folder consist of 2 imports and one main page

TopBarView.qml
BottomBarView.qml
MainPage.qml

MainPage.qml

import QtQuick 2.15
import QtQuick.Controls 2.15
import Mycroft 1.0 as Mycroft

Mycroft.Delegate {  <--- Base Delegate that comes from Mycroft Lib not a problem
          
          TopBarView { TopBarView.QT5 {} is not an accepted component object, The filename defines the Component name in QML, it does not follow QT naming scheme
            width: ..
            height :.. 
           other properties and attributes....
          } 
          
         BottomBarView {  <--- Lets say we move QT6 and QT5 in same folder the import structure of QT5 does not meet QT6 import directives, see below for QT5 vs QT6 implementation 
            width: .. 
            height :.. 
           other properties and attributes....
         }
}

QT5 Implementation of BottomBarView.qml which is imported and used as a component BottomBarView { } in an another QML File

BottomBarView.qml under QT5

import Qt.Quick 2.5
import QtQuick.Controls 2.5
import QtGraphicalEffects 1.0

Connections { 
      id: simpleConnectionCPPListener
      target: Mycroft.Something
      onIntentReceived: {
             var local_var = model.data 
            doSomethingWithData(local_var)
      } 
}

Button {
     id: someButton
     onClicked: {
                console.log("I was clicked")
     }
}

Now lets look at the Qt6 Version of that with all the Qt6 changes:

BottomBarView.qml under QT6

import Qt.Quick 2.15 <--- Versions cannot be any lower than this for Qt6
import QtQuick.Controls 2.15  
import Qt5CompatGraphicalEffects <--- First change the main import from QML has completely changed

Connections { 
      id: simpleConnectionCPPListener
      target: Mycroft.Something
      function onIntentReceived(type, data)  {   <-------- Connections syntax has fully changed, all functions and events in QT now require type definition like proper javascript and this is not backwards compatible
             var local_var = model.data 
            doSomethingWithData(local_var)
      } 
}

Button {
     id: someButton
     onClicked: (mouse) => {   <----------- Forward event deceleration is now compulsory for all native attribute events in Qt6 
                console.log("I was clicked")
     }
}

Now say you want to import BottomBarView.qml since BottomBarView.QT5.qml and BottomBarView.QT6.qml are not valid components and nor are components being named BottomBarView_QT5 and BottomBarView_QT6, and MainPage.qml is does not know what is difference between BottomBarView.qml qt6 and BottomBarView.qml qt5 implementation it will import the local component as it is, so a MainPage.qml based on QT5 importing BottomBarView.qml based on Qt6 will not work

This also now causes a larger headache that every skill out there will need to reimplement and rename files, using a folder approach makes the easier one can simply copy all the files from QT5 "UI" folder to QT6 "UI6" folder and only have to deal with internal QML changes than having to implement QT5 and QT6 in every file name and local import.

JarbasAl · 2023-02-08T15:55:38Z

i said

move to a subfolder OR use some naming convention

what you said excludes the naming convention, but thats an implementation details and everything else i said stands, you missed the main points being that:

a simple path replace is unsafe and needs better plumbing, it will mess full file paths
that qt5 is the one that needs moving to a new folder and qt6 should be default
this approach does not consider client server implementations at all, its all either qt6 or qt5, it should be per request
this PR as is requires duplicating every image file etc

AIIX · 2023-02-08T22:41:21Z

This PR is being closed, as its non conclusive and I do not agree to mixing and matching of QT5 and QT6 resources into a single folder for technical reasons mentioned above.
Images / Skills that need to package or support QT6 can use the QT6 branch of the skill until it replaces the normal branch, this means there will be no changes to the UI folder or Path going forward.
Mycroft-GUI being QT6 and OVOS-Shell being QT6 available will be marked as the start of when images can start packaging the QT6 branches of skill, if they wish to or they can simply use the QT5 release and continue packaging QT5 branches.

JarbasAl · 2023-02-09T13:51:31Z

this still needs to be done and all points i raised addressed, if you don't want to do it i'll take over the PR, reopening and will work directly in this branch

AIIX · 2023-02-09T13:57:45Z

This will not be supported at the Mycroft GUI protocol level, mixing and matching QT files and forcing stuff into sub folders inside UI folder will break bigscreen and QT file selectors mechanism.

AIIX · 2023-02-09T14:03:05Z

this approach does not consider client server implementations at all, its all either qt6 or qt5, it should be per request <

GUI protocol does not support this because it not a common practice neither in QT nor in KDE frameworks, it is implied that the version that QT based application / library is compiled against is the version for which the QML files it is expecting and that is decided by distributions on what version they package and they ship.

JarbasAl · 2023-02-09T14:15:58Z

this approach does not consider client server implementations at all, its all either qt6 or qt5, it should be per request <

GUI protocol does not support this because it not a common practice neither in QT nor in KDE frameworks, it is implied that the version that QT based application / library is compiled against is the version for which the QML files it is expecting and that is decided by distributions on what version they package and they ship.

so remote gui is not supported? can i remove the remote server url param that serves QML pages?

doesnt the android app run gui natively and connect to a server?

how does the server know if it should send qml for 5 or 6?

does the server only support qt6 clients and old qt5 clients will all suddenly break? Do we now need to run "legacy servers" so old clients keep working?

client are either qt5 or 6, but for all that core/workshop care they need to be able to serve both qt5 and qt6 files, otherwise you brick every single install out there using the remote setup, the server has no clue what clients were compiled against

simple, just like in this PR MycroftAI/mycroft-gui#100 , you add to context the keys "qt_version": 6 when migrating the libs, this will inform the server what qml version to serve for the client, on server side we do what ive been talking about and add support to send either qml file, the same running server will then support both old clients (qt5) and new (qt6)

JarbasAl · 2023-02-09T14:35:00Z

once this PR is merged workshop should be bumped 1 minor version

existing qt5 skills before migrating to qt6 should make a final release specifying <= X.X.X workshop, this marks the point where ui folder becomes QT6 and will allow setup.py to detect incompatible versions (skills shipping invalid QML still on version 5)

after this skills should start providing QT6 under ui and QT5 under ui5 and specifying >= X.X.X workshop, this marks the point where the skill adopted QT6, the ui5 folder will be optional but strongly recommended at least during initial migration if the skill already existed

over time skills will start being QT6 only, but by then hopefully nearly all existing clients will also have updated to latest

AIIX · 2023-02-09T14:38:09Z

so remote gui is not supported? can i remove the remote server url param that serves QML pages?

What does this have to even do with remote server ? The end application running and consuming the QML files is specifically built and compiled against a QT version

doesnt the android app run gui natively and connect to a server?

The Android application is compiled against a specific QT Version it does not randomly accept Qt5 / Qt6 QML files.

how does the server know if it should send qml for 5 or 6?

The server is hosted by the user using a docker image or setting it up themselves, there is no global hosting of the server currently except the test server Blue Systems uses. The user setting up the server is expected to install the correct versions of the skills and use the correct version of the android application.

client are either qt5 or 6, but for all that core/workshop care they need to be able to serve both qt5 and qt6 files, otherwise you brick every single install out there using the remote setup, the server has no clue what clients were compiled against

The clients could have easily accepted the above solution I proposed of using "UI" and "UI6" but you rejected that and want to force everything into a single UI folder which does not work on technical grounds, causing even more breakage.

simple, just like in this PR MycroftAI/mycroft-gui#100 , you add to context the keys "qt_version": 6 when migrating the libs, this will inform the server what qml version to serve for the client, on server side we do what ive been talking about and add support to send either qml file, the same running server will then support both old clients (qt5) and new (qt6)

This does not mean that UI files can live in the same folder, and also I think you have this backwards it is not GUI that is requesting QML files to be sent to it so there is no flow of that context message ever happening, the skills and GUI interface in core send the data to GUI, the GUI protocol sends no request on its own to a running core "That i want so and so page", the only time GUI sends anything to core on its own without a skill gui interface is connection and disconnection requests.

JarbasAl · 2023-02-09T14:55:16Z

why are you still stuck on "same folder"? it was a question, after you explained i never brought it up again and only requested we move qt5 files instead of qt6 to a subfolder, thats a implementation detail and the thing im less interested in discussing or getting across, dont want both inside ui fine, put ui5 + ui in root folder instead of having ui/5 + ui/6, it doesn't matter, we just need to pick a standard and do it only once since its a breaking change and it will be adopted by more people once we introduce it

I just want this done with clean versioning and without bricking every device out there, please focus on the other points and let's have an actual discussion instead killing dev talk and closing the PR

we can do this in a way that the server can be updated without updating all clients, and by clients here I mean the compiled mycroft-gui or shell or whatever, to illustrate what i mean better:

got a server setup at home, still on QT5
client 1 - android phone on QT5
client 2 - distro that can NOT update to QT6 for some random reason
update server + client 1
client 2 is now bricked forever

while bumping mycroft-gui you just need to add qt_version: 6 to the gui announced message, it's simple, then the server knows what subfolder to server to the clients, the server is only sending full qml file paths and picking the correct one, clients migrated to QT6 cleanly

AIIX · 2023-02-09T15:29:20Z

why are you still stuck on "same folder"? it was a question, after you explained i never brought it up again and only requested we move qt5 files instead of qt6 to a subfolder, thats a implementation detail and the thing im less interested in discussing or getting across, dont want both inside ui fine, put ui5 + ui in root folder instead of having ui/5 + ui/6, it doesn't matter, we just need to pick a standard and do it only once since its a breaking change and it will be adopted by more people once we introduce it

because you mentioned and quoted it again, at least how i read it.

put ui5 + ui in root folder instead of having ui/5 + ui/6, it doesn't matter

I don't disagree with this approach if its being put in the root folder and not forcing it into "ui"

I just want this done with clean versioning and without bricking every device out there, please focus on the other points and let's have an actual discussion instead killing dev talk and closing the PR

As the GUI maintainer I too want this done cleanly and in a logical way that is expected to work for every platform including distribution packaging, no one is killing the dev talk but forcing points is not constructive to dev talk specifically when someone is trying to explain why something cannot be done the way the points have forced it.

while bumping mycroft-gui you just need to add qt_version: 6 to the gui announced message, it's simple, then the server knows what subfolder to server to the clients, the server is only sending full qml file paths and picking the correct one, clients migrated to QT6 cleanly

It has been implemented, the GUI message where the GUID is sent will send the major version as context message on connect https://github.com/MycroftAI/mycroft-gui/blob/4ace3675833298a496b8bdc1c3ddd1f5a22723d2/import/mycroftcontroller.cpp#L74

Note without testing this on Android I cannot confirm if it works there yet ^ but it works for me on Linux when testing against machine running QT5 and QT6 (in docker)

JarbasAl · 2023-02-09T15:34:24Z

is this one valid? MycroftAI/mycroft-gui#123

would like 1 final release in qt5 branch adding it, i added this also in the utterance message, this was just to make the info available to skills in intents if needed for some reason

AIIX · 2023-02-09T15:40:24Z

is this one valid? MycroftAI/mycroft-gui#123

would like 1 final release in qt5 branch adding it, i added this also in the utterance message, this was just to make the info available to skills in intents if needed for some reason

There is a better method for version-detection that comes from QT, I will port the Qt6 branch method to Qt5 current master branch, which will then be released before QT6 is merged in master, will add that to utterance as well.

add support for qt6 qml resources

a188558

AIIX mentioned this pull request Feb 8, 2023

Supporting Qt5 and Qt6, Images and Dependencies Issues OpenVoiceOS/ovos-shell#29

Open

JarbasAl reviewed Feb 8, 2023

View reviewed changes

JarbasAl marked this pull request as draft February 8, 2023 13:55

JarbasAl mentioned this pull request Feb 8, 2023

roadmap - 0.0.8 OpenVoiceOS/ovos-core#274

Closed

AIIX closed this Feb 8, 2023

JarbasAl reopened this Feb 9, 2023

JarbasAl self-assigned this Feb 9, 2023

JarbasAl mentioned this pull request Feb 9, 2023

feat/report_qt_version MycroftAI/mycroft-gui#123

Closed

JarbasAl mentioned this pull request Feb 28, 2023

Gui roadmap OpenVoiceOS/ovos-gui#2

Closed

25 tasks

AIIX closed this Jun 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for qt6 qml resources #50

add support for qt6 qml resources #50

AIIX commented Feb 7, 2023

codecov bot commented Feb 8, 2023 •

edited

Loading

JarbasAl Feb 8, 2023

JarbasAl Feb 8, 2023

JarbasAl Feb 8, 2023

JarbasAl commented Feb 8, 2023

AIIX commented Feb 8, 2023

AIIX commented Feb 8, 2023

JarbasAl commented Feb 8, 2023

AIIX commented Feb 8, 2023 •

edited

Loading

JarbasAl commented Feb 8, 2023

AIIX commented Feb 8, 2023

JarbasAl commented Feb 9, 2023

AIIX commented Feb 9, 2023

AIIX commented Feb 9, 2023

JarbasAl commented Feb 9, 2023

JarbasAl commented Feb 9, 2023

AIIX commented Feb 9, 2023

JarbasAl commented Feb 9, 2023

AIIX commented Feb 9, 2023

JarbasAl commented Feb 9, 2023

AIIX commented Feb 9, 2023

add support for qt6 qml resources #50

add support for qt6 qml resources #50

Conversation

AIIX commented Feb 7, 2023

codecov bot commented Feb 8, 2023 • edited Loading

Codecov Report

JarbasAl Feb 8, 2023

Choose a reason for hiding this comment

JarbasAl Feb 8, 2023

Choose a reason for hiding this comment

JarbasAl Feb 8, 2023

Choose a reason for hiding this comment

JarbasAl commented Feb 8, 2023

AIIX commented Feb 8, 2023

AIIX commented Feb 8, 2023

JarbasAl commented Feb 8, 2023

AIIX commented Feb 8, 2023 • edited Loading

JarbasAl commented Feb 8, 2023

AIIX commented Feb 8, 2023

JarbasAl commented Feb 9, 2023

AIIX commented Feb 9, 2023

AIIX commented Feb 9, 2023

JarbasAl commented Feb 9, 2023

JarbasAl commented Feb 9, 2023

AIIX commented Feb 9, 2023

JarbasAl commented Feb 9, 2023

AIIX commented Feb 9, 2023

JarbasAl commented Feb 9, 2023

AIIX commented Feb 9, 2023

codecov bot commented Feb 8, 2023 •

edited

Loading

AIIX commented Feb 8, 2023 •

edited

Loading