Merge lp:~mixxxdevelopers/mixxx/tree_item_browser into lp:~mixxxdevelopers/mixxx/trunk

[RAFFI TEA]

This a rewrite of the BrowseFeature and has been tested on OS X, Windows and Linux.

The BrowseFeature is now expandable showing drive letters and folders. All supported audio files are listed in the right track table view along with their metadata.

Known bug:

Folders may have large content. Therefore, the population of the track table view is thread-based. Clicking quickly through the folder structure will cause Mixxx to crash. I need some help to get things thread-safe.

Thanks,

Tobias

[RJ Skerry-Ryan (rryan)]

Hey Tobias,

Checking out this I've got a couple of questions and things to fix:

1) Why the updates to libmad, libid3tag, and libtag?

2) I assume you're adding shell32 because you use system-specific calls in foldertreemodel.cpp? Could you comment that line in the SConscript to mention that's why it's there (mostly so I don't blindly remove it in the future thinking "gee we don't need this" :) )

3) In browsefeature.cpp the "My Music" literal needs a tr(). Also the naming of My Music might make Windows users actually think it's their "My Music" folder, while it's actually the library folder they picked in Mixxx. Maybe we should say "My Library" instead?

4) I think we should add some other useful links, e.g. a 'My Home' for the user's home directory, 'My Documents' or 'My Music' for their Music folder (if it isn't the same as their Mixxx library). You can do this in a cross-platform way with the QDesktopServices::storageLocation method

5) I noticed that when expanding directories in the sidebar, if you hit the expand triangle on a folder that you are not currently selecting, it won't expand. Is this the case in general? Can we fix it so that they expand? I'm on Ubuntu 10.04 with standard Qt.

6) BrowseTableModel::BrowseTableModel() all those header column names need tr()'s

7) (sorry for nitpicking) can isBackgroundThreadActive get an "m_b" prefix, also can you move the initialization of isBackgroundThreadActive and m_currentPath to the constructor initializer list?

8) For the populateModel method in BrowseTableModel, I think you should change it a little bit. Particularly, the lock'ing of the mutex in the setPath is problematic because it could block (not deadlock, but still) the GUI thread if it was called twice before the populateModel method runs. I would change it like this: get rid of the isBackgroundThreadActive boolean and m_currentPath. Make the currentPath an argument of the populateModel method and add the path to be set as the last argument of the QtConcurrent::run call. You should still lock the populateMutex in populateModel to prevent the populateModel() method from running more than once concurrently. This may result in populateModel being run more than once if someone quickly sets the path and sets it again. I think that's not too terrible, as the UI will eventually update to be correct.

9) In SidebarModel::hasChildren, can you use dynamic_cast instead of a bare cast (and check that it's not null as a result of the dynamic_cast?)

General comment: It looks like there are still a lot of places where there are tabs. One place I noticed is the addition to sidebarmodel.h and also the additions to depends.py -- could you double check your editor is using 4-space indents?

Based on these things getting fixed, everything else looks good. Thanks Tobias!

If you don't have time to work on this stuff I'm happy to lend a hand.

RJ

review: Needs Fixing

[RAFFI TEA (raffitea)]

Thanks RJ for your detailed review :-)

> Hey Tobias,
>
> Checking out this I've got a couple of questions and things to fix:
>
> 1) Why the updates to libmad, libid3tag, and libtag?

Sean has used MSVC++ 2005 on Windows to compile Mixxx and its dependencies.
If I compile Mixxx with MSVC++ 2008 library scanning but also track loadings will cause a segfault by taglib for some reason. Other contributors have observed same issues, see Bug #714337. Solution: Compile taglib with MSVC++ 2008 and replace the *.lib and *.dll.
>
> 2) I assume you're adding shell32 because you use system-specific calls in
> foldertreemodel.cpp? Could you comment that line in the SConscript to mention
> that's why it's there (mostly so I don't blindly remove it in the future
> thinking "gee we don't need this" :) )

Good catch, I'll place a comment. System-specific calls are necessary to quickly test if a folder has subfolders. QT is too slow here.
>
> 3) In browsefeature.cpp the "My Music" literal needs a tr(). Also the naming
> of My Music might make Windows users actually think it's their "My Music"
> folder, while it's actually the library folder they picked in Mixxx. Maybe we
> should say "My Library" instead?

I'm happy for this discussion here. I think "My Library" is the best word and others common places should be added, too.

> 4) I think we should add some other useful links, e.g. a 'My Home' for the
> user's home directory, 'My Documents' or 'My Music' for their Music folder (if
> it isn't the same as their Mixxx library). You can do this in a cross-platform
> way with the QDesktopServices::storageLocation method
>
> 5) I noticed that when expanding directories in the sidebar, if you hit the
> expand triangle on a folder that you are not currently selecting, it won't
> expand. Is this the case in general? Can we fix it so that they expand? I'm on
> Ubuntu 10.04 with standard Qt.

At the moment, this behavior is same on all platforms. I'm pretty sure we can change the behavior somehow. We'll need to change the sidebar view. The models can't change the behavior.

> 6) BrowseTableModel::BrowseTableModel() all those header column names need
> tr()'s

Will be done soon.

> 7) (sorry for nitpicking) can isBackgroundThreadActive get an "m_b" prefix,
> also can you move the initialization of isBackgroundThreadActive and
> m_currentPath to the constructor initializer list?

The points 7 and 8 are my problem. Have you ever tried to browse very quickly through the folder structure? Just to for reproduction. Create some folder that that contain > 100 tracks. Now try to click through the folder in Mixxx very quickly using keyboard. At least on Windows and OS X, Mixxx crashes in a regular manner .

I really don't know why. I introduced 'populateMutex ' to prevent the populateModel() method
from running more than once concurrently. However, I think more threads are created. You, RJ, are more thread-experienced as I am. Maybe an implementation of QRunnable is mode suitable. using signal/slots for updating the track table view. I would be thankful if someone can help me out. All previous points can be quickly addressed by me. What really prevents this feature to ge...

[RAFFI TEA (raffitea)]

RJ, I've addresses most of the problems. In particular, I've categorized the BrowseFeature. There's now a 'Quick Link" and a 'Devices' item under the root item. The first provides quick access to common places, e.g., Mixxx library, My Music, Desktop and so on. The second item provides access to (removable) drives/devices on all operating systems.

I would be happy if you could lend a hand. I face a second problem: At the moment you can only expand an item by double clicking. The underlying foldertreemodel populates incrementally (lazy). The SidebarTreeView calls canFetchMore() in lazy models if an item is activated somehow (triangle, double click, etc.). The SidebarTreeView will fetch more data from the model if canFetchMore() returns true.

I've implemented the (can)fetchMore methods in SideBarModel and FolderTreeModel. Recall that QAbstracItemModel::canFetchMore returns false by default, whenever it returns true in the overridden FolderTreeModel::canFetchMore() Mixxx crashes.

RJ, could you please take a look at my code? This might be helpful http://benjamin-meyer.blogspot.com/2006/10/dynamic-models.html

Thank you very much!

[RAFFI TEA (raffitea)]

I have solved the triangle issue!!! Please ignore my previous comment :-)

[RJ Skerry-Ryan (rryan)]

All my issues are fixed now, looks good to me.

Thanks Tobias!

review: Approve

[RAFFI TEA (raffitea)]

RJ, could you review once again please?

I've introduced signal/slots now to prevent segfaults through quick library navigation. Revision r2653 which is part of trunk works on OS X and Linux, but on Win it caused segfaults. This should be fixed now (at least for me it is).

[RJ Skerry-Ryan (rryan)]

It's looking good -- just merged it into trunk. Thanks Tobias!

review: Approve

[lp:~mixxxdevelopers/mixxx/tree_item_browser]

2659. By Raffitea on 2011-03-10

Fixed some typos.

2660. By Raffitea on 2011-03-11

Implemented Playlist Export to M3U and PLS.

2661. By Raffitea on 2011-03-11

merging from trunk

2662. By Raffitea on 2011-03-12

Implemented relative M3U and PLS playlist export. Works fine on OS X with VLC but iTunes does not support relative paths.

2663. By Raffitea on 2011-03-12

Added an option to preferences to make relative playlist export optional.

2664. By Raffitea on 2011-03-12

Fixed a compiler warning on Windows. Also tested PLS and M3U export on Windows. Works as expected with Media Player and iTunes but also VLC, RealPlayer and Winamp.

2665. By Raffitea on 2011-03-12

Added a comment about QDir::relativePath().

2666. By Raffitea on 2011-03-14

Merging from trunk

2667. By Raffitea on 2011-03-14

Remove qDebug for TIO

2668. By Raffitea on 2011-03-14

Improved Playlist export dialog.

2669. By Raffitea on 2011-03-14

Drastically improved the population of the tracktable view in BrowseFeature. Hopefully does not freez the GUI while clicking through the folder structure.

2670. By Tobias <Tobias@Silverstar> on 2011-03-22

Merging manually from trunk

2671. By Tobias <Tobias@Silverstar> on 2011-03-22

Merging manually from trunk the third time.

2672. By Tobias <Tobias@Silverstar> on 2011-03-25

Merging from trunk to get library caching fixes

2673. By Raffitea on 2011-03-26

made BrowseTableModel singleton because the recording feature will use the BrowseTableModel to display contents of a special recording directory. This approach avoids two BrowseThreads. If I had made BrowseThread singleton there would be two BrowseTableModel objects and both would populate when the Thread is active. This is not performant

2674. By Raffitea on 2011-03-26

Merging from trunk to get PrepareFeature fixes

2675. By Raffitea on 2011-03-26

Merging from trunk

2676. By Raffitea on 2011-03-27

Merging from trunk

2677. By Raffitea on 2011-03-27

Removed the singleton character of BrowseTableModel. This was a really stupid idea....

2678. By Raffitea on 2011-03-27

Now made BrowseThread singleton. BrowseTableModel objects will only process received signals if they have asked the thread to do so.

2679. By Raffitea on 2011-03-27

Merging from trunk to get auto-expand feature

2680. By Raffitea on 2011-03-28

Merging from trunk

2681. By Raffitea on 2011-03-28

Renamed context menu entry for crate export/import; If file extension is missing, append .m3u to the export file and continue to export in m3u; Replaced Qt::BlockingQueuedConnection with QueuedConnection, which seems to work for rapid sidebar folder iterations with keyboard/MIDI; Added a global static member for BrowseThread::getInstance().

Unmerged revisions

=== modified file 'mixxx/build/depends.py'
--- mixxx/build/depends.py	2011-02-13 21:30:36 +0000
+++ mixxx/build/depends.py	2011-02-16 20:55:20 +0000
@@ -498,6 +498,7 @@
                    "library/traktorfeature.cpp",
                    "library/traktortablemodel.cpp",
                    "library/traktorplaylistmodel.cpp",
+                   "library/browsethread.cpp",
 
                    "xmlparse.cpp",
                    "library/parser.cpp",

=== modified file 'mixxx/src/library/browsetablemodel.cpp'
--- mixxx/src/library/browsetablemodel.cpp	2011-02-13 22:09:54 +0000
+++ mixxx/src/library/browsetablemodel.cpp	2011-02-16 20:55:20 +0000
@@ -3,26 +3,13 @@
 #include <QtSql>
 #include <QStringList>
 #include <QtConcurrentRun>
+#include <QMetaType>
 
 #include "browsetablemodel.h"
 #include "soundsourceproxy.h"
 #include "mixxxutils.cpp"
 
-//constants
-const int COLUMN_FILENAME = 0;
-const int COLUMN_ARTIST = 1;
-const int COLUMN_TITLE = 2;
-const int COLUMN_ALBUM = 3;
-const int COLUMN_TRACK_NUMBER = 4;
-const int COLUMN_YEAR = 5;
-const int COLUMN_GENRE = 6;
-const int COLUMN_COMMENT = 7;
-const int COLUMN_DURATION = 8;
-const int COLUMN_BPM = 9;
-const int COLUMN_KEY = 10;
-const int COLUMN_TYPE = 11;
-const int COLUMN_BITRATE = 12;
-const int COLUMN_LOCATION = 13;
+
 
 BrowseTableModel::BrowseTableModel(QObject* parent)
         : QStandardItemModel(parent),
@@ -53,26 +40,22 @@
     addSearchColumn(COLUMN_GENRE);
     addSearchColumn(COLUMN_KEY);
     addSearchColumn(COLUMN_COMMENT);
-     
-    setHorizontalHeaderLabels(header_data); 
-
-    /*
-     * Start background thread.
-     * Used to read the ID3 tags
-     */
-    m_bStopThread = false;
-    m_future = QtConcurrent::run(this, &BrowseTableModel::browserThread);
-    m_path = "";
+
+    //m_backgroundThread.moveToThread(&m_backgroundThread);
+    m_backgroundThread.start(QThread::LowestPriority);
+
+    setHorizontalHeaderLabels(header_data);
+
+    QObject::connect(&m_backgroundThread, SIGNAL(clearModel()),
+            this, SLOT(slotClear()));
+    QObject::connect(&m_backgroundThread, SIGNAL(rowDataAppended(const QList<QStandardItem*>&)),
+            this, SLOT(slotInsert(const QList<QStandardItem*>&)), Qt::DirectConnection);
+
 }
 
 BrowseTableModel::~BrowseTableModel()
 {
-    qDebug() << "Wait to finish browser background thread";
-    m_bStopThread = true;
-    //wake up thread since it might wait for user input
-    m_locationUpdated.wakeAll();
-    m_future.waitForFinished();
-    qDebug() << "Browser background thread terminated!";
+
 }
 
 const QList<int>& BrowseTableModel::searchColumns() const {
@@ -83,8 +66,7 @@
 }
 void BrowseTableModel::setPath(QString absPath)
 {
-    m_path = absPath;
-    m_locationUpdated.wakeAll();
+   m_backgroundThread.setPath(absPath);
 
 }
 
@@ -169,99 +151,15 @@
     mimeData->setUrls(urls);
     return mimeData;
 }
-void BrowseTableModel::populateModel()
+
+void BrowseTableModel::slotClear()
 {
-    //Refresh the name filters in case we loaded new
-    //SoundSource plugins.
-    QStringList nameFilters(SoundSourceProxy::supportedFileExtensionsString().split(" "));
-    QDirIterator fileIt(m_path, nameFilters, QDir::Files | QDir::NoDotAndDotDot);
-    QString thisPath(m_path);
-    //remove all rows
     removeRows(0, rowCount());
-    
-    int row = 0;
-    //Iterate over the files
-    while (fileIt.hasNext())
-    {
-        /*
-         * If a user quickly jumps through the folders
-         * the current task becomes "dirty"
-         */
-        if(thisPath != m_path){
-            //qDebug() << "Exit populateModel()";
-            return;
-        }
-        QString filepath = fileIt.next();
-        TrackInfoObject tio(filepath);
-
-        QStandardItem* item = new QStandardItem(tio.getFilename());
-        setItem(row, COLUMN_FILENAME, item);
-           
-        item = new QStandardItem(tio.getArtist());
-        setItem(row, COLUMN_ARTIST, item);
-          
-        item = new QStandardItem(tio.getTitle());
-        setItem(row, COLUMN_TITLE, item);
-           
-        item = new QStandardItem(tio.getAlbum());
-        setItem(row, COLUMN_ALBUM, item);
-
-        QString duration = MixxxUtils::secondsToMinutes(qVariantValue<int>(tio.getDuration()));
-        item = new QStandardItem(duration);
-        setItem(row, COLUMN_DURATION, item);
-
-        item = new QStandardItem(tio.getBpmStr());
-        setItem(row, COLUMN_BPM, item);
-           
-        item = new QStandardItem(tio.getKey());
-        setItem(row, COLUMN_KEY, item);
-            
-        item = new QStandardItem(tio.getYear());
-        setItem(row, COLUMN_YEAR, item);
-
-        item = new QStandardItem(tio.getBitrateStr());
-        setItem(row, COLUMN_BITRATE, item);
-            
-        item = new QStandardItem(tio.getType());
-        setItem(row, COLUMN_TYPE, item);
-            
-        item = new QStandardItem(tio.getGenre());
-        setItem(row, COLUMN_GENRE, item);
-
-        item = new QStandardItem(tio.getTrackNumber());
-        setItem(row, COLUMN_TRACK_NUMBER, item);
-
-        item = new QStandardItem(tio.getComment());
-        setItem(row, COLUMN_COMMENT, item);
-
-        item = new QStandardItem(filepath);
-        setItem(row, COLUMN_LOCATION, item);
-          
-        ++row;
-   
-    }
 }
-void BrowseTableModel::browserThread()
+void BrowseTableModel::slotInsert(const QList<QStandardItem*> &column_data)
 {
-    //Give the thread low priority to prevent GUI freezing
-    QThread* thisThread = QThread::currentThread();
-    thisThread->setPriority(QThread::LowestPriority);
-
-    while(1){
-        m_mutex.lock();
-        //Wait until the user has selected a folder
-        m_locationUpdated.wait(&m_mutex);
-
-        //Terminate thread if Mixxx closes
-        if(m_bStopThread)
-            return;
-
-        /*
-         * Populate the model
-         */
-        populateModel();
-
-        m_mutex.unlock();
-
-    }
+    qDebug() << "BrowseTableModel::slotInsert";
+    appendRow(column_data);
+    //Does not work for some reason
+    //setItem(row, column, item);
 }

=== modified file 'mixxx/src/library/browsetablemodel.h'
--- mixxx/src/library/browsetablemodel.h	2011-02-13 19:42:53 +0000
+++ mixxx/src/library/browsetablemodel.h	2011-02-16 20:55:20 +0000
@@ -3,9 +3,26 @@
 
 #include <QStandardItemModel>
 #include "trackmodel.h"
+#include "browsethread.h"
 
 class QMimeData;
 
+//constants
+const int COLUMN_FILENAME = 0;
+const int COLUMN_ARTIST = 1;
+const int COLUMN_TITLE = 2;
+const int COLUMN_ALBUM = 3;
+const int COLUMN_TRACK_NUMBER = 4;
+const int COLUMN_YEAR = 5;
+const int COLUMN_GENRE = 6;
+const int COLUMN_COMMENT = 7;
+const int COLUMN_DURATION = 8;
+const int COLUMN_BPM = 9;
+const int COLUMN_KEY = 10;
+const int COLUMN_TYPE = 11;
+const int COLUMN_BITRATE = 12;
+const int COLUMN_LOCATION = 13;
+
 class BrowseTableModel : public QStandardItemModel, public TrackModel
 {
     
@@ -30,20 +47,13 @@
         virtual bool isColumnHiddenByDefault(int column);
         virtual const QList<int>& searchColumns() const;
     private:
-       void populateModel();
-       //This method is executed in a Thread
-       void browserThread();
-       // Add a column to be searched when searching occurs
        void addSearchColumn(int index);
 
-       QMutex m_mutex;
-       QWaitCondition m_locationUpdated;
-
-       QFuture<void> m_future;
-       QList<int> m_searchColumns;
-       QString m_path;
-
-       bool m_bStopThread;
+    BrowseThread m_backgroundThread;
+    QList<int> m_searchColumns;
+public slots:
+       void slotClear();
+       void slotInsert(const QList<QStandardItem*> &item);
 };
 
 #endif

=== added file 'mixxx/src/library/browsethread.cpp'
--- mixxx/src/library/browsethread.cpp	1970-01-01 00:00:00 +0000
+++ mixxx/src/library/browsethread.cpp	2011-02-16 20:55:20 +0000
@@ -0,0 +1,140 @@
+#include "browsethread.h"
+#include "browsetablemodel.h"
+#include "soundsourceproxy.h"
+#include "mixxxutils.cpp"
+
+#include <QStringList>
+#include <QDirIterator>
+#include <QtCore>
+
+
+BrowseThread::BrowseThread(QObject *parent) : QThread(parent)
+{
+    //start thread
+    m_bStopThread = false;
+
+    //QObject::moveToThread(this);
+}
+BrowseThread::~BrowseThread()
+{
+    qDebug() << "Wait to finish browser background thread";
+    m_bStopThread = true;
+    //wake up thread since it might wait for user input
+    m_locationUpdated.wakeAll();
+    //Wait until thread terminated
+    //terminate();
+    wait();
+    qDebug() << "Browser background thread terminated!";
+
+}
+
+void BrowseThread::setPath(QString& path)
+{
+    m_path = path;
+    m_locationUpdated.wakeAll();
+
+}
+
+void BrowseThread::run()
+{
+
+    while(1){
+        m_mutex.lock();
+        //Wait until the user has selected a folder
+        m_locationUpdated.wait(&m_mutex);
+
+        //Terminate thread if Mixxx closes
+        if(m_bStopThread)
+            return;
+
+        /*
+         * Populate the model
+         */
+        populateModel();
+        m_mutex.unlock();
+
+    }
+
+}
+void BrowseThread::populateModel()
+{
+    //Refresh the name filters in case we loaded new
+    //SoundSource plugins.
+    QStringList nameFilters(SoundSourceProxy::supportedFileExtensionsString().split(" "));
+    QDirIterator fileIt(m_path, nameFilters, QDir::Files | QDir::NoDotAndDotDot);
+    QString thisPath(m_path);
+    //remove all rows
+    emit(clearModel());
+    QCoreApplication::processEvents();
+
+    int row = 0;
+    //Iterate over the files
+    while (fileIt.hasNext())
+    {
+        /*
+         * If a user quickly jumps through the folders
+         * the current task becomes "dirty"
+         */
+        if(thisPath != m_path){
+            qDebug() << "Exit populateModel()";
+            return populateModel();
+        }
+
+        QString filepath = fileIt.next();
+        TrackInfoObject tio(filepath);
+        QList<QStandardItem*> column_data;
+
+        QStandardItem* item = new QStandardItem(tio.getFilename());
+        column_data.insert(COLUMN_FILENAME, item);
+
+
+        item = new QStandardItem(tio.getArtist());
+        column_data.insert(COLUMN_ARTIST, item);
+
+        item = new QStandardItem(tio.getTitle());
+        column_data.insert(COLUMN_TITLE, item);
+
+        item = new QStandardItem(tio.getAlbum());
+        column_data.insert(COLUMN_ALBUM, item);
+
+        item = new QStandardItem(tio.getTrackNumber());
+        column_data.insert(COLUMN_TRACK_NUMBER, item);
+
+        item = new QStandardItem(tio.getYear());
+        column_data.insert(COLUMN_YEAR, item);
+
+        item = new QStandardItem(tio.getGenre());
+        column_data.insert(COLUMN_GENRE, item);
+
+        item = new QStandardItem(tio.getComment());
+        column_data.insert(COLUMN_COMMENT, item);
+
+        QString duration = MixxxUtils::secondsToMinutes(qVariantValue<int>(tio.getDuration()));
+        item = new QStandardItem(duration);
+        column_data.insert(COLUMN_DURATION, item);
+
+        item = new QStandardItem(tio.getBpmStr());
+        column_data.insert(COLUMN_BPM, item);
+
+        item = new QStandardItem(tio.getKey());
+        column_data.insert(COLUMN_KEY, item);
+
+        item = new QStandardItem(tio.getType());
+        column_data.insert(COLUMN_TYPE, item);
+
+        item = new QStandardItem(tio.getBitrateStr());
+        column_data.insert(COLUMN_BITRATE, item);
+
+        item = new QStandardItem(filepath);
+        column_data.insert(COLUMN_LOCATION, item);
+
+        emit(rowDataAppended(column_data));
+
+        QCoreApplication::processEvents();
+        ++row;
+
+        //Sleep for 50ms which prevents us from GUI freezes
+        msleep(50);
+    }
+
+}

=== added file 'mixxx/src/library/browsethread.h'
--- mixxx/src/library/browsethread.h	1970-01-01 00:00:00 +0000
+++ mixxx/src/library/browsethread.h	2011-02-16 20:55:20 +0000
@@ -0,0 +1,37 @@
+#ifndef BROWSETHREAD_H
+#define BROWSETHREAD_H
+
+#include <QThread>
+#include <QMutex>
+#include <QWaitCondition>
+#include <QStandardItem>
+#include <QList>
+
+class BrowseThread : public QThread
+{
+    Q_OBJECT
+public:
+    BrowseThread(QObject *parent = 0);
+    ~BrowseThread();
+    void setPath(QString& path);
+    void run();
+signals:
+    void rowDataAppended(const QList<QStandardItem*>&);
+    void clearModel();
+private:
+     void populateModel();
+     QMutex m_mutex;
+     QWaitCondition m_locationUpdated;
+
+
+     QList<int> m_searchColumns;
+     QString m_path;
+
+     bool m_bStopThread;
+
+
+
+
+};
+
+#endif // BROWSETHREAD_H