Added QML files that meet Mark II design specs. #71
Conversation
| @@ -149,7 +149,7 @@ def _communicate_create_account_url(self): | |||
| self.log.info("Communicating account URL to user") | |||
| self.account_creation_requested = True | |||
| if self.gui.connected: | |||
| self.gui.show_page("create_account.qml", override_idle=True) | |||
| self._show_page("create_account") | |||
There was a problem hiding this comment.
For some reason this screen disappeared half way through the progress bar loading. It seems to be when the Mark II Skill loads. So we can switch the priority skill order so that the skill-mark-2 is loaded first.
However it still shows the homescreen for a second or two between this screen and the next but perhaps that is from the clear() on line 401?
There was a problem hiding this comment.
nope it looks like the mark-2 skill is reloading once all Skills are ready. All the resting screens get registered and it shows the Homescreen Skill for a short time.
There was a problem hiding this comment.
For the sake of replication steps, to test this I:
- moved identity2.json
- switched to feature branch of pairing skill
- rebooted device
- completed pairing
- re-ordered priority skill loading in SYSTEM config to bump mark-2 to the front
- rebooted device
- completed pairing
- commented out the
self.gui.clear()on line 401 - rebooted device
- completed pairing
There was a problem hiding this comment.
Self.gui.clear() should not be used to remove pages at all, self.gui.clear() is only used for clearing session data, it's triggering a bug in mark2 skill that causes the gui to disappear which should be addressed instead. self.gui.remove_page("example.qml") should be used instead if a page is to be removed, or self.gui.release() if an entire skill and all its pages including session data is to be removed.
Usage of self.gui.clear() as per the api:
https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/enclosure/gui.py#L121
There was a problem hiding this comment.
I replaced the clear() calls with remove(). seems to have fixed the issue. except I think there is a race condition in the GUI code because sometimes when calling remove() and then show() the screen is not removed properly.
I don't think there are any more issues with this PR though. is it ready for approval?
There was a problem hiding this comment.
Can you check in the enclosure logs if this protocol message being sent to the gui when the page is not being removed on self.gui.remove_page() also a enclosure log would help to see what's happening there https://github.com/MycroftAI/mycroft-core/blob/df78af15cb8e736e849b96bcd9a0c36e6e4315c9/mycroft/client/enclosure/base.py#L237
There was a problem hiding this comment.
Please run this stress test skill: I have attached my enclosure log in this, As per the logs, the enclosure doesnt seem to be sending the "mycroft.gui.list.remove" protocol message at all sometimes especially in subsequent show_page, remove_page calls without any delay between them to the mycroft-GUI
This stress test will add and remove a number of pages, with delay and without delay, the bug of the enclosure not sending the above message can be seen when there is no delay involved each page add and remove is logged
To run the test: "run stress test one" in cli
There was a problem hiding this comment.
Can you check in the enclosure logs if this protocol message being sent to the gui when the page is not being removed on self.gui.remove_page() also a enclosure log would help to see what's happening there https://github.com/MycroftAI/mycroft-core/blob/df78af15cb8e736e849b96bcd9a0c36e6e4315c9/mycroft/client/enclosure/base.py#L237
I looked at the enclosure logs with debug turned on and they were empty, which seemed odd. Either way, the remaining issue is with the GUI/Enclosure, not this skill. We can continue this discussion elsewhere.
There was a problem hiding this comment.
I've opened an issue on core so we don't lose sight of this
|
I've switched the priority_skills loading order to be:
This seems to be working better and not flashing the Home screen. If we can find and squash the issue linked above it should be smooth as. 👏 ⛵ |
Description
The QML files for this skill did not conform to the new screen design best practices implemented for the Mark II.
Type of PR
If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR.
Testing
Remove the identity file from your device and re-pair on a Mark II
Documentation
N/A