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

Disallow adding a destructive, or realtime, effect which has been deleted since application start up. #3801

Merged

Conversation

ksoze95
Copy link
Contributor

@ksoze95 ksoze95 commented Oct 12, 2022

Resolves: #3719 #3817

(short description of the changes and the motivation to make the changes)

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

@ksoze95 ksoze95 self-assigned this Oct 12, 2022
Copy link
Collaborator

@vsverchinsky vsverchinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dialog message text needs translation

src/RealtimeEffectPanel.cpp Outdated Show resolved Hide resolved
src/effects/EffectUI.cpp Outdated Show resolved Hide resolved
@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch from d90a5b4 to 7e6dfad Compare October 14, 2022 14:15
@ksoze95 ksoze95 added this to the Audacity 3.3 milestone Oct 14, 2022
@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch from 7e6dfad to a8777ea Compare October 14, 2022 14:23
return;

if(!PluginManager::IsPluginAvailable(*plug)) {
wxMessageBox(_("This plugin could not be loaded.\nIt may have been deleted."), _("Plugin Error"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ::AudacityMessageBox

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced wxMessageBox with BasicUI::ShowMessageBox as suggested below.

@@ -1231,6 +1231,12 @@ DialogFactoryResults EffectUI::DialogFactory(wxWindow &parent,
if (!plug)
return false;

if (!PluginManager::IsPluginAvailable(*plug))
{
wxMessageBox(_("This plugin could not be loaded.\nIt may have been deleted."), _("Plugin Error"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ::AudacityMessageBox

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better, use BasicUI::ShowMessageBox in both places.

@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch from a8777ea to 7d8ba28 Compare October 18, 2022 12:29
@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch from 7d8ba28 to 13bd133 Compare October 19, 2022 09:40
@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch from 13bd133 to ac75f13 Compare October 21, 2022 11:23
@ksoze95 ksoze95 linked an issue Oct 26, 2022 that may be closed by this pull request
Comment on lines 219 to 222
BasicUI::ShowMessageBox(
XO("This project contains some plugins that cannot be found on the system."),
BasicUI::MessageBoxOptions()
.Caption(XO("Project Error")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do

Window title: Missing Plugins
This project contains some realtime effect plugins that cannot be found on this system.
The project may sound different than intended. Learn more
[OK]

with the "learn more" having an accessible link to https://audacityteam.org/errors#missing-plugins

@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch 2 times, most recently from bfd6cbf to 5300bae Compare November 15, 2022 11:57
@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch from 5300bae to 34f95a5 Compare November 15, 2022 13:04
Copy link
Collaborator

@Paul-Licameli Paul-Licameli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions to improve it, but nothing that should block it.

@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch from 34f95a5 to 1cafa45 Compare December 16, 2022 12:54
@ksoze95 ksoze95 force-pushed the use_deleted_vst_plugin_before_rescan_crash branch from 1cafa45 to bdfd848 Compare December 19, 2022 12:36
@ksoze95 ksoze95 merged commit 6081b48 into audacity:master Dec 19, 2022
@ksoze95 ksoze95 deleted the use_deleted_vst_plugin_before_rescan_crash branch December 19, 2022 13:32
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.

Warn of missing real-time effects Crash when trying to use a deleted VST plugin before a Rescan
4 participants