Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn about LADSPA problems #7267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelgregorius
Copy link
Contributor

Introduce a method which checks if a file that's loaded has the LADSPA controls saved in an old format that was written with a version before commit e99efd5.

The method is run at the end so that problems in all file versions are detected. If a real upgrade was to be implemented it would have to run between DataFile::upgrade_0_4_0_rc2 and DataFile::upgrade_1_0_99.

See #5738 for more details.

Introduce a method which checks if a file that's loaded has the LADSPA
controls saved in an old format that was written with a version before
commit e99efd5.

The method is run at the end so that problems in all file versions are
detected. If a real upgrade was to be implemented it would have to run
between `DataFile::upgrade_0_4_0_rc2` and `DataFile::upgrade_1_0_99`.

See LMMS#5738 for more details.
@zonkmachine
Copy link
Member

zonkmachine commented May 18, 2024

Nice! Maybe the pop-up message should mention that you can try and save the project on a previous version of lmms like 1.1 or 1.2 ? At least it seem to have worked for Greshz-CoolSnip.

@tresf
Copy link
Member

tresf commented May 20, 2024

@michaelgregorius thanks for this warning. Some initial thoughts:

  • Like other plugin-specific warnings, we've tried to move towards context-aware. If a hint as to the plugin name can be provided in the error, this would be much appreciated.
  • // This is not an upgrade. Can it be a downgrade then? Put simpler, can we disable/mute this plugin as a courtesy to the user?

From what I understand, this PR can be close (or if merged first, reverted) if we find a way to patch this behavior, which last I remember was held up in #5738 awaiting some feedback from @JohannesLorenz.

@michaelgregorius
Copy link
Contributor Author

Hi @tresf,

* Like other plugin-specific warnings, we've tried to move towards context-aware.  If a hint as to the plugin name can be provided in the error, this would be much appreciated.

In this case the routine would have to collect a list of the names of all affected plugins and show them in the message. This should be feasible.

* `// This is not an upgrade`.  Can it be a downgrade then?  Put simpler, can we disable/mute this plugin as a courtesy to the user?

The comment was rather meant as a hint in that it is not a classical upgrade routine which moves data around but rather something that detects problems and warns the users about them.

Muting the affected plugins could also be an option as it should be similar to the collection of the names as in that we'd have to find the parent that can be muted instead of finding the parent with the name. In that case a combination might be nice where the error message states names of the affected plugins that have been muted.

From what I understand, this PR can be close (or if merged first, reverted) if we find a way to patch this behavior, which last I remember was held up in #5738 awaiting some feedback from @JohannesLorenz.

Exactly! Fixing this problem for good would be the better option than showing warning messages to the users. The latter might undermine the users trust in the application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Demo project Greshz-CoolSnip buggy in master
3 participants