From e66fdd86da03f5de5b5ac990ae5f462eb7b992b0 Mon Sep 17 00:00:00 2001 From: Antonio Russo Date: Fri, 6 Sep 2019 22:02:48 -0600 Subject: [PATCH] Periodically save settings Instead of immediately saving, which leads to poor performance, and possible hardware damage (see #6057), limit saves to once per second (similar to how KDE does it). It also guarantees that only one save is required per second, by sharing a QSettings object, and establishes a signaling framework to put other setting save events into (but only uses this for the two major offenders: playlist tab switching and window resizing). This is in contrast to 6a312e7, which simply deferred the save until program exit, and caused problems for some people (see #6217 and #6209). Signed-off-by: Antonio Russo --- src/core/application.cpp | 17 +++++- src/core/application.h | 8 +++ src/main.cpp | 1 + src/playlist/playlistview.cpp | 63 +++++++++++++++-------- src/playlist/playlistview.h | 9 +++- src/ui/mainwindow.cpp | 94 +++++++++++++++++++--------------- src/ui/mainwindow.h | 9 +++- src/widgets/fancytabwidget.cpp | 15 +++--- src/widgets/fancytabwidget.h | 3 +- 9 files changed, 141 insertions(+), 78 deletions(-) diff --git a/src/core/application.cpp b/src/core/application.cpp index 50092ab95..e3704df6a 100644 --- a/src/core/application.cpp +++ b/src/core/application.cpp @@ -20,6 +20,9 @@ along with Clementine. If not, see . */ +#include +#include + #include "application.h" #include "config.h" @@ -67,7 +70,8 @@ bool Application::kIsPortable = false; class ApplicationImpl { public: ApplicationImpl(Application* app) - : tag_reader_client_([=]() { + : settings_timer_(app), + tag_reader_client_([=]() { TagReaderClient* client = new TagReaderClient(app); app->MoveToNewThread(client); client->Start(); @@ -150,6 +154,9 @@ class ApplicationImpl { }) { } + QTimer settings_timer_; + QSettings settings_; + Lazy tag_reader_client_; Lazy database_; Lazy album_cover_loader_; @@ -188,6 +195,10 @@ Application::Application(QObject* parent) // TODO(John Maguire): Make this not a weird singleton. tag_reader_client(); + + p_->settings_timer_.setInterval(1000); + p_->settings_timer_.setSingleShot(true); + connect(&(p_->settings_timer_), SIGNAL(timeout()), SLOT(SaveSettings_())); } Application::~Application() { @@ -229,6 +240,8 @@ QString Application::language_without_region() const { return language_name_; } +void Application::SaveSettings_() { emit SaveSettings(&(p_->settings_)); } + void Application::ReloadSettings() { emit SettingsChanged(); } void Application::OpenSettingsDialogAtPage(SettingsDialog::Page page) { @@ -326,3 +339,5 @@ TagReaderClient* Application::tag_reader_client() const { TaskManager* Application::task_manager() const { return p_->task_manager_.get(); } + +void Application::DirtySettings() { p_->settings_timer_.start(); } diff --git a/src/core/application.h b/src/core/application.h index 679c45335..1c1a1cb07 100644 --- a/src/core/application.h +++ b/src/core/application.h @@ -28,6 +28,8 @@ #include "ui/settingsdialog.h" +class QSettings; + class AlbumCoverLoader; class Appearance; class ApplicationImpl; @@ -98,6 +100,8 @@ class Application : public QObject { TagReaderClient* tag_reader_client() const; TaskManager* task_manager() const; + void DirtySettings(); + void MoveToNewThread(QObject* object); void MoveToThread(QObject* object, QThread* thread); @@ -109,8 +113,12 @@ class Application : public QObject { signals: void ErrorAdded(const QString& message); void SettingsChanged(); + void SaveSettings(QSettings* settings); void SettingsDialogRequested(SettingsDialog::Page page); + private slots: + void SaveSettings_(); + private: QString language_name_; std::unique_ptr p_; diff --git a/src/main.cpp b/src/main.cpp index ecf47b9d4..edb883a12 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -430,6 +430,7 @@ int main(int argc, char* argv[]) { QtConcurrent::run(&ParseAProto); Application app; + QObject::connect(&a, SIGNAL(aboutToQuit()), &app, SLOT(SaveSettings_())); app.set_language_name(language); // Network proxy diff --git a/src/playlist/playlistview.cpp b/src/playlist/playlistview.cpp index 28137b110..963715c3f 100644 --- a/src/playlist/playlistview.cpp +++ b/src/playlist/playlistview.cpp @@ -135,6 +135,8 @@ PlaylistView::PlaylistView(QWidget* parent) cached_current_row_row_(-1), drop_indicator_row_(-1), drag_over_(false), + dirty_geometry_(false), + dirty_settings_(false), dynamic_controls_(new DynamicPlaylistControls(this)) { setHeader(header_); header_->setSectionsMovable(true); @@ -153,12 +155,13 @@ PlaylistView::PlaylistView(QWidget* parent) currenttrack_pause_ = currenttrack_pause.pixmap(currenttrack_pause.actualSize(QSize(32, 32))); - connect(header_, SIGNAL(sectionResized(int, int, int)), SLOT(SaveGeometry())); - connect(header_, SIGNAL(sectionMoved(int, int, int)), SLOT(SaveGeometry())); + connect(header_, SIGNAL(sectionResized(int, int, int)), + SLOT(DirtyGeometry())); + connect(header_, SIGNAL(sectionMoved(int, int, int)), SLOT(DirtyGeometry())); connect(header_, SIGNAL(sortIndicatorChanged(int, Qt::SortOrder)), - SLOT(SaveGeometry())); + SLOT(DirtyGeometry())); connect(header_, SIGNAL(SectionVisibilityChanged(int, bool)), - SLOT(SaveGeometry())); + SLOT(DirtyGeometry())); connect(header_, SIGNAL(SectionRatingLockStatusChanged(bool)), SLOT(SetRatingLockStatus(bool))); connect(header_, SIGNAL(sectionResized(int, int, int)), @@ -167,7 +170,7 @@ PlaylistView::PlaylistView(QWidget* parent) SLOT(InvalidateCachedCurrentPixmap())); connect(header_, SIGNAL(SectionVisibilityChanged(int, bool)), SLOT(InvalidateCachedCurrentPixmap())); - connect(header_, SIGNAL(StretchEnabledChanged(bool)), SLOT(SaveSettings())); + connect(header_, SIGNAL(StretchEnabledChanged(bool)), SLOT(DirtySettings())); connect(header_, SIGNAL(StretchEnabledChanged(bool)), SLOT(StretchChanged(bool))); connect(header_, SIGNAL(MouseEntered()), SLOT(RatingHoverOut())); @@ -196,7 +199,6 @@ PlaylistView::PlaylistView(QWidget* parent) } PlaylistView::~PlaylistView() { - SaveGeometry(); delete style_; } @@ -210,6 +212,10 @@ void PlaylistView::SetApplication(Application* app) { connect(app_->player(), SIGNAL(Playing()), SLOT(StartGlowing())); connect(app_->player(), SIGNAL(Stopped()), SLOT(StopGlowing())); connect(app_->player(), SIGNAL(Stopped()), SLOT(PlayerStopped())); + connect(app_, SIGNAL(SaveSettings(QSettings*)), + SLOT(SaveGeometry(QSettings*))); + connect(app_, SIGNAL(SaveSettings(QSettings*)), + SLOT(SaveSettings(QSettings*))); } void PlaylistView::SetItemDelegates(LibraryBackend* backend) { @@ -416,13 +422,19 @@ void PlaylistView::LoadRatingLockStatus() { ratings_locked_ = s.value("RatingLocked", false).toBool(); } -void PlaylistView::SaveGeometry() { - if (read_only_settings_ || !header_loaded_) return; +void PlaylistView::DirtyGeometry() { + dirty_geometry_ = true; + app_->DirtySettings(); +} - QSettings settings; - settings.beginGroup(Playlist::kSettingsGroup); - settings.setValue("state", header_->SaveState()); - settings.setValue("state_version", kStateVersion); +void PlaylistView::SaveGeometry(QSettings* settings) { + if (!dirty_geometry_ || read_only_settings_ || !header_loaded_) return; + dirty_geometry_ = false; + + settings->beginGroup(Playlist::kSettingsGroup); + settings->setValue("state", header_->SaveState()); + settings->setValue("state_version", kStateVersion); + settings->endGroup(); } void PlaylistView::SetRatingLockStatus(bool state) { @@ -1179,20 +1191,28 @@ void PlaylistView::ReloadSettings() { setEditTriggers(editTriggers() | QAbstractItemView::SelectedClicked); } -void PlaylistView::SaveSettings() { - if (read_only_settings_) return; +void PlaylistView::DirtySettings() { + dirty_settings_ = true; + app_->DirtySettings(); +} - QSettings s; - s.beginGroup(Playlist::kSettingsGroup); - s.setValue("glow_effect", glow_enabled_); - s.setValue("column_alignments", QVariant::fromValue(column_alignment_)); - s.setValue(kSettingBackgroundImageType, background_image_type_); +void PlaylistView::SaveSettings(QSettings* settings) { + if (!dirty_settings_ || read_only_settings_) return; + dirty_settings_ = false; + + settings->beginGroup(Playlist::kSettingsGroup); + settings->setValue("glow_effect", glow_enabled_); + settings->setValue("column_alignments", + QVariant::fromValue(column_alignment_)); + settings->setValue(kSettingBackgroundImageType, background_image_type_); + settings->endGroup(); } void PlaylistView::StretchChanged(bool stretch) { setHorizontalScrollBarPolicy(stretch ? Qt::ScrollBarAlwaysOff : Qt::ScrollBarAsNeeded); - SaveGeometry(); + dirty_geometry_ = true; + app_->DirtySettings(); } void PlaylistView::DynamicModeChanged(bool dynamic) { @@ -1261,7 +1281,8 @@ void PlaylistView::SetColumnAlignment(int section, Qt::Alignment alignment) { column_alignment_[section] = alignment; emit ColumnAlignmentChanged(column_alignment_); - SaveSettings(); + dirty_settings_ = true; + app_->DirtySettings(); } Qt::Alignment PlaylistView::column_alignment(int section) const { diff --git a/src/playlist/playlistview.h b/src/playlist/playlistview.h index aa4b144a1..8e24e6b46 100644 --- a/src/playlist/playlistview.h +++ b/src/playlist/playlistview.h @@ -145,7 +145,9 @@ signals: private slots: void LoadGeometry(); void LoadRatingLockStatus(); - void SaveGeometry(); + void DirtyGeometry(); + void DirtySettings(); + void SaveGeometry(QSettings* settings); void SetRatingLockStatus(bool state); void GlowIntensityChanged(); void InhibitAutoscrollTimeout(); @@ -153,7 +155,7 @@ signals: void InvalidateCachedCurrentPixmap(); void PlaylistDestroyed(); - void SaveSettings(); + void SaveSettings(QSettings* s); void StretchChanged(bool stretch); void RatingHoverIn(const QModelIndex& index, const QPoint& pos); @@ -251,6 +253,9 @@ signals: int drop_indicator_row_; bool drag_over_; + bool dirty_geometry_; + bool dirty_settings_; + bool ratings_locked_; // To store Ratings section lock status DynamicPlaylistControls* dynamic_controls_; diff --git a/src/ui/mainwindow.cpp b/src/ui/mainwindow.cpp index 58419de01..2922b6508 100644 --- a/src/ui/mainwindow.cpp +++ b/src/ui/mainwindow.cpp @@ -223,6 +223,8 @@ MainWindow::MainWindow(Application* app, SystemTrayIcon* tray_icon, OSD* osd, track_position_timer_(new QTimer(this)), track_slider_timer_(new QTimer(this)), initialized_(false), + dirty_geometry_(false), + dirty_playback_(false), saved_playback_position_(0), saved_playback_state_(Engine::Empty), doubleclick_addmode_(AddBehaviour_Append), @@ -258,6 +260,9 @@ MainWindow::MainWindow(Application* app, SystemTrayIcon* tray_icon, OSD* osd, connect(global_search_view_, SIGNAL(AddToPlaylist(QMimeData*)), SLOT(AddToPlaylist(QMimeData*))); + // Set up the settings group early + settings_.beginGroup(kSettingsGroup); + // Add tabs to the fancy tab widget ui_->tabs->addTab(global_search_view_, IconLoader::Load("search", IconLoader::Base), @@ -301,6 +306,9 @@ MainWindow::MainWindow(Application* app, SystemTrayIcon* tray_icon, OSD* osd, connect(track_slider_timer_, SIGNAL(timeout()), SLOT(UpdateTrackSliderPosition())); + connect(app_, SIGNAL(SaveSettings(QSettings*)), + SLOT(SaveSettings(QSettings*))); + // Start initialising the player qLog(Debug) << "Initialising player"; app_->player()->Init(); @@ -977,7 +985,6 @@ MainWindow::MainWindow(Application* app, SystemTrayIcon* tray_icon, OSD* osd, // Load settings qLog(Debug) << "Loading settings"; - settings_.beginGroup(kSettingsGroup); // Set last used geometry to position window on the correct monitor // Set window state only if the window was last maximized @@ -1064,13 +1071,11 @@ MainWindow::MainWindow(Application* app, SystemTrayIcon* tray_icon, OSD* osd, if (!options.contains_play_options()) LoadPlaybackStatus(); initialized_ = true; - SaveGeometry(); qLog(Debug) << "Started"; } MainWindow::~MainWindow() { - SaveGeometry(); delete ui_; } @@ -1085,18 +1090,15 @@ void MainWindow::ReloadSettings() { show(); #endif - QSettings s; - s.beginGroup(kSettingsGroup); - - doubleclick_addmode_ = - AddBehaviour(s.value("doubleclick_addmode", AddBehaviour_Append).toInt()); + doubleclick_addmode_ = AddBehaviour( + settings_.value("doubleclick_addmode", AddBehaviour_Append).toInt()); doubleclick_playmode_ = PlayBehaviour( - s.value("doubleclick_playmode", PlayBehaviour_IfStopped).toInt()); - doubleclick_playlist_addmode_ = - PlaylistAddBehaviour(s.value("doubleclick_playlist_addmode", - PlaylistAddBehaviour_Play).toInt()); - menu_playmode_ = - PlayBehaviour(s.value("menu_playmode", PlayBehaviour_IfStopped).toInt()); + settings_.value("doubleclick_playmode", PlayBehaviour_IfStopped).toInt()); + doubleclick_playlist_addmode_ = PlaylistAddBehaviour( + settings_.value("doubleclick_playlist_addmode", PlaylistAddBehaviour_Play) + .toInt()); + menu_playmode_ = PlayBehaviour( + settings_.value("menu_playmode", PlayBehaviour_IfStopped).toInt()); bool show_sidebar = settings_.value("show_sidebar", true).toBool(); ui_->sidebar_layout->setVisible(show_sidebar); @@ -1278,54 +1280,61 @@ void MainWindow::ScrobbleButtonVisibilityChanged(bool value) { } } -void MainWindow::changeEvent(QEvent*) { +void MainWindow::SaveSettings(QSettings* settings) { if (!initialized_) return; - SaveGeometry(); + settings->beginGroup(kSettingsGroup); + if (dirty_geometry_) SaveGeometry(settings); + if (dirty_playback_) SavePlaybackStatus(settings); + settings->endGroup(); +} + +void MainWindow::changeEvent(QEvent*) { + dirty_geometry_ = true; + app_->DirtySettings(); } void MainWindow::resizeEvent(QResizeEvent*) { - if (!initialized_) return; - SaveGeometry(); + dirty_geometry_ = true; + app_->DirtySettings(); } -void MainWindow::SaveGeometry() { +void MainWindow::SaveGeometry(QSettings* settings) { if (!initialized_) return; + dirty_geometry_ = false; was_maximized_ = isMaximized(); - settings_.setValue("maximized", was_maximized_); + settings->setValue("maximized", was_maximized_); // Save the geometry only when mainwindow is not in maximized state if (!was_maximized_) { - settings_.setValue("geometry", saveGeometry()); + settings->setValue("geometry", saveGeometry()); } - settings_.setValue("splitter_state", ui_->splitter->saveState()); - settings_.setValue("current_tab", ui_->tabs->currentIndex()); - settings_.setValue("tab_mode", ui_->tabs->mode()); + settings->setValue("splitter_state", ui_->splitter->saveState()); + settings->setValue("current_tab", ui_->tabs->currentIndex()); + settings->setValue("tab_mode", ui_->tabs->mode()); - ui_->tabs->saveSettings(kSettingsGroup); + // Leaving this here for now + ui_->tabs->saveSettings(settings); } -void MainWindow::SavePlaybackStatus() { - QSettings settings; - settings.beginGroup(MainWindow::kSettingsGroup); - settings.setValue("playback_state", app_->player()->GetState()); +void MainWindow::SavePlaybackStatus(QSettings* settings) { + dirty_playback_ = false; + settings->setValue("playback_state", app_->player()->GetState()); if (app_->player()->GetState() == Engine::Playing || app_->player()->GetState() == Engine::Paused) { - settings.setValue( + settings->setValue( "playback_position", app_->player()->engine()->position_nanosec() / kNsecPerSec); } else { - settings.setValue("playback_position", 0); + settings->setValue("playback_position", 0); } } void MainWindow::LoadPlaybackStatus() { - QSettings settings; - settings.beginGroup(MainWindow::kSettingsGroup); bool resume_playback = - settings.value("resume_playback_after_start", false).toBool(); + settings_.value("resume_playback_after_start", false).toBool(); saved_playback_state_ = static_cast( - settings.value("playback_state", Engine::Empty).toInt()); - saved_playback_position_ = settings.value("playback_position", 0).toDouble(); + settings_.value("playback_state", Engine::Empty).toInt()); + saved_playback_position_ = settings_.value("playback_position", 0).toDouble(); if (!resume_playback || saved_playback_state_ == Engine::Empty || saved_playback_state_ == Engine::Idle) { return; @@ -1433,12 +1442,10 @@ void MainWindow::StopAfterCurrent() { } void MainWindow::closeEvent(QCloseEvent* event) { - QSettings s; - s.beginGroup(kSettingsGroup); - bool keep_running(false); if (tray_icon_) - keep_running = s.value("keeprunning", tray_icon_->IsVisible()).toBool(); + keep_running = + settings_.value("keeprunning", tray_icon_->IsVisible()).toBool(); if (keep_running && event->spontaneous()) { event->ignore(); @@ -2794,10 +2801,13 @@ bool MainWindow::winEvent(MSG* msg, long*) { #endif // Q_OS_WIN32 void MainWindow::Exit() { - SaveGeometry(); - SavePlaybackStatus(); + // FIXME: This may add an extra write. + dirty_playback_ = true; settings_.setValue("show_sidebar", ui_->action_toggle_show_sidebar->isChecked()); + settings_.endGroup(); + SaveSettings(&settings_); + settings_.sync(); if (app_->player()->engine()->is_fadeout_enabled()) { // To shut down the application when fadeout will be finished diff --git a/src/ui/mainwindow.h b/src/ui/mainwindow.h index 2f62f8375..74a6ff8cb 100644 --- a/src/ui/mainwindow.h +++ b/src/ui/mainwindow.h @@ -151,6 +151,7 @@ signals: private slots: void FilePathChanged(const QString& path); + void SaveSettings(QSettings* settings); void MediaStopped(); void MediaPaused(); void MediaPlaying(); @@ -266,8 +267,8 @@ signals: void OpenSettingsDialogAtPage(SettingsDialog::Page page); void ShowSongInfoConfig(); - void SaveGeometry(); - void SavePlaybackStatus(); + void SaveGeometry(QSettings* settings); + void SavePlaybackStatus(QSettings* settings); void LoadPlaybackStatus(); void ResumePlayback(); @@ -380,6 +381,10 @@ signals: QSettings settings_; bool initialized_; + + bool dirty_geometry_; + bool dirty_playback_; + bool was_maximized_; int saved_playback_position_; Engine::State saved_playback_state_; diff --git a/src/widgets/fancytabwidget.cpp b/src/widgets/fancytabwidget.cpp index 78f1e6e69..ec14a5d34 100644 --- a/src/widgets/fancytabwidget.cpp +++ b/src/widgets/fancytabwidget.cpp @@ -307,16 +307,13 @@ void FancyTabWidget::loadSettings(const char *kSettingsGroup) { } } -void FancyTabWidget::saveSettings(const char *kSettingsGroup) { - QSettings settings; - settings.beginGroup(kSettingsGroup); +void FancyTabWidget::saveSettings(QSettings* settings) { + for (int i = 0; i < count(); i++) { + int originalIndex = tabBar()->tabData(i).toInt(); + std::string k = "tab_index_" + std::to_string(originalIndex); - for(int i =0;itabData(i).toInt(); - std::string k = "tab_index_" + std::to_string(originalIndex); - - settings.setValue(QString::fromStdString(k), i); - } + settings->setValue(QString::fromStdString(k), i); + } } diff --git a/src/widgets/fancytabwidget.h b/src/widgets/fancytabwidget.h index e426ef46f..5a4acc2d6 100644 --- a/src/widgets/fancytabwidget.h +++ b/src/widgets/fancytabwidget.h @@ -25,6 +25,7 @@ class QActionGroup; class QMenu; +class QSettings; class QSignalMapper; namespace Core { @@ -44,7 +45,7 @@ class FancyTabWidget : public QTabWidget { void addSpacer(); void loadSettings(const char *); - void saveSettings(const char *); + void saveSettings(QSettings*); // Values are persisted - only add to the end enum Mode { Mode_None = 0,