Toktok migration

[Diadlo]

Migration qtox on toktok/c-toxcore

• Group API migration
• Error handling

---

This change is

[zetok]

needed:

• update travis
• update INSTALL.md – version =0.1
• mark as a breaking change
• versioned TokTok toxcore release with ~stable API

Edit: also

• update bootstrap scripts – once toxcore gets stable v0.1.0 release, git checkout it (also in docs)

[sudden6]

repository in scripts needs to be updated to toktok/c-toxcore

[initramfs]

Reviewed 4 of 9 files at r2.
Review status: 4 of 11 files reviewed at latest revision, 2 unresolved discussions.

---

src/core/core.cpp, line 236 at r2 (raw file):

 */
void Core::start()
{

Add a print statement to show the version of toxcore in use. This will help debug issues with runtime loading of the toxcore (and to verify that the correct toxcore version is being used).

Something like:

qDebug().nospace() << "Using toxcore version " << tox_version_major() << '.' << tox_version_minor() << '.' << tox_version_patch();

---

src/core/core.cpp, line 529 at r2 (raw file):

}

#if TOX_VERSION_IS_API_COMPATIBLE(0, 0, 1)

Why not use TOX_VERSION_IS_API_COMPATIBLE(0, 0, 0) or simply invert the #if?

---

Comments from Reviewable

[Diadlo]

Review status: 4 of 11 files reviewed at latest revision, 2 unresolved discussions.

---

src/core/core.cpp, line 236 at r2 (raw file):

Previously, initramfs wrote…

Add a print statement to show the version of toxcore in use. This will help debug issues with runtime loading of the toxcore (and to verify that the correct toxcore version is being used).

Something like:

qDebug().nospace() << "Using toxcore version " << tox_version_major() << '.' << tox_version_minor() << '.' << tox_version_patch();

We have toxcore version on `About` tab in settings

---

src/core/core.cpp, line 529 at r2 (raw file):

Previously, initramfs wrote…

Why not use TOX_VERSION_IS_API_COMPATIBLE(0, 0, 0) or simply invert the #if?

Because

 #define TOX_VERSION_IS_API_COMPATIBLE(MAJOR, MINOR, PATCH)      \                
     (TOX_VERSION_MAJOR == MAJOR &&                                \                
     (TOX_VERSION_MINOR > MINOR ||                                \                
     (TOX_VERSION_MINOR == MINOR &&                              \                
     TOX_VERSION_PATCH >= PATCH)))                         

How inver #if?

---

Comments from Reviewable

[initramfs]

I feel like PR makes is such that once qTox has been linked at compile time to a newer version of toxcore, it won't be able to load older versions of toxcore at runtime (since you're actively avoiding compiling stuff for older toxcore instances). The similar problem occurs the other way around. Whilst this is perfectly fine (and optimal for distributions that compile on the target machine), it may pose an issue where pre-packaged versions of qTox are stored in repositories against a toxcore revision that's different to the one it's been compiled for.

At least in the case of transitioning from irungetoo/toxcore to TokTok/c-toxcore, would it be possible to make a runtime decision between which revision to load for?

---

Review status: 4 of 11 files reviewed at latest revision, 2 unresolved discussions.

---

src/core/core.cpp, line 236 at r2 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

We have toxcore version on About tab in settings

Fair enough, though I still argue it can be useful in future debugging should bug reports neglect to mention the version of toxcore but this is optional.

---

src/core/core.cpp, line 529 at r2 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Because

 #define TOX_VERSION_IS_API_COMPATIBLE(MAJOR, MINOR, PATCH)      \                
     (TOX_VERSION_MAJOR == MAJOR &&                                \                
     (TOX_VERSION_MINOR > MINOR ||                                \                
     (TOX_VERSION_MINOR == MINOR &&                              \                
     TOX_VERSION_PATCH >= PATCH)))                         

How inver #if?

My bad, I read the code wrong. If I read it right this time, you want code to only compile if it's running on toxcore 0.0.0. Well, in that case, you want to check incompatibility with 0.0.1+. As such, invert the if with:

#if !TOX_VERSION_IS_API_COMPATIBLE(0, 0, 1)

---

Comments from Reviewable

[zetok]

(…) it may pose an issue where pre-packaged versions of qTox are stored in repositories against a toxcore revision that's different to the one it's been compiled for.

At least in the case of transitioning from irungetoo/toxcore to TokTok/c-toxcore, would it be possible to make a runtime decision between which revision to load for?

There's no need to do that. As this point, development moved to c-toxcore, and that's what should be packaged.

[zetok]

There are conflicts, rebase is needed.

[Diadlo]

Review status: 18 of 20 files reviewed at latest revision, 4 unresolved discussions.

---

src/core/core.cpp, line 1063 at r9 (raw file):

Previously, sudden6 wrote…

The comment is now wrong

Done.

---

src/core/core.cpp, line 1125 at r9 (raw file):

Previously, sudden6 wrote…

getGroupNumberPeers no longer returns int please update this accordingly, there are probably other occurences that need updates

Done.

---

Comments from Reviewable

[zetok]

---

Review status: 18 of 20 files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[sudden6]

---

Reviewed 1 of 10 files at r4, 1 of 4 files at r5, 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[initramfs]

Review status: all files reviewed at latest revision, 16 unresolved discussions.

---

src/core/core.cpp, line 507 at r10 (raw file):

void Core::onGroupInvite(Tox*, uint32_t friendId, TOX_CONFERENCE_TYPE type,
                         const uint8_t *data, size_t length, void* _core)

Pointer aligns to the left.

---

src/core/core.cpp, line 507 at r10 (raw file):

void Core::onGroupInvite(Tox*, uint32_t friendId, TOX_CONFERENCE_TYPE type,
                         const uint8_t *data, size_t length, void* _core)

See point below on reserved identifiers.

---

src/core/core.cpp, line 528 at r10 (raw file):
The use of an identifier who's name starts with an underscore is prohibited in C++:

As per § 17.6.4.3.2:

Each name that begins with an underscore is reserved to the implementation for use as a name in the
global namespace.

---

src/core/core.cpp, line 533 at r10 (raw file):

    QString message = CString::toString(_message, length);
    bool isAction;
    isAction = type == TOX_MESSAGE_TYPE_ACTION;

bool isAction;
isAction = type == TOX_MESSAGE_TYPE_ACTION;

can be combined into 1 line.

---

src/core/core.cpp, line 545 at r10 (raw file):

void Core::onGroupTitleChange(Tox*, uint32_t groupId, uint32_t peerId,
                              const uint8_t* _title, size_t length, void* _core)

See point above.

---

src/core/core.cpp, line 553 at r10 (raw file):

}

void Core::onReadReceiptCallback(Tox*, uint32_t friendId, uint32_t receipt, void *core)

You fixed pointer alignment above, but not here. Consider fixing this one too.

---

src/core/core.cpp, line 646 at r10 (raw file):

}

void Core::sendGroupMessageWithType(int groupId, const QString &message, TOX_MESSAGE_TYPE type)

Reference alignment.

---

src/core/core.cpp, line 1086 at r10 (raw file):

    size_t length = tox_conference_peer_get_name_size(tox, groupId, peerId, &error);
    if (!parsePeerQueryError(error))
        return QString();

Prefer using QString{}; or just {}; to indicate an invocation of the QString no-arg/default constructor rather than an arbitary global function called QString. This goes for every instance replaced by this PR.

---

src/core/core.cpp, line 1133 at r10 (raw file):

    }

    uint16_t nPeers = static_cast<uint16_t>(result);

Why is the number of peers represented as a unsigned 16-bit number here whereas the getGroupNumberPeers() returns a 32-bit number. Is it stated anywhere that peer numbers are 16-bits or less?

---

src/core/core.cpp, line 1134 at r10 (raw file):

    uint16_t nPeers = static_cast<uint16_t>(result);
    std::unique_ptr<uint8_t[][TOX_MAX_NAME_LENGTH]> namesArray{new uint8_t[nPeers][TOX_MAX_NAME_LENGTH]};

Could this not be refactored into using a std::vector? It seems to be much safer than manipulating multidimensional arrays within unique_ptrs.

---

src/core/core.cpp, line 1139 at r10 (raw file):

    uint32_t count = tox_conference_peer_count(tox, groupId, &error);
    if (!parsePeerQueryError(error))

Missing braces around if-statement.

---

src/core/core.cpp, line 1245 at r10 (raw file):

        return;
    case TOX_ERR_CONFERENCE_DELETE_CONFERENCE_NOT_FOUND:
        qCritical() << "Conferenct not found";

Possible spelling mistake.

---

src/core/core.cpp, line 1265 at r10 (raw file):

        break;
    case TOX_ERR_CONFERENCE_INVITE_FAIL_SEND:
        qCritical() << "Fail send";

I think this error message needs to be more descriptive.

---

src/core/core.cpp, line 1319 at r10 (raw file):

{
    // Valid length check
    if (addr.length() != (TOX_ADDRESS_SIZE * 2))

Why was the braces removed around this if-statement?

---

Comments from Reviewable

[Diadlo]

Review status: 16 of 20 files reviewed at latest revision, 14 unresolved discussions.

---

src/core/core.cpp, line 507 at r10 (raw file):

Previously, initramfs wrote…

Pointer aligns to the left.

Done.

---

src/core/core.cpp, line 507 at r10 (raw file):

Previously, initramfs wrote…

See point below on reserved identifiers.

Done.

---

src/core/core.cpp, line 528 at r10 (raw file):

Previously, initramfs wrote…

The use of an identifier who's name starts with an underscore is prohibited in C++:

As per § 17.6.4.3.2:

Each name that begins with an underscore is reserved to the implementation for use as a name in the
global namespace.

Done.

---

src/core/core.cpp, line 533 at r10 (raw file):

Previously, initramfs wrote…

bool isAction;
isAction = type == TOX_MESSAGE_TYPE_ACTION;

can be combined into 1 line.

Done.

---

src/core/core.cpp, line 545 at r10 (raw file):

Previously, initramfs wrote…

See point above.

Done.

---

src/core/core.cpp, line 553 at r10 (raw file):

Previously, initramfs wrote…

You fixed pointer alignment above, but not here. Consider fixing this one too.

Done.

---

src/core/core.cpp, line 646 at r10 (raw file):

Previously, initramfs wrote…

Reference alignment.

Done.

---

src/core/core.cpp, line 1086 at r10 (raw file):

Previously, initramfs wrote…

Prefer using QString{}; or just {}; to indicate an invocation of the QString no-arg/default constructor rather than an arbitary global function called QString. This goes for every instance replaced by this PR.

Done.

---

src/core/core.cpp, line 1133 at r10 (raw file):

Previously, initramfs wrote…

Why is the number of peers represented as a unsigned 16-bit number here whereas the getGroupNumberPeers() returns a 32-bit number. Is it stated anywhere that peer numbers are 16-bits or less?

Done.

---

src/core/core.cpp, line 1134 at r10 (raw file):

Previously, initramfs wrote…

Could this not be refactored into using a std::vector? It seems to be much safer than manipulating multidimensional arrays within unique_ptrs.

TODO added

---

src/core/core.cpp, line 1139 at r10 (raw file):

Previously, initramfs wrote…

Missing braces around if-statement.

Done.

---

src/core/core.cpp, line 1245 at r10 (raw file):

Previously, initramfs wrote…

Possible spelling mistake.

Done.

---

src/core/core.cpp, line 1265 at r10 (raw file):

Previously, initramfs wrote…

I think this error message needs to be more descriptive.

Done.

---

src/core/core.cpp, line 1319 at r10 (raw file):

Previously, initramfs wrote…

Why was the braces removed around this if-statement?

Done.

---

Comments from Reviewable

[initramfs]

Review status: 16 of 20 files reviewed at latest revision, 2 unresolved discussions.

---

src/core/core.cpp, line 1152 at r11 (raw file):

    QList<QString> names;
    for (uint16_t i = 0; i < nPeers; ++i)

Since you've updated nPeers to a std::uint32_t, you need to update this for-loop temp variable type as well. A future-proof way is to declare is as a decltype(nPeers), if not, simply change it to a std::uint32_t.

---

Comments from Reviewable

[Diadlo]

Review status: 15 of 20 files reviewed at latest revision, 2 unresolved discussions.

---

src/core/core.cpp, line 1152 at r11 (raw file):

Previously, initramfs wrote…

Since you've updated nPeers to a std::uint32_t, you need to update this for-loop temp variable type as well. A future-proof way is to declare is as a decltype(nPeers), if not, simply change it to a std::uint32_t.

Done.

---

Comments from Reviewable

[initramfs]

Reviewed 1 of 10 files at r4, 3 of 4 files at r11, 2 of 2 files at r12.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/core/core.h, line 215 at r12 (raw file):

    static void onConnectionStatusChanged(Tox* tox, uint32_t friendId,
                                          TOX_CONNECTION status, void* core);
    static void onGroupInvite(Tox* tox, uint32_t friendId, TOX_CONFERENCE_TYPE type,

The variable names changed in the function definition should be changed here in the declaration too. Or else you have inconsistent variable names.

---

Comments from Reviewable

[initramfs]

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[zetok]

cc package maintainers: @abbat @farseerfc @mpxc @vith @yurivict

Heads up: with this qTox depends on c-toxcore, and 1.7 release (due in a week) will contain this change.

[abbat]

Is there a way to compile qTox with original (iru) toxcore (through #define for example)? I have no motivation to support another (and conflicting) package which brings no new except breaking compatibility.

[sudden6]

@abbat toktok/c-toxcore doesn't break the protocol compatibility with irungentoo/toxcore

Additionally toktok/c-toxcore has active development, community and testing. All actively developed clients will make the switch, so I'd drop irungentoo/toxcore from the repo.