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

[Merged by Bors] - fix re-adding a plugin to a plugin group #2039

Closed
wants to merge 4 commits into from

Conversation

mockersf
Copy link
Member

In a PluginGroupBuilder, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling finish. As the plugin is kept in an HashMap, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

@mockersf mockersf force-pushed the plugin-disable-readd branch from c110e48 to a2fa954 Compare April 28, 2021 22:44
@NathanSWard
Copy link
Contributor

NathanSWard commented Apr 28, 2021

I'm curious, is this the best behavior here?
With this implementation we silently succeed by simply removing the duplicated plugin, however, I would argue that having a duplicate plugin is incorrect and we should panic! and notify the user that they are attempting to add in an already existing plugin.

Edit: I could be wrong, and there could be a valid use case, I just can't personally think of one. 😅

@mockersf
Copy link
Member Author

mockersf commented Apr 29, 2021

The error can happen when re-adding a plugin with a new configuration.

To do that you would do:

App::build()
    .add_plugin_group_with(PluginGroup, |group| {
        group.disable::<PluginA>()
             .add_after::< PluginA, _>(PluginA {
                // custom values here
            })
    })

but that didn't work before this PR: it marked PluginA as disabled, then added a new entry in the ordered list and re-enabled it with the new values in the hash map, making it added twice when building the app. This could be worked around by adding a remove::<T: Plugin>() method but then you wouldn't be able to add your new plugin immediately after the previous one in the ordered list.

@mtsr
Copy link
Contributor

mtsr commented Apr 29, 2021

Maybe it could issue a warning when replacing a pluging that is not disabled?

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior core labels Apr 29, 2021
@alice-i-cecile
Copy link
Member

Maybe it could issue a warning when replacing a pluging that is not disabled?

I'm in favor of this behavior if possible.

@mockersf
Copy link
Member Author

added a warn log

@mockersf mockersf force-pushed the plugin-disable-readd branch from 2495686 to 891cef0 Compare May 10, 2021 23:48
@blaind blaind mentioned this pull request May 16, 2021
@mockersf mockersf force-pushed the plugin-disable-readd branch from 891cef0 to 3cb9b4f Compare June 30, 2021 20:45
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf force-pushed the plugin-disable-readd branch from 3cb9b4f to 405f5a3 Compare July 23, 2021 22:02
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@mockersf mockersf force-pushed the plugin-disable-readd branch 2 times, most recently from 5c6ab13 to 9cd8d0f Compare July 27, 2021 20:59
@mockersf mockersf force-pushed the plugin-disable-readd branch from 9cd8d0f to 5250fac Compare December 30, 2021 22:08
@mockersf mockersf force-pushed the plugin-disable-readd branch from 5250fac to 86368c4 Compare January 20, 2022 22:02
@mockersf mockersf force-pushed the plugin-disable-readd branch from 86368c4 to ead2b23 Compare April 25, 2022 23:19
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 3, 2022
@mockersf mockersf force-pushed the plugin-disable-readd branch from ead2b23 to ebe230e Compare May 6, 2022 22:38
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot pushed a commit that referenced this pull request May 9, 2022
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <[email protected]>
@bors bors bot changed the title fix re-adding a plugin to a plugin group [Merged by Bors] - fix re-adding a plugin to a plugin group May 9, 2022
@bors bors bot closed this May 9, 2022
robtfm pushed a commit to robtfm/bevy that referenced this pull request May 10, 2022
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <[email protected]>
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants