Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 336159 - user can see catalog branding in MPC wizard
Summary: user can see catalog branding in MPC wizard
Status: RESOLVED FIXED
Alias: None
Product: MPC
Classification: Technology
Component: wizard (show other bugs)
Version: unspecified   Edit
Hardware: PC Linux
: P3 enhancement (vote)
Target Milestone: 1.1   Edit
Assignee: Benjamin Muskalla CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-02 16:04 EST by Benjamin Muskalla CLA
Modified: 2011-03-16 14:55 EDT (History)
3 users (show)

See Also:


Attachments
patch (62.83 KB, patch)
2011-03-12 16:29 EST, Benjamin Muskalla CLA
no flags Details | Diff
mylyn/context/zip (462.19 KB, application/octet-stream)
2011-03-12 16:30 EST, Benjamin Muskalla CLA
no flags Details
patch (66.46 KB, patch)
2011-03-15 14:50 EDT, Benjamin Muskalla CLA
no flags Details | Diff
core patch (39.45 KB, patch)
2011-03-16 12:46 EDT, Benjamin Muskalla CLA
greensopinion: iplog+
Details | Diff
tests patch (9.96 KB, patch)
2011-03-16 12:46 EDT, Benjamin Muskalla CLA
greensopinion: iplog+
Details | Diff
ui (17.42 KB, patch)
2011-03-16 12:48 EDT, Benjamin Muskalla CLA
greensopinion: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Muskalla CLA 2011-02-02 16:04:33 EST
It should be possible for the markets to brand the MPC in different ways (eg.  banner area, naming of the wizard, ...)
Comment 1 Benjamin Muskalla CLA 2011-02-14 12:08:55 EST
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
Comment 2 Ian Skerrett CLA 2011-02-14 13:24:35 EST
(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?
Comment 3 Benjamin Muskalla CLA 2011-02-16 10:34:53 EST
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.
Comment 4 Ian Skerrett CLA 2011-02-16 15:52:03 EST
(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.
Comment 5 Benjamin Muskalla CLA 2011-02-17 06:48:46 EST
(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?
Comment 6 Ian Skerrett CLA 2011-02-17 11:19:48 EST
(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.
Comment 7 David Green CLA 2011-02-18 00:22:42 EST
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.
Comment 8 Ian Skerrett CLA 2011-02-18 16:12:54 EST
(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.
Comment 9 Benjamin Muskalla CLA 2011-02-23 15:35:45 EST
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.
Comment 10 Nathan Gervais CLA 2011-02-28 14:07:49 EST
As per Bug 328605 I've provided these fields in

http://marketplace.eclipse.org/catalogs/api/p

Please review and provide feedback.
Comment 11 Benjamin Muskalla CLA 2011-03-07 12:56:22 EST
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.
Comment 12 Nathan Gervais CLA 2011-03-07 13:08:52 EST
(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.
Comment 13 Benjamin Muskalla CLA 2011-03-08 13:22:19 EST
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.
Comment 14 Benjamin Muskalla CLA 2011-03-12 16:29:56 EST
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.
Comment 15 Benjamin Muskalla CLA 2011-03-12 16:30:06 EST
Created attachment 191060 [details]
mylyn/context/zip
Comment 16 Benjamin Muskalla CLA 2011-03-12 16:30:55 EST
Over to you Nathan.
Comment 17 David Green CLA 2011-03-14 13:06:30 EDT
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?
Comment 18 Benjamin Muskalla CLA 2011-03-15 14:48:42 EDT
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.
Comment 19 Benjamin Muskalla CLA 2011-03-15 14:50:05 EDT
Created attachment 191238 [details]
patch

Patches includes suggested refactoring (impl vs interface) and adapted the API changes Nathan has done (dependenciesRepository & icon)
Comment 20 Benjamin Muskalla CLA 2011-03-16 12:46:42 EDT
Created attachment 191315 [details]
core patch

.
Comment 21 Benjamin Muskalla CLA 2011-03-16 12:46:53 EDT
Created attachment 191316 [details]
tests patch

.
Comment 22 Benjamin Muskalla CLA 2011-03-16 12:48:29 EDT
Created attachment 191317 [details]
ui

Split the patch into three patches for better consumability
Comment 23 Benjamin Muskalla CLA 2011-03-16 13:05:10 EDT
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.
Comment 24 David Green CLA 2011-03-16 14:55:28 EDT
reopening so that I can give Benjamin credit
Comment 25 David Green CLA 2011-03-16 14:55:44 EDT
giving Benjamin credit