-
-
So there are a few overall things we need to consider here.
Someone may want to bind to a property change or use the notify signal, but right now there
g_object_notify*
isn't being called at all. The use case is a little weird, but someone may want to do the following.IbisFeatures *features = NULL; features = ibis_features_new(); g_signal_connect(feature, "notify::AWAYLEN", G_CALLBACK(...), NULL);
Right now that signal handler will never be called because, as mentioned above,
g_object_notify*
isn't being called. This should be done in the parse method, but we should probably useg_object_freeze_notify
andg_object_thaw_notify
around the bulk of that function.Likewise, it might also be nice to have constants for the names in
ibisconstants.h
. Something likeIBIS_FEATURE_NICKLEN
and so on.That said, right now this doesn't handle unknown features either. That's kind of a tough topic, but it would be nice if we could at least store them.
My initial thought for this is to add 2 new methods,
IbisFeatures.get_string
andIbisFeature.get_int
. Both whould take a parameter of the feature name and then return it. This would allows users to look up unknown features.If the internal representation of the values is a
GHashTable
that's keyed on the string of thePARAMETER NAME
, then the value could be aGValue
that's initialized to the either string or int depending on the parameter.The methods could look up and return the proper value. Going that route, all of the getters would just call
get_string
orget_int
as necessary. -
-
Typically we put the doc comment for the type we're defining here above the
G_DECLARE*_TYPE
macro.Something like
/** * IbisFeatures: * * An object that keeps try of the features the server supports. * */
Or something like that.
-
Body goes after the parameters, and parameters should be directly under the symbol name without a blank line.
-
We should probably link to where these properties like
AWAYLEN
are defined for additional reading. Similar to what we did inibisconstants.h
-
-
-
this can probably just be a
guint
I can't imagine anyone supports a nick len ofMAX_UINT
...After a quick glance, I'd say the same for the other properties that are marked as
guint64
. -
-
-
-
-
Typically our multiline comments have a
*
at the start of each line and the closing*/
on it's own line. So something like/* Set teh default value specified at * https://modern.ircdocs.horse/#rplisupport-parameters */
-
-
Body of the doc comment is missing for all of these. If you have the docs enabled and run the unit tests, it can alert you to these.
-
-
-
-
-
should be
< IBIS_VERSION_0_2
. The check in the the0_2
stuff is wrong too and can be fixed here as well. -
-
it might be cleaner to use
message = ibis_message_new(IBIS_RPL_ISUPPORT); ibis_message_set_paramsv(message, msg_params);
Add a features object to parse RPL_ISUPPORT messages into
Review Request #3267 — Created June 20, 2024 and submitted
Add a features object to parse RPL_ISUPPORT messages into
Ran the unit tests in valgrind.
Summary | ID |
---|---|
ce1c7236fc04cff0339803179b40a99456524b9f |
Description | From | Last Updated |
---|---|---|
there should be a blank line between dependencies and internal headers. |
|
|
Typically we put the doc comment for the type we're defining here above the G_DECLARE*_TYPE macro. Something like /** * … |
|
|
Body goes after the parameters, and parameters should be directly under the symbol name without a blank line. |
|
|
We should probably link to where these properties like AWAYLEN are defined for additional reading. Similar to what we did … |
|
|
char not gchar |
|
|
char |
|
|
this can probably just be a guint I can't imagine anyone supports a nick len of MAX_UINT... After a quick … |
|
|
if the function is called get_int but returns guint. Since all of the values I looked at (which wasn't all … |
|
|
char |
|
|
char |
|
|
I was imagining that we'd use the hash table to hold all the properties and not just the unknown ones. |
|
|
You can use ibis_str_equal instead of g_strcmp0 |
|
|
pretty sure we can just use atoi here and then we don't need 64bit values anywhere. |
|
|
Freezing notification is meant to be used when you're changing multiple properties at once to make sure thay're all set … |
|
|
as I mentioned earlier, I was imaging this would be something like: gpointer value = NULL; if(ibis_str_equal(feature, IBIS_FEATURE_AWAYLEN)) { ibis_features_set_uint(features, … |
|
|
Typically our multiline comments have a * at the start of each line and the closing */ on it's own … |
|
|
AWAYLEN needs to be the exact same as the property name (the first parameter to g_param_spec_*). |
|
|
Body of the doc comment is missing for all of these. If you have the docs enabled and run the … |
|
|
Again, these should probably be guint's so g_param_spec_uint instead of g_param_spec_uint64. |
|
|
This shouldn't be necessary as the unit tests should fail horribly if the regex failed to compile. |
|
|
char |
|
|
char |
|
|
should be < IBIS_VERSION_0_2. The check in the the 0_2 stuff is wrong too and can be fixed here as … |
|
|
adding a g_assert_true(IBIS_IS_FEATURES(features)); would be nice here. |
|
|
it might be cleaner to use message = ibis_message_new(IBIS_RPL_ISUPPORT); ibis_message_set_paramsv(message, msg_params); |
|
|
Should probably add a line break after the first sentenance. The first section is used as a quick summary in … |
|
|
So as I was trying to fall asleep last night I realized we can just pass the GParamSpec via properties[PROP_FOO] … |
|
|
Not sure what this check is for? Per https://modern.ircdocs.horse/#rplisupport-005 A single RPL_ISUPPORT reply MUST NOT contain the same parameter multiple … |
|
|
As mentioned earlier, the TRUE here should instead be the known paramspec. ie properties[PROP_AWAYLEN]. |
|
|
This is a sentenance so it should start with a capital and end in a period. |
|
|
g_return*_if_fail makes this not nullable as it output a warning or critical at runtime. Something like the following is fine … |
|
|
I'm not sure the pointers are always going to be the same here. We're probably better off using ibis_str_equal |
|
![ivanhoe](https://reviews.imfreedom.org/media/uploaded/avatars/i/iv/ivanhoe__047bb4c9-8953-4e08-a766-0c757501caa9.png)
- Change Summary:
-
Addressed the issues and updated the version in meson.build to 0.3.0-dev.
- Commits:
-
Summary ID d001e347480711cd49653376cb694a3d949fe3ef 73de118a20fa1f2d3741fa290ccbfdc773f6664e
-
-
if the function is called
get_int
but returnsguint
. Since all of the values I looked at (which wasn't all of them) we should probably make thisibis_features_get_uint
to avoid confusion and in casewe need to add a signed version in the future. -
I was imagining that we'd use the hash table to hold all the properties and not just the unknown ones.
-
Freezing notification is meant to be used when you're changing multiple properties at once to make sure thay're all set before anything acts upons the changes.
Since this function is only setting a single feature it shouldn't be used here. However it should be used in the caller of this function as that's knows when all of the property changes have been completed.
-
as I mentioned earlier, I was imaging this would be something like:
gpointer value = NULL; if(ibis_str_equal(feature, IBIS_FEATURE_AWAYLEN)) { ibis_features_set_uint(features, IBIS_FEATURE_AWALEN, ibis_str_to_uint(value), TRUE); } if(ibis_str_equal(...) { ... }
Obviously we'd need private helpers like the mentioned
ibis_features_set_uint
to actually insert everything into the hash table, but then all the values are in a common place.I imagine the helpers for them sould be something like the following. The known property names would set the notify parameter to true and then we'd use
g_object_notify
instead ofg_object_notify_by_pspec
.static void ibis_features_set_uint(IbisFeatures *features, const char *name, guint value, gboolean notify) { GValue *value = NULL; value = g_new0(GValue, 1); g_value_init(value, G_TYPE_UINT); g_value_set_uint(value); g_hash_table_insert(features->features, g_strdup(name), value); if(notify) { g_object_notify(G_OBJECT(features), name); } }
Likewise, this would mean the getters would need to lookup everything in the hash table too, but since everything is in the hash table, they just call the generic functions. So
get_awaylen
would become something likeguint ibis_features_get_awaylen(IbisFeatures *features) { g_return_val_if_fail(IBIS_IS_FEATURES(features), 0); return ibis_features_get_uint(features, IBIS_FEATURE_AWAYLEN); }
That said, we could argue that we don't need the specific getters either, but they're fine for now as it helps people avoid requesting the wrong type.
-
-
g_return*_if_fail
makes this not nullable as it output a warning or critical at runtime.Something like the following is fine to for nullable:
if(ibis_str_is_empty(str)) { return 0; }
![ivanhoe](https://reviews.imfreedom.org/media/uploaded/avatars/i/iv/ivanhoe__047bb4c9-8953-4e08-a766-0c757501caa9.png)
- Change Summary:
-
Addressed 2nd round of issues.
- Commits:
-
Summary ID 73de118a20fa1f2d3741fa290ccbfdc773f6664e dd3b073260f1e002bc2537f8dcf3b9b0c679ffd4
-
-
Should probably add a line break after the first sentenance. The first section is used as a quick summary in the docs.
See the screen shot below. The first sentenance is fine for the list of all the methods, and the second should probably only be in the specific documentation for the symbol.
-
So as I was trying to fall asleep last night I realized we can just pass the
GParamSpec
viaproperties[PROP_FOO]
instead of the boolean. If it's that not null, then you just callg_object_notify_by_pspec
.Doing that you'll avoid the need for the helper function and everything too...
Sorry again, I meant to get this mentioned in the morning and forgot :-/
-
Not sure what this check is for? Per https://modern.ircdocs.horse/#rplisupport-005
A single RPL_ISUPPORT reply MUST NOT contain the same parameter multiple times nor advertise and negate the same parameter. However, the server is free to advertise or negate the same parameter in separate replies.
Likewise for the other helpers.
-
As mentioned earlier, the
TRUE
here should instead be the known paramspec. ieproperties[PROP_AWAYLEN]
. -
![ivanhoe](https://reviews.imfreedom.org/media/uploaded/avatars/i/iv/ivanhoe__047bb4c9-8953-4e08-a766-0c757501caa9.png)
- Change Summary:
-
Addressed the 3rd round of issues but kept the "changed value check" in for now (to wait for a potential answer to my comment).
- Commits:
-
Summary ID dd3b073260f1e002bc2537f8dcf3b9b0c679ffd4 67401762a7911f7965e2ae6bf60075d1bae72d0b
- Commits:
-
Summary ID 67401762a7911f7965e2ae6bf60075d1bae72d0b ce1c7236fc04cff0339803179b40a99456524b9f