-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Serato Cue refactorings #11467
Serato Cue refactorings #11467
Conversation
bool CueDAO::deleteCue(Cue* cue) const { | ||
//qDebug() << "CueDAO::deleteCue" << QThread::currentThread() << m_database.connectionName(); | ||
if (!cue->getId().isValid()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a good reason to delete this honestly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that it is no longer used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is there no valid usecase for it to be used? What is gained from removing the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It improved compile time and the time of developers for reading and understanding the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The improvements in compile time are utterly negligible. This will only cause confusions for developers when they see a class that can save a cue, but not delete it. The interface is incomplete and thus confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a private function. The intended use is to use of this class is to use it via the track with all cues at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine, then lets hope nobody will need this in the future then.
I have rebase the faulty edits out. I hope that was OK. Nothing else has been changed. |
QA: Please fix obvious typos in PR titles. Otherwise you won't be able to find closed issues or PRs when searching for "serato". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is a collection of small refactorings on top of: #11466
They have been discovered while debugging #11283
See commits.