From kde-commits Mon Nov 15 18:46:02 2010 From: Martin Blumenstingl Date: Mon, 15 Nov 2010 18:46:02 +0000 To: kde-commits Subject: [Amarok] b11cd9d: Improve the lyrics handling in Amarok. -Update the Message-Id: <20101115184602.2A938A60AA () git ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-commits&m=128984681608713 commit b11cd9dba88a2407015badc76239bb395ca9ad65 branch master Author: Martin Blumenstingl Date: Wed Nov 3 21:05:40 2010 +0100 Improve the lyrics handling in Amarok. -Update the lyrics in the LyricsApplet if they were changed in the TagDialog. -Improve the check for cached lyrics: lyrics with just whitespaces in it are now also "empty". -Add a warning dialog which notifies the user if changes in the LyricsApplet would get lost. -Remove the KMessageBox in the LyricsApplet and replace it by Context::Applet::showWarning(). -Added a way to show warnings which lock one specific applet. -Minor code cleanup. BUG: 207621 REVIEWBOARD: http://git.reviewboard.kde.org/r/100013/ diff --git a/src/context/Applet.cpp b/src/context/Applet.cpp index c25a143..55424a9 100644 --- a/src/context/Applet.cpp +++ b/src/context/Applet.cpp @@ -44,6 +44,7 @@ Context::Applet::Applet( QObject * parent, const QVariantList& args ) , m_canAnimate( !KServiceTypeTrader::self()->query("Plasma/Animator", QString()).isEmpty() ) , m_collapsed( false ) , m_transient( 0 ) + , m_isMessageShown( false ) , m_standardPadding( 6.0 ) , m_textBackground( 0 ) { @@ -390,9 +391,50 @@ Context::Applet::paletteChanged( const QPalette & palette ) Q_UNUSED( palette ) } +void +Context::Applet::plasmaMessageHidden() +{ + // Disconnect from the messageButtonPressed() signal so the next + // dialog can be shown. + disconnect( SIGNAL( messageButtonPressed( const MessageButton ) ) ); + + // No dialog is shown anymore. + m_isMessageShown = false; +} + bool Context::Applet::canAnimate() { return m_canAnimate; } +void +Context::Applet::showWarning( const QString &message, const char *slot ) +{ + // Show a message with the "warning" icon. + showMessage( message, slot, KIcon( "dialog-warning" ) ); +} + +void +Context::Applet::showMessage( const QString &message, const char *slot, const KIcon &icon ) +{ + DEBUG_BLOCK + + // Only show the message if none is shown yet. + if( !m_isMessageShown ) + { + // Make sure no one else can show a dialog. + m_isMessageShown = true; + + // Get the "Yes" and "No" buttons. + Plasma::MessageButtons plasmaYesNoButtons = Plasma::ButtonYes | Plasma::ButtonNo; + + // Connect Plasma's messageButtonPressed SIGNAL to the given slot. + connect( this, SIGNAL( messageButtonPressed( const MessageButton ) ), slot ); + connect( this, SIGNAL( messageButtonPressed( const MessageButton ) ), SLOT( plasmaMessageHidden() ) ); + + // Show a dialog and ask the user what to do. + Plasma::Applet::showMessage( icon, message, plasmaYesNoButtons ); + } +} + #include "Applet.moc" diff --git a/src/context/Applet.h b/src/context/Applet.h index 137e9ea..e8cad32 100644 --- a/src/context/Applet.h +++ b/src/context/Applet.h @@ -26,6 +26,8 @@ #include #include +#include + class QPainter; class QPropertyAnimation; @@ -89,6 +91,27 @@ class AMAROK_EXPORT Applet : public Plasma::Applet */ qreal animationValue() const; + /** + * Shows a warning dialog which blocks access to the applet. + * This gives the user the message and a "Yes" and a "No" button. + * NOTE: Only one message/warning can be shown at a time. + * + * @param message The warning message. + * @param slot The slot which is called after either "Yes" or "No" has been clicked. + */ + void showWarning( const QString &message, const char *slot ); + + /** + * Shows a message dialog which blocks access to the applet. + * This gives the user the message and a "Yes" and a "No" button. + * NOTE: Only one message/warning can be shown at a time. + * + * @param message The warning message. + * @param slot The slot which is called after either "Yes" or "No" has been clicked. + * @param icon The icon which will be shown. + */ + void showMessage( const QString &message, const char *slot, const KIcon &icon ); + public Q_SLOTS: virtual void destroy(); @@ -99,6 +122,13 @@ class AMAROK_EXPORT Applet : public Plasma::Applet private slots: void paletteChanged( const QPalette & palette ); + /** + * A private slot used to cleanup internal things like + * signals/slots and the flag if a dialog is shown. + * This is needed to avoid duplicate code in the applets. + */ + void plasmaMessageHidden(); + protected: /** * Paint the background of an applet, so it fits with all the other applets. @@ -121,6 +151,7 @@ class AMAROK_EXPORT Applet : public Plasma::Applet void cleanUpAndDelete(); bool m_transient; + bool m_isMessageShown; qreal m_standardPadding; Plasma::FrameSvg *m_textBackground; QWeakPointer m_animation; diff --git a/src/context/LyricsManager.cpp b/src/context/LyricsManager.cpp index 67c975d..b8460f9 100644 --- a/src/context/LyricsManager.cpp +++ b/src/context/LyricsManager.cpp @@ -17,12 +17,14 @@ #include "LyricsManager.h" +#include "core-impl/collections/support/CollectionManager.h" #include "core/support/Debug.h" #include "EngineController.h" #include #include +#include //////////////////////////////////////////////////////////////// //// CLASS LyricsObserver @@ -150,7 +152,7 @@ LyricsManager::lyricsResult( const QString& lyricsXML, bool cached ) //SLOT // FIXME: lyrics != "Not found" will not work when the lyrics script displays i18n'ed // error messages - if ( !lyrics.isEmpty() && + if ( !isEmpty( lyrics ) && lyrics != "Not found" ) { // overwrite cached lyrics (as either there were no lyircs available previously OR @@ -158,16 +160,14 @@ LyricsManager::lyricsResult( const QString& lyricsXML, bool cached ) //SLOT debug() << "setting cached lyrics..."; The::engineController()->currentTrack()->setCachedLyrics( lyrics ); // TODO: setLyricsByPath? } - else if( !The::engineController()->currentTrack()->cachedLyrics().isEmpty() && + else if( !isEmpty( The::engineController()->currentTrack()->cachedLyrics() ) && The::engineController()->currentTrack()->cachedLyrics() != "Not found" ) { // we found nothing, so if we have cached lyrics, use it! debug() << "using cached lyrics..."; lyrics = The::engineController()->currentTrack()->cachedLyrics(); - // check if the lyrics data contains "prettyName() here? const QString title = el.attribute( "title" ); @@ -204,7 +204,7 @@ LyricsManager::lyricsResultHtml( const QString& lyricsHTML, bool cached ) Meta::TrackPtr currentTrack = The::engineController()->currentTrack(); if( currentTrack && - !lyricsHTML.isEmpty() ) + !isEmpty( lyricsHTML ) ) { sendNewLyricsHtml( lyricsHTML ); @@ -254,15 +254,13 @@ bool LyricsManager::showCached() { //if we have cached lyrics there is absolutely no point in not showing these.. Meta::TrackPtr currentTrack = The::engineController()->currentTrack(); - if( currentTrack && !currentTrack->cachedLyrics().isEmpty() ) + if( currentTrack && !isEmpty( currentTrack->cachedLyrics() ) ) { // TODO: add some sort of feedback that we could not fetch new ones // so we are showing a cached result debug() << "showing cached lyrics!"; - // check if the lyrics data contains "cachedLyrics().contains( "cachedLyrics() ) ) { //we have stored html lyrics, so use that directly sendNewLyricsHtml( currentTrack->cachedLyrics() ); @@ -284,3 +282,40 @@ bool LyricsManager::showCached() } return false; } + +void LyricsManager::setLyricsForTrack( const QString &trackUrl, const QString &lyrics ) const +{ + DEBUG_BLOCK + + Meta::TrackPtr track = CollectionManager::instance()->trackForUrl( KUrl( trackUrl ) ); + + if( track ) + track->setCachedLyrics( lyrics ); + else + debug() << QString("could not find a track for the given URL (%1) - ignoring.").arg( trackUrl ); +} + +bool LyricsManager::isHtmlLyrics( const QString &lyrics ) const +{ + // Check if the lyrics data contains " #include @@ -56,6 +57,8 @@ public: , editIcon( 0 ) , reloadIcon( 0 ) , closeIcon( 0 ) + , currentTrack( 0 ) + , modifiedLyrics( QString() ) , hasLyrics( false ) , isRichText( true ) , showBrowser( false ) @@ -69,8 +72,11 @@ public: void collapseToMin(); void determineActionIconsState(); void clearLyrics(); + void refetchLyrics(); void showLyrics( const QString &text, bool isRichText ); void showSuggested( const QVariantList &suggestions ); + void showUnsavedChangesWarning( Meta::TrackPtr ); + const QString currentText(); // private slots void _editLyrics(); @@ -79,6 +85,9 @@ public: void _saveLyrics(); void _suggestionChosen( const QModelIndex &index ); void _unsetCursor(); + void _trackDataChanged( Meta::TrackPtr ); + void _lyricsChangedMessageButtonPressed( const MessageButton ); + void _refetchMessageButtonPressed( const MessageButton ); // data / widgets QString titleText; @@ -98,6 +107,10 @@ public: Ui::lyricsSettings ui_settings; + Meta::TrackPtr currentTrack; + Meta::TrackPtr modifiedTrack; + QString modifiedLyrics; + bool hasLyrics; bool isRichText; bool showBrowser; @@ -243,6 +256,75 @@ LyricsAppletPrivate::showSuggested( const QVariantList &suggestions ) showSuggestions = true; } +const QString +LyricsAppletPrivate::currentText() +{ + return isRichText ? browser->nativeWidget()->toHtml() : browser->nativeWidget()->toPlainText(); +} + +void +LyricsAppletPrivate::refetchLyrics() +{ + ScriptManager::instance()->notifyFetchLyrics( currentTrack->artist()->name(), + currentTrack->name() ); +} + +void +LyricsAppletPrivate::showUnsavedChangesWarning( Meta::TrackPtr newTrack ) +{ + Q_Q( LyricsApplet ); + + // Set the track which was modified and store the current + // lyircs from the UI. + modifiedTrack = currentTrack; + modifiedLyrics = currentText(); + + QString artistName = modifiedTrack->artist() ? modifiedTrack->artist()->name() : i18nc( "Used if the current track has no artist.", "Unknown" ); + QString warningMessage; + + // Check if the track has changed. + if( newTrack != modifiedTrack ) + { + // Show a warning that the track has changed while the user was editing the lyrics for the current track. + warningMessage = i18n( "While you were editing the lyrics of %1 - %2 the track has changed. Do you want to save your changes?", + artistName, + modifiedTrack->prettyName() ); + } + else + { + // Show a warning that the lyrics for the track were modified (for example by a script). + warningMessage = i18n( "The lyrics of %1 - %2 changed while you were editing them. Do you want to save your changes?", + artistName, + modifiedTrack->prettyName() ); + } + + // Show the warning message. + q->showWarning( warningMessage, SLOT( _lyricsChangedMessageButtonPressed( MessageButton ) ) ); + + // Make the contents readonly again. + // Since the applet is now blocked the user can not enable this again. + // Thus we can make sure that we won't overwrite modifiedTrack. + setEditing( false ); +} + +void +LyricsAppletPrivate::_refetchMessageButtonPressed( const MessageButton button ) +{ + // Check if the user pressed "Yes". + if( button == Plasma::ButtonYes ) + // Refetch the lyrics. + refetchLyrics(); +} + +void +LyricsAppletPrivate::_lyricsChangedMessageButtonPressed( const MessageButton button ) +{ + // Check if the user pressed "Yes". + if( button == Plasma::ButtonYes ) + // Update the lyrics of the track. + modifiedTrack->setCachedLyrics( modifiedLyrics ); +} + void LyricsAppletPrivate::_changeLyricsFont() { @@ -275,9 +357,9 @@ LyricsAppletPrivate::_closeLyrics() int savedPosition = vbar->isVisible() ? vbar->value() : vbar->minimum(); if( isRichText ) - browser->nativeWidget()->setHtml( The::engineController()->currentTrack()->cachedLyrics() ); + browser->nativeWidget()->setHtml( currentTrack->cachedLyrics() ); else - browser->nativeWidget()->setPlainText( The::engineController()->currentTrack()->cachedLyrics() ); + browser->nativeWidget()->setPlainText( currentTrack->cachedLyrics() ); vbar->setSliderPosition( savedPosition ); @@ -298,19 +380,16 @@ LyricsAppletPrivate::_closeLyrics() void LyricsAppletPrivate::_saveLyrics() { - Meta::TrackPtr curtrack = The::engineController()->currentTrack(); - - if( curtrack ) + if( currentTrack ) { - if( !browser->nativeWidget()->toPlainText().isEmpty() ) + if( !LyricsManager::self()->isEmpty( browser->nativeWidget()->toPlainText() ) ) { - const QString lyrics = isRichText ? browser->nativeWidget()->toHtml() : browser->nativeWidget()->toPlainText(); - curtrack->setCachedLyrics( lyrics ); + currentTrack->setCachedLyrics( currentText() ); hasLyrics = true; } else { - curtrack->setCachedLyrics( QString() ); + currentTrack->setCachedLyrics( QString() ); hasLyrics = false; } // emit sizeHintChanged(Qt::MaximumSize); @@ -347,12 +426,39 @@ LyricsAppletPrivate::_unsetCursor() suggestView->unsetCursor(); } +void +LyricsAppletPrivate::_trackDataChanged( Meta::TrackPtr track ) +{ + DEBUG_BLOCK + + // Check if we previously had a track. + // If the lyrics currently shown in the browser (which + // additionally is in edit mode) are different from the + // lyrics of the track we have to show a warning. + if( currentTrack && + currentTrack->cachedLyrics() != currentText() && + !browser->nativeWidget()->isReadOnly() ) + { + showUnsavedChangesWarning( track ); + } + + // Update the current track. + currentTrack = track; +} + LyricsApplet::LyricsApplet( QObject* parent, const QVariantList& args ) : Context::Applet( parent, args ) , d_ptr( new LyricsAppletPrivate( this ) ) { setHasConfigurationInterface( true ); setBackgroundHints( Plasma::Applet::NoBackground ); + + EngineController* engine = The::engineController(); + + connect( engine, SIGNAL( trackChanged( Meta::TrackPtr ) ), + this, SLOT( _trackDataChanged( Meta::TrackPtr ) ) ); + connect( engine, SIGNAL( trackMetadataChanged( Meta::TrackPtr ) ), + this, SLOT( _trackDataChanged( Meta::TrackPtr ) ) ); } LyricsApplet::~LyricsApplet() @@ -628,20 +734,21 @@ void LyricsApplet::refreshLyrics() { Q_D( LyricsApplet ); - Meta::TrackPtr curtrack = The::engineController()->currentTrack(); - - if( !curtrack || !curtrack->artist() ) + if( !d->currentTrack || !d->currentTrack->artist() ) return; - bool refetch = true; if( d->hasLyrics ) { + // Ask the user if he really wants to refetch the lyrics. const QString text( i18nc( "@info", "Do you really want to refetch lyrics for this track? All changes you may have made will be lost.") ); - refetch = KMessageBox::warningContinueCancel( 0, text, i18n( "Refetch lyrics" ) ) == KMessageBox::Continue; + showWarning( text, SLOT( _refetchMessageButtonPressed( MessageButton ) ) ); + } + else + { + // As we don't have lyrics yet we will + // refetch without asking the user. + d->refetchLyrics(); } - - if( refetch ) - ScriptManager::instance()->notifyFetchLyrics( curtrack->artist()->name(), curtrack->name() ); } void diff --git a/src/context/applets/lyrics/LyricsApplet.h b/src/context/applets/lyrics/LyricsApplet.h index bba44ee..4aaaf99 100644 --- a/src/context/applets/lyrics/LyricsApplet.h +++ b/src/context/applets/lyrics/LyricsApplet.h @@ -25,6 +25,8 @@ class LyricsAppletPrivate; +using namespace Plasma; + class LyricsApplet : public Context::Applet { Q_OBJECT @@ -60,6 +62,9 @@ private: Q_PRIVATE_SLOT( d_ptr, void _saveLyrics() ) Q_PRIVATE_SLOT( d_ptr, void _suggestionChosen(const QModelIndex&) ) Q_PRIVATE_SLOT( d_ptr, void _unsetCursor() ) + Q_PRIVATE_SLOT( d_ptr, void _trackDataChanged( Meta::TrackPtr ) ) + Q_PRIVATE_SLOT( d_ptr, void _lyricsChangedMessageButtonPressed( const MessageButton ) ) + Q_PRIVATE_SLOT( d_ptr, void _refetchMessageButtonPressed( const MessageButton ) ) }; K_EXPORT_AMAROK_APPLET( lyrics, LyricsApplet ) diff --git a/src/context/engines/lyrics/LyricsEngine.cpp b/src/context/engines/lyrics/LyricsEngine.cpp index a03aa53..104742d 100644 --- a/src/context/engines/lyrics/LyricsEngine.cpp +++ b/src/context/engines/lyrics/LyricsEngine.cpp @@ -65,7 +65,7 @@ bool LyricsEngine::sourceRequestEvent( const QString& name ) if( m_prevLyricsList.size() > 0 ) setData( "lyrics", "lyrics", m_prevLyricsList ); - else if( !m_prevLyrics.isEmpty() ) + else if( !LyricsManager::self()->isEmpty( m_prevLyrics ) ) setData( "lyrics", "html", m_prevLyrics ); if( m_prevSuggestionsList.size() > 0 ) @@ -81,6 +81,38 @@ bool LyricsEngine::sourceRequestEvent( const QString& name ) return true; } +bool LyricsEngine::testLyricsChanged( const QString& newLyrics, + const QString& oldHtmlLyrics, + QStringList oldPlainLyrics ) const +{ + DEBUG_BLOCK + + bool retVal = false; + + if ( LyricsManager::self()->isHtmlLyrics( newLyrics ) ) + // Compare the old HTML lyrics with the new lyrics. + retVal = newLyrics != oldHtmlLyrics; + else + { + // If the given oldPlainLyrics list has >= 3 items + // then plaintext lyrics were provided. + bool hasPlainLyrics = oldPlainLyrics.count() >= 3; + + if ( hasPlainLyrics ) + // Previously we got plaintext lyrics -> compare them with + // the new ones. + retVal = newLyrics != oldPlainLyrics[ 3 ]; + else + // There were no old lyrics -> if the new lyrics are + // non-empty they have changed. + retVal = !LyricsManager::self()->isEmpty( newLyrics ); + } + + debug() << "compared lyrics are the same = " << retVal; + + return retVal; +} + void LyricsEngine::update() { DEBUG_BLOCK @@ -145,8 +177,9 @@ void LyricsEngine::update() } // Check if the title, the artist and the lyrics are still the same. - // -- check if something changed for the previous fetched lyrics - if( title == m_title && artist == m_artist ) + if( title == m_title && + artist == m_artist && + !testLyricsChanged( currentTrack->cachedLyrics(), m_currentLyrics, m_currentLyricsList ) ) return; // nothing changed // -- really need new lyrics @@ -163,13 +196,12 @@ void LyricsEngine::update() QString lyrics = currentTrack->cachedLyrics(); // don't rely on caching for streams - const bool cached = !lyrics.isEmpty() && !The::engineController()->isStream(); + const bool cached = !LyricsManager::self()->isEmpty( lyrics ) && + !The::engineController()->isStream(); if( cached ) { - // check if the lyrics data contains "isHtmlLyrics( lyrics ) ) newLyricsHtml( lyrics ); else { diff --git a/src/context/engines/lyrics/LyricsEngine.h b/src/context/engines/lyrics/LyricsEngine.h index bf4a702..994b12d 100644 --- a/src/context/engines/lyrics/LyricsEngine.h +++ b/src/context/engines/lyrics/LyricsEngine.h @@ -54,6 +54,19 @@ private slots: void update(); private: + /** + * Tests if the lyrics have changed. + * + * @param newLyrics The new lyrics. + * @param oldHtmlLyrics The old (unchanged) HTML lyrics. + * @param oldPlainLyrics The old plain lyrics (as provided by the LyricsEngine). + * + * @return true if the lyrics for the current track have changed, otherwise false. + */ + bool testLyricsChanged( const QString& newLyrics, + const QString& oldHtmlLyrics, + QStringList oldPlainLyrics ) const; + // stores is we have been disabled (disconnected) bool m_requested; diff --git a/src/core-impl/collections/db/sql/SqlMeta.cpp b/src/core-impl/collections/db/sql/SqlMeta.cpp index 1d70872..ff899fe 100644 --- a/src/core-impl/collections/db/sql/SqlMeta.cpp +++ b/src/core-impl/collections/db/sql/SqlMeta.cpp @@ -1352,6 +1352,8 @@ SqlTrack::setCachedLyrics( const QString &lyrics ) m_collection->sqlStorage()->escape( m_rpath ) ); m_collection->sqlStorage()->query( update ); } + + notifyObservers(); } bool diff --git a/src/core-impl/collections/nepomukcollection/NepomukTrack.cpp b/src/core-impl/collections/nepomukcollection/NepomukTrack.cpp index 02f5bc7..7db01cf 100644 --- a/src/core-impl/collections/nepomukcollection/NepomukTrack.cpp +++ b/src/core-impl/collections/nepomukcollection/NepomukTrack.cpp @@ -327,6 +327,8 @@ void NepomukTrack::setCachedLyrics ( const QString& value ) { m_nepores.setProperty( QUrl( "http://amarok.kde.org/metadata/1.0/track#lyrics" ), value ); + + notifyObservers(); } void diff --git a/src/dialogs/TagDialog.cpp b/src/dialogs/TagDialog.cpp index ddd73cb..fa0b33a 100644 --- a/src/dialogs/TagDialog.cpp +++ b/src/dialogs/TagDialog.cpp @@ -848,6 +848,9 @@ TagDialog::metadataChanged( Meta::AlbumPtr album ) // If the metadata of the current album has changed, reload the cover if( album == m_currentTrack->album() ) loadCover(); + + // TODO: if the lyrics changed: should we show a warning and ask the user + // if he wants to use the new lyrics? } void diff --git a/src/scriptengine/AmarokLyricsScript.cpp b/src/scriptengine/AmarokLyricsScript.cpp index 5e7a94e..d045b79 100644 --- a/src/scriptengine/AmarokLyricsScript.cpp +++ b/src/scriptengine/AmarokLyricsScript.cpp @@ -18,7 +18,6 @@ #include "AmarokLyricsScript.h" #include "core/support/Amarok.h" -#include "core-impl/collections/support/CollectionManager.h" #include "core/support/Debug.h" #include "EngineController.h" #include "LyricsManager.h" @@ -86,10 +85,7 @@ AmarokLyricsScript::escape( const QString& str ) void AmarokLyricsScript::setLyricsForTrack( const QString& trackUrl, const QString& lyrics ) const { - DEBUG_BLOCK - Meta::TrackPtr track = CollectionManager::instance()->trackForUrl( KUrl( trackUrl ) ); - if( track ) - track->setCachedLyrics( lyrics ); + LyricsManager::self()->setLyricsForTrack( trackUrl, lyrics ); } QString diff --git a/src/themes/context/Amarok-Mockup/colors b/src/themes/context/Amarok-Mockup/colors index a0b9488..3fde41e 100644 --- a/src/themes/context/Amarok-Mockup/colors +++ b/src/themes/context/Amarok-Mockup/colors @@ -64,7 +64,7 @@ ForegroundInactive=152,152,152 ForegroundLink=0,0,255 ForegroundNegative=107,0,0 ForegroundNeutral=0,90,95 -ForegroundNormal=0,0,0 +ForegroundNormal=255,255,255 ForegroundPositive=0,95,0 ForegroundVisited=88,55,150