Conversation
leolost2605
left a comment
There was a problem hiding this comment.
Makes sense to me! A few comments :)
Also maybe we should use the glib.test built in way to specify which tests to run? https://valadoc.org/glib-2.0/GLib.Test.init.html says we can just use -p to specify what to run. So we would just construct all testcases, put them in an array and then call test.init and test.run. This would also automatically give us more complete and finegrained control, e.g. to not only specify tests but also skip some, etc. We could also keep the name property if we maybe want to name them differently because we don't rely on the type name then.
|
@leolost2605 I don't think it's worth adding name field, at least until we really need it. Currently it's just an unneeded repetition in the code |
06d7cec to
e3b3482
Compare
|
@leolost2605 for some reason this broke tests for mutter 50. I pushed a commit with a fix to this branch but let me know if you want to merge it separately and I'll create a new PR |
e3b3482 to
103e9de
Compare
leolost2605
left a comment
There was a problem hiding this comment.
Looks good, just a few more comments :)
@leolost2605 for some reason this broke tests for mutter 50. I pushed a commit with a fix to this branch but let me know if you want to merge it separately and I'll create a new PR
Yeah seems like we were doing remove_child (null) there so makes sense. If you clean up the commit history and add the fix as the first commit I think it's fine to merge with this
2e1951d to
4b0eba5
Compare
|
@leolost2605 Done! |
4b0eba5 to
e525f16
Compare
e525f16 to
159a70f
Compare
Closes #2833
Achieves the same performance boost as #2833, but doesn't introduce unnecessary #if conditions