Refactor/coreav calls

[sudden6]

• Commits follow our git commit guidelines

@anthonybilinski as we discussed I decided to split this part from #6227 in order to make it easier to merge.

The changes:

• remove functions implemented by ToxCall from CoreAV, it was mostly just looking up the calls and then forwarding to ToxCall
• starting a call returns a std::shared_pt<ToxCall>, it must be ended using endCall() or there will be a resource leak
• made some properties in ToxCall atomic to prevent threading issues, because ToxCall objects are used by the Audio, CoreAV and UI threads

---

This change is 

[sudden6]

Good catch, fixed.

[sudden6]

@bodwok can you re-review and give lgtm when you are satisfied?

[bodwok]

Sure, sorry for the delay

Reviewed 7 of 8 files at r1, 5 of 5 files at r2.
Reviewable status: 0 of 1 LGTMs obtained

---

src/widget/form/chatform.cpp, line 357 at r2 (raw file):

if(call) {

need a space

---

src/widget/form/chatform.cpp, line 378 at r2 (raw file):

if(call) {

same

[sudden6]

Reviewable status: 0 of 1 LGTMs obtained

---

src/widget/form/chatform.cpp, line 357 at r2 (raw file):

Previously, bodwok wrote…

if(call) {

need a space

Done.

---

src/widget/form/chatform.cpp, line 378 at r2 (raw file):

Previously, bodwok wrote…

if(call) {

same

Done.

[bodwok]

Reviewed 4 of 8 files at r1, 1 of 6 files at r3, 5 of 5 files at r4.
Reviewable status: complete! 1 of 1 LGTMs obtained

[anthonybilinski]

Reviewed 4 of 8 files at r1, 4 of 6 files at r3, 5 of 5 files at r4.
Reviewable status: complete! 1 of 1 LGTMs obtained

---

src/core/coreav.h, line 52 at r3 (raw file):

public:
    using CoreAVPtr = std::unique_ptr<CoreAV>;
    using ToxFriendCallPtr = std::shared_ptr<ToxFriendCall>;

since ownership isn't actually shared for ToxFriendCalls, having owning unique_ptr's held by CoreAV and non-owing raw pointers handed out on startCall and answerCall would make the relationship more clear, and additionally more performant. See note here: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#note-65

---

src/core/coreav.h, line 139 at r3 (raw file):

    using ToxGroupCallPtr = std::shared_ptr<ToxGroupCall>;

same re: ownership

---

src/core/coreav.cpp, line 272 at r3 (raw file):

    // Set inactive for others
    it->second->setActive(false);

why should anyone be using the ToxFriendCall after cancelling? If it's being used as a start of a call is the creation of the object, then I'd think cancelling the call should invalidate the object?

---

src/core/coreav.cpp, line 642 at r3 (raw file):

        qWarning() << "Call with friend" << friendNum << "died of unnatural causes!";
        // Set inactive for others
        it->second->setActive(false);

same question, marking for reviewable

---

src/core/coreav.cpp, line 649 at r3 (raw file):

        qDebug() << "Call with friend" << friendNum << "finished quietly";
        // Set inactive for others
        it->second->setActive(false);

same

---

src/core/toxcall.h, line 109 at r3 (raw file):

    // ToxCall interface
public:
    void endCall() override;

Keeping with the RAII view of the lifetime of a call being represented as a ToxCall, it seems like ending call should be part of the destructor of ToxCall. Additionally every call to endCall happens right before destroying the ToxCall object. To simplify usage and reduce the risk of using ToxCall in an invalid state, what about doing the endCall logic within the destructor and removing the explicit API?

---

src/widget/form/chatform.cpp, line 381 at r3 (raw file):

        call->endCall();
        call.reset();
    }

if endCall is moved within destructor, this could unconditionally call .reset()

---

src/widget/form/chatform.cpp, line 434 at r3 (raw file):

void ChatForm::onMicMuteToggle()
{
    call->setMuteMic(!call->getMuteMic());

this loses thread safety compared to the previous implementation. Before the get/set were both done within CoreAV::callsLock, but now, even though ToxCall::muteMicis atomic, there could be a race between the get and set. This seems not trivial to fix: https://stackoverflow.com/a/9806613.

Maybe a toggle method could be added with a class mutex? Or the underlying type could be changed from bool and the xor method could be used, but that doesn't show intent as clearly IMO.

---

src/widget/form/groupchatform.cpp, line 244 at r4 (raw file):

    if (call) {
        call->removePeer(user);
    }

any reason to check call != nullptr here but not in onMicMuteToggle or onVolMuteToggle? I'd think that on any of these slots our call should still be valid, assuming our signals come in in the right order :P

[anthonybilinski]

Reviewable status: complete! 1 of 1 LGTMs obtained

---

src/core/coreav.h, line 52 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

since ownership isn't actually shared for ToxFriendCalls, having owning unique_ptr's held by CoreAV and non-owing raw pointers handed out on startCall and answerCall would make the relationship more clear, and additionally more performant. See note here: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#note-65

ah I should have taken a closer look, and was giving mixed advice before. Ownership is kind of shared because the starter of the call has a reference and CoreAV keeps a reference to all calls as well to handle toxcore callbacks. I was also saying things that were amix of "CoreAV should return a unique_ptr" and "CoreAV should hold on to the unique_ptr and return a raw pointer".. which was dumb.

I think startCall could still return the owning unique_ptr, CoreAV could hang on to a non-owning raw pointer of it, then the caller can end the call just by deleting the object, and in ToxCall's destructor it can call CoreAV::cancelCall?

[sudden6]

Reviewable status: complete! 1 of 1 LGTMs obtained

---

src/core/coreav.h, line 52 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

ah I should have taken a closer look, and was giving mixed advice before. Ownership is kind of shared because the starter of the call has a reference and CoreAV keeps a reference to all calls as well to handle toxcore callbacks. I was also saying things that were amix of "CoreAV should return a unique_ptr" and "CoreAV should hold on to the unique_ptr and return a raw pointer".. which was dumb.

I think startCall could still return the owning unique_ptr, CoreAV could hang on to a non-owning raw pointer of it, then the caller can end the call just by deleting the object, and in ToxCall's destructor it can call CoreAV::cancelCall?

I tried to do that, but a problem turned up: Calls can be allocated in CoreAV::callCallback and if we use raw pointers for this map, we risk never deallocating them properly. We could solve this, by making sure non of the ToxCall objects are alive when CoreAV is destroyed, but with our current mess of Widget destruction I don't see this happening. I don't think overhead of shared_ptr vs unique_ptr can be an argument here.

---

src/core/coreav.cpp, line 272 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

why should anyone be using the ToxFriendCall after cancelling? If it's being used as a start of a call is the creation of the object, then I'd think cancelling the call should invalidate the object?

I have not yet figured out the complete flow of setActive, it's used here and in stateCallback, I just remember things broke when I touched it, so it stays in for now.

---

src/core/coreav.cpp, line 642 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

same question, marking for reviewable

See above

---

src/core/coreav.cpp, line 649 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

same

See above

---

src/core/toxcall.h, line 109 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

Keeping with the RAII view of the lifetime of a call being represented as a ToxCall, it seems like ending call should be part of the destructor of ToxCall. Additionally every call to endCall happens right before destroying the ToxCall object. To simplify usage and reduce the risk of using ToxCall in an invalid state, what about doing the endCall logic within the destructor and removing the explicit API?

Done.

---

src/widget/form/chatform.cpp, line 381 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

if endCall is moved within destructor, this could unconditionally call .reset()

Done.

---

src/widget/form/chatform.cpp, line 434 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

this loses thread safety compared to the previous implementation. Before the get/set were both done within CoreAV::callsLock, but now, even though ToxCall::muteMicis atomic, there could be a race between the get and set. This seems not trivial to fix: https://stackoverflow.com/a/9806613.

Maybe a toggle method could be added with a class mutex? Or the underlying type could be changed from bool and the xor method could be used, but that doesn't show intent as clearly IMO.

I'd just leave it at not atomic, because I don't think in this specific case the probability of hitting the non-atomic case is close non existant.

---

src/widget/form/groupchatform.cpp, line 244 at r4 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

any reason to check call != nullptr here but not in onMicMuteToggle or onVolMuteToggle? I'd think that on any of these slots our call should still be valid, assuming our signals come in in the right order :P

because for the other two cases the call is guarded by audioInputFlag = false; audioOutputFlag = false; when reseting the call ptr.

[anthonybilinski]

Reviewed 4 of 4 files at r5.
Reviewable status: complete! 1 of 1 LGTMs obtained

---

src/core/coreav.h, line 52 at r3 (raw file):

Previously, sudden6 wrote…

I tried to do that, but a problem turned up: Calls can be allocated in CoreAV::callCallback and if we use raw pointers for this map, we risk never deallocating them properly. We could solve this, by making sure non of the ToxCall objects are alive when CoreAV is destroyed, but with our current mess of Widget destruction I don't see this happening. I don't think overhead of shared_ptr vs unique_ptr can be an argument here.

I guess the way to push the RAII further would be to not create a ToxCall in CoreAV::callCallback, but instead do it in CoreAV::answerCall and return it there. Then if ChatForm rejects the call, there would be no ToxCall to clean up. It would have the added benefit(?) of not opening the webcam and mic before the call is answered.

My bigger concern with the shared_ptr is that it muddies ownership rather than the performance hit. It won't block it on this change now, it can always be done later.

---

src/core/coreav.cpp, line 272 at r3 (raw file):

Previously, sudden6 wrote…

I have not yet figured out the complete flow of setActive, it's used here and in stateCallback, I just remember things broke when I touched it, so it stays in for now.

Ok, I guess this is at least existing weirdness

---

src/widget/form/chatform.cpp, line 434 at r3 (raw file):

Previously, sudden6 wrote…

I'd just leave it at not atomic, because I don't think in this specific case the probability of hitting the non-atomic case is close non existant.

Alright, I might make a quick PR after then, but don't want to keep holding this up.

[anthonybilinski]

Reviewable status: complete! 1 of 1 LGTMs obtained

a discussion (no related file):
This segfaults if a you join then leave a group call:

Thread 1 "qtox" received signal SIGSEGV, Segmentation fault.                                                                                                                                               
std::__atomic_base<bool>::load (__m=std::memory_order_seq_cst, this=0x28) at /usr/include/c++/10/bits/atomic_base.h:426
426             return __atomic_load_n(&_M_i, int(__m));                                                                                                                                                   
(gdb) bt                                                                                             
#0  std::__atomic_base<bool>::load(std::memory_order) const (__m=std::memory_order_seq_cst, this=0x28) at /usr/include/c++/10/bits/atomic_base.h:426
#1  std::atomic<bool>::operator bool() const (this=0x28) at /usr/include/c++/10/atomic:87
#2  0x00005555558305d9 in ToxCall::getMuteMic() const (this=0x0) at ../src/core/toxcall.cpp:88
#3  0x000055555592f48a in GroupChatForm::onCallClicked() (this=0x555558368f90) at ../src/widget/form/groupchatform.cpp:333

---

[anthonybilinski]

Reviewable status: complete! 1 of 1 LGTMs obtained

a discussion (no related file):
The call button in 1:1 also seems to have the wrong state in a few cases, e.g. while it's ringing if you press it again to cancel, it does nothing and logs:

[22:34:38.704 UTC] core/coreav.cpp:232 : Debug: "Starting call with 0"
[22:34:38.704 UTC] core/coreav.cpp:235 : Warning: "Can't start call with 0, we're already in this call!"
[22:34:38.704 UTC] widget/form/chatform.cpp:394 : Debug: Failed to start Audio call

Once they pick up it then goes green instead of red and you can't end call. The other end gets a red call button when they answer, but clicking it tries to start a new call instead of ending the call.

---

[anthonybilinski]

Reviewable status: complete! 1 of 1 LGTMs obtained

a discussion (no related file):
This also doesn't destroy the ToxCall in the case of the friend ending the call. CoreAv emits avEnd which reaches ChatForm::onAvEnd, but that doesn't actually clear the call.

I'm working on changing ownership of the ToxCall and handling these three problems, so I can update the PR soon with the fixes.

---

[anthonybilinski]

Ok well I'm happy enough with this now, I've addressed my concerns except for the atomic get/set. I don't really want to LGTM since I made some non-trivial changes on top of your branch, if you could review my last 3 commits I'd be happy for it to go it.

Reviewable status: complete! 1 of 1 LGTMs obtained

a discussion (no related file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

This segfaults if a you join then leave a group call:

Thread 1 "qtox" received signal SIGSEGV, Segmentation fault.                                                                                                                                               
std::__atomic_base<bool>::load (__m=std::memory_order_seq_cst, this=0x28) at /usr/include/c++/10/bits/atomic_base.h:426
426             return __atomic_load_n(&_M_i, int(__m));                                                                                                                                                   
(gdb) bt                                                                                             
#0  std::__atomic_base<bool>::load(std::memory_order) const (__m=std::memory_order_seq_cst, this=0x28) at /usr/include/c++/10/bits/atomic_base.h:426
#1  std::atomic<bool>::operator bool() const (this=0x28) at /usr/include/c++/10/atomic:87
#2  0x00005555558305d9 in ToxCall::getMuteMic() const (this=0x0) at ../src/core/toxcall.cpp:88
#3  0x000055555592f48a in GroupChatForm::onCallClicked() (this=0x555558368f90) at ../src/widget/form/groupchatform.cpp:333

Fixed, chatform was accessing the null call in the second argument but was checking it in the first.

---

a discussion (no related file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

The call button in 1:1 also seems to have the wrong state in a few cases, e.g. while it's ringing if you press it again to cancel, it does nothing and logs:

[22:34:38.704 UTC] core/coreav.cpp:232 : Debug: "Starting call with 0"
[22:34:38.704 UTC] core/coreav.cpp:235 : Warning: "Can't start call with 0, we're already in this call!"
[22:34:38.704 UTC] widget/form/chatform.cpp:394 : Debug: Failed to start Audio call

Once they pick up it then goes green instead of red and you can't end call. The other end gets a red call button when they answer, but clicking it tries to start a new call instead of ending the call.

Fixed, think this was related to the next issue where call wasn't being reset in chatform.

---

a discussion (no related file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

This also doesn't destroy the ToxCall in the case of the friend ending the call. CoreAv emits avEnd which reaches ChatForm::onAvEnd, but that doesn't actually clear the call.

I'm working on changing ownership of the ToxCall and handling these three problems, so I can update the PR soon with the fixes.

Fixed. Little more complicated than just .reset()ing in onAvEnd because that causes a loop where it calls back into cancel call. I changed coreav to erase the call before emitting avEnd, and to ignore cancels to calls that it doesn't know about. It doesn't seem super elegant..

---

---

src/core/coreav.h, line 52 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

I guess the way to push the RAII further would be to not create a ToxCall in CoreAV::callCallback, but instead do it in CoreAV::answerCall and return it there. Then if ChatForm rejects the call, there would be no ToxCall to clean up. It would have the added benefit(?) of not opening the webcam and mic before the call is answered.

My bigger concern with the shared_ptr is that it muddies ownership rather than the performance hit. It won't block it on this change now, it can always be done later.

Amended this myself, doing it in answerCall isn't great because then it splits ownership between CoreAv and ChatForm, and when ChatForm wants to cancel a ringing call, it has to ask CoreAv to do that instead of doing it itself. Instead I return it through the invite signal, which requires it staying as a shared_ptr since signals require the argument to be copyable. Groupcalls could be made a unique_ptr since those are only ever returned directly. In both cases CoreAv has a purely non-owning map.

---

src/core/coreav.cpp, line 272 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

Ok, I guess this is at least existing weirdness

I removed it only in the cases where it's set right before being destroyed, since it isn't used in the destructor and doesn't emit any signal, it's just dead code. Have tested with that change. Left in other uses of it.

[bodwok]

Additional testing is required

Reviewable status: 0 of 1 LGTMs obtained

[bodwok]

Reviewed 1 of 1 files at r7, 1 of 5 files at r8.
Reviewable status: 0 of 1 LGTMs obtained

---

src/core/coreav.cpp, line 516 at r8 (raw file):

    assert(call != nullptr);

    auto it = self->calls.emplace(friendNum, std::move(call));

calls is of type: std::map<uint32_t, ToxFriendCall*>, need to use call.get() here?

[sphaerophoria]

At the request of @anthonybilinski I'm looking over this. Just throwing my 2c into the ring hoping that it doesn't rub anyone the wrong way.

Spent some time getting familiar with the layout before the change in order to understand why the change was good. @sudden6 I like the direction you went here. Creating an RAII abstraction around CoreAV so that upper classes don't have to use core directly is definitely a direction I agree with.

As far as I understand before ChatForm didn't see ToxCall at all. ToxCall was more of a helper class for CoreAv to keep track of it's call states and ChatForm asked CoreAv for call information. Something like this...

After this change it looks a little more like this

The ownership is a little more muddled. It looks like @anthonybilinski wanted to move the ownership to a single spot which is good, but with the current code layout CoreAv still needs access to the ToxCall object. There isn't anything wrong with this in particular, but @anthonybilinski indicated in a private chat that the design goal moving forward is for Core/CoreAv to mostly be Qt wrappers around the toxcore APIs. If that is the case I believe the following changes could solidify ownership of responsibilities while moving us in the desired direction.

What do you guys think?

Reviewable status: 0 of 1 LGTMs obtained

[sudden6]

@sphaerophoria I'm not yet deep into the changed code by @anthonybilinski but it seems that your proposed architecture additionally complicates the dependencies (diagram 3). AFAICT with the dispatcher you're mostly moving the routing of signals from CoreAV out to a separate class, correct? Personally my intention wasn't for CoreAV to be a think wrapper around c-toxcore, but rather a nice (maybe fat) abstraction around it so it can be conveniently used with Qt. So I see no problem when CoreAV keeps references to ToxCalls. I see it more as a factory, it creates object and as any good factory keeps track of where they go, after they are not needed the resources are recycled by the factory ;P

[sudden6]

Reviewable status: 0 of 1 LGTMs obtained

---

src/core/coreav.h, line 139 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

same re: ownership

I'm still not very happy about storing raw pointers here, what about weak_ptr? it seems designed for exactly this problem.

---

src/core/coreav.cpp, line 272 at r3 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

I removed it only in the cases where it's set right before being destroyed, since it isn't used in the destructor and doesn't emit any signal, it's just dead code. Have tested with that change. Left in other uses of it.

LGTM, my problems were probably caused by one of the other things you fixed.

---

src/widget/form/chatform.cpp, line 285 at r8 (raw file):

    }

    if (this->call) {

Minor thing: do you expect to run into this regularly? Else it should be turned into an assert IMO

[sphaerophoria]

I think where youre coming from. Why add extra classes resulting in more complex relationships between classes?

My thoughts were that the core layer should basically just convert callbacks to signals and do some minor conversion between tox types and qtox core types. The surface area of the toxcore api is already very wide, if we start mixing responsibilities of the core classes with higher level abstractions (like an RAII call wrapper) the scope of the class will become huge. If we move the ownership into its own class we now have a class with a single responsibility, in theory this can make testing of the ToxCall area simple as we can test the manager/dispatcher and ToxCalls with a mocked out CoreAv as well.

That being said I have no say in this, so feel free to just ignore me and do what you prefer ;)

Reviewable status: 0 of 1 LGTMs obtained

[sudden6]

@sphaerophoria yes that is exactly my main concern.

Another concern is that now instead of one slightly complicated class (CoreAV) you have two classes (CoreAV and the Dispatcher) that need to manage and track the state.

As for testing, I don't really see how additionally testing ToxCall and Dispatcher is better than just mocking CoreAV.

[sphaerophoria]

As for testing, I don't really see how additionally testing ToxCall and Dispatcher is better than just mocking CoreAV.

My thoughts were that we could now test the logic around creation/removal of ToxCalls with a mocked CoreAV. Without the split you have to instantiate CoreAV which I would imagine is not trivial to do easily in a testing context.

Personally I'd prefer more classes with more complex relationships as it makes reasoning about each smaller class easier (part of my understanding of SOLID and why people like it).

[sudden6]

Without the split you have to instantiate CoreAV which I would imagine is not trivial to do easily in a testing context.

I'd have imagined to use a mock of CoreAV for testing the upper layers, and using the real one for testing CoreAV + Core interactions. Not sure if that's enough, but it's already more than we have.

[bodwok]

I tested the new changes with the latest version qTox and found no new bugs (other than the compile-level error mentioned above).

It looks like that at the beginning, when I was reviewing the code, I compared this PR with a similar one. Thanks to @anthonybilinski, we managed to discover this.

A test for CoreAV would also be useful here and for future changes.

[anthonybilinski]

Rebased and addressed review comments just to keep this version up to date, even though we're not necessarily going with it.

RE: design conversation,

Personally my intention wasn't for CoreAV to be a thin wrapper around c-toxcore, but rather a nice (maybe fat) abstraction around it so it can be conveniently used with Qt.

Whoops my mistake for incorrectly relaying that.

I agree with @sphaerophoria that Core is already a very wide class with complex responsibilities. Although like @sudden6 said we want a possibly fat abstraction layer that makes using Tox easy with Qt, that layer including multiple classes instead of one bigger class doesn't seem like a problem to me. To keep them together they could be stuck in a Core lib or just a namespace, but it doesn't seem like it needs to stay as one class to form that layer.

It's true the inter-class relationship gets more complex, but the complexity is moved out of intra-class relationships in Core where there's already a lot going on. Having a few classes working together each with state can be more easily reasoned about e.g. either looking inside of Core or ToxCallDispatcher with a few things going on in each, or looking at the external interactions between them with a few more things there. I think this is the idea of the Single Responsibility Principle in SOLID like @sphaerophoria said.

So overall I'm in favour of the new class to reduce work done by Core and trying to keep Core thin even if we want a fat abstraction layer overall.

For ownership, my idea of moving ownership to be owned solely by the starter of the call only really makes sense if they're also the only one ending the call, but if we ever have a case where multiple view classes could start or end a call, e.g. ChatForm could start it, but if theoretically the FriendListWidget had a right click -> end call or the tray icon did or something, it wouldn't really handle that shared control well. Better might be for ToxCallDispatcher to be the owner, when a call is started they emit a signal with a shared_ptr of it which anyone interested in the call state can hang onto, and when one of the users wants to end the call instead of just deleting their ToxCall, they could ask the dispatcher to end it which could emit a signal to all reference holders which could all remove their shared_ptr on their slots to then delete the ToxCall when done. This would mean a change back to shared_pointer even though the ownership would be managed by ToxCallDispatcher. This might even be worth doing now with only 1 view class touching lifetime, since both CoreAv and ChatForm can want to end a call based on either user input or a call state callback. My current method CoreAV::stateCallback -> ChatForm::onAvEnd -> CoreAV::cancelCall leads to the weird case of sometimes CoreAV already knows the call is ended before cancelCall is called.

Overall I think my favourite direction would be for ToxCallDispatcher to be changed into ToxCallManager which would own the lifetime a bit loosely holding a shared_ptr, with both CoreAV and ChatForm getting signals from it when a call is started containing a shared_ptr, and both signaling to ToxCallManager when they want to end the call, which signals to all owners which causes them each to delete their shared_ptr of ToxCall.

Thoughts?

Reviewed 10 of 10 files at r9, 5 of 5 files at r10, 4 of 4 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 5 of 5 files at r14.
Reviewable status: 0 of 1 LGTMs obtained

---

src/core/coreav.h, line 139 at r3 (raw file):

Previously, sudden6 wrote…

I'm still not very happy about storing raw pointers here, what about weak_ptr? it seems designed for exactly this problem.

I think raw pointers are the recommended way to hold non-owning pointers according to ISOCPP: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-ptr. The lifetime here is guaranteed by ToxCall, so I don't think weak_ptr to check if the pointer is still valid is really necessary.

It seems like the only place they recommend weak_ptr is for breaking loops of shared_ptrs, but I think either with sole ownership unique_ptr is better and shared ownership shared_ptr is better.

---

src/core/coreav.cpp, line 516 at r8 (raw file):

Previously, bodwok wrote…

calls is of type: std::map<uint32_t, ToxFriendCall*>, need to use call.get() here?

Oops yeah, missed in a commit split I think. Thanks.

---

src/widget/form/chatform.cpp, line 285 at r8 (raw file):

Previously, sudden6 wrote…

Minor thing: do you expect to run into this regularly? Else it should be turned into an assert IMO

Agreed, changed 👍

[sudden6]

Thoughts?

To me this seems a bit over engineered for what we use it now and I fear we'll never use the functionality of the extra manager class to the extent we really need it. On the other hand, making one class do a single thing makes sense and I'd rather agree to the over engineered solution now and get it in in a reasonable timeframe than to drag this issue to infinity.

[bodwok]

ToxCallManager reduces CoreAV's responsibility which is good.
In my opinion, the following behavior can help distribute functionality more evenly. Although more changes to the code are required:
CoreAV - passing events from Tox API to ToxCallManager.
ChatForm - passing events from user to ToxCallManager.
ToxCallManager - owns and processes ToxCall instances, provides an interface to CoreAV and ChatForm.

All functions that process ToxCall (such as isCallStarted, isCallActive, toggleMuteCallInput, etc.) will be transferred to ToxCallManager.

What do you think?

[sudden6]

All functions that process ToxCall (such as isCallStarted, isCallActive, toggleMuteCallInput, etc.) will be transferred to ToxCallManager.

This is exactly the opposite of what I was trying to do here 😅

[bodwok]

😄 OK, just as one of the possible updates.

[codecov-commenter]

Codecov Report

Merging #6235 (b600571) into master (0afc11f) will increase coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head b600571 differs from pull request most recent head 1a64746. Consider uploading reports for the commit 1a64746 to get more accurate results

@@            Coverage Diff             @@
##           master    #6235      +/-   ##
==========================================
+ Coverage   10.20%   10.24%   +0.04%     
==========================================
  Files         296      296              
  Lines       20117    20026      -91     
==========================================
  Hits         2052     2052              
+ Misses      18065    17974      -91     

Impacted Files	Coverage Δ
src/core/coreav.cpp	0.00% <0.00%> (ø)
src/core/coreav.h	0.00% <0.00%> (ø)
src/core/toxcall.cpp	0.00% <0.00%> (ø)
src/widget/form/chatform.cpp	0.23% <0.00%> (-0.01%)	⬇️
src/widget/form/chatform.h	0.00% <ø> (ø)
src/widget/form/groupchatform.cpp	2.16% <0.00%> (+0.04%)	⬆️
src/widget/form/groupchatform.h	0.00% <ø> (ø)
src/widget/widget.cpp	0.00% <ø> (ø)
src/widget/widget.h	0.00% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0afc11f...1a64746. Read the comment docs.