Bugzilla – Full Text Bug Listing
| Summary: | user can see catalog branding in MPC wizard | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Technology] MPC | Reporter: | Benjamin Muskalla <b.muskalla> | ||||||||||||||
| Component: | wizard | Assignee: | Benjamin Muskalla <b.muskalla> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P3 | CC: | greensopinion, ian.skerrett, nathan | ||||||||||||||
| Version: | unspecified | ||||||||||||||||
| Target Milestone: | 1.1 | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Linux | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Benjamin Muskalla
We need to clarify which attributes should be allowed to brand the wizard. These attributes need to be reflected in the server-side API (task 328605) as in the extension point. Current items that I see that could be marketplace specific: * Wizard shell title (Eclipse Marketplace -> Marketplace XY) * Wizard image in the top right corner (In reply to comment #1) > We need to clarify which attributes should be allowed to brand the wizard. > These attributes need to be reflected in the server-side API (task 328605) as > in the extension point. > > Current items that I see that could be marketplace specific: > * Wizard shell title (Eclipse Marketplace -> Marketplace XY) > * Wizard image in the top right corner can we provide an option to change the background colors of the wizard? Can we also allow third party catalogs to change the names of the tabs? Changing the name of the tabs depending on the marketplace is no problem. The color of the wizard is harder as SWT and JFace put some restrictions on us that certain areas in the wizard cannot be coloured. I'd also be very careful with such an approach to not undermine the Eclipse UI guidelines. For example: Guideline 1.1 Follow and apply good user interface design principles: user in control, directness, consistency, forgiveness, feedback, aesthetics, and simplicity. (In reply to comment #3) > Changing the name of the tabs depending on the marketplace is no problem. How about we let them define the # of tabs and the names. > The color of the wizard is harder as SWT and JFace put some restrictions on us > that certain areas in the wizard cannot be coloured. > I'd also be very careful with such an approach to not undermine the Eclipse UI > guidelines. For example: > OK, so lets not let them change colour. (In reply to comment #4) > How about we let them define the # of tabs and the names. Makes sense to me. Rather then defining the number, I think we should give them the option to enable/disable the existing tabs. In my eyes, this is the list of attributes for each marketplace we need to incorporate into the REST API and the extension point: - wizardTitle[String] - the title of the wizard - wizardImage[URL] - the image used in the wizard (75x66px), see also http://wiki.eclipse.org/User_Interface_Guidelines#Graphic_Types -needsSearchTab[boolean] - indicating if the marketplace provides recent listings -needsRecentTab[boolean] - indicating if the marketplace provides recent listings -needsPopularTab[boolean] - indicating if the marketplace provides popular listings -searchTabTitle[String] - title of the Search tab -recentTabTitle[String] - title of the recent tab -popularTabTitle[String] - title of the popular tab Remaining question is whether we want to let marketplaces disable the search tab at all. Ian, David, how does this suggestion sound to you? (In reply to comment #5) > (In reply to comment #4) > > How about we let them define the # of tabs and the names. > Makes sense to me. Rather then defining the number, I think we should give them > the option to enable/disable the existing tabs. > > In my eyes, this is the list of attributes for each marketplace we need to > incorporate into the REST API and the extension point: > - wizardTitle[String] - the title of the wizard > - wizardImage[URL] - the image used in the wizard (75x66px), see also > http://wiki.eclipse.org/User_Interface_Guidelines#Graphic_Types > -needsSearchTab[boolean] - indicating if the marketplace provides recent > listings > -needsRecentTab[boolean] - indicating if the marketplace provides recent > listings > -needsPopularTab[boolean] - indicating if the marketplace provides popular > listings > -searchTabTitle[String] - title of the Search tab > -recentTabTitle[String] - title of the recent tab > -popularTabTitle[String] - title of the popular tab > > Remaining question is whether we want to let marketplaces disable the search > tab at all. > > Ian, David, how does this suggestion sound to you? Sounds good to me. I would also agree with providing the option to disable the search. btw, Nathan's wife just had a baby so he won't be able to get to this for 1-2 weeks. Looks good to me. I see that you've excluded the 'installed' tab from those that can be disabled. It makes sense to me -- just wanted to be sure that was intentional. (In reply to comment #7) > Looks good to me. I see that you've excluded the 'installed' tab from those > that can be disabled. It makes sense to me -- just wanted to be sure that was > intentional. +1 for not excluding the installed tab. The idea was that the marketplace should only be able to control what comes from the marketplace (featured, popuplar, ..) while "installed" is something specific to the user, not the marketplace. That's why I initially suggested to not let the marketplace mess around with that. As per Bug 328605 I've provided these fields in http://marketplace.eclipse.org/catalogs/api/p Please review and provide feedback. Nathan, one change that would allow is to simplify the parsing of the xml: * instead of having a description and icon item, this could be an attribute on <catalog> Having the branding aspect as sub-tag is ok. (In reply to comment #11) > Nathan, one change that would allow is to simplify the parsing of the xml: > * instead of having a description and icon item, this could be an attribute on > <catalog> > > Having the branding aspect as sub-tag is ok. I made two assumptions with the description field. - generally a long field - could support <html> tags I'll move the icon into an attribute tho. Long field yes, but we do not support html tags in the tooltip - see task 336157 Just leave the description field as is. Having the icon as attribute would be great. Created attachment 191059 [details] patch Here is a patch to integrate remote catalogs and their branding into MPC. David, please do not commit until we get the go from Nathan Nathan, as discussed on the mailing list and on task 332073 and task 328605, there are still two blocking items before we can commit this change * add dependencyRepository to the metadata - currently the Eclipse marketplace can use the helios repository, will keep you updated on task 332073 to use a composite repository * provide proper metadata for the Eclipse Marketplace and Yoxos. As discussed with Ian, remote catalogs will override local information and it's critical that we have proper metadata in place. If we'd apply the changes right now, this would mean that users would see rabbits in the MPC due to the test metadata. See http://dev.eclipse.org/mhonarc/lists/mpc-dev/msg00145.html for the informations that need to be returned. For the branding, I'd go with providing the default settings as currently returned. Created attachment 191060 [details]
mylyn/context/zip
Over to you Nathan. Benjamin, the patch looks good. A couple of things: * is there a reason why CatalogService is separate from MarketplaceService? (Why weren't the new methods just added to MarketplaceService?) * if CatalogService should remain separate, please split into interface and implementation (the same way as we have for MarketplaceService) * Why does the wizard command use a separate dialog for downloading catalogs instead of just running in the wizard? David, the intention of the marketplace service is to query information from a specific marketplace. I didn't want to mix it up with methods to query for *marketplaces*. Mixing them would mean that a query for marketplaces would return pointers to itself. Makes sense to split into interface and implementation, done. Using a progress monitor before the actual wizard comes up makes sense in terms of having the catalogs already populated when the wizard is shown. If we don't do this (and have no more local marketplace informations), we'd need to open up an empty wizard, letting the user wait for it to fill (with the new UI, we would show an empty icon list). This may also require several refactorings in the MPC wizard. Could be done in a separate refactoring if needed. Created attachment 191238 [details]
patch
Patches includes suggested refactoring (impl vs interface) and adapted the API changes Nathan has done (dependenciesRepository & icon)
Created attachment 191315 [details]
core patch
.
Created attachment 191316 [details]
tests patch
.
Created attachment 191317 [details]
ui
Split the patch into three patches for better consumability
We're done here. The whole retrieve remote catalogs and branding story is implement for M6. Will open new tasks for remaining bugs or feature enhancements. reopening so that I can give Benjamin credit giving Benjamin credit |