Bugzilla – Full Text Bug Listing
| Summary: | [Workbench] WorkbenchLabelProvider and IWorkbenchAdapter doesn't support StyledString | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Hemant Singh <Hemant.Singh> | ||||||||||||||
| Component: | UI | Assignee: | Hemant Singh <Hemant.Singh> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Paul Webster <pwebster> | ||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P3 | CC: | bokowski, daniel_megert, francisu, remy.suen, thatnitind | ||||||||||||||
| Version: | 3.6 | ||||||||||||||||
| Target Milestone: | 3.7 M5 | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Hemant Singh
Hemant, would you be able to suggest how to do this by attaching a patch? http://wiki.eclipse.org/Platform_UI/How_to_Contribute (In reply to comment #1) > Hemant, would you be able to suggest how to do this by attaching a patch? > http://wiki.eclipse.org/Platform_UI/How_to_Contribute Hello Boris, Thanks for the getting-started link, it is helpful. I'll try to create a solution for this issue over the weekend and get back to you. Created attachment 181047 [details]
Proposed solution for the defect 326695
Attached the patch as proposed solution for this defect.
The following are some points which explains the changes:
1) Added new interface IWorkbenchAdapter3 which contains method getStyledText, just like interface IStyledLabelProvider.
2) Modified WorkbenchAdapter to implement IWorkbenchAdapter3, the default implementation method, simply wraps the existing label value in StyledString.
3) Modified the WorkbenchLabelProvider to implement IStyledLabelProvider, and implementation method simply check if the WorkbenchAdapter class implements IWorkbenchAdapter3, if yes, it returns the StyledString from it, else it wraps the label as StyledString. The changes in this class are good to have, and not necessary, because most of the third party plug-ins simply adapt the element and make attempt to getLabel from it, now they can get even StyledString, if they want.
I'll be happy to make further changes in this, if you have any suggestion or comment.
Created attachment 181084 [details]
IWA3 v02
Thanx Hemant,
This patch just fixes some complaints against the 3.6 baseline (missing @since tags, etc).
General Comments on your patch: you need to add your information to the "Contributors" section of the copyright notice at the top where you've modified files. Also, make sure the copyright notice of IWorkbenchAdapter3 (new file) reflects your information (if you work for IBM, you're set :-)
IWorkbenchAdapter3: add an @see for the styled text label provider that can consume this.
org.eclipse.ui.model.WorkbenchAdapter.getStyledText(Object): add the @return, specify if null is allowed.
minor niggle: org.eclipse.ui.model.WorkbenchLabelProvider.getStyledText(Object) you already have txt you shouldn't do another getText(*) in "return new StyledString(getText(element));"
PW
Created attachment 181118 [details]
IWA3 v03
Thank you Paul for the detailed review.
The attached patch accommodates all your suggestions on your previous patch(IWA3 v02).
Indeed, I was unaware, with some of the process here, like as a contributor, we get to put our names in the files :).
Also, good catch in WorkbenchLabelProvider.getStyledText, where the code was calling getText twice.
Let me know, if you have any further suggestions or comments.
Francis, could you please comment on the patch? Hemant, just out of curiousity, what are some of the 3rd party views that you would expect to see your changes in? Subclasses of CNF other than the Project Explorer? PW (In reply to comment #6) > Hemant, just out of curiousity, what are some of the 3rd party views that you > would expect to see your changes in? Subclasses of CNF other than the Project > Explorer? > > PW Paul, Third party view in this defect context can be any view which gets the object ref, from either say selection or any API call, and tries to get details like name, or icon about the selected object using WorkbenchAdapter class, by adding IWorkbenchAdapter3, that view will have option to get StyledString instead of plain text base name, because most of the time, name will be shown somewhere in third party view UI. For e.g. if you have project explorer which let you browse Database structure like database, table, etc. and a third party view uses the selection and other API’s provided by this plug-in to add support for say, searching in database tables or across different databases. Then in the search results the third party view will be able to show the table or column name using StyledString, instead of plain text, if the original plug-in made use of IWorkbenchAdapter3. Does it make sense? The API per se makes perfect sense to me. The code itself has a bug though: it does not decorating the styled elements anymore which means lost functionality as soon as someone switches to calling the new API along with a new adapter. The question to me is how/where it is used: existing views like Search or Project Explorer already use styled strings to render decorations or emphasize the match inside the line. In the past we tried to minimize a color explosion by only using a very small set of colors with a well defined purpose. This is also important for for accessibility. Therefore, I think it's fine for custom views and RCPs to make (careful) use of the new feature but the SDK views should not do this. Also, the label provider is used at many places inside the SDK and if some views start to use the new API, then we either end up with inconsistencies in the different views and dialogs or we have to invest quite some work to change all usages. BTW: the version is set to 4.1 - is this on purpose? (In reply to comment #8) > The API per se makes perfect sense to me. The code itself has a bug though: it > does not decorating the styled elements anymore which means lost functionality > as soon as someone switches to calling the new API along with a new adapter. > > The question to me is how/where it is used: existing views like Search or > Project Explorer already use styled strings to render decorations or emphasize > the match inside the line. In the past we tried to minimize a color explosion > by only using a very small set of colors with a well defined purpose. This is > also important for for accessibility. Therefore, I think it's fine for custom > views and RCPs to make (careful) use of the new feature but the SDK views > should not do this. Also, the label provider is used at many places inside the > SDK and if some views start to use the new API, then we either end up with > inconsistencies in the different views and dialogs or we have to invest quite > some work to change all usages. Hello Dani, Can you elaborate on the bug in implementation here? Also, regarding your second question/comment. I think, this whole implementation is required, because the platform as of now, have half support of StyledString, as label providers have support of StyledString(IStyledLabelProvider), but not work workbench adapters, and since most label provider implementation like WorkbenchLabelProvider, simply adapts the element to IWorkbenchAdapter, and request the label from there, it becomes quite tough to support label with StyledString, isn't? >Can you elaborate on the bug in implementation here? The current implementation decorates the element label by calling WorkbenchLabelProvider.decorateText(String, Object) which subclasses can override. If one decides to use the new styled API then elements which have a styled text won't get decorated. >Also, regarding your second question/comment. I think, this whole >implementation is required, Sure, please read my first sentence in comment 8 again. However, I would not want the existing Eclipse SDK views to use this. (In reply to comment #11) > The current implementation decorates the element label by calling > WorkbenchLabelProvider.decorateText(String, Object) which subclasses can > override. If one decides to use the new styled API then elements which have a > styled text won't get decorated. Dani, Do you have any suggestion on how to fix this issue? I see two possible options here a) Declare new method decorateStyledString, and the sub-classes will have the responsibility of overriding this method, just like decorateText. b) Do nothing, but we rather modify javadoc of WorkbenchLabelProvider.getStyledText to mention that it will not implement decoration on text, if workbench adapter returns StyledString. > > Sure, please read my first sentence in comment 8 again. However, I would not > want the existing Eclipse SDK views to use this. Agree with you here, I wouldn't want the SDK views to be polluted with this new API as well. b) is not an option for me and a) is not working well because you might have situations where not all elements implement the new adapter. This would result in inconsistent UI, for example some elements might have a path appended and some won't. We should 1. Add decorateText(StyledString, Object) This allows subclasses to use styles in the decoration. We should call that method even if the element itself does not implement the new adapter. 2. By default this new method would decorate the given styled string using decorateText(...). The result should be the styled text from the element plus the un-styled decoration. (In reply to comment #13) > b) is not an option for me and a) is not working well because you might have > situations where not all elements implement the new adapter. This would result > in inconsistent UI, for example some elements might have a path appended and > some won't. > > We should > 1. Add decorateText(StyledString, Object) > This allows subclasses to use styles in the decoration. We should call > that method even if the element itself does not implement the new adapter. Okay. We can certainly call this method when element implement new adapter, but, if the element in getStyledText doesn't implement new adapter, than we are using getText which itself calls decorateText(String, Object). Maybe, I am missing the point here. > 2. By default this new method would decorate the given styled string using > decorateText(...). The result should be the styled text from the element > plus the un-styled decoration. Actually, I am not sure how we can re-use the existing implementations of decorateText(String, Object), because this API can change the text anywhere, no necessary prefix or suffix any additional text, So, I don't know how we can mix StyledString from new workbench adapter with this API, again, maybe, I am missing something here, will really appreciate, if you can correct myself here. Thanks (In reply to comment #14) > (In reply to comment #13) > > b) is not an option for me and a) is not working well because you might have > > situations where not all elements implement the new adapter. This would result > > in inconsistent UI, for example some elements might have a path appended and > > some won't. > > > > We should > > 1. Add decorateText(StyledString, Object) > > This allows subclasses to use styles in the decoration. We should call > > that method even if the element itself does not implement the new adapter. > Okay. We can certainly call this method when element implement new adapter, > but, if the element in getStyledText doesn't implement new adapter, than we are > using getText which itself calls decorateText(String, Object). Maybe, I am > missing the point here. The point is that since we add support for styled strings (returned by the adapter) we should add the same level of support for the subclasses that decorate. Basically you can end up with 4 cases: 1. nothing is styled 2. element implements new adapter, subclass adds non-styled decoration 3. element implements new adapter, subclass adds styled decoration 4. element does not implement new adapter, subclass adds styled decoration > > > 2. By default this new method would decorate the given styled string using > > decorateText(...). The result should be the styled text from the element > > plus the un-styled decoration. > Actually, I am not sure how we can re-use the existing implementations of > decorateText(String, Object), because this API can change the text anywhere, no > necessary prefix or suffix any additional text, So, I don't know how we can mix > StyledString from new workbench adapter with this API, again, maybe, I am > missing something here, will really appreciate, if you can correct myself here. The code has to be smart and check whether the decorator does more than just decorate (which 90% do). Created attachment 183273 [details] IWA3 v04.patch Hello Dani, As we discussed in Comment 15 and before. I have attached the new patch in which it adds new method decorateText(StyledString, Object). This new method addition, and it's usage in WorkbenchLabelProvider is the only difference in this patch, from the previous patch. Here is the explanation about what this new method do. 1) Firstly, it checks, if the input StyledString is valid or not. 2) It calls decorateText using StyledString text, and if decorated text is same as original StyledString text, return original StyledString. 3) If decorated text output contains original text, it makes attempt to find, if prefix, or/and suffix was added in decorated text, If so, it create new StyledString with prefix/suffix and the original StyledString in between. 4) If the decorated text output doesn't contain the original text at all, than it creates new StyledString with decorated text and return it. Now, like most String manipulations, this code for sure doesn't look clean, and may have some issues, please let me know, how this can be improved further. After looking at the patch and the existing DecoratingStyledCellLabelProvider it might be overkill to introduce decorateText(StyledString, Object). Instead, we should implement the getStyledText(Object) similar to DecoratingStyledCellLabelProvider.getStyledText(Object) and also introduce getDecorationStyle(Object) like we have in DecoratingStyledCellLabelProvider. Minor issues: - WorkbenchAdapter.getStyledText(Object) must never return 'null' as this is against the spec in IWorkbenchAdapter3 and since getLabel(...) also never returns 'null' the method can just return new StyledString(getLabel(element)); - The Javadoc of WorkbenchAdapter.getStyledText(Object) needs some work - IWorkbenchAdapter -> IWorkbenchAdapter3 - StyledString -> "styled string" or use @link - should not mention 'null' - The copyright date needs to be updated in several files Created attachment 183978 [details]
IWA3 v05
Updated the implementation of WorkbenchLabelProvider.getStyledText, as suggested. Also, fixed the minor issues.
Almost good, except: - You call WorkbenchLabelProvider.getText(Object) in getStyledString(...) which already decorates the text and then you decorate it again. - WorkbenchLabelProvider.decorateText(...) never returns 'null' hence a check for 'null' is not needed. Created attachment 185355 [details] IWA3 v06 > - WorkbenchLabelProvider.decorateText(...) never returns 'null' hence a check > for 'null' is not needed. What if the sub-class of WorkbenchLabelProvider override decorateText which may return null? I guess, we are assuming programming by contract here, So, if they return null, they will see NPE in eclipse API. Anyways, I have made both the suggested changes in attached patch. > What if the sub-class of WorkbenchLabelProvider override decorateText which may
> return null? I guess, we are assuming programming by contract here, So, if they
> return null, they will see NPE in eclipse API.
Yes, the Javadoc of an API is binding.
Hemant, I was thinking a bit more about the concern you raised in your e-mail: "I have noticed, Util.getAdapter(IWorkbenchAdapter2.class) or Util.getAdapter(IWorkbenchAdapter3.class) both doesn't work, as the eventually request for adapt goes to WorkbenchAdapterFactory.getAdapter, which checks, if the class is IWorkbenchAdapter.class or not, So, I don't see how it can work, though maybe I am missing here something, let me know, if you think, I should open a separate defect for this one, I'll be happy to provide patch for this one as well." I think you're right and in fact the changes (yours and the existing ones for IWorkbenchAdapter2) in WorkbenchAdapter are not needed/useless. If you agree, I can apply your patch minus the changes for WorkbenchAdapter plus updating the copyright date to 2011. (In reply to comment #22) > I think you're right and in fact the changes (yours and the existing ones for > IWorkbenchAdapter2) in WorkbenchAdapter are not needed/useless. If you agree, I > can apply your patch minus the changes for WorkbenchAdapter plus updating the > copyright date to 2011. WorkbenchAdapter implements new interface IWorkbenchAdapter3, So, I am not sure how without these changes, it will work out? Indeed, WorkbenchAdapter already implements IWorkbenchAdapter2, and since it is public class, how we can revert it? Or did you meant WorkbenchLabelProvider here, instead of WorkbenchAdapter? As, ideally, we should be fixing WorkbenchLabelProvider.getAdapter2/getAdapter3 to fix this issue? > WorkbenchAdapter implements new interface IWorkbenchAdapter3, So, I am not sure > how without these changes, it will work out? The changes are useless as they don't provide anything new/smart: they do the same as the fallback code in the label provider. >Indeed, WorkbenchAdapter already > implements IWorkbenchAdapter2, and since it is public class, how we can revert > it? We can't but I never said we should/can do that. > As, ideally, we should be fixing WorkbenchLabelProvider.getAdapter2/getAdapter3 > to fix this issue? No. (In reply to comment #24) Alright, go ahead with your suggested changes in that case. Thanks Comment on attachment 185355 [details]
IWA3 v06
Committed the patch to HEAD with the following changes:
- ignored changes to WorkbenchAdapter
- updated copyright dates to 2011
. |