Improve GObject property implementation in AsReview #448
Conversation
This allows `-Wswitch-enum` to catch unhandled properties in the `get_property()` and `set_property()` vfuncs. It doesn’t catch any bugs in practice at the moment, but is best practice. Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This will marginally speed up class initialisation, as all the locks for adding pspecs to the `GObjectClass` will only have to be taken once, rather than N times. It also means we have a handy indexed `GParamSpec*` array ready to use in the following commit. Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
If these properties ever get names or descriptions added, this will avoid them being `strdup()`ed pointlessly by GObject. Since none of the properties currently have any strings set, this is currently a no-op. Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This is best practice for GObjects, as it allows binding things together (particularly UI widgets) to update themselves when the properties of the object they represent are updated. In particular, this will eventually let us bind review property updates in gnome-software to the `GsReviewRow` widget so that it updates automatically when a review is upvoted/downvoted/etc. See https://gitlab.gnome.org/GNOME/gnome-software/-/merge_requests/1545. The `G_PARAM_EXPLICIT_NOTIFY` flag now needs to be passed to the pspecs so that `g_object_set_property()` doesn’t implicitly emit the `GObject::notify` signal when it’s called. That would result in a duplicate signal emission. Unfortunately we can’t rely on this implicit behaviour because C code is more likely to call `as_review_set_*()` directly, which doesn’t use the implicit behaviour. Hence `G_PARAM_EXPLICIT_NOTIFY`. Where possible, the `as_review_set_*()` methods have been changed to return early if the new value is the same as the old one, to avoid an unnecessary `GObject::notify` emission. Where functions from `as-utils.c` have been used, though, that’s not straightforward, so there is still currently an unnecessary signal emission. That’s not a great problem, as consumers of the signal must be able to accept spurious emissions of it. Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This allows property change notifications for it, property bindings to it, and for it to be correctly exposed as a property in language bindings. Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
|
Some improvements to this code which could be made in future, but which I didn’t make in this PR because I ran out of time:
|
|
This looks good! Thank you for the patch! |
|
I do intend to release AppStream 1.0 probably next year, which will contain a bunch of API breaks. I am sort of dreading this a bit, since quite a bit of code will need adjustments then, but it's a necessary step to keep this project easy to maintain. So, when this happens we can have any API break that would be useful to you. If you want to, you can add API adjustment plans for 1.0 to docs/API-TODO.md as well, so we remember them when we can make those changes :-) I also wonder whether it would make sense to add the fetch & parse ODRS code into AppStream as well, so it's not duplicated for every software center. Possibly as a new library, as this would likely pull in JSON parsing as well... |
Probably best as a new library, to keep the dependencies separate and make API stability more manageable. |
See the commit messages for details.
The bulk of the changes here are to correctly emit
GObject::notifywhen properties onAsRevieware changed. The rest of the changes are mostly best practice / cleanup.