EionRobb / purple-mattermost

A libpurple/Pidgin plugin for Mattermost
GNU General Public License v3.0
93 stars 23 forks source link

W.I.P.: Porting plugin to V4 API #74

Closed jaroslawp closed 6 years ago

jaroslawp commented 6 years ago

Hi @EionRobb , this is not production ready, needs some further fixing and reviewing .. but perhaps you would have some time to have a look at it in next weeks ? (while I'm away from keyboard)

EionRobb commented 6 years ago

Are all the calls to mm_fetch_url() using /api/v{3,4,5}/ as the prefix? If so, it might make sense to move that prefix to the mm_fetch_url() function itself, then we can more easily support multiple versions of the api within the plugin, with an account preference or something.

Thoughts?

jaroslawp commented 6 years ago

Yes, all have same prefix. OK I'll do that now. (athough supporting multiple versions may be painful to implement in practice since both api endpoints and api responses format changed for some calls). done.

jaroslawp commented 6 years ago

I think it would be time to start splitting the code into multiple files .. it is getting too large (6000 lines ...). I would start by moving defines, data structures and function prototypes to libmattermost.h ? ..

EionRobb commented 6 years ago

Sure, but maybe we'll get the v4 api stuff merged first and then we can do the cleanup after?

jaroslawp commented 6 years ago

OK, but I thought it may still take some time (well my code got little bit messy, I think it may be easier to clean it up while splitted).

Current code seems to work OK with MM server v5 (api v4), no crashes seen, functionality seems to be there, there may be some memory leaks and function call order can possibly be optimized (in startup sequence in some cases users list can be read twice) .. plus of course any bugs I may have introduced.

(and some parts are probably not implemented best way ...)

Then v3 api support is removed (not sure if it is worth trying to put it back in, seems that soon (already) supported MM server versions do not provide v3 api ? ...)

So: what would be your opinion on merging that ?

hoerup commented 6 years ago

I would like to help testing, if somebody could provide a win build of the plugin

EionRobb commented 6 years ago

@hoerup here ya go libmattermost.zip

jaroslawp commented 6 years ago

indeed, not needed at all - removed now.

jaroslawp commented 6 years ago

Yes I know that you wanted to split files later: but I could not implement image send with libpurple 2.X (g_strdup() used internally, cannot handle binary data in http post ...).. therefore I just copied purple2compat/* from your purple-hangouts/purple-skypeweb. (then adapted code ).

jaroslawp commented 6 years ago

@EionRobb : would you be able to test libpurple 3 build (and functionality) ?

jaroslawp commented 6 years ago

@EionRobb : I think that would be it about adding new features for now, probably some fixing will be needed but (if it works with libpurple 3) this may be good enough as starting point for future v4 api release..

EionRobb commented 6 years ago

Lets just get this in here, so we can start cleaning it up :)