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.

Sign up for GitHub

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

lp1719474: Fix unresponsive scrolling through crates & playlists using encoder #1619

Merged
merged 1 commit into from
Apr 17, 2018
Merged

lp1719474: Fix unresponsive scrolling through crates & playlists using encoder #1619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/library/baseplaylistfeature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,17 @@
#include "widget/wlibrarytextbrowser.h"
#include "util/assert.h"

namespace {

const int kClickedChildActivationTimeoutMillis = 100;

} // anonymous namespace

BasePlaylistFeature::BasePlaylistFeature(QObject* parent,
UserSettingsPointer pConfig,
TrackCollection* pTrackCollection,
QString rootViewName)
: LibraryFeature(pConfig, parent),
: LibraryFeature(pConfig, kClickedChildActivationTimeoutMillis, parent),
m_pTrackCollection(pTrackCollection),
m_playlistDao(pTrackCollection->getPlaylistDAO()),
m_trackDao(pTrackCollection->getTrackDAO()),
Expand Down
4 changes: 3 additions & 1 deletion src/library/crate/cratefeature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@

namespace {

const int kClickedChildActivationTimeoutMillis = 100;

QString formatLabel(
const CrateSummary& crateSummary) {
return QString("%1 (%2) %3").arg(
Expand All @@ -42,7 +44,7 @@ QString formatLabel(
CrateFeature::CrateFeature(Library* pLibrary,
TrackCollection* pTrackCollection,
UserSettingsPointer pConfig)
: LibraryFeature(pConfig),
: LibraryFeature(pConfig, kClickedChildActivationTimeoutMillis),
m_cratesIcon(":/images/library/ic_library_crates.png"),
m_lockedCrateIcon(":/images/library/ic_library_locked.png"),
m_pTrackCollection(pTrackCollection),
Expand Down
29 changes: 24 additions & 5 deletions src/library/libraryfeature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,37 @@
// KEEP THIS cpp file to tell scons that moc should be called on the class!!!
// The reason for this is that LibraryFeature uses slots/signals and for this
// to work the code has to be precompiles by moc
LibraryFeature::LibraryFeature(QObject *parent)
: QObject(parent) {

LibraryFeature::LibraryFeature(
QObject *parent)
: QObject(parent),
m_clickedChildActivationTimeoutMillis(0) {
}

LibraryFeature::LibraryFeature(UserSettingsPointer pConfig, QObject* parent)
LibraryFeature::LibraryFeature(
int clickedChildActivationTimeoutMillis,
QObject *parent)
: QObject(parent),
m_pConfig(pConfig) {
m_clickedChildActivationTimeoutMillis(clickedChildActivationTimeoutMillis) {
DEBUG_ASSERT(m_clickedChildActivationTimeoutMillis >= 0);
}

LibraryFeature::~LibraryFeature() {
LibraryFeature::LibraryFeature(
UserSettingsPointer pConfig,
QObject* parent)
: QObject(parent),
m_pConfig(pConfig),
m_clickedChildActivationTimeoutMillis(0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is 0 here allowed?

Copy link
Contributor Author

@uklotzde uklotzde Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Because this is the default behavior, i.e. just process all system events and let the timer fire immediately. Almost the same behavior as before, just one roundtrip through the event loop. Prevents special case handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we could also define a default > 0 if that makes sense. Currently it is opt-in, only used by crates and playlists.

Copy link
Member

Choose a reason for hiding this comment

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

So the timer fires mediately with 0? The Qt doc is not clear about that, or negative values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally clear about that: "As a special case, a QTimer with a timeout of 0 will time out as soon as all the events in the window system's event queue have been processed. This can be used to do heavy work while providing a snappy user interface: ..."

}

LibraryFeature::LibraryFeature(
UserSettingsPointer pConfig,
int clickedChildActivationTimeoutMillis,
QObject* parent)
: QObject(parent),
m_pConfig(pConfig),
m_clickedChildActivationTimeoutMillis(clickedChildActivationTimeoutMillis) {
DEBUG_ASSERT(m_clickedChildActivationTimeoutMillis >= 0);
}

QStringList LibraryFeature::getPlaylistFiles(QFileDialog::FileMode mode) const {
Expand Down
21 changes: 17 additions & 4 deletions src/library/libraryfeature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,23 @@ class KeyboardEventFilter;
class LibraryFeature : public QObject {
Q_OBJECT
public:
LibraryFeature(QObject* parent = NULL);
explicit LibraryFeature(
QObject* parent);
explicit LibraryFeature(
int clickedChildActivationTimeoutMillis,
QObject* parent = nullptr);
explicit LibraryFeature(
UserSettingsPointer pConfig,
QObject* parent = nullptr);
explicit LibraryFeature(
UserSettingsPointer pConfig,
int clickedChildActivationTimeoutMillis,
QObject* parent = nullptr);
~LibraryFeature() override = default;

LibraryFeature(UserSettingsPointer pConfig,
QObject* parent = NULL);
virtual ~LibraryFeature();
int clickedChildActivationTimeoutMillis() const {
return m_clickedChildActivationTimeoutMillis;
}

virtual QVariant title() = 0;
virtual QIcon getIcon() = 0;
Expand Down Expand Up @@ -124,6 +136,7 @@ class LibraryFeature : public QObject {
private:
QStringList getPlaylistFiles(QFileDialog::FileMode mode) const;

const int m_clickedChildActivationTimeoutMillis;
};

#endif /* LIBRARYFEATURE_H */
47 changes: 34 additions & 13 deletions src/library/sidebarmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@
#include "library/browse/browsefeature.h"
#include "util/assert.h"

SidebarModel::SidebarModel(QObject* parent)
SidebarModel::SidebarModel(
QObject* parent)
: QAbstractItemModel(parent),
m_iDefaultSelectedIndex(0) {
}

SidebarModel::~SidebarModel() {

m_iDefaultSelectedIndex(0),
m_clickedChildActivationTimer(new QTimer(this)),
m_clickedFeature(nullptr) {
m_clickedChildActivationTimer->setSingleShot(true);
connect(m_clickedChildActivationTimer, SIGNAL(timeout()), this, SLOT(slotActivateChildAtClickedFeatureIndex()));
}

void SidebarModel::addLibraryFeature(LibraryFeature* feature) {
Expand Down Expand Up @@ -221,6 +222,20 @@ QVariant SidebarModel::data(const QModelIndex& index, int role) const {
return QVariant();
}

void SidebarModel::onFeatureIndexClicked(
LibraryFeature* feature,
QModelIndex index) {
m_clickedChildActivationTimer->stop();
m_clickedFeature = feature;
m_clickedIndex = index;
}

void SidebarModel::slotActivateChildAtClickedFeatureIndex() {
if (m_clickedFeature) {
m_clickedFeature->activateChild(m_clickedIndex);
}
}

void SidebarModel::clicked(const QModelIndex& index) {
//qDebug() << "SidebarModel::clicked() index=" << index;

Expand All @@ -236,8 +251,12 @@ void SidebarModel::clicked(const QModelIndex& index) {
} else {
TreeItem* tree_item = (TreeItem*)index.internalPointer();
if (tree_item) {
LibraryFeature* feature = tree_item->feature();
feature->activateChild(index);
onFeatureIndexClicked(tree_item->feature(), index);
DEBUG_ASSERT(m_clickedFeature);
// Deferred activation is required for smooth scrolling when using
// encoder knobs
m_clickedChildActivationTimer->start(
m_clickedFeature->clickedChildActivationTimeoutMillis());
}
}
}
Expand All @@ -249,8 +268,9 @@ void SidebarModel::doubleClicked(const QModelIndex& index) {
} else {
TreeItem* tree_item = (TreeItem*)index.internalPointer();
if (tree_item) {
LibraryFeature* feature = tree_item->feature();
feature->onLazyChildExpandation(index);
onFeatureIndexClicked(tree_item->feature(), index);
DEBUG_ASSERT(m_clickedFeature);
m_clickedFeature->onLazyChildExpandation(m_clickedIndex);
}
}
}
Expand All @@ -267,9 +287,10 @@ void SidebarModel::rightClicked(const QPoint& globalPos, const QModelIndex& inde
{
TreeItem* tree_item = (TreeItem*)index.internalPointer();
if (tree_item) {
LibraryFeature* feature = tree_item->feature();
feature->activateChild(index);
feature->onRightClickChild(globalPos, index);
onFeatureIndexClicked(tree_item->feature(), index);
DEBUG_ASSERT(m_clickedFeature);
m_clickedFeature->activateChild(m_clickedIndex);
m_clickedFeature->onRightClickChild(globalPos, m_clickedIndex);
}
}
}
Expand Down
17 changes: 15 additions & 2 deletions src/library/sidebarmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,17 @@
#include <QAbstractItemModel>
#include <QList>
#include <QModelIndex>
#include <QTimer>
#include <QVariant>

class LibraryFeature;

class SidebarModel : public QAbstractItemModel {
Q_OBJECT
public:
explicit SidebarModel(QObject* parent = 0);
virtual ~SidebarModel();
explicit SidebarModel(
QObject* parent = nullptr);
~SidebarModel() override = default;

void addLibraryFeature(LibraryFeature* feature);
QModelIndex getDefaultSelection();
Expand Down Expand Up @@ -64,11 +66,22 @@ class SidebarModel : public QAbstractItemModel {
signals:
void selectIndex(const QModelIndex& index);

private slots:
void slotActivateChildAtClickedFeatureIndex();

private:
QModelIndex translateSourceIndex(const QModelIndex& parent);
void featureRenamed(LibraryFeature*);
QList<LibraryFeature*> m_sFeatures;
unsigned int m_iDefaultSelectedIndex; /** Index of the item in the sidebar model to select at startup. */

QTimer* const m_clickedChildActivationTimer;
LibraryFeature* m_clickedFeature;
QModelIndex m_clickedIndex;

void onFeatureIndexClicked(
LibraryFeature* feature,
QModelIndex index);
};

#endif /* SIDEBARMODEL_H */