fix #274540: save user choice of instrument genre #3830

jthistle · 2018-07-25T07:54:13Z

This addresses a feature request at: https://musescore.org/en/node/274540

The request was to keep the user's selection of instrument genre so that they don't have to change it every time they add/change an instrument. This PR adds this functionality by saving the user's last choice of instrument in QSettings, which actually means that the last selected genre will be saved even after the application has closed.

ecstrema · 2018-07-25T19:46:39Z

mscore/instrwidget.cpp

@@ -362,6 +362,8 @@ QString InstrumentTemplateListItem::text(int col) const
 InstrumentsWidget::InstrumentsWidget(QWidget* parent)
   : QWidget(parent)
      {
+      storeChangeInGenre = false;


Why is this needed?
Couldn't it be set directly to true?

This variable is needed because when the InstrumentsWidget is being created, the selected instrument genre is changed by some other scripts a couple of times. I want to be able to save changes to the genre, but only ones made by the user. So, when loading is complete and this variable has been set to true, only then will any genre changes made be stored, because they are user-made changes, not automatic ones.

The variable isn't needed though if you see my reply below I guess

ecstrema · 2018-07-25T19:52:03Z

mscore/instrwidget.cpp

+      if ( ! settings.value("selectedInstrumentGenre").isNull() ){
+            int selectedGenre = settings.value("selectedInstrumentGenre").value<int>();
+            instrumentGenreFilter->setCurrentIndex(selectedGenre);
+            }


these last lines are not needed.
just use

instrumentGenreFilter->setCurrentIndex(settings.value("selectedInstrumentGenre", /*default value, probably 0*/ 0).toInt());

same for in SelectInstrument.cpp

The default is whatever index 'common' has, since that's what it gets set to in some script (I can't remember which one). This creates a problem, because the index of common could change, but the default should still be common. So, to avoid having to look up the index of t default, it's easier and simpler to just check whether the selectedInstrumentGenre is set and then leave the filter as the default value if not.

You are right about that

ecstrema · 2018-07-25T19:54:31Z

mscore/instrwidget.cpp

+      if (storeChangeInGenre){
+            settings.setValue("selectedInstrumentGenre", index);
+            settings.sync();
+            }


couldn't these settings be saved in the destructor?
They would only be saved once, and there would be no need for the storeChangeInGenre private variable.

That's a good point actually. I can't test it right now, but I seem to remember that if you access the value of a setting after it's been changed but not synced, it will have the last synced value. The variable would be unneeded, I guess because:

The widget is initialized

The genre settings value is changed a couple of times automatically but not synced

The genre filter is set to the genre settings value (but the last saved one, which would be user chosen)

This change in genre filter is saved to the genre settings variable (meaning that any automatic genre changes are basically ignored)

User possibly changes genre

Widget closes, last chosen genre is saved

Sounds good to me. Basically, you're right :)

jthistle · 2018-07-25T23:39:19Z

Thanks for your review @Marr11317. Unfortunately I'll be unable to implement any changes for the next 3 weeks, but your feedback is very useful.

jthistle · 2018-08-14T09:21:09Z

Feedback addressed, a final review would be appreciated @Marr11317 (or anyone else)

ecstrema · 2018-08-14T10:23:29Z

mscore/instrwidget.cpp

+      {
+      QSettings settings;
+      settings.sync();
+      }


Couldn't you simply save the index in the destructor?
you would have this kind of destructor:

InstrumentsWidget::~InstrumentsWidget() { QSettings settings; settings.beginGroup(objectName()); settings.setValue("selectedInstrumentGenre", instrumentGenreFilter->index()); settings.endGroup(); }

and such a constructor

InstrumentsWidget::InstrumentsWidget() { ... QSettings settings; settings.beginGroup(objectName()); if ( ! settings.value("selectedInstrumentGenre").isNull() ){ int selectedGenre = settings.value("selectedInstrumentGenre").toInt(); instrumentGenreFilter->setCurrentIndex(selectedGenre); } settings.endGroup(); ... }

That way, no need for saving it every time the index changes. It will simply be saved on destruction, and initialised on construction.

Plus, here Qt's doc about QSettings::sync()
The settings are already synced in the destructor so there shouldn't be any need for line 1073 here.

Thanks for the review. You're right, the settings value could be written in the destructor. I'm a bit new to C++ so I don't know the ins and outs of it very well yet, so your feedback really helps :)

ecstrema · 2018-08-14T10:29:52Z

mscore/selinstrument.h

@@ -50,6 +50,7 @@ class SelectInstrument : public QDialog, private Ui::SelectInstrument {
   public:
      SelectInstrument(const Instrument*, QWidget* parent = 0);
      const InstrumentTemplate* instrTemplate() const;
+      ~SelectInstrument();
      };


same as upper

ecstrema · 2018-08-14T10:47:17Z

Here's a few things you could also do around:

remove the InstrumentsWidget::writeSettings() function, since it seems like it has no definition, and is never called.
replace the few Qt's foreach by _c++ 11's(the link is for an example) for_ in instrwidget.cpp and instrtemplate.cpp.

ecstrema · 2018-08-14T11:21:39Z

Actually, I think the index should not be saved as an int, but as a QString, since the indexes may vary if new instrument lists are added (such as with Extensions and PREF_APP_PATHS_INSTRUMENTLIST2).
I imagine something like

///constructor:
//...
QSettings settings;
settings.beginGroup(objectName());
if ( ! settings.value("selectedInstrumentGenre").isNull() ){
      QString selectedGenre = settings.value("selectedInstrumentGenre").toString();
      instrumentGenreFilter->setCurrentText(selectedGenre);
      }
settings.endGroup();
//...

///and the destructor:
//...
QSettings s;
s.beginGroup(objectName());
s.setValue("selectedInstrumentGenre", instrumentGenreFilter->currentText());
s.endGroup();
//...

jthistle · 2018-08-14T15:38:58Z

@Marr11317

Actually, I think the index should not be saved as an int, but as a QString

I don't think it should because you could have the reverse of the problem you're talking about: you're worried that instrument genre indices may change, but what about the instrument genre names? Surely those could change as well, and if they do then there would be a similar problem. Because of this, I'm going to stick with saving the index.

jthistle · 2018-08-14T18:52:55Z

Any final review needed @Marr11317 ?

ecstrema · 2018-08-15T11:47:50Z

you're worried that instrument genre indices may change, but what about the instrument genre names? Surely those could change as well, and if they do then there would be a similar problem

But then, if the string changes, and so the saved string is not found, the index will be set to 0, which means general instruments. But if new instruments lists are added, and you try to put the index to let's say 3, It could be something completely random such as "Indian folk instruments of the nineteenth century"

ecstrema · 2018-08-15T12:22:00Z

settings.beginGroup(objectName()) is widely used for two reasons:

Since the objectName() is almost all of the time set to the class' name, and you can't have two class with the same name (under the same namespace), it avoids saving two values at the same place.
An example will be easier in that case: suppose in an other dialog you use that same InstrumentsWidget class. Now, this dialog is specific to some kind off particular music (let's say earlyMusic). People that use this dialog may not want it to have the same default value as in the new score wizard, but since the 2 values are saved at the same place, they will. So what you do is this:

InstrumentsWidget* iw = new InstrumentsWidget();
iw->setObjectName("Some new specific name");

And now the values won't be saved at the same place because objectName() doesn't return the same thing anymore.
Of course, they will still be loaded at the same place so you will have to create a readSettings() function which will be reading your settings under the objectName(), and you will have to add this line in InstrumentsWidget's constructor:

connect(this, &QObject::objectNameChanged, this, &InstrumentsWidget::readSettings);

So could you just change these "instrumentGenre" to objectName()?
PS: it looks like the index in instrwidget.cpp and selinstrument.cpp are now saved under the same adress.

jthistle · 2018-08-15T12:56:03Z

@Marr11317

The whole point of this feature request is so that the genre is the same. It doesn't make sense to have it different in different dialogues. The genre, when changed in the 'add instrument' windows, should also change in the 'change instrument' window (for changing the instrument of a stave). The reasoning being that when working on a project with a particular genre of instruments you don't want to have to keep changing the genre, neither when you're adding an instrument nor when you're changing one. So, the value must be saved in the same place, something that can't be done across objects using the objectName() as the group name.

ecstrema · 2018-08-15T12:59:28Z

looks good to me.

jthistle · 2018-08-15T16:44:47Z

@Marr11317 after some investigation, I've found out that things aren't as simple as they might be :/

The instruments widget doesn't ever destruct during runtime - it constructs once, when first opened, then destructs once when the program closes. The select instrument window, on the other hand, does construct every time when opened, and destructs every time it's closed. I'm going to have to go back to writing and syncing in the on_instrumentGenreFilter_currentIndexChanged method, as inelegant as it may be. This may require me having to use a variable again, but hey, what can you do sometimes 🤷‍♂️

jthistle · 2018-08-16T08:34:08Z

@Marr11317 (one final review maybe?)

Right. This should be the final commit. Things have had to be changed a bit - as I said before, the Instrument Dialog wasn't being destroyed on close. Now, it is destroyed on close, but it's not simple, because sometimes the Instrument Widget (a child of the Dialog) needs to persist for a while after closure to allow instrument changes to be made. So the, Dialog needs to be closed at different points in the code depending on if a change has been made or not (and so it needs to happen in the editInstrList method). And, because the Dialog is a child of the main window, cleanup needs to happen in the main MuseScore class, which is why I've added a method to it in the musescore.cpp file. Phew. Saving does now happen in destructors though, which is what you wanted I think :)

ecstrema · 2018-08-16T09:45:22Z

mscore/musescore.cpp

+      {
+      if (instrList){
+            // ugly, but instrList->done(1) doesn't work, so instrList must be directly destroyed
+            instrList->~InstrumentsDialog();


what about delete instrList;

ah, yes that would be better 🤦‍♂️

jthistle · 2018-08-16T10:15:58Z

There, all done @Marr11317 . Ready for merge now @anatoly-os ?

Jojo-Schmitz · 2019-01-05T09:05:17Z

Rebase needed, conflict should not be too difficult to fix

jthistle · 2019-01-05T11:04:37Z

Accidently force-pushed branch without committing 🤦‍♂️
Reopening.

jthistle · 2019-01-05T12:03:04Z

This PR is outdated. The code is also badly written. I will rewrite it, and open a new PR, and as such I am closing this one.

jthistle force-pushed the 274540-save-instrument-group-selection branch from 748ceb7 to 6aed008 Compare July 25, 2018 09:39

ecstrema reviewed Jul 25, 2018

View reviewed changes

jthistle force-pushed the 274540-save-instrument-group-selection branch 2 times, most recently from 6af1486 to af96235 Compare August 14, 2018 09:19

ecstrema reviewed Aug 14, 2018

View reviewed changes

jthistle force-pushed the 274540-save-instrument-group-selection branch 2 times, most recently from 55fb1e9 to 6da831c Compare August 14, 2018 15:50

jthistle force-pushed the 274540-save-instrument-group-selection branch from 6da831c to bcd67b5 Compare August 16, 2018 08:30

jthistle force-pushed the 274540-save-instrument-group-selection branch 2 times, most recently from 767720e to 7d18eb3 Compare August 16, 2018 08:44

ecstrema reviewed Aug 16, 2018

View reviewed changes

jthistle force-pushed the 274540-save-instrument-group-selection branch from 7d18eb3 to 3e1903f Compare August 16, 2018 10:15

anatoly-os assigned jthistle Aug 21, 2018

jthistle changed the title ~~Save user choice of instrument genre~~ Fix #274540: save user choice of instrument genre Jan 5, 2019

jthistle changed the title ~~Fix #274540: save user choice of instrument genre~~ fix #274540: save user choice of instrument genre Jan 5, 2019

jthistle closed this Jan 5, 2019

jthistle force-pushed the 274540-save-instrument-group-selection branch from 3e1903f to cb982cd Compare January 5, 2019 11:02

fix #274540: save instrument group selection

132a27b

jthistle reopened this Jan 5, 2019

jthistle closed this Jan 5, 2019

jthistle mentioned this pull request Jan 5, 2019

fix #274540: save user choice of instrument genre #4539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #274540: save user choice of instrument genre #3830

fix #274540: save user choice of instrument genre #3830

jthistle commented Jul 25, 2018

ecstrema Jul 25, 2018

jthistle Jul 25, 2018

jthistle Jul 25, 2018

ecstrema Jul 25, 2018

jthistle Jul 25, 2018

ecstrema Aug 14, 2018

ecstrema Jul 25, 2018 •

edited

jthistle Jul 25, 2018

jthistle commented Jul 25, 2018

jthistle commented Aug 14, 2018

ecstrema Aug 14, 2018 •

edited

jthistle Aug 14, 2018

ecstrema Aug 14, 2018

ecstrema commented Aug 14, 2018 •

edited

ecstrema commented Aug 14, 2018 •

edited

jthistle commented Aug 14, 2018 •

edited

jthistle commented Aug 14, 2018

ecstrema commented Aug 15, 2018

ecstrema commented Aug 15, 2018

jthistle commented Aug 15, 2018

ecstrema commented Aug 15, 2018

jthistle commented Aug 15, 2018

jthistle commented Aug 16, 2018 •

edited

ecstrema Aug 16, 2018

jthistle Aug 16, 2018

jthistle commented Aug 16, 2018

Jojo-Schmitz commented Jan 5, 2019

jthistle commented Jan 5, 2019

jthistle commented Jan 5, 2019

fix #274540: save user choice of instrument genre #3830

fix #274540: save user choice of instrument genre #3830

Conversation

jthistle commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstrema Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jthistle commented Jul 25, 2018

jthistle commented Aug 14, 2018

ecstrema Aug 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstrema commented Aug 14, 2018 • edited

ecstrema commented Aug 14, 2018 • edited

jthistle commented Aug 14, 2018 • edited

jthistle commented Aug 14, 2018

ecstrema commented Aug 15, 2018

ecstrema commented Aug 15, 2018

jthistle commented Aug 15, 2018

ecstrema commented Aug 15, 2018

jthistle commented Aug 15, 2018

jthistle commented Aug 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jthistle commented Aug 16, 2018

Jojo-Schmitz commented Jan 5, 2019

jthistle commented Jan 5, 2019

jthistle commented Jan 5, 2019

ecstrema Jul 25, 2018 •

edited

ecstrema Aug 14, 2018 •

edited

ecstrema commented Aug 14, 2018 •

edited

ecstrema commented Aug 14, 2018 •

edited

jthistle commented Aug 14, 2018 •

edited

jthistle commented Aug 16, 2018 •

edited