Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[overridable API] Added Tox API in a structure form. #502

Closed
wants to merge 2 commits into from

Conversation

yurivict
Copy link
Member

@yurivict yurivict commented Mar 6, 2017

This is useful to facilitate API subclassing.
Some library may want to overload several functions, keeping the rest, like this:

MyLibToxApi mylibtox_api = tox_api;
mylib_tox_api.tox_func1 = &mylibtox_func1;
mylib_tox_api.tox_func2 = &mylibtox_func2;

This change is Reviewable

@iphydf
Copy link
Member

iphydf commented Mar 6, 2017

Can you add a comment about how this is supposed to be used? I don't expect users will search git history for this PR and the usage docs you wrote in the PR body.

toxcore/tox.h Outdated
uint16_t (*tox_self_get_tcp_port)(const Tox *tox, TOX_ERR_GET_PORT *error);
} ToxApi;

extern ToxApi tox_api;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are users supposed to change this global mutable variable?

Copy link
Member Author

@yurivict yurivict Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, users are supposed to copy it to create a flavor of the API based on the original API.

I am planning to create the mid-level library that will expose ToxApi xtox_api. When used, it will allow sending arbitrarily long messages using the same interface. All clients will need to do - to use xtox_api instead of tox_api.

@yurivict
Copy link
Member Author

yurivict commented Mar 6, 2017

Made variables const.

@yurivict yurivict force-pushed the api-struct branch 3 times, most recently from 96c0db1 to 5939f96 Compare March 6, 2017 19:37
@nurupo
Copy link
Member

nurupo commented Mar 6, 2017

Uh, do we really want toxcore to provide this? I have yet to see a library that does this. I'd say this belongs in the code base of whoever uses toxcore and needs this functionality.

Also, the structhe names are shortened for some reason. tox_api vs toxcore_api, same with toxencryptsave.

@yurivict
Copy link
Member Author

yurivict commented Mar 6, 2017

@nurupo I am going to create such mid-level library.
Mature API should provide this for robustness.
I will fix the structure names.

@yurivict
Copy link
Member Author

yurivict commented Mar 6, 2017

I fixed the structure names.

@sudden6
Copy link

sudden6 commented Mar 6, 2017

Why is this better than just calling the original Tox functions from a library on top and forward all the parameters + return value?

@yurivict
Copy link
Member Author

yurivict commented Mar 6, 2017

@sudden6 I don't understand your suggestion. Could you elaborate?

tox_friend_send_message will be in both toxcore and library. How would you call it?

@sudden6
Copy link

sudden6 commented Mar 6, 2017

I'd overwrite it with my own libraries prefix like supertox_friend_send_message

@yurivict
Copy link
Member Author

yurivict commented Mar 6, 2017

I'd overwrite it with my own libraries prefix like supertox_friend_send_message

Then you will have to provide all ~150 functions, when with this approach you only need to provide what you actually need.

@sudden6
Copy link

sudden6 commented Mar 7, 2017

I understand the purpose now, thanks to qTox/qTox#4238 (comment)

This doesn't seem to build right now.

@yurivict
Copy link
Member Author

yurivict commented Mar 7, 2017

It doesn't build? What's the error?

@Diadlo
Copy link

Diadlo commented Mar 8, 2017

@yurivict
Copy link
Member Author

Please merge this for the tox defragmenter: #492 (comment)

@iphydf iphydf modified the milestone: v0.1.8 Mar 26, 2017
@iphydf
Copy link
Member

iphydf commented Mar 26, 2017

I'm reluctant to merge this. I see no reason why you couldn't have this struct in a separate library. What other libraries provide this kind of overriding facility?

@iphydf iphydf removed this from the v0.1.8 milestone Mar 26, 2017
@iphydf iphydf modified the milestones: v0.1.9, v0.1.8 Mar 26, 2017
@iphydf iphydf modified the milestones: v0.1.9, 0.1.10 Jun 3, 2017
@iphydf iphydf modified the milestones: v0.2.x, v0.1.10 Jun 24, 2017
@yurivict
Copy link
Member Author

yurivict commented Aug 2, 2017

Please merge this PR.

qTox has plugins system planned. I would like to add the type of plugin that will allow to alter the API. One application is previously proposed by me arbitrary size messaging, which I think will fit well as a plugin. But API needs to be alterable.

@yurivict yurivict changed the title Added Tox API in a structure form. [overridable API] Added Tox API in a structure form. Aug 2, 2017
@CLAassistant
Copy link

CLAassistant commented Aug 2, 2017

CLA assistant check
All committers have signed the CLA.

@Diadlo
Copy link

Diadlo commented Aug 2, 2017

@yurivict In your case, you need to reimplement qTox Core. I don't think, that there is really reason to change toxcore API. Just use wrapper

@yurivict
Copy link
Member Author

yurivict commented Aug 2, 2017

In your case, you need to reimplement qTox Core. I don't think, that there is really reason to change toxcore API.

The reasons:

  1. If other clients were to develop advanced features like plugins, etc., they would need this in the same way as qTox.
  2. This belongs near to where the API is defined, in toxcore, and not in clients.

@Diadlo
Copy link

Diadlo commented Aug 2, 2017

@yurivict Change toxcore API it's wrong way to do it. You can create your wrapper and force all(?) clients to use it. But client should always give an avility to use toxcore directly
I'm against of this changes in toxcore

@Diadlo
Copy link

Diadlo commented Aug 2, 2017

@yurivict You can write you library, which will provide few function line yurivict_tox_friend_send_message and custom callback setter. Anyone can integrate it by using your functions => profit

@yurivict
Copy link
Member Author

yurivict commented Aug 2, 2017

@Diadlo Plugins interface should be generic, and shouldn't operate in terms of individual functions. It can operate through the interface:

namespace QToxPlugin
{
class ApiInterface : public QObject
{
    Q_OBJECT
public:
    virtual const ToxcoreApi* overrideCoreApi(const ToxcoreApi* prevApi) = 0;
    virtual const ToxavApi* overrideAvApi(const ToxavApi* prevApi) = 0;
    virtual const ToxencryptsaveApi* overrideEncryptsaveApi(const ToxencryptsaveApi* prevApi) = 0;
};
};

This way it is generic, expandable, and doesn't have to implement a few functions here and there.

I am motivated to work on this, because I essentially have one plugin ready that is very useful IMO. But this PR needs to be merged, otherwise it just doesn't feel right to drag this into clients.

@Diadlo
Copy link

Diadlo commented Aug 2, 2017

You may don't believe, but you still need "to implement a few functions here and there" to use it inside child classes to override toxcore function

Note: What if someone don't whant to use Qt or C++? This way not enough generic from this side.
In C terms you need +1 function to get custom API struct

@yurivict
Copy link
Member Author

yurivict commented Aug 2, 2017

Other languages have other ways to deal with such things. APIs are normally defined in C and later other languages bind to them in their own way. And no, C apps can use it as is, without defining anything else.

@iphydf
Copy link
Member

iphydf commented Jan 29, 2018

The checkbox allowing maintainers to rebase is not checked. We can't operate on this PR as is, and I'm not happy with the PR itself. I'm closing it for now. We may revisit it in the future.

@iphydf iphydf closed this Jan 29, 2018
@iphydf iphydf modified the milestones: v0.2.x, v0.2.0 Feb 1, 2018
@iphydf iphydf added refactor Refactoring production code, eg. renaming a variable, not affecting semantics and removed code cleanup labels May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Development

Successfully merging this pull request may close these issues.

7 participants