search

stefano-m / lua-dbus_proxy

Simple API around GLib's GIO:GDBusProxy built on top of lgi
https://stefano-m.github.io/lua-dbus_proxy/
Apache License 2.0
17 stars 7 forks source link

extend set_property? #3

Open terminar opened 5 years ago

terminar commented 5 years ago

In _proxy.lua, the getter's and setter's are not as intuitive as they should be.

get_property returns some sort of Lua'ized version of the result where some of the GVariants are translated to plain Lua. set_property does nothing like auto conversion to GVariant.

Maybe a minimum solution would be something like:

local function set_property(proxy, name, opts)
  local variant_value
  local vtype = type(opts)
  if vtype == "userdata" and tostring(opts):find("GLib%.Variant$") then
    variant_value = opts
  elseif vtype == "table" and opts.signature and opts.value then
    variant_value = GVariant(opts.signature, opts.value)
  else
    error("Wrong parameter, should be table with { .signature and .value } or GVariant")
  end
  proxy._proxy:set_cached_property(name, variant_value)
end

This may report a correct error and also accepts GVariants if they are passed. It may also be nice to have a converter for the basic types (number, string) but that's just an idea.

What do you think?

stefano-m commented 5 years ago

get_property returns some sort of Lua'ized version of the result where some of the GVariants are translated to plain Lua.

get_property should return plain Lua types, if it doesn't it's probably a bug. Do you have an example where the GVariants are not stripped?

set_property does nothing like auto conversion to GVariant.

Not entirely correct. set_property assumes that only simple properties will be set (like a string or a number) and wraps the opts table in a GVariant.

Looking at your suggested code, you would like to allow the caller to pass either a GVariant or a table that can be wrapped in a simple GVariant (as already happens). This solution would enable passing more complex GVariants, but I'd like to keep that opaque to the caller, they should only be concerned with passing simple tables.

Do you have some examples where more complex properties can be set? If that's the case, we could look into adding a wrap function in the _variant module that does the opposite of strip and then use it with set_property. We should also add a comprehensive suite of test for that (similar to what's been done for strip).

terminar commented 5 years ago

get_property should return plain Lua types, if it doesn't it's probably a bug. Do you have an example where the GVariants are not stripped?

If I am stumbling upon an example, I'll give feedback - currently I can't remember where I had that problem. The main reason I mentioned it was: It returns plain Lua types (without signature-value table).

Not entirely correct. set_property assumes that only simple properties will be set (like a string or a number) and wraps the opts table in a GVariant.

Maybe I am using it completely wrong and maybe I am missing something but I just can't do:

        local dev = NetworkManager(nm:GetDeviceByIpIface(devicename),"$.Device")
        dev.Managed = true
        dev.Autoconnect = true

(which, in my understanding, will be a real auto conversion by basic type) - it's complaining that "opts" is not a table.

I have to pass a table with "signature" and "value"

        local dev = NetworkManager(nm:GetDeviceByIpIface(devicename),"$.Device")
        dev.Managed = { signature = "b", value = true }
        dev.Autoconnect = { signature = "b", value = true }

That's why I mentioned: set_property does no auto conversion from basic types. When I look at the code at: https://github.com/stefano-m/lua-dbus_proxy/blob/dacbc56d22436e05c5f8c60c7fa660cac78d3002/src/dbus_proxy/_proxy.lua#L259

my understanding seems to be correct. I can only pass a table with "signature" and "value" or I get an error.

A really nice version of set_property would be something like (be careful, untested copied code):

local function set_property(proxy, name, opts)
  local variant_value
  local vtype = type(opts)
  if vtype == "boolean" then
      variant_value = GVariant("b",opts)
  elseif vtype == "string" then
      variant_type = GVariant("s",opts)
  elseif vtype == "number" then
      variant_type = GVariant("u",opts)
  elseif vtype == "userdata" and tostring(opts):find("GLib%.Variant$") then
    variant_value = opts
  elseif vtype == "table" and opts.signature and opts.value then
    variant_value = GVariant(opts.signature, opts.value)
  else
    error("Wrong parameter, should be table with { .signature and .value } or GVariant")
  end
  proxy._proxy:set_cached_property(name, variant_value)
end

That would be awesome and really auto converts basic types AND gives the possibility to pass complex GVariants.

Looking at your suggested code, you would like to allow the caller to pass either a GVariant or a table that can be wrapped in a simple GVariant (as already happens). This solution would enable passing more complex GVariants, but I'd like to keep that opaque to the caller, they should only be concerned with passing simple tables.

One part is: it doesn't throw an error if I doesn't pass a table to the property. That took some time to understand: ok, no basic types possible (as I get them via get_property).

The other part is: when it should be opaque (and equal), let get_property also return a table with {signature = "...", value = "..."}?

Do you have some examples where more complex properties can be set? If that's the case, we could look into adding a wrap function in the _variant module that does the opposite of [strip]

I made a mistake mixing some things - The real complex types and problems I had was when I called a method on NetworkManager (GetSettings() and Update(settings)), not on properties. Using set_property I had only the mentioned problem above (need to pass a table even for basic types).

stefano-m commented 5 years ago

OK, I see your point more clearly now. Thanks for taking time to explain everything.

To recap:

  1. At the moment we don't have an example where get_property does not return plain Lua types. If one is found, I suggest that a new issue is opened in regard.
  2. set_property's interface is limiting and a bit awkward in that it accepts an opts table that enables one to set only simple GVariant properties. In this case I agree that we could introduce a breaking change to make it nicer. We can continue the discussion on this issue.

So, regarding set_property, we need a "Lua to DBus" conversion table, but I agree that we should also allow the caller to pass raw GVariants. The main issue I see with the conversion table is numbers, as DBus' numeric types are much richer than Lua's. Moreover Lua 5.3 can make a difference between integers and float (using math.type) while Lua 5.2 doesn't.

Also, Lua strings could be DBus's strings, signatures or object paths. There should be some functionality in GIO (exposed via lgi) that could help.

And there are container types too: struct, array, dict...

So, our step zero would be to have a basic mapping by "reversing" what happens in _variant.strip. Do you think you can do that?

References: DBus Type System:

DBus funtionality from GIO:

terminar commented 5 years ago
  1. yes
  2. great

I'll take a look at _variant.strip and give response here.