Do not save MIDI connections in presets #7445

michaelgregorius · 2024-08-11T09:01:04Z

The relevant change can be found in InstrumentTrack::saveTrackSpecificSettings where the new method isPresetMode is used to decide whether to store the MIDI port or not.

The other changes are mostly refactoring.

Fixes #3922.

Ensure that no MIDI information is stored in presets. Add the new protected method `isSimpleSerializing` that can be used by sub classes to find out if they are temporarily in preset saving mode or not. Please note that the existing implementation is very brittle because an instance might accidentally stay in that mode. Use the information in `InstrumentTrack::saveTrackSpecificSettings`.

Reduce direct accesses to `m_simpleSerializingMode` by using the method `isSimpleSerializing` and extending `setSimpleSerializing` with a parameter.

Rename "SimpleSerializing" to "PresetMode" everywhere because that is what this mode describes in fact.

michaelgregorius · 2024-08-11T09:05:39Z

Added @zonkmachine as a reviewer because he has created issue #3922 and @PhysSong because he's been active in there as well and did the changes in #4207.

Rossmaxx

One minor styling nitpick. Everything else looks fine for me.

src/core/Track.cpp

zonkmachine

Works like a charm. No midi channel information are in the presets but are still saved/loaded for projects. The preset are also backward compatible.

src/tracks/InstrumentTrack.cpp

JohannesLorenz

Looks good, only minor questions and optional points.

include/Track.h

src/core/Track.cpp

include/Track.h

Make `isPresetMode` a one-liner and initialize `m_presetMode` in the header instead of in the constructor (list).

Rossmaxx · 2024-08-12T15:35:18Z

A bit out of scope but will it make sense to seperate out saving presets into it's own area?

michaelgregorius · 2024-08-12T16:01:05Z

A bit out of scope but will it make sense to seperate out saving presets into it's own area?

What do you mean exactly?

Rossmaxx · 2024-08-12T16:12:44Z

I mean move the preset saving, loading and handling logic to it's own file.

michaelgregorius · 2024-08-12T16:24:00Z

I mean move the preset saving, loading and handling logic to it's own file.

I am not really sure if that would work because the presets are subsets of the full data that's stored for an instrument for example. If that code was to be extracted and moved into it's own file then it might diverge.

Depending of what constitutes to a preset it might be an option though to structure the data differently so that in the full set there's one section with everything related to preset data and one section with the rest that does not go into the presets like for example the MIDI connections. In that case saving a preset would just mean to save the preset section into a separate file. In a way the full set would be build additively by adding preset and non-preset data whereas now the presets seem to be stored in a subtractive or selective way.

Enable to make `setPresetMode` protected. This is accomplished by adding the new methods `savePreset` and `loadPreset` to `InstrumentTrack`. These methods manage the calling of `setPresetMode` so that callers won't have to know about this implementation detail. Then `saveSettings` and `loadSettings` are called respectively which in turn will delegate to `Track`. All context that use `loadPreset` or `savePreset` already known that they are dealing with an `InstrumentTrack` which is the reason why this works: * Loading of a preset file * Saving a preset * Replacement of an instrument Therefore in these place `loadPreset` and `savePreset` can be called and it's not necessary anymore to call `setPresetMode` there.

michaelgregorius · 2024-08-14T15:44:57Z

@JohannesLorenz, commit eb8633a improves a bit more on the situation.

It adds the new methods savePreset and loadPreset to InstrumentTrack. These can be thought of an equivalent to the methods that you have proposed with an additional boolean but they communicate better what's the actual intention. These methods now also manage the calling of setPresetMode so that outside callers, e.g. TrackContainerView, won't have to know about this implementation detail anymore. This enables to make setPresetMode protected as well. After setting presets mode the methods saveSettings and loadSettings are called respectively which in turn will delegate to Track.

Track will save the data that's common to all tracks, e.g. "solo", "mute", "name", etc., and will then use the virtual functions loadTrackSpecificSettings and saveTrackSpecificSettings to load/save the information that's specific to instruments, samples, patterns and automation. Both levels need information about whether a preset is saved or not.

I will have to check if this can somehow be disentangled any further. However, currently it's a strange back and forth between Track and InstrumentTrack anyway, which is a code smell to me.

By the way are there any other presets than instrument presets? If there are only instrument presents then this might be what's causing these problems and this back and forth.

Another thing that's strange is that the presets are not really instrument presets but rather track presets. So I am pushing a button in the instrument window and what it will do is to also save information about the track that the instrument is in. This is also a bit smelly with regards to the design. I would have expected that an instrument preset only stores instrument specific information. This oddity is also reflected in the preset saving code in InstrumentTrackWindow::saveSettingsBtnClicked. Here the attributes "muted", "solo" and "mutedBeforeSolo" must be explicitly set to false in some afterburner code.

What do you think about merging this PR as is? It fixes a problem and the remaining stuff rather seem to be design problems that could be tackled in an extra PR.

Rossmaxx · 2024-08-14T15:49:10Z

Another thing that's strange is that the presets are not really instrument presets but rather track presets. So I am pushing a button in the instrument window and what it will do is to also save information about the track that the instrument is in. This is also a bit smelly with regards to the design. I would have expected that an instrument preset only stores instrument specific information.

This is basically what i thought of when i asked for splitting the preset logic to it's own file. It was poor articulation on my end.

Also for presets, ideally we should save the preset xpf and add an entry in the mmp xml as a node for a track if it's a preset. I hope you understand.

michaelgregorius · 2024-08-14T16:09:14Z

Another thing that's strange is that the presets are not really instrument presets but rather track presets. So I am pushing a button in the instrument window and what it will do is to also save information about the track that the instrument is in. This is also a bit smelly with regards to the design. I would have expected that an instrument preset only stores instrument specific information.

This is basically what i thought of when i asked for splitting the preset logic to it's own file. It was poor articulation on my end.

Also for presets, ideally we should save the preset xpf and add an entry in the mmp xml as a node for a track if it's a preset. I hope you understand.

I think we are almost on the same page.

IMO there could be at least two types of presets:

Instrument presets
Track presets

Instrument presets only contain instrument information. They do not contain any information about tracks because logically these are enclosing objects. So instrument presets do not contain any information about mute, solo, height, color, etc.

A track preset is used to add templated tracks. A track preset contains the track information like solo, mute, height, color, etc. It can also optionally contain a track object like an instrument, sample, pattern or automation. The object in turn can also optionally contain a preset. If no preset is contained then the potentially contained object is created in its default state.

The project save files, i.e. MMP, should not contain contain any information about whether something was added as a preset or not because that information is not really useful. I think we have a different view here.

PhysSong · 2024-08-16T05:41:04Z

It looks like the value of m_presetMode won't be restored after loading/saving presets

michaelgregorius · 2024-08-16T15:27:23Z

@PhysSong, it only looks like this. The restore is currently done in Track::saveSettings and Track::loadSettings. Both have implementations like:

if (isPresetMode())
{
   [Do something]

   setPresetMode(false);
}

The implementation was like that before and it is of course sub-optimal (to put it nicely). If an exception occurs anywhere in between setting the member to preset mode and restoring it then it might not be restored at all.

I guess I will implement a guard-like RAII class which will get the m_presetMode boolean as a reference, store it's current value and then set it to a given new value. In the destructor it will then reset the boolean to the previous value. This class will then only be used in InstrumentTrack::savePreset and InstrumentTrack::loadPreset.

Add the new local class `BoolGuard` which uses RAII to safely set a boolean to a value and later reset it to the old value. The guard is needed to ensure that the boolean `m_presetMode` is safely set to `true` in `savePreset` and `loadPreset` and reset even in case of exceptions. For this to work `m_presetMode` had to be changed to protected visibility. On this other hand this enables the removal of `setPresetMode`. Add comments to the two places where previously the mode was reset so that people do not wonder why nothing is reset and potentially put the code back in there.

michaelgregorius · 2024-08-16T15:58:57Z

@PhysSong, I have implemented the BoolGuard with commit e3bccb0.

See its documentation on why it is only added locally. If there are other places where it might be useful it should be moved into its own file in a more general place.

JohannesLorenz · 2024-08-16T19:48:24Z

I will have to check if this can somehow be disentangled any further. However, currently it's a strange back and forth between Track and InstrumentTrack anyway, which is a code smell to me.

I am just a bit sad about the git-blame-pollution, and I think as the author, you would be the optimal person to have a clear view on my proposal. Ultimately, the scope of the PR is the author's decision, and I would open a further PR if you do not want to handle it here.

JohannesLorenz

Only editorials

src/tracks/InstrumentTrack.cpp

src/core/Track.cpp

Get rid of the protected member `m_presetMode` and its corresponding method `isPresetMode()`. This is accomplished by introducing two new methods `saveTrack` and `loadTrack`. These methods have a similar interface to `saveSettings` and `loadSettings` but they additionally contain a boolean which indicates if a preset is saved/loaded or a whole track. Both new methods contain the previous code of `saveSettings` and `loadSettings`. The latter two now only delegate to the new methods assuming that the full track is to be stored/loaded if called via the overridden methods `saveSettings` and `loadSettings`. Adjust `saveTrackSpecificSettings` so that it also passes information of whether a preset or a whole track is stored. This leads to changes in the interfaces of `AutomationTrack`, `InstrumentTrack`, `PatternTrack` and `SampleTrack`. Only the implementation of `InstrumentTrack` uses the new information though. Remove the `BoolGuard` class and adjust the implementation of `InstrumentTrack::savePreset` and `InstrumentTrack::loadPreset` to simply delegate to the `saveTrack` and `loadTrack` methods with the correct parameters. The methods `savePreset` and `loadPreset` are kept because they make the code more readable. Implementation notes --------------------- It could be considered to move `savePreset` and `loadPreset` into `Track`. The method `loadTrackSpecificSettings` was not adjusted with an additional boolean because for now no implementation needs to explicitly know if a preset or a track is restored. It could be considered to make the interfaces symmetrical though.

michaelgregorius · 2024-08-17T07:58:50Z

I will have to check if this can somehow be disentangled any further. However, currently it's a strange back and forth between Track and InstrumentTrack anyway, which is a code smell to me.

I am just a bit sad about the git-blame-pollution, and I think as the author, you would be the optimal person to have a clear view on my proposal. Ultimately, the scope of the PR is the author's decision, and I would open a further PR if you do not want to handle it here.

@JohannesLorenz, I don't know what you mean with "git-blame-pollution" but after some more thinking I guess it has finally clicked what you meant with your proposal. Commit 327aa9e removes the member m_presetMode, the method isPresetMode() and the class BoolGuard (no use of NoCopyNoMove though 😉).

Please refer to the commit message for more details and two potential considerations which are listed at the end of the message.

JohannesLorenz · 2024-08-17T13:30:10Z

no use of NoCopyNoMove though 😉

Noooooo 😭

Commit 327aa9e removes the member m_presetMode

I thought we could get around extending the virtual saveTrackSpecificSettings by a bool, and possibly because, for comparison, I looked at the code before your PR. The essential part of the PR - not saving the MIDI port - seems to require the parameter. It's a bit ugly because, by design, track types like SampleTrack or AutomationTrack can not have presets in general.

All in all, the PR is good. I still wonder, though, if the parameter presetMode of InstrumentTrack::saveTrackSpecificSettings is redundant to doc being of type DataFile of type DataFile::Type::InstrumentTrackSettings. That could save us the change to the virtuals, but I wonder if making that assumption will improve the code 🙈 Possibly not?

michaelgregorius · 2024-08-18T06:52:44Z

Commit 327aa9e removes the member m_presetMode

I thought we could get around extending the virtual saveTrackSpecificSettings by a bool, and possibly because, for comparison, I looked at the code before your PR. The essential part of the PR - not saving the MIDI port - seems to require the parameter. It's a bit ugly because, by design, track types like SampleTrack or AutomationTrack can not have presets in general.

If this was only about the essential part of the PR then we could have kept the fix with the old brittle implementation. In fact I will attempt to pull savePreset and loadPreset into Track itself. The thing is that it would reflect the fact that other types of tracks could have presets as well:

A sample track has volume, pan and an effect chain. The instrument track saves these things in a preset so why shouldn't it be possible to do the same for a sample track?
A pattern track could save the set of sub tracks with their instruments and patterns into a preset. Users could then for example save their drum sets this way and use them as starting points when creating new projects/tracks.
An automation track could save the automation clips without any associations to parameters and and when loaded these could later be associated by the users. There could for example be an option to drag and drop a parameter (control) onto the button of an automation track and this would be interpreted as "connect all clips to this parameter".

All in all, the PR is good. I still wonder, though, if the parameter presetMode of InstrumentTrack::saveTrackSpecificSettings is redundant to doc being of type DataFile of type DataFile::Type::InstrumentTrackSettings. That could save us the change to the virtuals, but I wonder if making that assumption will improve the code 🙈 Possibly not?

To me this implementation would sound rather brittle. That way the implementation of the InstrumentTrack::saveTrackSpecificSettings would have to look outside of its own scope to know how it should behave. It would have to check what its calling code has written into an XML element, i.e. how that outside code has behaved, to determine how it should behave itself. If the outside implementation changes this leads to potential breakage. Right now it gets told what to do and for me its a bit like the Hollywood principle ("Don't call us, we call you!") being applied in the small.

The current implementation also makes the code much easier to understand. Looking at the interface of saveTrackSpecificSettings one immediately sees that presets might have to be considered one way or the other. It makes the preset feature a first-class citizen in the code while the other implementation would almost seem as if it was trying to hide the fact that presets are supported by LMMS.

Move the methods `savePreset` and `loadPreset` into track because it potentially makes sense for all types of tracks to save a preset. Make `Track::saveTrack` and `Track::loadTrack` private because the public interface for saving and loading full tracks is `saveSettings` and `loadSettings` and for saving and loading presets it's `savePreset` and `loadPreset`.

michaelgregorius · 2024-08-18T07:33:59Z

With commit 4e2158b savePreset and loadPreset are now first-class citizens in Track.

JohannesLorenz · 2024-08-20T17:09:46Z

Ok from my side. Feel free to ask for more reviews/testing or merge.

michaelgregorius · 2024-08-20T17:52:17Z

Ok from my side. Feel free to ask for more reviews/testing or merge.

I have merged the changes. If any problems show up we either revert the changes or fix forward.

Ensure that no MIDI information (connected inputs, outputs, etc.) is stored in presets. This main fix can be found in `InstrumentTrack::saveTrackSpecificSettings` where the state of the MIDI ports are now only saved if we are not in preset mode. The remaining changes are concered with a refactoring of the code that's related to saving and loading presets. The refactoring mainly revolves around the removal of the member `m_simpleSerializingMode` and the method `setSimpleSerializing` in `Track`. This is accomplished by introducing two new methods `saveTrack` and `loadTrack`. These methods have a similar interface to `saveSettings` and `loadSettings` but they additionally contain a boolean which indicates if a preset is saved/loaded or a whole track. Both new methods contain the previous code of `saveSettings` and `loadSettings`. The latter two now only delegate to the new methods assuming that the full track is to be stored/loaded if called via the overridden methods `saveSettings` and `loadSettings`. The methods `savePreset` and `loadPreset` are added as well. They call `saveTrack` and `loadTrack` with the preset boolean set to `true`. These methods are now used by all places in the code where presets are saved or loaded which makes the code more readable. Clients also do not need to know any implementation details of `Track`, e.g. like having to call `setSimpleSerializing`. Adjust `saveTrackSpecificSettings` so that it also passes information of whether a preset or a whole track is stored. This leads to changes in the interfaces of `AutomationTrack`, `InstrumentTrack`, `PatternTrack` and `SampleTrack`. Only the implementation of `InstrumentTrack` uses the new information though.

michaelgregorius added 4 commits August 11, 2024 10:41

Fix wrong comment

b1134d1

Reduce private member accesses

3f8bef6

Reduce direct accesses to `m_simpleSerializingMode` by using the method `isSimpleSerializing` and extending `setSimpleSerializing` with a parameter.

Rename "SimpleSerializing"

00235c7

Rename "SimpleSerializing" to "PresetMode" everywhere because that is what this mode describes in fact.

michaelgregorius mentioned this pull request Aug 11, 2024

Remove the saving midi connection option completely. #7442

Closed

michaelgregorius requested review from zonkmachine and PhysSong August 11, 2024 09:03

Rossmaxx reviewed Aug 11, 2024

View reviewed changes

src/core/Track.cpp Outdated Show resolved Hide resolved

Minor code style fixup

9a45c97

zonkmachine approved these changes Aug 11, 2024

View reviewed changes

src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved

Code style

c822c10

JohannesLorenz added this to the 1.3 milestone Aug 11, 2024

JohannesLorenz approved these changes Aug 11, 2024

View reviewed changes

include/Track.h Outdated Show resolved Hide resolved

src/core/Track.cpp Outdated Show resolved Hide resolved

include/Track.h Outdated Show resolved Hide resolved

Code style and initialization

a1c276d

Make `isPresetMode` a one-liner and initialize `m_presetMode` in the header instead of in the constructor (list).

JohannesLorenz approved these changes Aug 16, 2024

View reviewed changes

src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved

src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved

src/core/Track.cpp Outdated Show resolved Hide resolved

src/core/Track.cpp Outdated Show resolved Hide resolved

JohannesLorenz approved these changes Aug 20, 2024

View reviewed changes

michaelgregorius merged commit 88ee83b into LMMS:master Aug 20, 2024
11 checks passed

michaelgregorius deleted the 3922-DoNotSaveMIDIConnectionsInPresets branch August 20, 2024 17:51

michaelgregorius mentioned this pull request Aug 20, 2024

Instrument presets saves MIDI settings #3922

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not save MIDI connections in presets #7445

Do not save MIDI connections in presets #7445

michaelgregorius commented Aug 11, 2024

michaelgregorius commented Aug 11, 2024

Rossmaxx left a comment

zonkmachine left a comment

JohannesLorenz left a comment

Rossmaxx commented Aug 12, 2024

michaelgregorius commented Aug 12, 2024

Rossmaxx commented Aug 12, 2024

michaelgregorius commented Aug 12, 2024

michaelgregorius commented Aug 14, 2024

Rossmaxx commented Aug 14, 2024 •

edited

Loading

michaelgregorius commented Aug 14, 2024

PhysSong commented Aug 16, 2024

michaelgregorius commented Aug 16, 2024

michaelgregorius commented Aug 16, 2024

JohannesLorenz commented Aug 16, 2024

JohannesLorenz left a comment

michaelgregorius commented Aug 17, 2024

JohannesLorenz commented Aug 17, 2024

michaelgregorius commented Aug 18, 2024

michaelgregorius commented Aug 18, 2024

JohannesLorenz commented Aug 20, 2024

michaelgregorius commented Aug 20, 2024

Do not save MIDI connections in presets #7445

Do not save MIDI connections in presets #7445

Conversation

michaelgregorius commented Aug 11, 2024

michaelgregorius commented Aug 11, 2024

Rossmaxx left a comment

Choose a reason for hiding this comment

zonkmachine left a comment

Choose a reason for hiding this comment

JohannesLorenz left a comment

Choose a reason for hiding this comment

Rossmaxx commented Aug 12, 2024

michaelgregorius commented Aug 12, 2024

Rossmaxx commented Aug 12, 2024

michaelgregorius commented Aug 12, 2024

michaelgregorius commented Aug 14, 2024

Rossmaxx commented Aug 14, 2024 • edited Loading

michaelgregorius commented Aug 14, 2024

PhysSong commented Aug 16, 2024

michaelgregorius commented Aug 16, 2024

michaelgregorius commented Aug 16, 2024

JohannesLorenz commented Aug 16, 2024

JohannesLorenz left a comment

Choose a reason for hiding this comment

michaelgregorius commented Aug 17, 2024

JohannesLorenz commented Aug 17, 2024

michaelgregorius commented Aug 18, 2024

michaelgregorius commented Aug 18, 2024

JohannesLorenz commented Aug 20, 2024

michaelgregorius commented Aug 20, 2024

Rossmaxx commented Aug 14, 2024 •

edited

Loading