Settings API can make a remote crash

(Jan Koudijs) #1

Hi @ben,

I’m running into an issue with the settings API and I hope you can help me.

The issue is that opening the settings of extension A and then switching to the settings of extension B can make the remote crash. Whether it crashes seems to depend on the settings layout, especially the use of groups seems to be relevant.

I have been trying to track this issue down for a couple of hours, and at this point my impression is that the issue is not in my code. I succeeded in isolation the issue by creating 2 very basic extensions. I hope that you can reproduce the issue by running these extensions from the code below.

The steps I take are:

  • Open the settings of extension 2 and open the drop down item
  • Cancel the dialog by pressing outside the dialog (all is well)
  • Open the settings of extension 1 and open the drop down item
  • Cancel the dialog by pressing outside the dialog (all is well)
  • Open the settings of extension 2 and try to open the drop down item (remote crashes)

I noticed that placing the zone picker of extension 1 in a group prevents the crash.

Extension 1:

"use strict";

var RoonApi          = require("node-roon-api"),
    RoonApiSettings  = require('node-roon-api-settings');

var core = undefined;

var roon = new RoonApi({
    extension_id:        'com.example.1',
    display_name:        'Example 1',
    display_version:     '0.1.0',
    publisher:           'Example',
    email:               'one@example.com',

    core_paired: function(core_) {
        core = core_;
    },
    core_unpaired: function(core_) {
        core = undefined;
    }
});

var my_settings = roon.load_config("settings") || {
};

function makelayout(settings) {
    let l = {
        values:    settings,
        layout:    [],
        has_error: false
    };

    l.layout.push({
        type:    "zone",
        title:   "Zone",
        setting: "zone"
    });

    return l;
}

var svc_settings = new RoonApiSettings(roon, {
    get_settings: function(cb) {
        cb(makelayout(my_settings));
    },
    save_settings: function(req, isdryrun, settings) {
        let l = makelayout(settings.values);
        req.send_complete(l.has_error ? "NotValid" : "Success", { settings: l });

        if (!isdryrun && !l.has_error) {
            my_settings = l.values;
            svc_settings.update_settings(l);
            roon.save_config("settings", my_settings);
        }
    }
});

roon.init_services({
    required_services:   [ ],
    provided_services:   [ svc_settings ]
});

roon.start_discovery();

Extension 2:

"use strict";

var RoonApi          = require("node-roon-api"),
    RoonApiSettings  = require('node-roon-api-settings');

var core = undefined;

var roon = new RoonApi({
    extension_id:        'com.example.2',
    display_name:        'Example 2',
    display_version:     '0.1.0',
    publisher:           'Example',
    email:               'two@example.com',

    core_paired: function(core_) {
        core = core_;
    },
    core_unpaired: function(core_) {
        core = undefined;
    }
});

var my_settings = roon.load_config("settings") || {
};

function makelayout(settings) {
    let l = {
        values:    settings,
        layout:    [],
        has_error: false
    };

    l.layout.push({
        type:    "group",
        title:   "group1",
        items:   [{
            type:    "dropdown",
            title:   "Drop Down",
            values: [
                { "title": "Off", "value": 0 },
                { "title": "On",  "value": 1 }
            ],
            setting: "dropdown"
        }]
    });

    return l;
}

var svc_settings = new RoonApiSettings(roon, {
    get_settings: function(cb) {
        cb(makelayout(my_settings));
    },
    save_settings: function(req, isdryrun, settings) {
        let l = makelayout(settings.values);
        req.send_complete(l.has_error ? "NotValid" : "Success", { settings: l });

        if (!isdryrun && !l.has_error) {
            my_settings = l.values;
            svc_settings.update_settings(l);
            roon.save_config("settings", my_settings);
        }
    }
});

roon.init_services({
    required_services:   [ ],
    provided_services:   [ svc_settings ]
});

roon.start_discovery();
Roon Extension: Deep Harmony - rich feature set for Logitech Harmony
(Ben) #2

I think I have seen this before too. The fundamental bug is that the GUI layout code for the extension settings page is totally unhardened against invalid data, so any time something is missing or broken in the settings pages it crashes.

I’ll add this to our test cases at least, thanks for the clear report :slight_smile:

(Jan Koudijs) #3

Do you see any invalid data in the given example code?

It might be that some property is missing. The difficulty with the settings API is that there is no documentation available, all possibilities have to be extracted from the Roon extensions. I made an attempt to collect all UI elements and their properties, both required and optional. It would be great if you can have a look at it and make it complete/fix errors.

I have searched for a switch element, like the one that’s used in the Roon UI, just by guessing a type (e.g. button, switch, toggle) but without success. Is there a switch available within the settings API?

Something like this:
switches

This is the UI element overview I created:

type type title subtitle values setting items min max max
length
collaps
able
needs_
feedback
zone R R O R
dropdown R R O R R O
integer R R O R O O
string R R O R O
group R R O R O
label R R O

R = Required, O = Optional

(Ben) #4

Looking at this crash more carefully, I think I agree that it’s a Roon client bug. I agree that there doesn’t seem to be a problem with the data, and the actual location of the crash is different from the ones I found.

Regarding the switch element question, here’s a slightly expanded table:

type type title setting values items min max max
length
collaps
able
group button_id
password R R R O
string R R R O
integer R R R O O
ip R R R
checkbox R R R
radio R R R R
dropdown R R R R
zone R R R
title R R
status R R
group R R R O
button R R R

R = Required, O = Optional

I am not sure what you mean by “needs_feedback” or “subtitle”. I also don’t know where you got the label element type.

I think the IP, Password, and String types are basically the same right now, and the “status” type also needs a “is_error” bool. I think the checkbox type does the thing you need.

2 Likes
Provide button and checkbox for roon extension
(Jan Koudijs) #5

Thanks Ben, very useful information!
I have some settings dialog rework to do :slight_smile:

I found them in roon-extension-iport-sm-buttons:

subtitle (line 77-84)

let v = {
    type:    "integer",
    min:     100,
    max:     2000,
    title:   "Long Press Timeout (milliseconds)",
    subtitle: "This is how long you have to hold the button down to register as a long press.",
    setting: "longpresstimeout",
};

needs_feedback (line 165-171)

ledgroup.items.push({
    type:           "dropdown",
    title:          "Music Playing Color",
    setting:        "led_playing_" + i,
    values:         colorvalues,
    needs_feedback: true
});

label (line 226-229)

group.items.push({
    type:    "label",
    title:   "Connecting to " + ip,
});

The label type is the only one I have used, it might be equal to a group without items.

(Jan Koudijs) #6

I tried the checkbox by adding the following code to the makelayout function of extension 1:

l.layout.push({
    type:    "checkbox",
    title:   "Checkbox",
    setting: "checkbox"
});

Unfortunately only the title is shown, there is no switch:

Do you know what’s going on?

(Jan Koudijs) #7

Hi @ben,

I got a report from a user about a crashing Roon Remote.

Is there any action going on to solve the Settings API issue?

#8

Hi I tried the button to trigger a custom action, but it is not showing up on the extension menu.

items.push({
type:    "button",
title:   "Button ",
button_id: "pushButton",
});

I also tried an integer for the button_id.
The text is shown normally but no button is drawn.

(Jan Koudijs) #9

Hi @ben, @danny,

Is the fixing of this crash still on your TODO list? Currently there is always the risk that two extensions are running side by side and the bug hits the user, causing a pretty bad user experience. It is also not very efficient if the individual extension developers have to implement a workaround for this.

I hope it can get some attention.

(Adam Goodfellow) #10

Has anyone discovered a way too make a group pre-expanded, but still collapsible?

On the subject of groups, it seems another way to crash the settings UI is as follows:

  • Create a non collapsible group, stick some drop downs in it.
  • In response to a change in a setting (outside the group), omit one of the drop downs from the group on the next save_settings with dryrun = true callback.

This seems to regularly crash the UI, OTOH if the groups are collapsible, then it works fine.

(Jan Koudijs) #11

I searched for it but no luck :frowning: