Add QML file server for Container implementations #38
Conversation
…ocal server is disabled
|
Im still not convinced this belongs in skills.... if the gui service is not running then this should not be running, makes more logical sense for the server to go into the gui service imho we have a gui section in mycroft.conf, having to edit the skill section for gui options is very confusing for end users, i think this further validates that the server belong in the gui module I assume the only reason you put this in skills service was because of docker containers? |
Correct; |
|
i anticipate some issues later in OCP but out of scope of this PR, this class is duplicated in ovos_utils (the aim was to allow standalone apps before ovos-core and ovos_workshop existed) so it will need to be updated or deprecated in there in a follow up PR standalone ovos apps from ovos workshop are the main use case where we want a gui outside skill class, so maybe lets move discussion there update the mycroft.conf please and this PR can be merged |
|
Ah, forgot to say |
|
remote-server makes some sense, because it can be shared with other stuff non gui specific @AIIX can you do the final approval since gui is your department? i think this looks good |
| if page: | ||
| if self.remote_url: | ||
| page_urls.append(self.remote_url + "/" + page) | ||
| elif page.startswith("file://"): | ||
| elif "://" in page: |
There was a problem hiding this comment.
will this not conflict with remote, if self.remote_url = "https://127.0.0.1/" , elif will also catch "://" ? maybe this should depend on if the inbult http server is enabled in addition to "://"
There was a problem hiding this comment.
If self.remote_url is defined, this clause will never be parsed. I added this because I think :// will always relate to an absolute path, so some absolute path like smb:// or http:// could be defined without self.remote_url (i.e. I want to reference an exact QML file I host without leaving it up to a file resolver method)
There was a problem hiding this comment.
Could it still run the startswith or would that be some slow regex stuff?
| super().end_headers() | ||
|
|
||
|
|
||
| def start_qml_http_server(skills_dir: str, port: int = 8000): |
There was a problem hiding this comment.
I wonder if using port 8000 is a wise thing todo as it is being used as alternative http port and many other programs already use it as well. This might run into conflicts a lot.
Perhaps choose another port as default, just like the 18181 for the websocket?
There was a problem hiding this comment.
good call, many demo apps use 8000 ...
There was a problem hiding this comment.
Or make it a config variable just like the gui websocket port and if not set, make it gui websocket +1
There was a problem hiding this comment.
Oh yeah, thanks for catching this. I meant to put this in a config option too
|
Per discussion on Matrix, this needs changes to account for multiple skills directories. Proposed solution: within the now defined qml dir in |
|
@NeonDaniel the latest resource refactor might be of help here |
|
Worked out an initial implementation in Neon: |
|
Re-implemented in NeonCore to handle plugin skills NeonGeckoCom/NeonCore#259 |
|
replaced by OpenVoiceOS/ovos-gui#9 |
No description provided.