Added a button to show launch bundles #1922
Conversation
Test Results 752 files - 13 752 suites - 13 52m 3s ⏱️ - 1m 57s For more details on these failures and errors, see this check. Results for commit 0f5e757. ± Comparison against base commit 9213517. ♻️ This comment has been updated with latest results. |
|
@laeubi kindly review this PR |
| PluginsTabToolBar_validate=&Validate {0} | ||
| PluginsTabToolBar_validate_plugins=&Validate Plug-ins | ||
| PluginsTabToolBar_validate_bundles=&Validate Bundles | ||
| PluginsTabToolBar_show_launch_bundles=&Show launch bundles |
There was a problem hiding this comment.
your screen shot is however showing "Show plug-ins", how is that possible? was this modified later?
There was a problem hiding this comment.
ohh yes I renamed it later. Thanks for pointing that out
I will update the screenshot now
There was a problem hiding this comment.
Thanks for this proposal.
I have a few remarks below, but there are two more points:
- The button should also be available for Eclipse-apps based on features (at least personally I'm hardly using bundle-based apps). This can be done easily by repeating the same code of the
AbstractPluginBlockin theFeatureBlock, around here:
Ideally features would then also be listed (in addition to bundles), but I assume the currently used dialog is not capable of that. - While using the
PluginSelectionDialogis certainly a quick solution, I'm not sure if it's really suitable. At least the title of the dialog should be changed to something more meaningful in this use-case.
What I'm currently missing is the ability to copy the list of plugins (ideally just the selected sub-set) and being able to past it e.g. into a text- or diff-editor. This way I often try to figure what has changed in the full application. But that's not working at the moment.
For #1850 I initially thought about a separate tab or something like that.
Maybe we could instead also re-use the selection tree in the center and just have a check-box on the right to list all included (features and) bundles. Of course selecting them should then not be possible.
But yes that would probably be more work and I'm not sure if that's overall much better in the end. It's currently just an idea.
| private Label fCounter; | ||
|
|
||
| private Button fValidateButton; | ||
| private Button fShowPlugin; |
There was a problem hiding this comment.
As the button is used in only one method, this field is not necessary and the button can just be a local variable.
| private Button fShowPlugin; |
| fShowPlugin.addSelectionListener(new SelectionAdapter() { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent evt) { | ||
| handleShowPluginsPressed(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
| fShowPlugin.addSelectionListener(new SelectionAdapter() { | |
| @Override | |
| public void widgetSelected(SelectionEvent evt) { | |
| handleShowPluginsPressed(); | |
| } | |
| }); | |
| fShowPlugin.addSelectionListener( | |
| SelectionListener.widgetSelectedAdapter(e -> handleShowPluginsPressed(fLaunchConfig))); |
| } | ||
|
|
||
| // Dialog to Show the launch bundles | ||
| protected void handleShowPluginsPressed() { |
There was a problem hiding this comment.
This method should be static and have the launch config as parameter so that it can be called from the FeatureBlock too.
| protected void handleShowPluginsPressed() { | |
| static void handleShowPluginsPressed(ILaunchConfiguration launchConfig) { |
| models.toArray(new IPluginModelBase[0]), true) { | ||
| @Override | ||
| protected void createButtonsForButtonBar(Composite parent) { | ||
| createButton(parent, IDialogConstants.CANCEL_ID, IDialogConstants.CANCEL_LABEL, false); |
There was a problem hiding this comment.
The close label is more suitable here.
| createButton(parent, IDialogConstants.CANCEL_ID, IDialogConstants.CANCEL_LABEL, false); | |
| createButton(parent, IDialogConstants.CANCEL_ID, IDialogConstants.CLOSE_LABEL, false); |
| try { | ||
| models = BundleLauncherHelper.getMergedBundleMap(fLaunchConfig, false).keySet(); | ||
| PluginSelectionDialog dialog = new PluginSelectionDialog(PDEPlugin.getActiveWorkbenchShell(), | ||
| models.toArray(new IPluginModelBase[0]), true) { |
There was a problem hiding this comment.
| models.toArray(new IPluginModelBase[0]), true) { | |
| models.toArray(IPluginModelBase[]::new), true) { |
| @Override | ||
| protected void handleDoubleClick() { | ||
| } |
There was a problem hiding this comment.
A comment would clarify the situation here:
| @Override | |
| protected void handleDoubleClick() { | |
| } | |
| @Override | |
| protected void handleDoubleClick() { | |
| // disable super-class behavior | |
| } |
thanks @HannesWell . I have these questions:
In case we add a new separate tab:
|
nburnwal09
force-pushed
the
show_bundles
branch
from
4897456 to
ad6c8db
Compare
I added the same for Eclipse-apps based on features. Also, with current changes, if we select one/more features and click on 'show launch bundles', it is showing the list of plug-ins(direct and transitive dependent plug-ins both) respective to the features selected.
Yeah I missed that. I have changed the title to 'Launch Plug-ins list'
Yeah I have that in my mind(based on our conversation in #1850 (comment)). I will be taking the copy/paste option in another PR. |
nburnwal09
force-pushed
the
show_bundles
branch
from
ad6c8db to
cc39b5b
Compare
Features are exploded and do not remain in the final runtime, so I would say this is not really useful here. One might want to show what a feature pulls in but then we need a tree to represent the data and it would possibly complicate things a lot. So I would vote for starting simple and improve on that iteratively. |
With my changes, we will still be able to see what a feature pulls in(the direct and indirect dependent plug-in).
|
|
While this can be interesting I don't think the selection should matter and would expect to always see the final result of what will happen when I click |
|
Hi @HannesWell |
|
Hi @HannesWell |
nburnwal09
force-pushed
the
show_bundles
branch
from
cc39b5b to
0f5e757
Compare
|
@HannesWell - do you still have outstanding suggestions on this? |
Sorry for the delay. I'll look into the new state tomorrow. |
Good question. I cannot answer this clearly, but I would say that it would allow to support more scenarios or 'tools' (later). But to come back to the current proposal and as my counter-suggestion from #1922 (review), is indeed much more complex, what about re-using the approach used for the If we use that to show the list of resolved bundles in CSV style (if we want to have more then the name) and also add a search field to for the text, then we would have, what would be provided with your proposal plus the ability to copy (parts) of the list. To feed the content of that text field you could actually use what's generated into the Overall I think it should still be not too complicated to achieve and should provide more information than today. What do you think? |
Sure Hannes, that sounds better given that we will have selected text block/field. I will check this. |
Added a button to show launch bundles: #1850
It has filter option and shows number of bundles selected(attached screenshot)