notification: Don't fail requests with unsupported properties #1303
Conversation
src/notification.c
Outdated
| { | ||
| if (!check_value_type (key, value, G_VARIANT_TYPE_STRING, error)) | ||
| return FALSE; | ||
| label = g_steal_pointer (&value); |
There was a problem hiding this comment.
this is leaking when there are multiple "label" entries. some for the other keys. either g_clear_pointer (&label, g_variant_unref) or change the if before to check for label == NULL depending on if you want to use the first or last occurrence of the key.
There was a problem hiding this comment.
Seeing the changed code: is it documented what happens with multiple keys? Should this maybe even return an error?
There was a problem hiding this comment.
The documentation of the portal doesn't mention anything so this seems to be undefined and the code should be fine. Either way, we should update the spec to either throw an error when a key exists multiple times (which might not be possible if there are actual apps which already do this), or at least update if the first or last occurrence of a key is used.
There was a problem hiding this comment.
not sure, the docs don't say anything about it, and the old code just hands what ever keys are present to the backend. The gtk backend simple passes everything to GNOME Shell or uses g_variant_lookup_value() when using FDO. The docs for g_variant_lookup_value() don't specify a behavior, but a quick look at the code it seams that it just returns the first found value.
There was a problem hiding this comment.
Alright, this is fine then.
It would still be good to define what happens with multiple keys in a dict for all portals (first or last occurrence wins) but let's not block this MR on it.
jsparber
force-pushed
the
filter_notification
branch
from
0b80213
to
90b194f
Compare
|
@swick thanks for the review :) |
jsparber
force-pushed
the
filter_notification
branch
2 times, most recently
from
c98a948
to
d1d6492
Compare
|
LGTM. Even though portals are versioned and this isn't really required, this makes it much nicer for apps to use. e: I'll redact this. The notifications test is crashing. This needs fixing first. |
The tests fails on CI because, |
|
Probably just some environment variable to override the icon_validator executable path that you set in test-portals.c and the python test framework. |
Not sure if that's so easy, we don't want to add the possibility to set the icon_validator path via env variable. |
jsparber
force-pushed
the
filter_notification
branch
from
d1d6492
to
730c5db
Compare
|
Might want to take a look at https://github.com/flatpak/xdg-desktop-portal/pull/1309/commits. If the env var is only for testing then we probably want the names to be consistent ( Also, let's set the environment from within the test harnesses and not the meson environment. |
I think the env var can be generally useful, not just for testing. |
|
the icon validator doesn't work, but the CI logs don't show any reason. Locally it works just fine. It could have to do something with the sandbox. |
|
It's running into a timeout ( |
that's normal, if a test fails. Since the mocked backend just stops talking back. The real issue is in the meson log. |
|
Sight that should be fixed in the test harness... Anyway, the test backend crashes in response to |
jsparber
force-pushed
the
filter_notification
branch
4 times, most recently
from
d452635
to
e5eca5e
Compare
Adding a notification may fail, instead of waiting for the timeout we can check the result we get in the callback.
jsparber
force-pushed
the
filter_notification
branch
6 times, most recently
from
a958c42
to
0df47d2
Compare
|
Fixing the CI took way to long. 😮💨 |
For debugging it's important that the backend doesn't just crash without telling the reason. It would be even nicer if the test would fail immediately instead of hitting the timeout, but that' currently not possible since the client doesn't know whether the backend failed.
We can reuse most of the test setup for all tests. This will make it easier to test more properties.
This filters unsupported properties from a notification request without failing it. This allows backwards compatibility after adding new properties. This should had been the case from the beginning so that we don't have to break API when extending the notification interface. See [1]. [1] https://dbus.freedesktop.org/doc/dbus-api-design.html#extensibility
This allows setting a custom path for the icon validator, especially useful when testing without installing.
The sandbox for the icon-validator needs a privileged container. And it needs librsvg to be able to validate SVGs.
If debug messages are turned on the output of validate-icon isn't a valid key file since it contains log output as well.
jsparber
force-pushed
the
filter_notification
branch
from
0df47d2
to
e66041b
Compare
|
Alright, this looks all good to me! |
This filters unsupported properties from a notification request without
failing it. This allows backwards compatibility after adding new
properties. This should had been the case from the beginning so that we
don't have to break API when extending the notification interface. See
[1].
[1] https://dbus.freedesktop.org/doc/dbus-api-design.html#extensibility
Also improve tests and add icon tests to make sure that this doesn't break icons.