[kamailio/kamailio] presence: add API endpoints to update presentity and notify watchers (#1417)
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1417
-- Commit Summary --
* presence: add API endpoints to update presentity and notify watchers
-- File Changes --
M src/modules/presence/bind_presence.c (2) M src/modules/presence/bind_presence.h (5) M src/modules/presence/presence.c (16) M src/modules/presence/presence.h (1) M src/modules/presence/presentity.c (133) M src/modules/presence/presentity.h (6)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1417.patch https://github.com/kamailio/kamailio/pull/1417.diff
Is there a reason for presentity updates made in this way not to be replicated (if enable_dmq = 1)?
Also, why not re-use the existing update_presentity() function instead of creating a new function to do the same thing?
oops sorry i didn't think about dmq support. the API endpoint is scrictly to update the presentity database, the existing update_presentity() function seems to do a lot more than that... i will look into integrating the 2 or at least add dmq support to the API function
No worries - the existing function will work with only a minimal set of non-null parameters and will also take care of dmq replication at the same time.
@charlesrchance thanks for the comment on this, turned out it was not too difficult to call the existing function. when you get a chance, please let me know what you think about this. my tests show it's working (calling the API with some test code updates presentity table)
@charlesrchance went a step further and allowed the type to get passed into pres_refresh_watchers() another API func can be added later to allow the last 2 params if needed
Nice...why not include the last two params now, though, for completeness?
Then it's just a question of the docs - seems to be a lot of API functions missing already, so could be a good time to bring them up to date. What do others think?
looking a few lines below my code, the KI interface uses a separate function if the last 2 params are required. i dont think we need to bloat the code just for completeness-- if someone desires to use that API function they can easily add it. but for my purposes, i will not be using it and probably never will
To me, adding a separate function for an extra two params is bloating the code.
The final two params are a direct passthrough, anyway. So your function simply becomes:
``` int _api_pres_refresh_watchers(str *pres, str *event, int type, str *file_uri, str *filename) { return pres_refresh_watchers(pres, event, type, file_uri, filename); } ```
Compared with:
``` int _api_pres_refresh_watchers(str *pres, str *event, int type) { return pres_refresh_watchers(pres, event, type, NULL, NULL); }
int _api_pres_refresh_watchers_file(str *pres, str *event, int type, str *file_uri, str *filename) { return pres_refresh_watchers(pres, event, type, file_uri, filename); } ```
Plus you consider others' needs in future and not just your own right now.
Just my opinion - perhaps others think differently.
the problem i see with this: ``` int _api_pres_refresh_watchers(str *pres, str *event, int type, str *file_uri, str *filename) { return pres_refresh_watchers(pres, event, type, file_uri, filename); } ``` is the last 2 parameters are option, yet you have no way of knowing that by looking at the API function. therefore the API function becomes confusing. and another developer could spend a lot of time parsing through the code to figure out which params are required and which are optional (much like i just had to do in order to implement these 2 API functions). it will save developers a lot more time to create API functions with *only* required parameters
i am considering others' needs as Im preparing these API functions in order to submit a new module which will use them. The new module will be for everyone to freely use-- and if there becomes a need to use any more API functions, i expect the developer that requires them, will have to do exactly as I did, and create those API functions. But there is also a good chance that the API function we are talking about will *never* be required so IMO it becomes bloat
the last 2 parameters are option, yet you have no way of knowing that by looking at the API function.
Then there are, of course, the docs :)
No worries, anyway - it was just an observation - it was by no means supposed to detract from your contribution. I'll leave it for others now to comment/review further.
merging this since i haven't received further comment
Merged #1417.
3 days for the entire PR (and 2 days from last comment) is not a long period of time, specially if people are traveling or vacation.
Before merging, if it is not a module you developed, then ask again like:
``` If no further comments in 3 days, then I will merge ... ```
This is a generic remark, not saying that this PR should be reverted. But overall, it's not advisable to rush and merge specially on big changes.
ok, i will keep that in mind going forward
@eschmidbauer - also just another remark for the future - the personal branches should use `userid/branch-name`, not only `branch-name` -- e.g., `eschmidbauer/presence-api`. It should be written somewhere in the contribution guidelines or in the wiki for devs info. Branches without the userid that are not officially created for a specific purpose (releases, continuous integration) can be removed without notice.
good to know, thanks for the info
-
Charles Chance -
Daniel-Constantin Mierla
-
Emmanuel Schmidbauer