Community
Participate
Working Groups
While working on 476037, I noticed that despite the command icons show up as expected in the Shortcuts preference page, they don't appear in the quick access search. It seems to be the same blue ball for all commands. It seems like the part which populates quick access with commands simply doesn't try to load the images associated with commands.
The issue is in CommmandElement#getImageDescriptor(), which returns null.
New Gerrit change created: https://git.eclipse.org/r/79115
With the provided code, the Quick access commands are now using the associated command image. see: http://i.imgur.com/kPkD5UX.png
I believe it would be a nice candidate for 4.6.2.
Gerrit change https://git.eclipse.org/r/79115 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6663f9b077c9f23a0f9dc52a28e6380aeaa63842
Targettinf for 4.6.1. However, for 4.6.1, the change should include Brian's suggestion about retrieving the commandservice in a more reliable way.
As suggested, I'm going to add a method CommandProvider#getCommandImageService(), like happen for the other methods. private ICommandImageService commandImageService; ICommandImageService getCommandImageService() { if (commandImageService == null) { if (currentSnapshot instanceof ExpressionContext) { IEclipseContext ctx = ((ExpressionContext) currentSnapshot).eclipseContext; commandImageService = ctx.get(CommandImageService.class); } else { commandImageService = PlatformUI.getWorkbench().getService(CommandImageService.class); } } return commandImageService; } The problem now, is I always get null when calling getService(CommandImageService .class), despite I'm getting a service instance like for the other services. So, I get a non null service only when calling PlatformUI.getWorkbench().getService(CommandImageService.class); This is because the context is a child context, related to the current selection for the quickaccess. Can you give me an hint how to get the parent context, so I can get a non null instance of CommandImageService ? The only point against, is that the method, as written below, doesn't work. It works only If you return straight PlatformUI.getWorkbench().getService(CommandImageService.class);
correction / clarification: with the below code: if (currentSnapshot instanceof ExpressionContext) { // (#1) get the context - but service always null IEclipseContext ctx = ((ExpressionContext) currentSnapshot).eclipseContext; commandImageService = ctx.get(CommandImageService.class); } else { // (#2) the static way - but service has value commandImageService = PlatformUI.getWorkbench().getService(CommandImageService.class); } In (#1), I use the child context, but the service is always null. How to get the parent context, so the ctx.get(CommandImageService.class) has value?
New Gerrit change created: https://git.eclipse.org/r/79144
In the latest Gerrit change you can see the issue. I was tempted to write the following: if (currentSnapshot instanceof ExpressionContext) { // (#1) get the context - but service always null IEclipseContext ctx = ((ExpressionContext) currentSnapshot).eclipseContext; commandImageService = ctx.get(CommandImageService.class); } if(commandImageService==null){ // at first null it simply uses the static way commandImageService = PlatformUI.getWorkbench().getService(CommandImageService.class); } But again, this would be a workaround, instead of a complete solution
With the latest change, I removed the call to PlatformUI.getWorkbench(), in favor of passing an IEclipseContext to the constructor of CommandProvider. --> the CommandProvider is the only responsible to provide the service Hope you don't mind reviewing this change.
I'm also going to add a N&N Entry for this.
New Gerrit change created: https://git.eclipse.org/r/79229
Gerrit change https://git.eclipse.org/r/79144 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=235f36e62c9746c28c2393e85e7ef5b9065cc8b0
Do you think it's going to fit in 4.6.1 or should we move it to 4.6.2 ?
Thanks, Set to 4.7 M2
(In reply to Patrik Suzzi from comment #16) > Thanks, Set to 4.7 M2 Why not 4.6.1 or 4.6.2? The change seems purely internal and not risky.
Ok, I will set 4.6.1 again, and send an e-mail to eclipse-pmc to request the backport. Please, note: to get this fix into 4.6.x, we should backport two changes: - https://git.eclipse.org/r/#/c/79115/ : simple fix to provide images - https://git.eclipse.org/r/#/c/79144/ : fix also the CommandProvider Looking at #1, to backport to 4_6_maintenance in RC1, we need +1 from a PMC +1 from another committer, other than me [#1] https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_6_1.php
(In reply to Patrik Suzzi from comment #18) > Please, note: to get this fix into 4.6.x, we should backport two changes: > - https://git.eclipse.org/r/#/c/79115/ : simple fix to provide images > - https://git.eclipse.org/r/#/c/79144/ : fix also the CommandProvider I believe you can merge those into a single commit and put only one on Gerrit.
Yes, thanks, I'll mention this in the e-mail
New Gerrit change created: https://git.eclipse.org/r/79305
New Gerrit change created: https://git.eclipse.org/r/79304
Sorry, didn't squash the two cherry picks. I'm going to abandon the two above changes and submit a new one for R4_6_maintenance
New Gerrit change created: https://git.eclipse.org/r/79307
The last change submitted brings command icons in Neon's quickaccess. I already tested the change on Eclipse Neon on R4_6_maintenance stream, and it works as expected. Note: The combined change contains the code from Change 79115 + Change 79144. Asking Mickael to approval as another committer, other than me.
Brian, hope you don't mind reviewing this for backport
Gerrit change https://git.eclipse.org/r/79229 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=dd737522f0d1e6efd98c188dc2a3af64750ac3c7
The last commit was N&N for Oxygen M2.
As discussed with Brian, for consistency reasons, I'm going to change the current state of the master. After that, I will push another change to the 4_6_maintenance. Details: The code will use PlatformUI.getWorkbench() only when needed, consistently w.r.t. getters for other services provided by CommandProvider.
New Gerrit change created: https://git.eclipse.org/r/79377
Now both the changes have the same, consistent change: master: https://git.eclipse.org/r/#/c/79377/ r4_6_maintenance: https://git.eclipse.org/r/#/c/79307/
Gerrit change https://git.eclipse.org/r/79377 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8af0ffd49167f7af0b73f3264c4cd27ba0527fb5
The new, consistent change is in master now.
Gerrit change https://git.eclipse.org/r/79307 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2f1d5f81b862dede246e26c7e3f96ed3da49764e
The same, consistent change, is also in R4_6_maintenance (Neon) Thanks everyone for helping with this!
Brian, please set the +1 here in the bug report.
Verified in 4.6.1 RC2
