Community
Participate
Working Groups
I20100429-1549 * Create a plug-in in the workspace called "a.b.c" based on the "plug-in with a view" example * Create an Eclipse Application launch using "features selected below" on the Plug-ins tab * Select all features and add additional plug-in "a.b.c" * Select the "External" radio button for "Default plug-in location" * Apply * Close/re-open The additional plug-in is gone. Looks like the bundle is removed from the list since it does not exist externally.
Same problem if "external" is chosen in the column cell editor. The bundle should remain in the tree. It should likely be launched from the workspace if it does not exist externally.
Or perhaps when the user selects "external", we need to update the workspace only bundles to "workspace" in the cell editor (i.e. automatically)? It just seems strange that the bundle disappears since it was there when the user pressed apply.
Currently the logic in bundle launcher helper goes: Find the resolution (external/workspace) if it is default find the default resolution setting and use that If resolution is workspace, look for a workspace model If resolution is external, or it was workspace and couldn't find a workspace model, look for a target model. So in Darin's scenario the workspace plug-in never gets found since default resolution is external. I think this logic could be improved. The resolution is not intended to be 'this location only', it is supposed to be 'this location first'. We should fall back and look for a workspace model if we can't find an external model.
Created attachment 166731 [details] Patch The logic has been changed as follows 1. if a plug-in is missing in both workspace and external (target) then it is removed from the additional plug-ins list. 2. If a plug-in is marked as external or default (and default is external) AND it is missing in external (target) but present in workspace, then the workspace one is picked but the plug-in appears unchecked in the config.
I like the behaviour, but I feel the changes haven't gone far enough. Removing when the model is completely gone is the correct behaviour. Unchecking when it can't be resolved is a fair compromise. The problem is that we allow the user to set things that will not be used in the launch and will be removed the next time they open the LCD. I know time is tight for RC1, so if this isn't doable, I can live with the patch as is for 3.6. 3) Prevent the user from choosing an option that won't work. If the plug-in is only available in the workspace, the user shouldn't be able to set it to external. However, default should always remain available if (4) is implemented. 4) Have the default mean "external first" instead of "external only". Ankur, what issues do you think there would be with a change like this. If we can keep the impact minimal I think this is a good change. Alternatively, if we can't make these changes, perhaps we can put an error icon beside the resolution when it won't be allowed (unchecking it and removing the icon when the tab is reloaded).
Created attachment 167053 [details] Patch Logic and Labels updated to 'Workspace first' and 'External first'. Added red error image to the cell when the plug-in resolution is non-default and missing in that location.
Created attachment 167238 [details] Updated Fix This patch changes a number of things from the previous patch. 1) The behaviour has been changed so that we always do plug-in resolution the same way, we use the resolution setting as the preferred choice, but fall back to looking in other locations if that fails. In the previous patch this was happening in most places, but not when the user had an additional plug-in with a specific location. I changed this because it was confusing to have two different behaviours using the same wording. The addition of error images (which I changed to warnings as they don't cause problems anymore) immediately informs the user that they have chosen a resolution which may not make sense. 2) Changed the error icons to warnings. Removed 'first' from the default radio buttons as it just didn't read correctly. 3) Fixed some minor formatting issues a) Enclosing code in if statements instead of using if not then continue type statements b) using if statements without brackets c) using checked == true rather than just checked or !checked in if statements d) added javadoc comments to a method with a confusing argument.
Ankur, please review my updated patch. Ping me on sametime if you have any questions about the behaviour. The behaviour we discussed previously sounded reasonable, but actually working with it just didn't feel right.
Removing my review flag as Ankur needs to review the patch.
+1 Applied to HEAD
