Bug 243562 - Dynamic playlist criteria not saved properly
Summary: Dynamic playlist criteria not saved properly
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Unclassified
Component: Playlists/Dynamic Playlists (show other bugs)
Version: 2.4-GIT
Platform: Ubuntu Packages Linux
: NOR major with 20 votes (vote)
Target Milestone: 2.4.0
Assignee: Amarok Developers
URL:
Keywords:
: 247898 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-07-04 10:32 UTC by Dean Spirov
Modified: 2010-10-15 11:45 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.4


Attachments
How it should look (47.99 KB, image/png)
2010-07-04 10:38 UTC, Dean Spirov
Details
How it looks (45.54 KB, image/png)
2010-07-04 10:41 UTC, Dean Spirov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Spirov 2010-07-04 10:32:50 UTC
Version:           2.3.0 (using KDE 4.4.2) 
OS:                Linux

after restart, the score magically jumps from 90 to 9, and the "match:" field which should contain "rating" is blank

Reproducible: Always

Steps to Reproduce:
Create a new dynamic playlist:
 * fuzzy bias, 80%, score, 90
 * fuzzy bias, 80%, rating, 4.5 stars
Save it with some name, I used "Favorite".
Restart amarok, select the playlist.

Actual Results:  
Fields are not restored properly

Expected Results:  
the playlist criteria should be restored as they were.

OS: Linux (i686) release 2.6.32-22-generic
Compiler: cc
Comment 1 Dean Spirov 2010-07-04 10:38:24 UTC
Created attachment 48582 [details]
How it should look
Comment 2 Dean Spirov 2010-07-04 10:41:11 UTC
Created attachment 48583 [details]
How it looks
Comment 3 Myriam Schweingruber 2010-07-04 14:10:18 UTC
Could you please update to Amarok 2.3.1 and try again? You can find more information about it here: http://kubuntu.org/news/amarok-2.3.1
Comment 4 Dean Spirov 2010-07-19 09:31:26 UTC
I just reproduced the issue in  amarok 2.3.1.
Comment 5 Myriam Schweingruber 2010-07-19 10:36:28 UTC
Thank you for the feedback.
Comment 6 Myriam Schweingruber 2010-08-15 15:38:09 UTC
*** Bug 247898 has been marked as a duplicate of this bug. ***
Comment 7 Myriam Schweingruber 2010-08-15 15:38:56 UTC
Confirmed by duplicate.
Comment 8 Mikko C. 2010-10-06 11:42:45 UTC
confirmed in amarok master with the test above
Comment 9 Ralf Engels 2010-10-09 22:19:26 UTC
The fix is in reviewboard.

One additional problem that was not mentioned: PlaylistDuration was behaving strange.
Comment 10 Mark Kretschmann 2010-10-15 10:01:42 UTC
commit 4d2725cd4f946d0a4ffb0acbe564103ea4b5c535
Author: Mark Kretschmann <kretschmann@kde.org>
Date:   Fri Oct 15 09:58:55 2010 +0200

    Change order of setting filter and value (one resets the other)
    First set the values and then connect the signal
    For the layout set the layout policy.
    
    Patch by Ralf Engels <ralf-engels@gmx.de>.
    
    BUG: 243562

diff --git a/ChangeLog b/ChangeLog
index 68f7c32..2075097 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -26,6 +26,7 @@ VERSION 2.4
       pressing SHIFT while clicking the action will bypass trash.
 
   BUGFIXES:
+    * Dynamic Playlist criteria were not being saved properly. (BR 243562)
     * Collection Browser should show Artist names for Compilation Albums. (BR
       252790)
     * When Amarok starts and "Continue playing when Amarok is started" is selected,
diff --git a/src/browsers/playlistbrowser/DynamicBiasWidgets.cpp b/src/browsers/playlistbrowser/DynamicBiasWidgets.cpp
index ae1eb19..00024b7 100644
--- a/src/browsers/playlistbrowser/DynamicBiasWidgets.cpp
+++ b/src/browsers/playlistbrowser/DynamicBiasWidgets.cpp
@@ -182,21 +182,24 @@ PlaylistBrowserNS::BiasGlobalWidget::BiasGlobalWidget(
     m_weightSelection = new Amarok::Slider( Qt::Horizontal, 100, frame );
     m_weightSelection->setToolTip(
             i18n( "This controls what portion of the playlist should match the criteria" ) );
-    connect( m_weightSelection, SIGNAL(valueChanged(int)),
-            this, SLOT(weightChanged(int)) );
 
     QHBoxLayout* sliderLayout = new QHBoxLayout();
     sliderLayout->addWidget( m_weightSelection );
     sliderLayout->addWidget( m_weightLabel );
 
     m_queryWidget = new MetaQueryWidget( frame );
-    connect( m_queryWidget, SIGNAL(changed(const MetaQueryWidget::Filter&)), this, SLOT(syncBiasToControls()));
-
+    m_queryWidget->setSizePolicy( QSizePolicy( QSizePolicy::MinimumExpanding,
+                                               QSizePolicy::Preferred ) );
     layout->addRow( i18n( "Proportion:" ), sliderLayout );
     layout->addRow( i18n( "Match:" ), m_queryWidget );
 
     syncControlsToBias();
 
+    connect( m_weightSelection, SIGNAL(valueChanged(int)),
+             SLOT(weightChanged(int)) );
+    connect( m_queryWidget, SIGNAL(changed(const MetaQueryWidget::Filter&)),
+             SLOT(syncBiasToControls()));
+
     this->layout()->addWidget( frame );
 }
 
@@ -251,21 +254,22 @@ PlaylistBrowserNS::BiasNormalWidget::BiasNormalWidget( Dynamic::NormalBias* bias
     m_scaleSelection = new Amarok::Slider( Qt::Horizontal, 100, frame );
     m_scaleSelection->setToolTip(
             i18n( "This controls how strictly to match the given value." ) );
-    connect( m_scaleSelection, SIGNAL(valueChanged(int)),
-            SLOT(scaleChanged(int)) );
-
     QHBoxLayout* sliderLayout = new QHBoxLayout();
     sliderLayout->addWidget( m_scaleSelection );
     sliderLayout->addWidget( m_scaleLabel );
 
     m_queryWidget = new MetaQueryWidget( frame, true, true );
-    connect( m_queryWidget, SIGNAL(changed(const MetaQueryWidget&)), this, SLOT(syncBiasToControls()));
-
     layout->addRow( i18n( "Strictness:" ), sliderLayout );
     layout->addRow( i18n( "Match:" ), m_queryWidget );
 
     syncControlsToBias();
 
+    connect( m_scaleSelection, SIGNAL(valueChanged(int)),
+             SLOT(scaleChanged(int)) );
+    connect( m_queryWidget, SIGNAL(changed(const MetaQueryWidget::Filter&)),
+             SLOT(syncBiasToControls()));
+
+
     this->layout()->addWidget( frame );
 }
 
@@ -277,17 +281,19 @@ PlaylistBrowserNS::BiasNormalWidget::syncControlsToBias()
     scaleChanged(scale); // the widget value might not have changed and thus the signal not fired
 
     MetaQueryWidget::Filter filter;
-    filter.numValue = m_nbias->value();
     filter.field    = m_nbias->field();
+    filter.numValue = m_nbias->value();
 
     m_queryWidget->setFilter( filter );
+
+    filter = m_queryWidget->filter();
 }
 
 void
 PlaylistBrowserNS::BiasNormalWidget::syncBiasToControls()
 {
-    m_nbias->setValue( m_queryWidget->filter().numValue );
     m_nbias->setField( m_queryWidget->filter().field );
+    m_nbias->setValue( m_queryWidget->filter().numValue );
     m_nbias->setScale( m_scaleSelection->value() / 100.0 );
     m_nbias->setActive( true );
 
diff --git a/src/widgets/MetaQueryWidget.cpp b/src/widgets/MetaQueryWidget.cpp
index 5f9171a..ae07e6b 100644
--- a/src/widgets/MetaQueryWidget.cpp
+++ b/src/widgets/MetaQueryWidget.cpp
@@ -124,6 +124,7 @@ MetaQueryWidget::MetaQueryWidget( QWidget* parent, bool onlyNumeric, bool noCond
     : QWidget( parent )
     , m_onlyNumeric( onlyNumeric )
     , m_noCondition( noCondition )
+    , m_settingFilter( false )
     , m_andLabel(0)
     , m_compareSelection(0)
     , m_valueSelection1(0)
@@ -140,9 +141,9 @@ MetaQueryWidget::MetaQueryWidget( QWidget* parent, bool onlyNumeric, bool noCond
     m_layoutMain->addLayout(m_layoutValue);
 
     m_layoutValueLabels = new QVBoxLayout();
-    m_layoutValue->addLayout(m_layoutValueLabels);
+    m_layoutValue->addLayout(m_layoutValueLabels, 0);
     m_layoutValueValues = new QVBoxLayout();
-    m_layoutValue->addLayout(m_layoutValueValues);
+    m_layoutValue->addLayout(m_layoutValueValues, 1);
 
     if( m_onlyNumeric ) {
         m_filter.field = Meta::valYear;
@@ -176,6 +177,7 @@ MetaQueryWidget::filter() const
 void
 MetaQueryWidget::setFilter( const MetaQueryWidget::Filter &value )
 {
+    m_settingFilter = true;
     m_filter = value;
 
     // correct filter
@@ -187,13 +189,13 @@ MetaQueryWidget::setFilter( const MetaQueryWidget::Filter &value )
 
     int index = m_fieldSelection->findData( (int)m_filter.field );
     m_fieldSelection->setCurrentIndex( index == -1 ? 0 : index );
-    m_filter = value; // the value can be reset by setFilter
 
     if( !m_noCondition )
         makeCompareSelection();
     makeValueSelection();
     setValueSelection();
 
+    m_settingFilter = false;
     emit changed(m_filter);
 }
 
@@ -239,6 +241,9 @@ MetaQueryWidget::makeFieldSelection()
 void
 MetaQueryWidget::fieldChanged( int i )
 {
+    if( m_settingFilter )
+        return;
+
     qint64 field = qvariant_cast<qint64>( m_fieldSelection->itemData( i ) );
     if( m_filter.field == field )
         return; // nothing to do
@@ -515,7 +520,6 @@ void
 MetaQueryWidget::makeGenericComboSelection( bool editable, Collections::QueryMaker* populateQuery )
 {
     KComboBox* combo = new KComboBox( this );
-    combo->setSizePolicy( QSizePolicy::Ignored, QSizePolicy::Preferred );
     combo->setEditable( editable );
 
     if( populateQuery != 0 )
diff --git a/src/widgets/MetaQueryWidget.h b/src/widgets/MetaQueryWidget.h
index 4d544c7..c720b6d 100644
--- a/src/widgets/MetaQueryWidget.h
+++ b/src/widgets/MetaQueryWidget.h
@@ -169,6 +169,8 @@ class MetaQueryWidget : public QWidget
         bool m_onlyNumeric;
         bool m_noCondition;
 
+        bool m_settingFilter; // if set to true we are just setting the filter
+
         QVBoxLayout* m_layoutMain;
         QHBoxLayout* m_layoutValue;
         QVBoxLayout* m_layoutValueLabels;