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.

Sign up for GitHub

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

Jump to bottom

Added Rescan option to the PluginRegistrationDialog. #3359

Merged
merged 1 commit into from Sep 7, 2022
Merged

Added Rescan option to the PluginRegistrationDialog. #3359

merged 1 commit into from Sep 7, 2022

Conversation

ksoze95
Copy link
Contributor

@ksoze95 ksoze95 commented Aug 9, 2022

Resolves: #3279

(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 Aug 9, 2022
@ksoze95 ksoze95 force-pushed the plugin_manager_manual_rescan branch from 0c8c66d to 404ddb0 Compare August 9, 2022 16:34
@LWinterberg LWinterberg mentioned this pull request Aug 10, 2022
Closed
3 tasks
@ksoze95 ksoze95 force-pushed the plugin_manager_manual_rescan branch from 404ddb0 to 4db8173 Compare August 10, 2022 09:19
Paul-Licameli
Paul-Licameli suggested changes Aug 10, 2022
View reviewed changes
libraries/lib-module-manager/PluginManager.cpp
if (desc.GetPluginType() == PluginTypeEffect)
{
mLoadedInterfaces.erase(desc.GetID());
it = mRegisteredPlugins.erase(it);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Incorrect loop. Increment the iterator or erase it, not both. Or else you skip some elements. May crash if you erase the last element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Moved iterator increment to the body of the for loop, and when not erasing.

vsverchinsky
vsverchinsky reviewed Aug 10, 2022
View reviewed changes
libraries/lib-module-manager/PluginManager.cpp Outdated
for (auto it = mRegisteredPlugins.cbegin(); it != mRegisteredPlugins.cend(); ++it)
{
auto& desc = it->second;
if (desc.GetPluginType() == PluginTypeEffect)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you need to erase plugins with type PluginTypeStub as well

libraries/lib-module-manager/PluginManager.cpp Outdated
auto& desc = it->second;
if (desc.GetPluginType() == PluginTypeEffect)
{
mLoadedInterfaces.erase(desc.GetID());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably I was wrong when I said that effect should be erased from the mLoadedInterfaces. It may cause problems with effects being used in the project since they are unique ptrs. Removing description of the loaded plugin isn't good thing as well because there are code pathes that request plugin information from the PluginManager. Placing PluginDescription of the loaded plugins into special container in PluginManager::ClearEffectPlugins so that later calls to PluginManager::GetPlugin... could work

@ksoze95 ksoze95 force-pushed the plugin_manager_manual_rescan branch 2 times, most recently from 0d50047 to bd99a62 Compare August 11, 2022 06:30
@ksoze95 ksoze95 changed the base branch from master to alpha-3.2.0 September 7, 2022 09:33
@ksoze95 ksoze95 merged commit c39994d into audacity:alpha-3.2.0 Sep 7, 2022
@ksoze95 ksoze95 deleted the plugin_manager_manual_rescan branch September 7, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsverchinsky vsverchinsky vsverchinsky approved these changes

@Paul-Licameli Paul-Licameli Awaiting requested review from Paul-Licameli

Assignees

@ksoze95 ksoze95

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Manual rescan is required to reveal some plugins in the Plugin Manager
3 participants
@ksoze95 @Paul-Licameli @vsverchinsky