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 <antonio.e.russo@gmail.com>

src/core/application.cpp

@@ -20,6 +20,9 @@
    along with Clementine.  If not, see <http://www.gnu.org/licenses/>.
 */
 
+#include <QSettings>
+#include <QTimer>
+
 #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<TagReaderClient> tag_reader_client_;
   Lazy<Database> database_;
   Lazy<AlbumCoverLoader> 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(); }

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<ApplicationImpl> p_;

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

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(sectionResized(int, int, int)),
-  connect(header_, SIGNAL(sectionMoved(int, int, int)), SLOT(SaveGeometry()));
+          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() {
+void PlaylistView::DirtyGeometry() {
-  if (read_only_settings_ || !header_loaded_) return;
+  dirty_geometry_ = true;
+  app_->DirtySettings();
+}
 
-  QSettings settings;
+void PlaylistView::SaveGeometry(QSettings* settings) {
-  settings.beginGroup(Playlist::kSettingsGroup);
+  if (!dirty_geometry_ || read_only_settings_ || !header_loaded_) return;
-  settings.setValue("state", header_->SaveState());
+  dirty_geometry_ = false;
-  settings.setValue("state_version", kStateVersion);
+
+  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() {
+void PlaylistView::DirtySettings() {
-  if (read_only_settings_) return;
+  dirty_settings_ = true;
+  app_->DirtySettings();
+}
 
-  QSettings s;
+void PlaylistView::SaveSettings(QSettings* settings) {
-  s.beginGroup(Playlist::kSettingsGroup);
+  if (!dirty_settings_ || read_only_settings_) return;
-  s.setValue("glow_effect", glow_enabled_);
+  dirty_settings_ = false;
-  s.setValue("column_alignments", QVariant::fromValue(column_alignment_));
+
-  s.setValue(kSettingBackgroundImageType, background_image_type_);
+  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 {

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_;

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;
+  doubleclick_addmode_ = AddBehaviour(
-  s.beginGroup(kSettingsGroup);
+      settings_.value("doubleclick_addmode", AddBehaviour_Append).toInt());
-
-  doubleclick_addmode_ =
-      AddBehaviour(s.value("doubleclick_addmode", AddBehaviour_Append).toInt());
   doubleclick_playmode_ = PlayBehaviour(
-      s.value("doubleclick_playmode", PlayBehaviour_IfStopped).toInt());
+      settings_.value("doubleclick_playmode", PlayBehaviour_IfStopped).toInt());
-  doubleclick_playlist_addmode_ =
+  doubleclick_playlist_addmode_ = PlaylistAddBehaviour(
-      PlaylistAddBehaviour(s.value("doubleclick_playlist_addmode",
+      settings_.value("doubleclick_playlist_addmode", PlaylistAddBehaviour_Play)
-                                   PlaylistAddBehaviour_Play).toInt());
+          .toInt());
-  menu_playmode_ =
+  menu_playmode_ = PlayBehaviour(
-      PlayBehaviour(s.value("menu_playmode", PlayBehaviour_IfStopped).toInt());
+      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;
+  dirty_geometry_ = true;
-  SaveGeometry();
+  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("splitter_state", ui_->splitter->saveState());
-  settings_.setValue("current_tab", ui_->tabs->currentIndex());
+  settings->setValue("current_tab", ui_->tabs->currentIndex());
-  settings_.setValue("tab_mode", ui_->tabs->mode());
+  settings->setValue("tab_mode", ui_->tabs->mode());
 
-  ui_->tabs->saveSettings(kSettingsGroup);
+  // Leaving this here for now
+  ui_->tabs->saveSettings(settings);
 }
 
-void MainWindow::SavePlaybackStatus() {
+void MainWindow::SavePlaybackStatus(QSettings* settings) {
-  QSettings settings;
+  dirty_playback_ = false;
-  settings.beginGroup(MainWindow::kSettingsGroup);
+  settings->setValue("playback_state", app_->player()->GetState());
-  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<Engine::State>(
-      settings.value("playback_state", Engine::Empty).toInt());
+      settings_.value("playback_state", Engine::Empty).toInt());
-  saved_playback_position_ = settings.value("playback_position", 0).toDouble();
+  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();
+  // FIXME: This may add an extra write.
-  SavePlaybackStatus();
+  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

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 SaveGeometry(QSettings* settings);
-  void SavePlaybackStatus();
+  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_;

src/widgets/fancytabwidget.cpp

@@ -307,16 +307,13 @@ void FancyTabWidget::loadSettings(const char *kSettingsGroup) {
     }
 }
 
-void FancyTabWidget::saveSettings(const char *kSettingsGroup) {
+void FancyTabWidget::saveSettings(QSettings* settings) {
-    QSettings settings;
+  for (int i = 0; i < count(); i++) {
-    settings.beginGroup(kSettingsGroup);
+    int originalIndex = tabBar()->tabData(i).toInt();
+    std::string k = "tab_index_" + std::to_string(originalIndex);
 
-    for(int i =0;i<count();i++) {
+    settings->setValue(QString::fromStdString(k), i);
-        int originalIndex = tabBar()->tabData(i).toInt();
+  }
-        std::string k = "tab_index_" + std::to_string(originalIndex);
-
-        settings.setValue(QString::fromStdString(k), i);
-    }
 }
 
 

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,