Fix cue dirtiness#3544
Conversation
|
Alternative solution implemented in 1c18d96 - let me know whether I should revert it, was just an idea. |
| DEBUG_ASSERT(pCue->getId().isValid() || pCue->isDirty()); | ||
| // Update or save cue | ||
| if (pCue->isDirty()) { | ||
| if (!pCue->getId().isValid() || pCue->isDirty()) { |
There was a problem hiding this comment.
The removed debug assertion was failing very rarely (1 or 2 times) but I was not able to reproduce it nor have any idea what could have caused it.
@Holzhaus Any insights?
There was a problem hiding this comment.
I was able to trigger it reliably, that was the exactly why I made this PR ^^ but as said, I can also revert the last commit for the alternative solution.
| } | ||
|
|
||
| // Delete orphaned cues | ||
| // Purge orphaned cues |
There was a problem hiding this comment.
Please avoid to change code, formatting, or comments based on personal preferences without improving anything. This just causes noise.
There was a problem hiding this comment.
I am pretty sure we use the term "purge" whenever something gets removed irrevocably, whereas "delete" is a more broad term. So it seemed sensible to make it more precise here.
There was a problem hiding this comment.
Purge is used in the UI. It doesn't make sense to rephrase the following SQL "DELETE" command in the comment to "purge". Why should we use 2 different verbs in the same context??
There was a problem hiding this comment.
The SQL command is rather general - for machine-readabilty. Aren't the comments supposed to be as specific and clear as possible (without sacrificing conciseness)?
There was a problem hiding this comment.
I don't see any value and am not going to discuss this again.
| true); | ||
| auto cueInfo2 = cueObject.getCueInfo( | ||
| audio::SampleRate(44100)); | ||
| const Cue cueObject(cueInfo1, audio::SampleRate(44100)); |
There was a problem hiding this comment.
Why did you change this formatting manually?? The arguments have been placed onto single lines intentionally and our code formatting config preserve this style.
There was a problem hiding this comment.
If this is caused by your IDE then you are responsible to prevent it doing this.
There was a problem hiding this comment.
Or just use git add -p and only stage the hunks for commit that you actually want to change.
There was a problem hiding this comment.
@Holzhaus you can stop suggesting that, I almost always look at every single change before committing.
I thought that now with less arguments it might be sensible to compress this, especially as the arguments are not very interesting on their own.
|
#3599 should fix the actual cause. I suggest to close this PR. |
|
Obsolete |
3 participants
This fixes a regression introduced by #3016 which I am baffled stayed unnoticed for months:
Since new cues are now not assigned a track, they are never marked dirty and thus not saved. I haven't tested it extensively, but it seems that before this patch no new cues were ever saved.
It was fixed by marking cues as dirty upon initialization. It also makes the debug assertion more graceful.
Now that I write this, there would be an alternative solution: Do not mark new cues as dirty, but simply always save them when they don't have a valid id. Why that extra roundtrip?