Fix missing remove page implementation, show idle if all pages removed

[AIIX]

Description

• Added missing check for "remote" and "system" page urls in remove pages GUI API
• Added check and emit show idle screen if current page position is 0 and no other pages are left in the namespace, ensures the idle screen is activated for the mark-2 enclosure when all pages are removed from the skill

How to test

# Add a page in some function
self.gui.show_page("somepage.qml")
self.gui.show_page("somepage2.qml")
self.gui,remove_page("somepage2.qml")
self.gui.remove_page("somepage.qml")

# if all pages are removed it should go back to idle screen for platform mycroft_mark_2

Contributor license agreement signed?

CLA [x] (Whether you have signed a CLA - Contributor Licensing Agreement

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[krisgesling]

Hey, thanks for this AIIX. I've been noticing this behaviour too.

[krisgesling]

Should this be more generic for anything with a GUI, rather than just the Mark II?

[AIIX]

since there is no other platform that uses the idle, resting screen except mark II would keep it this way until there is another platform that uses this, but if we want to make it more generic then we can have something more on the lines of "mycroft.devices.reset_idle", will need an added event here (https://github.com/MycroftAI/skill-mark-2/blob/d81aeadd1efcec9309e8d2b5e4519f611e992be9/__init__.py#L221) for it the same event listener as "mycroft.mark2.reset_idle" just with the change message type. Also the issue I see currently with the generic method is that we don't know which platforms / enclosure / devices support an idle screen and which don't.

[forslund]

Feels a bit weird to have a platform flag in the base class.

Could we instead make a method that the Mark2 enclosure overrides so the code is

        if pos == 0 and and len(self.loaded[0].pages) == 0:
            self.on_empty_namespace()

then in the base enclosure just implement that as an empty method

    def on_empty_namespace(self):
        """Override to add special action when a namespace is empty."""

And in the mark2 enclosure have the bus emit

    def on_empty_namespace(self):
        """Go to idle screen."""
        self.bus.emit(Message("mycroft.mark2.reset_idle"))

Alternatively add a special message on empty namespace that the Mark-2 skill can capture and perform the same action as on the "mycroft.mark2.reset_idle" message...

[krisgesling]

Could we emit a mycroft.gui.reset_idle message and then we'll add that into the Mark 2 enclosure (or the Skill for now)?

Any systems that don't have a handler for this message will just disregard it.

[AIIX]

mycroft.gui is reserved namespace for transport protocol so I wouldn't use it for targeting skills, I have added it under "mycroft.display.reset_idle" since we are targeting a display and also removed any platform checks for mk2, any platform displaying the idle screen can now listen to this event when all pages have been removed to reset to idle.

[krisgesling]

Actually there's also an existing bus message being used mycroft.device.show.idle. We should probably use this instead. If we want to change the message structure in the future we would need to update this anyway.

[krisgesling]

but everything is working great with the fixes - including removal of the SYSTEM pages 👍

[AIIX]

Familiarising myself with these bus messages types - I think we want to "show" idle rather than reset it. Based off the current usage, resetting / restoring suggests removing any idle override back to the default idle screen. Whilst I assume we just want to show whatever is registered as the current idle screen.

I think mycroft.display.show_idle would probably be best. I could also see an argument for mycroft.display.idle.show.

Certainly outside of this PR, but why isn't the idle screen handled by the GUI framework itself? I'm presuming this is because it's not wanted in other implementations like Big Screen - hence added where it's needed?

There is no concept of idle screen in gui protocol, the protocol only provides communication mechanism and methods to display qml pages, idle screen is a concept on mycroft core gui framework side that is handled by the mark-2 skill and the resting screen class, this resting screen class could have ideally lived in the enclosure code being configurable in some way by a platform if it should use it or not (in case of mobile / bigscreen where it can be configured to be turned off) because its not useful in environments that aren't a smart speaker or only a dedicated mycroft device, but @forslund might know better about its implementation.

[AIIX]

mycroft.device.show.idle

merge request updated, latest commit now uses mycroft.device.show.idle

[krisgesling]

Thanks, that does make sense.

We will shift the resting screen class across to the enclosure, along with most of the Mark 2 Skill I think.

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)