Re: [PATCH 2/4] netreg: Add CPHS CSP PLMN mode implementation
Hi Aki,
>> -static const char *registration_mode_to_string(int mode)
>> +static const char *registration_mode_to_string(int mode, gboolean forced_auto)
>> {
>> + if (forced_auto)
>> + return "auto-only";
>> +
>
> I see the benefit of keeping this in this function, but I would not have
> done it like this actually. I would have kept it next to the function
> needing that string and let it do the auto-only check by itself.
That is how I originally had it, but this ends up being a bit less
code. Also, I'd like to localize all of these constant strings into a
single function and not sprinkle them across the code.
so I am counting two callers to registration_mode_to_string.
> Another possibility might be to convert this into
>
> get_registration_mode_string(struct ofono_netreg *netreg)
>
> Since I am not mistaken, then the netreg->mode is always set before you
> are calling this, right?
No, you call it with the new value. The function will then compare the
old and the new, and based on this decide on sending a
PropertyChanged.
In the current code base it is always checked and set before:
if (netreg->mode == mode)
return;
netreg->mode = mode;
...
strmode = registration_mode_to_string(mode);
Just on a different thought here. The NETWORK_REGISTRATION_MODE_* is
local to src/network.c. We could just add the AUTO_ONLY mode there.
The only downside I is see the already nasty init_registration_status
check, but that might be acceptable compared to crippling the string
conversion function.
However at least the check would be clearly to just checking mode and
not mode + forced_mode.
Regards
Marcel