Bugzilla – Full Text Bug Listing
| Summary: | Number format pull down menu need to get choices from service or some component instead of hard coding the values | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Winnie Lai <wlai> | ||||
| Component: | cdt-debug-dsf | Assignee: | Pawel Piech <pawel.1.piech> | ||||
| Status: | RESOLVED FIXED | QA Contact: | Pawel Piech <pawel.1.piech> | ||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | cdtdoug, marc.khouzam | ||||
| Version: | 8.1.0 | ||||||
| Target Milestone: | 8.1.0 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Winnie Lai
I've actually implemented this feature in a patch for Bug 351479 and you've actually reviewed it too :-) I don't think I'm going to finish the implementation of bug 351479 in Juno (priority changes), but you could refactor my patch to pull out only the number format menu improvements. (In reply to comment #1) > I've actually implemented this feature in a patch for Bug 351479 and you've > actually reviewed it too :-) I don't think I'm going to finish the > implementation of bug 351479 in Juno (priority changes), but you could refactor > my patch to pull out only the number format menu improvements. The patch in 351479 is only good for single selection in pop up menu and rely on cache entry key. When I test it on MIExpressions, the 'Details' menu item shows up, which may be a good thing (or not). Also, as you mentioned, in the case of multiple selections, you want the pop up menu fall back to use the hard-coded default list. However, our requirement needs a different handling than your preference. In addition, we are looking for a way to get the list for pull down menu, instead of those hard-coded choices in the NumberFormatsContribution. Thus, I cannot use the cache entry key approach given in the patch of 351479. For now, I have only two approaches to address this bug. (1) Assume that no one changes the hard-coded format choices to include a non-common format, or to remove those agreed common formats. I can live with those hard-coded choices, and then append my additional choices and sub-menu by contributing through org.eclipse.ui.menu and referencing those menu and dynamic ids defined in org.eclipse.cdt.dsf.ui plugin.xml. I can do this for both pull down and pop up menu. In the case of pop up, a minor change is needed. In order to add my choices and sub menu just before the separator and the 'Restore to Prefernce' menu item, I will move the separator and Restore to Preference out from ElementNumberFormatsContribution to a separate contribution handler. This should not break anyone. (2) I add a new optional interface IVMFormatProvider and it is up to the corresponding VMProvider to implement it or not. The xxxFormatsContribution class checks the interface and if it is available, it gets the IContributionItem[] from IVMFormatProvider. If the inteface is not supported by the vm provider, it behavies as-is, that is, use hard-coded choices. In long term, I like (2) because it will still work even if someone breaks the assumption in (1) in the future, plus, it really gives the vm provider the full control. I just do not know what will happen to IContributionItem in e4 a couple years after - which may drive me to use (1). Do you have preference between (1) and (2)? (In reply to comment #2) I prefer #1 as I find that tying the VM api to the menu contribution API to be potentially limiting. E.g. would it still work if you wanted to put the number format menu in the toolbar? It might, but I'm not sure. However, I'm curious to understand better why solution from bug 351479 doesn't meet your requirements. It currently only implements the dynamic calculation for the context menu, but it could be extended to support the view menu as well. (In reply to comment #3) > (In reply to comment #2) > I prefer #1 as I find that tying the VM api to the menu contribution API to be > potentially limiting. E.g. would it still work if you wanted to put the number > format menu in the toolbar? It might, but I'm not sure. My understanding is that any IContributionItem in jface are for toolbar as well. I have code working that way. Without introducing e4, I would insist (1). Since e4 is our new trend, I prefer to learn more and foresee its future before I rush to define an API that uses legacy/3.x style (command) contribution. I know e4 team promises everything is backward compatible; however, if I can choose and afford, I would avoid legacy stuff. > However, I'm curious to understand better why solution from bug 351479 doesn't > meet your requirements. It currently only implements the dynamic calculation > for the context menu, but it could be extended to support the view menu as > well. I need some of the menu items which are not radio/check kind - It is a menu action that brings up a dialog. In addition, I need another submenu. This is what I think an API talks in the language of IContributionItem perfectly fits. I need all these for both pull down and pop up menu. I also spent quite a lot of time to figure out how cache entry can do something for pull down menu - I cannot come up with a good thing. (In reply to comment #4) > I need some of the menu items which are not radio/check kind... Fair enough, thanks for explaining. Created attachment 211012 [details] patch This patch takes the approach (1) as outlined in comment #2. Briefly, it pulls out the 'restore to preference' to a different class and makes correponding changes to plugin.xml. This split enables me to add my custom contributions before the separator and the 'restore to preference'. To add my custom contributions in view's pull down menu and in pop-up menu, I do it through org.eclipse.ui.menu by referencing the corresponding menu id and dynamic id and numberFormatSep. The 'hard-coded' choices are still common formats that must be agreed in general. If somoeone needs to add/change/remove the hard-coded choices, we need to revisit the approach (2) in comment #2. Pawel, Do you have time to review this patch for the coming milestone? (In reply to comment #7) > Pawel, > Do you have time to review this patch for the coming milestone? Yes, I should. Thanks! -Pawel I pushed the change out to master. Thanks Winnie. *** cdt git genie on behalf of Winndie Lai ***
Bug 371012 - Number format pull down menu need to get choices from service or some component instead of hard coding the values
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=4be0276d23c95deaaedb7ac41e57b8f8c322b7be
(In reply to Pawel Piech from comment #1) > I've actually implemented this feature in a patch for Bug 351479 and you've > actually reviewed it too :-) I don't think I'm going to finish the > implementation of bug 351479 in Juno (priority changes), but you could > refactor my patch to pull out only the number format menu improvements. As part of bug 439624, I will need to get the list of number formats from the services. I will therefore extract the number format changes implemented by Pawel in thte patch to bug 351479. I'm worried this may break the way Winnie/Patrick are using this class, so please let me know if I need to make additional changes for your dependencies. |