Avoid redundant messages for failed track load #10889
Conversation
src/mixer/basetrackplayer.cpp
Outdated
| if (pTrack && m_pPrevFailedTrack == pTrack) { | ||
| return; | ||
| } else if (pTrack) { | ||
| m_pPrevFailedTrack = pTrack; |
There was a problem hiding this comment.
Keeping TrackPtr cached for an unforeseeable time is not a good idea. Use the id or location.
There was a problem hiding this comment.
Makes sense, thanks.
Should I initialise (in the constructor) with m_pPrevFailedTrack() or m_pPrevFailedTrack(TrackId)?
There was a problem hiding this comment.
Sorry, @ronso0, I am no longer available for reviews. This was just a hint to prevent a regression.
ronso0
force-pushed
the
failed-track-load-message
branch
from
4eb384a to
8db1e40
Compare
src/mixer/basetrackplayer.cpp
Outdated
| // Alert user. | ||
| // Show only one message for this track at a time | ||
| TrackId currTrackId = pTrack->getId(); | ||
| if (pTrack && m_pPrevFailedTrackId == currTrackId) { |
There was a problem hiding this comment.
if pTrack is null, it has already crashed above. How about moving the if (pTrack) { to the beginning?
Can this ever be null, if not we can VERIFY_OR_DEBUG_ASSERT that.
Another idea would be to introduce a TrackPointerNotNull but that would be a bigger refactoring.
There was a problem hiding this comment.
if pTrack is null, it has already crashed above.
I don't understand where it would have "crashed above"?
IMO that is irrelevant here because the existing code expects having to deal with this and would do m_pChannelToCloneFrom = nullptr;, and I have no intention to change that.
There was a problem hiding this comment.
The line:
TrackId currTrackId = pTrack->getId();
will segfault if pTrack is null
so you will never reach the line below
if (pTrack &&
ronso0
force-pushed
the
failed-track-load-message
branch
from
8db1e40 to
540c386
Compare
After noticing the issue with my controller, at home I used a simple MIDI-Through script to trigger multiple track load actions: |
|
Thank you for the script. With it, I was able to reproduce the issue and confirm that it is fixed. I think the main issue is the at the modal message box disables the control via the main thread, but not via the controller thread so that you can continue control even if the message box is shown. This way one should be able to trick that workaround, by loading another not existing track and than the first. This way we can still stack up message boxes. The same happens if we load the same track into two or more decks. The later can even happen with the GUI, since track loading happens asynchronous. I am not against the proposed solution since it is a low impact salition for a real problem while my considerations are only an edge case. But at least we should add a comment about it. What do you think? |
|
How about returning early in |
Yes, that makes sense. |
with this LoadTrack tests fail (didn't look into it), so I'll revert e56a6dd |
ronso0
force-pushed
the
failed-track-load-message
branch
from
e56a6dd to
540c386
Compare
src/mixer/basetrackplayer.cpp
Outdated
| // Show only one message for this track at a time in case this slot is called | ||
| // repeatedly for the same track while any prior message is still shown. |
There was a problem hiding this comment.
| // Show only one message for this track at a time in case this slot is called | |
| // repeatedly for the same track while any prior message is still shown. | |
| // Due to the event loop inside QMessageBox, slotLoadFailed() can be called recursively | |
| // via repeated loads attempts from a controller. A prior message is still shown in this case. | |
| // In this case we don't show a second message box for the same track. |
An alternative suggestion for the comment, with a bit more details. An you take it or integrate it in your comment?
There was a problem hiding this comment.
Sure, I extended the comment.
will squash if you think it's okay.
OK |
Stumbled over this a few times after
LoadSelectedTrackwas triggered multiple times. Having multiple identical popups stacked was confusing, and is also pointless.This ensures there is only one QMessageBox for the same track.