Skip to content
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

Use checkbox menu in crate selection. #1341

Merged
merged 7 commits into from
Dec 23, 2017
Merged
43 changes: 43 additions & 0 deletions src/library/crate/cratestorage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,26 @@ class CrateQueryBinder {
FwdSqlQuery& m_query;
};

const QChar kSqlListSeparator(',');

// It is not possible to bind multiple values as a list to a query.
// The list of track ids has to be transformed into a single list
// string before it can be used in an SQL query.
QString joinSqlStringList(const QList<TrackId>& trackIds) {
QString joinedTrackIds;
// Reserve memory up front to prevent reallocation. Here we
// assume that all track ids fit into 6 decimal digits and
// add 1 character for the list separator.
joinedTrackIds.reserve((6 + 1) * trackIds.size());
for (const auto trackId: trackIds) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto& trackId to prevent unnecessary copy

if (!joinedTrackIds.isEmpty()) {
joinedTrackIds += kSqlListSeparator;
}
joinedTrackIds += trackId.toString();
}
return joinedTrackIds;
}

} // anonymous namespace


Expand Down Expand Up @@ -468,6 +488,29 @@ CrateTrackSelectResult CrateStorage::selectTrackCratesSorted(TrackId trackId) co
}
}

CrateSummarySelectResult CrateStorage::selectCratesWithTrackCount(const QList<TrackId>& trackIds) const {
FwdSqlQuery query(m_database, QString(
"SELECT *, ("
" SELECT COUNT(*) FROM %1 WHERE %2.%3 = %1.%4 and %1.%5 in (%9)"
" ) AS %6, 0 as %7 FROM %2 ORDER BY %8").arg(
CRATE_TRACKS_TABLE,
CRATE_TABLE,
CRATETABLE_ID,
CRATETRACKSTABLE_CRATEID,
CRATETRACKSTABLE_TRACKID,
CRATESUMMARY_TRACK_COUNT,
CRATESUMMARY_TRACK_DURATION,
CRATETABLE_NAME,
joinSqlStringList(trackIds)));

if (query.execPrepared()) {
return CrateSummarySelectResult(std::move(query));
} else {
return CrateSummarySelectResult();
}
}



CrateTrackSelectResult CrateStorage::selectTracksSortedByCrateNameLike(const QString& crateNameLike) const {
FwdSqlQuery query(m_database, QString(
Expand Down
3 changes: 2 additions & 1 deletion src/library/crate/cratestorage.h
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,8 @@ class CrateStorage: public virtual /*implements*/ SqlStorage {
CrateId crateId) const;
CrateTrackSelectResult selectTrackCratesSorted(
TrackId trackId) const;
CrateSummarySelectResult selectCratesWithTrackCount(
const QList<TrackId>& trackIds) const;
CrateTrackSelectResult selectTracksSortedByCrateNameLike(
const QString& crateNameLike) const;

Expand All @@ -284,7 +286,6 @@ class CrateStorage: public virtual /*implements*/ SqlStorage {
QSet<CrateId> collectCrateIdsOfTracks(
const QList<TrackId>& trackIds) const;


/////////////////////////////////////////////////////////////////////////
// CrateSummary view operations (read-only, const)
/////////////////////////////////////////////////////////////////////////
Expand Down
21 changes: 19 additions & 2 deletions src/library/setlogfeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@
#include "library/treeitem.h"
#include "mixer/playerinfo.h"
#include "mixer/playermanager.h"
#include "widget/wtracktableview.h"
#include "widget/wlibrary.h"

SetlogFeature::SetlogFeature(QObject* parent,
UserSettingsPointer pConfig,
TrackCollection* pTrackCollection)
: BasePlaylistFeature(parent, pConfig, pTrackCollection, "SETLOGHOME"),
m_playlistId(-1) {
m_playlistId(-1),
m_libraryWidget(nullptr) {
m_pPlaylistTableModel = new PlaylistTableModel(this, pTrackCollection,
"mixxx.db.model.setlog",
true); //show all tracks
Expand Down Expand Up @@ -60,6 +63,8 @@ void SetlogFeature::bindWidget(WLibrary* libraryWidget,
keyboard);
connect(&PlayerInfo::instance(), SIGNAL(currentPlayingTrackChanged(TrackPointer)),
this, SLOT(slotPlayingTrackChanged(TrackPointer)));
m_libraryWidget = libraryWidget;

}

void SetlogFeature::onRightClick(const QPoint& globalPos) {
Expand Down Expand Up @@ -268,7 +273,19 @@ void SetlogFeature::slotPlayingTrackChanged(TrackPointer currentPlayingTrack) {

if (m_pPlaylistTableModel->getPlaylist() == m_playlistId) {
// View needs a refresh
m_pPlaylistTableModel->appendTrack(currentPlayingTrackId);

WTrackTableView* view = dynamic_cast<WTrackTableView*>(m_libraryWidget->getActiveView());
if (view != nullptr) {
// We have a active view on the history. The user may have some
// important active selection. For example putting track into crates
// while the song changes trough autodj. The selection is then lost
// and dataloss occures
const QList<TrackId> trackIds = view->getSelectedTrackIds();
m_pPlaylistTableModel->appendTrack(currentPlayingTrackId);
view->setSelectedTracks(trackIds);
} else {
m_pPlaylistTableModel->appendTrack(currentPlayingTrackId);
}
} else {
// TODO(XXX): Care whether the append succeeded.
m_playlistDao.appendTrackToPlaylist(currentPlayingTrackId,
Expand Down
1 change: 1 addition & 0 deletions src/library/setlogfeature.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class SetlogFeature : public BasePlaylistFeature {
QAction* m_pJoinWithPreviousAction;
QAction* m_pGetNewPlaylist;
int m_playlistId;
WLibrary* m_libraryWidget;
};

#endif // SETLOGFEATURE_H
4 changes: 2 additions & 2 deletions src/util/parented_ptr.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ inline bool operator== (const parented_ptr<T>& lhs, const U* rhs) {
}

template<typename T, typename U>
inline bool operator== (const parented_ptr<T>& lhs, const parented_ptr<U>& rhs) const {
inline bool operator== (const parented_ptr<T>& lhs, const parented_ptr<U>& rhs) {
return lhs.get() == rhs.get();
}

Expand All @@ -115,7 +115,7 @@ inline bool operator!= (const parented_ptr<T>& lhs, const U* rhs) {
}

template<typename T, typename U>
inline bool operator!= (const parented_ptr<T>& lhs, const parented_ptr<U>& rhs) const {
inline bool operator!= (const parented_ptr<T>& lhs, const parented_ptr<U>& rhs) {
return !(lhs.get() == rhs.get());
}

Expand Down
118 changes: 102 additions & 16 deletions src/widget/wtracktableview.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#include <QUrl>
#include <QDrag>
#include <QShortcut>
#include <QWidgetAction>
#include <QCheckBox>
#include <QLinkedList>

#include "widget/wtracktableview.h"

Expand All @@ -26,6 +29,7 @@
#include "util/dnd.h"
#include "util/time.h"
#include "util/assert.h"
#include "util/parented_ptr.h"

WTrackTableView::WTrackTableView(QWidget * parent,
UserSettingsPointer pConfig,
Expand Down Expand Up @@ -69,7 +73,7 @@ WTrackTableView::WTrackTableView(QWidget * parent,
m_pPlaylistMenu = new QMenu(this);
m_pPlaylistMenu->setTitle(tr("Add to Playlist"));
m_pCrateMenu = new QMenu(this);
m_pCrateMenu->setTitle(tr("Add to Crate"));
m_pCrateMenu->setTitle(tr("Crates"));
m_pBPMMenu = new QMenu(this);
m_pBPMMenu->setTitle(tr("BPM Options"));
m_pCoverMenu = new WCoverArtMenu(this);
Expand All @@ -93,8 +97,9 @@ WTrackTableView::WTrackTableView(QWidget * parent,

connect(&m_playlistMapper, SIGNAL(mapped(int)),
this, SLOT(addSelectionToPlaylist(int)));
connect(&m_crateMapper, SIGNAL(mapped(int)),
this, SLOT(addSelectionToCrate(int)));

connect(&m_crateMapper, SIGNAL(mapped(QWidget *)),
this, SLOT(addRemoveSelectionInCrate(QWidget *)));

m_pCOTGuiTick = new ControlProxy("[Master]", "guiTick50ms", this);
m_pCOTGuiTick->connectValueChanged(SLOT(slotGuiTick50ms(double)));
Expand Down Expand Up @@ -827,21 +832,44 @@ void WTrackTableView::contextMenuEvent(QContextMenuEvent* event) {

if (modelHasCapabilities(TrackModel::TRACKMODELCAPS_ADDTOCRATE)) {
m_pCrateMenu->clear();
CrateSelectResult allCrates(m_pTrackCollection->crates().selectCrates());
Crate crate;
const QList<TrackId> trackIds = getSelectedTrackIds();

CrateSummarySelectResult allCrates(m_pTrackCollection->crates().selectCratesWithTrackCount(trackIds));

CrateSummary crate;
while (allCrates.populateNext(&crate)) {
auto pAction = std::make_unique<QAction>(crate.getName(), m_pCrateMenu);
auto pAction = make_parented<QWidgetAction>(m_pCrateMenu);
auto pCheckBox = make_parented<QCheckBox>(m_pCrateMenu);

pCheckBox->setText(crate.getName());
pCheckBox->setProperty("crateId",
QVariant::fromValue(crate.getId()));
pCheckBox->setEnabled(!crate.isLocked());
pAction->setEnabled(!crate.isLocked());
m_crateMapper.setMapping(pAction.get(), crate.getId().toInt());
connect(pAction.get(), SIGNAL(triggered()), &m_crateMapper, SLOT(map()));
pAction->setDefaultWidget(pCheckBox.get());

if(crate.getTrackCount() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (

pCheckBox->setChecked(false);
} else if(crate.getTrackCount() == (uint)trackIds.length()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else if (

pCheckBox->setChecked(true);
} else {
pCheckBox->setTristate(true);
pCheckBox->setCheckState(Qt::PartiallyChecked);
}

m_crateMapper.setMapping(pAction.get(), pCheckBox.get());
m_crateMapper.setMapping(pCheckBox.get(), pCheckBox.get());
m_pCrateMenu->addAction(pAction.get());
pAction.release();
connect(pAction.get(), SIGNAL(triggered()),
&m_crateMapper, SLOT(map()));
connect(pCheckBox.get(), SIGNAL(stateChanged(int)),
&m_crateMapper, SLOT(map()));

}
m_pCrateMenu->addSeparator();
QAction* newCrateAction = new QAction(tr("Create New Crate"), m_pCrateMenu);
m_pCrateMenu->addAction(newCrateAction);
m_crateMapper.setMapping(newCrateAction, CrateId().toInt());// invalid crate id for new crate
connect(newCrateAction, SIGNAL(triggered()), &m_crateMapper, SLOT(map()));
connect(newCrateAction, SIGNAL(triggered()), this, SLOT(addSelectionToNewCrate()));

m_pMenu->addMenu(m_pCrateMenu);
}
Expand Down Expand Up @@ -1355,6 +1383,31 @@ QList<TrackId> WTrackTableView::getSelectedTrackIds() const {
return trackIds;
}

void WTrackTableView::setSelectedTracks(QList<TrackId> trackIds) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter should be passed as const-ref to avoid a deep copy of the list. Unfortunately the range-based for loops do not work well with Qt containers. Declaring the loop variable as const-ref is not enough. The container itself must be const! There is already a link in our coding guidelines.

We have this issue at many places and I have done this wrong until recently. Checking the code with clazy could reveal those hidden performance killers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix this after merge

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already did in #1427

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

included in 187ec3d by @Be-ing

QItemSelectionModel* pSelectionModel = selectionModel();
VERIFY_OR_DEBUG_ASSERT(pSelectionModel != nullptr) {
qWarning() << "No selected tracks available";
return;
}

TrackModel* pTrackModel = getTrackModel();
VERIFY_OR_DEBUG_ASSERT(pTrackModel != nullptr) {
qWarning() << "No selected tracks available";
return;
}

foreach(TrackId trackId, trackIds) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use modern standard C++ instead of old Qt foreach macro: for (const auto& trackId : trackIds) {

const QLinkedList<int> gts = pTrackModel->getTrackRows(trackId);

QLinkedList<int>::const_iterator i;
for (i = gts.constBegin(); i != gts.constEnd(); ++i) {
pSelectionModel->select(model()->index(*i, 0),
QItemSelectionModel::Select | QItemSelectionModel::Rows);
}
}
}


void WTrackTableView::sendToAutoDJ(PlaylistDAO::AutoDJSendLoc loc) {
if (!modelHasCapabilities(TrackModel::TRACKMODELCAPS_ADDTOAUTODJ)) {
return;
Expand Down Expand Up @@ -1466,22 +1519,55 @@ void WTrackTableView::addSelectionToPlaylist(int iPlaylistId) {
playlistDao.appendTracksToPlaylist(trackIds, iPlaylistId);
}

void WTrackTableView::addSelectionToCrate(int iCrateId) {
void WTrackTableView::addRemoveSelectionInCrate(QWidget* pWidget) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function name is ambiguous. Does it add or does it remove?

auto pCheckBox = qobject_cast<QCheckBox*>(pWidget);
VERIFY_OR_DEBUG_ASSERT(pCheckBox) {
qWarning() << "crateId is not ef CrateId type";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"not ef"? Did you mean "not of"?

return;
}
CrateId crateId = pCheckBox->property("crateId").value<CrateId>();

const QList<TrackId> trackIds = getSelectedTrackIds();

if (trackIds.isEmpty()) {
qWarning() << "No tracks selected for crate";
return;
}

CrateId crateId(iCrateId);
if (!crateId.isValid()) { // i.e. a new crate is suppose to be created
crateId = CrateFeatureHelper(
m_pTrackCollection, m_pConfig).createEmptyCrate();
// we need to disable tristate again as the mixed state will now be gone and can't be brought back
pCheckBox->setTristate(false);
if(!pCheckBox->isChecked()) {
if (crateId.isValid()) {
m_pTrackCollection->removeCrateTracks(crateId, trackIds);
}
} else {
if (!crateId.isValid()) { // i.e. a new crate is suppose to be created
crateId = CrateFeatureHelper(
m_pTrackCollection, m_pConfig).createEmptyCrate();
}
if (crateId.isValid()) {
m_pTrackCollection->unhideTracks(trackIds);
m_pTrackCollection->addCrateTracks(crateId, trackIds);
}
}
}

void WTrackTableView::addSelectionToNewCrate() {
const QList<TrackId> trackIds = getSelectedTrackIds();

if (trackIds.isEmpty()) {
qWarning() << "No tracks selected for crate";
return;
}

CrateId crateId = CrateFeatureHelper(
m_pTrackCollection, m_pConfig).createEmptyCrate();

if (crateId.isValid()) {
m_pTrackCollection->unhideTracks(trackIds);
m_pTrackCollection->addCrateTracks(crateId, trackIds);
}

}

void WTrackTableView::doSortByColumn(int headerSection) {
Expand Down
8 changes: 5 additions & 3 deletions src/widget/wtracktableview.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ class WTrackTableView : public WLibraryTableView {
void loadSelectedTrack() override;
void loadSelectedTrackToGroup(QString group, bool play) override;

QList<TrackId> getSelectedTrackIds() const;
void setSelectedTracks(QList<TrackId> tracks);

public slots:
void loadTrackModel(QAbstractItemModel* model);
void slotMouseDoubleClicked(const QModelIndex &);
Expand All @@ -63,7 +66,8 @@ class WTrackTableView : public WLibraryTableView {
void slotReloadTrackMetadata();
void slotResetPlayed();
void addSelectionToPlaylist(int iPlaylistId);
void addSelectionToCrate(int iCrateId);
void addRemoveSelectionInCrate(QWidget* qc);
void addSelectionToNewCrate();
void loadSelectionToGroup(QString group, bool play = false);
void doSortByColumn(int headerSection);
void slotLockBpm();
Expand Down Expand Up @@ -104,8 +108,6 @@ class WTrackTableView : public WLibraryTableView {
TrackModel* getTrackModel() const;
bool modelHasCapabilities(TrackModel::CapabilitiesFlags capabilities) const;

QList<TrackId> getSelectedTrackIds() const;

UserSettingsPointer m_pConfig;
TrackCollection* m_pTrackCollection;

Expand Down