Feature/Skill-Settings-In-GUI Revised API

[AIIX]

Description

Adds the ability for a skill display its skill configuration parsed from Settings Meta JSON in skill GUI.
This Feature is a complete refactor over the previous implementation #2175, and works independently of any enclosure / platform skill.

It provides a cleaner and simpler interface for displaying Active Skills settings in GUI.

Adds two methods for skill authors:

• self.gui.register_settings(): Allows skills authors to register their skill configuration in GUI, Adds additional GUI handler events on registration to handle syncing and applying skill settings in the GUI UI interface.
• self.gui.display_settings(): Generates UI interface with settings as per the sections and fields on the fly as defined in the Settings Meta JSON with sync from settings.json and Displays the active skill settings in GUI.

The helper util class used internally is responsible for performing the following actions:

• Populate: Reads the Settings Meta JSON and skills own Settings dictionary to create a data model with synced values for GUI to parse.
• Fetch: Returns the populated data model.
• Update: Performs a sync on the new and updated values from Settings.JSON when settings are updated on the web interface and updates the data model parsed in GUI.
• Clear: Clears the previous data model.

How to test

def initialize(self):
    self.gui.register_settings()

def display_settings(self): 
    # Example function to call display settings 
    # Can be registered with a voice intent, or connected to any button click event in GUI.
    self.gui.show_settings()

Contributor license agreement signed?

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

[AIIX]

Responsive UI for sections is generated on the fly based on the Sections and Fields defined in the Settings Meta:

Horizontal Display Example:

Vertical Display Example:

Single Section Example:

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[JarbasAl]

love this <3 now we just need the selene issue fixed so it does not overwrite local changes

[JarbasAl]

i assume we need to call self.gui.update_settings on the skill settings changed callback in core? cant that be automated? somewhere in here https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/skills/mycroft_skill/mycroft_skill.py#L293

other than that the PR looks good, i dont understand QML very well to review that, but the api is nice and simple, i like it!

[AIIX]

i assume we need to call self.gui.update_settings on the skill settings changed callback in core? cant that be automated? somewhere in here https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/skills/mycroft_skill/mycroft_skill.py#L293

other than that the PR looks good, i dont understand QML very well to review that, but the api is nice and simple, i like it!

No, it is already automated, this function is not to be run manually, its automatically called on settings page when "mycroft.skills.settings.changed" event is emitted for the skill.

[JarbasAl]

sry, missed that in the QML files! was looking for it in python

I think the PR is great then, im ok with merging as is

[forslund]

This looks great! I haven't run it yet. but everything points to awesomeness!

I made some tiny code style comments but you may choose to ignore them if they don't make sense :)

[AIIX]

Latest commit also adds missing method in remove_pages() to check if the UI is provided by System and adds a handler/method for the back button to return to the previous page from the configuration page.

[devops-mycroft]

Voight Kampff Integration Test Failed (Results)

[AIIX]

All conversations/issues with the implementation seem to be resolved as per @forslund review, what is the merge status for this PR as per the new roadmap https://github.com/MyacroftAI/mycroft-core/projects/3#card-45932300

[AIIX]

Also Voight Kampff failing doesn't seem to relate to any issues with the current commit on the PR

[krisgesling]

Hey, it sounded like @forslund hadn't had a chance to actually test run it. So want to give it a run on a Mark II prototype before we merge it in.

The roadmap is still a very early draft, so lots needs to get added to that. I'm also wondering if the column "Longer term" gives the wrong impression. Really this is just to have "next" as the highest priority, and "longer term" being the next highest priority.

[forslund]

What's the best way to try this?

I tried adding a self.gui.show_settings() in the weather skill. It showed the page but it was blank.

Never mind... forgot to register the settings *face-palm*

Looks good :)

[forslund]

I also just realized that the settingsmeta may either be a json or a yaml file (slowly moving over to the yaml format)

[krisgesling]

Is the settings registration intensive at all? Can we do this automatically for each Skill if a GUI is connected?

It's looking pretty good on the Mark II as well. A few display things I noticed:

• Each setting should have a label property that we use as the "title" of that setting. Currently it seems to be using the name which is the variable name used in the Skill.
• The select type I think needs some tweaking. Checkout the News Skill as a good example. It seems to be showing the value, rather than the label of the option.
• Also in the News Skill because it's got a long list of options the block of radio buttons takes up more vertical space, and the name of block is vertically centered on the left. I wonder if this should be top aligned so the settings "hang" if that makes sense.

@dschweppe I'm sure you'll want to see these UI screens too.

[krisgesling]

I've been thinking through the experience of this a bit more, and I need to do some testing to see how it operates and answer these questions but wanted to get the thoughts out of my head...

Given we don't currently have two way settings syncing, how do we handle the different scenarios of users modifying a setting in different locations? Eg on device, then choosing something else in Selene. Does the local setting take precedence, do we track when a change was made and take the most recent, if the Skill is restarted will it pull the remote settings and overwrite local changes?

This becomes two questions - what is our desired behaviour, and how do we make that clear to users so we aren't setting up the expectation of a feature that doesn't currently exist?

[forslund]

It don't look to intensive. we could possibly do it when the show_settings is first called and then when it has been registered once.

your second question is harder to answer I think. These are my thoughts, (and things here are mostly from memory so they may be wrong)

I believe currently the settings set in the GUI will be dominant until a change is made on home. The problem I think is when core is restarted...then all messages from home is considered changed overwriting all settings.

One improvement if not solution is to store the big settings update (all skill's settings comes in one big json structure) from home to disk on shutdown. So when starting up and receiving settings it can check if these are new.

We might also want to separate gui and remote settings in the skill allowing them to exist side by side possibly adding a meta data field for each variable containing a timestamp for the update so the skill's settings can use the most recent version... Not sure how simple a system like that would be... Might not be needed either if the gui gets updated with new settings from home only if a change has been made.

[AIIX]

I also just realized that the settingsmeta may either be a json or a yaml file (slowly moving over to the yaml format)

I will add in the yaml file parsing, is there any mycroft skill using yaml settings that I can use ?

[AIIX]

Is the settings registration intensive at all? Can we do this automatically for each Skill if a GUI is connected?

I think it should be easy if the skillGUI class is initialized for every skill, could call the registration to happen there instead of manually for each skill, this is provided we want to always have all skills to display their configuration in GUI.

The manual registration provided the choice up to the skill authors if they wanted to display their skill configuration on the GUI.

It's looking pretty good on the Mark II as well. A few display things I noticed:

• Each setting should have a label property that we use as the "title" of that setting. Currently it seems to be using the name which is the variable name used in the Skill.
• The select type I think needs some tweaking. Checkout the News Skill as a good example. It seems to be showing the value, rather than the label of the option.
• Also in the News Skill because it's got a long list of options the block of radio buttons takes up more vertical space, and the name of block is vertically centered on the left. I wonder if this should be top aligned so the settings "hang" if that makes sense.

I'll look into Label instead of Name for display key of settings fields, I have tested this against the weather skill, time & date skill, and some more skills by Jarbas. Will test the display against the news skill settings fields, should move selection to a vertical column layout that should be better than the row layout so incase there are any number of options it doesn't get cramped up in the limited width of each card.

@dschweppe I'm sure you'll want to see these UI screens too.

[AIIX]

Added yaml support and also fixed the layouts more for different field types, also should now be using labels instead of name for display key:

• Moved Input type fields to double rows, since labels can be really long and there is no character cap set on them, having the label and entry field in double columns will end up with the entry field becoming barely visible if label is too long, hence each should be in its own row.

• Added support for more fields like numbers only field as per the settings yaml in the demo skill.

• Options type fields dependent of on the number and available options and width of the card should auto layout in columns in relation to the available width, its cleaner this way, than having a single column.

• If the options are limited to just 3 they should up as single rows, because even if the width can try fitting those in a the width card, if option labels are too long they will tend to cut of, so better approach is having them on different rows.

• Also in responsive view:

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[JarbasAl]

any update on this one?

[pep8speaks]

Hello @AIIX! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-13 12:34:44 UTC

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[AIIX]

any update on this one?

all issues hopefully as pointed are fixed and clear, if not please let me know the concerns to get this merged

[krisgesling]

I think we would need to at least merge #2707 before this can go in.

However even then it faces a big UX issue until we add in two way settings sync. How do we let users know that changes on the device won't be synced to the backend? Without some indication that this is the case I think this will be very confusing to users.

Really I don't think we can add it as a feature until we have two-way sync, or some way to inform users about it's behaviour.

[JarbasAl]

It would make more sense to merge this + #2734 instead of blocking on a backend side fix that has no ETA.

Don't block a solution without providing another....

[forslund]

I agree with @JarbasAI, this together with #2734 is also a good pairing. Users could switch to just use local settings.

I also think #2707 should be merged, it makes updating more sane in my opinion.

[forslund]

Closing PR since we're archiving the repo