Community
Participate
Working Groups
Eclipse 3.3 M7 introduced the option "Java > Appearance > Use colors in labels". (see http://download.eclipse.org/eclipse/downloads/drops/S-3.3M7-200705031400/images/colored-labels.png ) This is implemented in org.eclipse.jdt.internal.ui.viewsupport.OwnerDrawSupport which is used by org.eclipse.jdt.internal.ui.viewsupport.ColoredViewersManager. I believe the JFace viewers could benefit from such a feature, which should make the new SWT custom draw capabilities more accessable. The JDT implementations might be used as inspiration. But there is more than colors: multiple fonts in a cell would be also welcome. Showing different fonts is already being done in Eclipse, which shows the need for this feature: org.eclipse.ui.internal.quickaccess.QuickAccessDialog Maybe for 3.4? Before this is implemented multiple times by different teams? ;)
For org.eclipse.ui.internal.quickaccess.QuickAccessDialog in action see http://download.eclipse.org/eclipse/downloads/drops/S-3.3M7-200705031400/images/ctrl-3-1.png
We have an OwnerDrawLabelProvider already in JFace - in Bug 183376 we discuss doing an OwnerDrawDecoratingLabelProvider *** This bug has been marked as a duplicate of bug 183376 ***
Tod, I don't get fully understand why this request is has something todo with decorators although this can be done using decorators it might also be that this is done by an ordinary OwnerDrawLabelProvider. What I have in mind is an intermediate between OwnerDrawLabelProvider and ColumnLabelProvider allowing hooks to: - color text - define the position of the image - ...
I am sorry, apparently I did not describe properly what this enhancement request is about. I am aware of the OwnerDrawLabelProvider since Tom Schindl told me about it on the platform list. But it doesn't really help you, if you want to color a word in your labels, or if you want to make it bold. But what I'd like to have (and what both ColoredViewersManager and QuickAccessDialog would benefit from) is something like an optional IStyledTextProvider for viewers. public interface IStyledTextProvider{ StyledRange[] getStyledRange(Object element, String label); } The implementation should be possible by using both OwnerDrawLabelProvider and TextLayout. Only transferring StyledRange[] to TextLayout seems to be a bit tricky. The need I want to address has little to do with Bug 183376, as far as I can tell. It is about drawing multi-colored and or multi-font labels in viewers in the easiest way possible.
Reopening
BTW, the QuickAccess dialog does not use JFace viewers at all, but I agree that it would be useful to have support for this in a label provider.
Since I believe in the usefulness of this request, I did an implementation. This is the first time I try to contribute something to an open source project, so I am pretty exited about it. Just to warn you beforehand... I implemented an abstract label provider which allows implementors to provide StyleRange instances for each label. These are applied using a TextLayout, which does all the tricky stuff. Please try the two attached snippets. The code is neither perfect nor finished, but I hope it can serve as a base for the final implementation. What I like about it: - It introduces only minimal API - the class itself - It is easy to enrich existing implementations with the new features There are several issues with the code I am aware of: - The TextLayout and the StyleRange array are calculated both on a measure and on a paint event. Caching the TextLayout instance from the measure event seems sensible, but I am uncertain how to implement it. Is it ok to rely on the order of SWT events, i.e. measure item A, paint item A, measure item B, paint item B, ...? I don't believe so. Is it ok to rely on the observation, that there is always a paint event after a measure event? - There are two hacks for win32: Once an added pixel during vertical centering, and once preventing placing the layout too high: In some cases there is a wrong measurement of 3 pixels. - It doesn't support CellLabelProviders. I didn't come up with a good idea about how to achieve this, since I didn't want to have a reference to the viewer. - Automatic line wrapping is currently disabled. See Bug 130024 or Bug 148039 for the reason. As long as Table.setItemHeight(int) and Tree.setItemHeight(int) are package private, it has to stay so. But Steve seems to consider making these methods public (Bug 130024 comment 8) - The viewer's control should be created with SWT.FULL_SELECTION on win32 as long as Bug 168807 exists. I tested the code with XP, GTK and Carbon - I liked the results. I am looking forward to your inputs :)
Created attachment 72297 [details] proposed implementation for the request
Created attachment 72298 [details] Snippet using the new class
Created attachment 72299 [details] Snippet using the new class
Created attachment 72300 [details] 2nd snippet using the new class, more complex than 72298
Very cool! The second snippet looks great. Here is my initial feedback: 0. I'm amazed at how finished your code looks already, with proper copyright comments and all! 1. Could you please check out org.eclipse.jface using anonymous CVS so that you get the proper compiler settings? There were a couple of missing $NON-NLS and such. 2. I am not convinced that it is a good idea to let the new label provider wrap an existing label provider. Wouldn't it be simpler if the new label provider was a stand-alone abstract class, with abstract methods for providing the text and the formatting? If clients wanted to reuse their old label providers, they could implement the wrapping themselves. 3. This class is very useful even without the multi-line support. Do you see a way to split it into two where one just does the single-line case and the other adds multi-line support? That way, we could put in the single-line support earlier and wait for SWT support before we add multi-line support. Given the current restrictions on item heights in SWT tables and trees, I believe that the majority of clients would just use single lines. In the single-line case, you also don't need the resize listener. 4. Erasing: Steve recommends doing nothing at all on erase, which gives you native selection highlighting (the "glow" on Vista for example). Have a look at QuickAccessEntry.java in org.eclipse.ui.workbench - the implementation of erase is just "event.detail &= ~SWT.FOREGROUND;". 5. Could we require clients to do the font management, rather than using the default FontRegistry? I find it odd that the label provider creates bold fonts for clients but does not dispose of them. Once we are on the same page with regards to the basic architecture of this (mostly items 2 and 3 from the above list), I will add potential clients such as committers from JDT UI to this bug to gather feedback from them as well. Overall, I'm pretty excited about this too. Thanks for your work so far! Note though that it might take a while before we can put this into CVS, see page two of: http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf
Thanks for your feedback, it was not only nice but also very helpful. I did a refactoring, implementing all your suggestions. By this I got rid of some nasty stuff, but now there are two new issues which are not under my control: 1. Viewer.refresh() is currently broken. Users have to use Viewer.getControl().redraw() which is a terrible workaround. Reason: ColumnViewer.setLabelProvider(.) wraps a label provider in a TableColumnViewerLabelProvider even if it already is a CellLabelProvider. It only has to implement ITableLabelProvider or some other interfaces. (see CellLabelProvider.createViewerLabelProvider(.)) On a refresh() only CellLabelProviders get notified, in my case the TableColumnViewerLabelProvider which wraps my provider and eats the call to update(.). This unexpected wrapping also prevents me from re-using OwnerDrawLabelProvider.setUpOwnerDraw(.). Currently I removed any reference to OwnerDrawLabelProvider, which seems wrong to me. I guess I should file a bug report. But I don't know if this can be fixed at all, without breaking other stuff. 2. After the refactoring an empty erase method does not work anymore. (It worked before) If there is an image for some item of a column, the selection gets clipped. That's why I kept the old behaviour. I hope this was no refuctoring: http://inside-swt.blogspot.com/2007/03/refuctoring-your-code.html I do like the API better now, so I really hope to find a way to fix the first issue. A patch for CellLabelProvider which does an "instanceof OwnerDrawLabelProvider" would be very easy to do. But can that be the way? > 2. I am not convinced that it is a good idea to let the new label provider > wrap an existing label provider. You were completely right. I like it much more like it is now: Wrapping is not necessary and label providers are still reusable, because they only have to extend StyledLabelProvider. > 5. Could we require clients to do the font management, rather than using the > default FontRegistry? I find it odd that the label provider creates bold fonts > for clients but does not dispose of them. I don't want to burden the clients with the font management. StyledText also does this for the client. StyleRange has this option, so it should be supported. I removed the FontRegistry, now the fonts are managed by the provider itself. And properly disposed of. > Note though that it might take a while before we can put this into CVS, see page two > of: http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf Ooops. Looks more complicated than coding ;) I hope I will manage.
Created attachment 72421 [details] implementation for single line labels
Created attachment 72422 [details] implementation for multi line labels
Created attachment 72423 [details] Simple snippet for single line labels
Created attachment 72424 [details] Snippet for multi line labels
(In reply to comment #13) > Thanks for your feedback, it was not only nice but also very helpful. I did a > refactoring, implementing all your suggestions. By this I got rid of some nasty > stuff, but now there are two new issues which are not under my control: > > 1. Viewer.refresh() is currently broken. Users have to use > Viewer.getControl().redraw() which is a terrible workaround. > > Reason: ColumnViewer.setLabelProvider(.) wraps a label provider in a > TableColumnViewerLabelProvider even if it already is a CellLabelProvider. It > only has to implement ITableLabelProvider or some other interfaces. (see > CellLabelProvider.createViewerLabelProvider(.)) On a refresh() only > CellLabelProviders get notified, in my case the TableColumnViewerLabelProvider > which wraps my provider and eats the call to update(.). This unexpected > wrapping also prevents me from re-using > OwnerDrawLabelProvider.setUpOwnerDraw(.). Currently I removed any reference to > OwnerDrawLabelProvider, which seems wrong to me. Why would one pass a CellLabelProvider to ColumnViewer#setLabelProvider(). This doesn't make any sense to me. CellLabelProviders are passed to columns or am I mistaken completely? The only reason can be that you have a TableViewer without any column but in this case you don't implement the ITable*-Interfaces either. I think there was a reason why we first test for ITable* interfaces and afterwards CellLabelProvider but I could be wrong though. If I'm wrong reordering the instance checks could fix the problem not?
> Why would one pass a CellLabelProvider to ColumnViewer#setLabelProvider(). This > doesn't make any sense to me. CellLabelProviders are passed to columns or am I > mistaken completely? That's true, to pass a CellLabelProvider is strange. But it always happens, when you set an OwnerDrawLabelProvider. And if it implements one of the ITable* interfaces, OwnerDraw gets broken. Just add an ITable* interface to an OwnerDrawLabelProvider snippet to try it. > I think there was > a reason why we first test for ITable* interfaces and afterwards > CellLabelProvider but I could be wrong though. If I'm wrong reordering the > instance checks could fix the problem not? Reordering would fix it, but I also was reluctant to do that, because I don't know the side effects. Another fix is adding if (labelProvider instanceof OwnerDrawLabelProvider) return (CellLabelProvider) labelProvider; at the beginning of the method. I was just in the process of creating a bug with this as proposed patch when you wrote. Implementing this patch would fix both outstanding issues with my refactoring.
(In reply to comment #19) > > Why would one pass a CellLabelProvider to ColumnViewer#setLabelProvider(). This > > doesn't make any sense to me. CellLabelProviders are passed to columns or am I > > mistaken completely? > > That's true, to pass a CellLabelProvider is strange. But it always happens, > when you set an OwnerDrawLabelProvider. And if it implements one of the ITable* > interfaces, OwnerDraw gets broken. Just add an ITable* interface to an > OwnerDrawLabelProvider snippet to try it. But why would one do that? It's simply wrong API usage :-) Using CellLabelProviders with ITable* interfaces is simply a bad idea. > > > I think there was > > a reason why we first test for ITable* interfaces and afterwards > > CellLabelProvider but I could be wrong though. If I'm wrong reordering the > > instance checks could fix the problem not? > > Reordering would fix it, but I also was reluctant to do that, because I don't > know the side effects. Another fix is adding > > if (labelProvider instanceof OwnerDrawLabelProvider) > return (CellLabelProvider) labelProvider; > > at the beginning of the method. I was just in the process of creating a bug > with this as proposed patch when you wrote. > Go ahead and file one then we can discuss there whether this makes sense or not
I filed a bug, including a proposal for a patch, which is the precondition for fixing both issues of the current version. The alternative to fixing Bug 194388 would be going back and try another refactoring. I am convinced though, that clients will come up with the idea of implementing ITable* interfaces with their StyledLabelProvider in any case, so that Bug 194388 will not be as theoretical as it seems today.
(In reply to comment #19) > That's true, to pass a CellLabelProvider is strange. But it always happens, > when you set an OwnerDrawLabelProvider. The intended use for CellLabelProvider and therefore OwnerDrawLabelProvider was to set it as the label provider for a viewer column (TableViewerColumn or TreeViewerColumn), not the viewer itself.
With the help of Tom and Boris I finally understood what CellLabelProviders are about ;) That's how I could fix the two outstanding issues without a fix to Bug 194388. I hope this version satifies your points, Boris.
Created attachment 72495 [details] implementation for single line labels
Created attachment 72496 [details] implementation for multi line labels
*** Bug 121736 has been marked as a duplicate of this bug. ***
Created attachment 72539 [details] Make StyledText Michael I took a short look at the implementation. This is really great work but there are something I think could be improved and I've done a first draft version for you how I think this support has to be integrated into JFace 3.4. Another thing maybe interesting would be that when we use ViewerLabel information we might have the advantage that we wouldn't have recalc the item height everytime but only if the item really changed but for this we have make ViewerLabel a bit more inteligent. By the way there seems to be a problem on OSX with the top-left cell which is not shown correctly initially. I'll attach a screen shot :-)
Created attachment 72542 [details] OSX-Screenshot looks like a problem from SWT, need to investigate
(In reply to comment #28) > Created an attachment (id=72542) [details] > OSX-Screenshot > > looks like a problem from SWT, need to investigate > Fix is simple adding these lines at the end of StyledTextCellLabelProvider#measure(Event event, Object element): ------------8<------------ Rectangle rect = cell.getBounds(); viewer.getControl().redraw(rect.x,rect.y,rect.width,rect.height,true); ------------8<------------ Maybe StyledTextSupport#measure should return whether it changed the height/width of the cell?
Thanks for the feedback and the patch, Tom! Your refactoring fixes two issues I was really bothered about (see above): Missing support for CellLabelProviders and having an own setUpOwnerDraw(.) method. The API makes sense now :) Unfortunately one of the things I was very concerned about is missing: Legacy support for I*Providers. I believe, that most users will only want to style a word in their single line labels, as the JDT UI guys are already doing (see comment 0). And to do this, they will want to re-use their label providers. To make this possible I added a wrapper class for IBaseLabelProvider. The first snippet shows its usage, although maybe we shouldn't provide a snippet showing "deprecated" usage. I also fixed some minor points and added a StyledMultiLineLabelSupport class, which will support automatically wrapping multi line labels, once Table.setItemHeight(int) and Tree.setItemHeight(int) are made public. I hope this does happen, since I think this is the coolest feature of all. On my workstation it works very well with a patched SWT :) I am confused about the copyright notice. In my initial version I simply copied the style from the snippets. You replaced it with (c) IBM. Why is this done? Eclipse foundation is not IBM, neither is you or me. I am very happy how the code is improving, even though we have four classes meanwhile, where we started with one ;) Just as a sidenote: I will be offline from Thursday to Tuesday next week.
Created attachment 72592 [details] Implementation and snippets
> Another thing maybe interesting would be that when we use ViewerLabel > information we might have the advantage that we wouldn't have recalc the item > height everytime but only if the item really changed but for this we have make > ViewerLabel a bit more inteligent. That would be cool. Storing data in ViewerLabel might be useful in general. > By the way there seems to be a problem on OSX with the top-left cell which is > not shown correctly initially. I'll attach a screen shot :-) Actually I also saw that, but since other snippets had the same problem (e.g. Snippet006TableMultiLineCells) I didn't worry too much. You fixed it amazingly fast! I integrated your fix in the recent version.
(In reply to comment #30) > Thanks for the feedback and the patch, Tom! Your refactoring fixes two issues I > was really bothered about (see above): Missing support for CellLabelProviders > and having an own setUpOwnerDraw(.) method. The API makes sense now :) > There would be multiple problems with your implementation: - What would you do if a user adds a column dynamically after it already has setup the viewer. One of main advantages is that CellLabelProviders make adding/removing/reordering of columns a piece of cake :-) - The user had exactly one point in the programm where he/she need call the setup-method else things would have failed. This not API should work :-) > Unfortunately one of the things I was very concerned about is missing: Legacy > support for I*Providers. I believe, that most users will only want to style a > word in their single line labels, as the JDT UI guys are already doing (see > comment 0). And to do this, they will want to re-use their label providers. > > To make this possible I added a wrapper class for IBaseLabelProvider. The first > snippet shows its usage, although maybe we shouldn't provide a snippet showing > "deprecated" usage. I agree with you and will take a look as soon as possible. I think a snippet could not harm anyone and the old API is not deprecated and people are still free to use it. If we can provide new features without to much work this is is a win-win situation. The only thing questionable is whether we provide API or simply a snippet demonstrating how this could done using the "OLD" API (there are many snippets available in our collection doing this exactly) > > I also fixed some minor points and added a StyledMultiLineLabelSupport class, > which will support automatically wrapping multi line labels, once > Table.setItemHeight(int) and Tree.setItemHeight(int) are made public. I hope > this does happen, since I think this is the coolest feature of all. On my > workstation it works very well with a patched SWT :) > > I am confused about the copyright notice. In my initial version I simply copied > the style from the snippets. You replaced it with (c) IBM. Why is this done? > Eclipse foundation is not IBM, neither is you or me. You are completely right, I did this yesterday in the night and by default the IBM copyright is inserted. Feel free to replace with your copyright and add me as initial implementor as well as you. > > I am very happy how the code is improving, even though we have four classes > meanwhile, where we started with one ;) > But we gain flexibility as outlined above I uncertain we should provide new API classes for old API. I lean towards providing snippets and no API but the "API-Polzei" = Boris will decide this.
(In reply to comment #33) > There would be multiple problems with your implementation: I desperately hope you are writing about the old version. I don't think the current one has these problems. > - What would you do if a user adds a column dynamically after it already has > setup the viewer. One of main advantages is that CellLabelProviders make > adding/removing/reordering of columns a piece of cake :-) > - The user had exactly one point in the programm where he/she need call the > setup-method else things would have failed. This not API should work :-) Calling OwnerDrawLabelProvider.setUpOwnerDraw(viewer) or StyledMultiLineLabelSupport.setUpMultiLineLabelSupport(viewer) is still necessary before showing the viewer, but I see no way to prevent this. But I understand what you mean concerning the old implementation - that was not well done. > But we gain flexibility as outlined above I uncertain we should provide new API > classes for old API. I lean towards providing snippets and no API but the > "API-Polzei" = Boris will decide this. A snippet might be enough, even though I think fewer people will find it that way. But since I think it will be an often needed usecase, generic and specific wrappers will be written by many people, then, instead of using a generic one in the API.
Created attachment 73167 [details] Implementation and snippets I did a bit of cleanup and fixed the 100% CPU consumption of the Carbon fix (see comment #28). Also I splitted this bug report in two, putting the multi line support to an own bug report, bug 195597. The reason for this is the dependency on a pending SWT enhancement which will enable automatic line wrapping for labels. Since the rest of the code has no such dependency it seems sensible to do the split.
Created attachment 73272 [details] Implementation and snippets Fixed missing support for erase event in StyledCellLabelProvider. Also added missing snippet again.
The last patch is not in "### Eclipse Workspace Patch 1.0" format (unlike the one from June 27, for example). Could you please upload the latest patch again, using this format? Or is there a way to apply this patch anyway that I don't know of?
Created attachment 73353 [details] Implementation and snippets > The last patch is not in "### Eclipse Workspace Patch 1.0" format (unlike the > one from June 27, for example). Could you please upload the latest patch > again, using this format? Sorry, here we go. > Or is there a way to apply this patch anyway that I don't know of? You could have also used Team > Apply Patch from the projects context menu. But then you would have to do this twice, once for jface and once for jface snippets. And select only the appropriate classes. But this is not what I intended.
I just filed bug 196128 without knowing of this bug. I implemented the colored labels in JDT and wanted to start the discussion on how to move it down to JFace. Great to see that there is already progress here. Here's some feedback: - From the standpoint of a label provider implementor, we found that it is more efficient and easiest if text and style ranges can be computed at once. For example in concatenated labels like 'foo(int i, int y) - MyClass - mypackage [1.4]' each segment is computed separately, in a recursive way, from different providers. You don't want to make make two passes, one for the label and one for the styles. So one idea I wanted to throw up is to extend 'ViewerCell' (or 'ViewerLabel') to also contain the styled ranges. Users can then set text and styled ranges at once in update(...) - In the JDT UI implementation we paid extra care to make sure that the colored label is positioned exactly the same as in a native viewer. So for example the indentation of the image, the size of the gap between image and text. - As Steve told me, selected elements should not be colored. - Performance is of course a hot issue. The owner draw requests are coming in a quick rate; on resizes, reselections, even when the mouse moves over an item. Computing the label text and the styles is expensive, and can' be done on every paint. As Michael already said, it should be investigated if the text layout can be cached somehow. In JDT we avoided this by only supporting normal text (no bold, italic...) so the native measure event can be used. - Looking at the implementation, I think StyledLabelSupport looks a bit strange and unusual. Wouldn't it be possible to merged it into StyledCellLabelProvider? It's also not clear to me if StyledCellLabelProvider should really implement IColorProvider, IFontProvider ect.
I agree with most if not all of Martin's comments. In particular, I would like to push for just one class StyledCellLabelProvider that does not implement IColorProvider et al. The helper class for adding styles to existing label providers could be implemented as a subclass - I don't see the need for a separate class StyledLabelSupport. > - As Steve told me, selected elements should not be colored. Yes, it is safer to not do anything with colour if the item is selected. We could make this configurable though, with the default being no colour when selected. > Computing the label text and the styles is expensive, and can't be done on > every paint. This is where I am not sure if I agree (at this point). Caching text and styles is going to add complexity to the implementation. I would rather implement a simple solution that we can put to use (ideally in JDT!) so that we can optimize the right thing based on real profiling data. I like the idea of having a subclass of ViewerLabel that holds the style information, and that this would allow a single pass if you are collecting information from multiple sources.
A minor thing - for the next patch, would you mind to auto-format (Ctrl+Shift+F) the files to make them consistent with our formatting style? You should already have the project-specific formatter settings if you checked out org.eclipse.jface from CVS. Thanks!
*** Bug 196128 has been marked as a duplicate of this bug. ***
> So one idea I wanted to throw up is to extend 'ViewerCell' (or 'ViewerLabel') > to also contain the styled ranges. Users can then set text and styled ranges at > once in update(...) Great idea! I think that should be easy to do. > - In the JDT UI implementation we paid extra care to make sure that the colored > label is positioned exactly the same as in a native viewer. So for example the > indentation of the image, the size of the gap between image and text. The simple snippet looks pretty much the same on xp as the native table. Only if there is no image, there is no indentation in the styled version. Indenting it might not be expected in many usecases, though. > - As Steve told me, selected elements should not be colored. Cool, I'll add a style bit to control this, as Boris proposed. > - Looking at the implementation, I think StyledLabelSupport looks a bit strange > and unusual. Wouldn't it be possible to merged it into > StyledCellLabelProvider? This means merging it back again. StyledLabelSupport was introduced by Tom in comment 27. Since Boris also wants it integrated again, I'll do so. > It's also not clear to me if StyledCellLabelProvider should really implement > IColorProvider, IFontProvider ect. It doesn't have to. But if a subclass does, the provided colors and fonts should be used as default for the label. And to make users aware of this behaviour, it might be easier to implement the I*Provider interfaces. But maybe a Snippet is enough. > > Computing the label text and the styles is expensive, and can't be done on > > every paint. > > This is where I am not sure if I agree (at this point). Caching text and > styles is going to add complexity to the implementation. I would rather > implement a simple solution that we can put to use (ideally in JDT!) so that we > can optimize the right thing based on real profiling data. Currently there are two performance issues: 1. For each measure event AND for each paint event the text, styles and TextLayout have to be computed. This is once too often. I'll propose a simple caching in the next patch. 2. For each paint everything is calculated from scratch. I agree with Boris, that caching here should be controlled by the user. Lets see if we come up with something after the next patch. Thanks for the feedback, I'll start working right away :)
Created attachment 73614 [details] Implementation and snippets Here we go, the promised patch. > - In the JDT UI implementation we paid extra care to make sure that the colored > label is positioned exactly the same as in a native viewer. So for example the > indentation of the image, the size of the gap between image and text. Meanwhile I understand, what you meant. Both GTK and Carbon had some utterly wrong default font. I fixed that, but native GTK Table still calculates its height a bit differently than the styled variant. I'll look at your code to see what can be done. I implemented a primitive cache. Unfortunately it does nothing on GTK, because the event order is different there from what I expect. On GTK first all outstanding measureItem events happen, and afterwards the paintItem events. The implementation realizes that it cannot work and no caching is done in this case.
(In reply to comment #44) > Created an attachment (id=73614) [details] > Implementation and snippets > > Here we go, the promised patch. > > > - In the JDT UI implementation we paid extra care to make sure that the colored > > label is positioned exactly the same as in a native viewer. So for example the > > indentation of the image, the size of the gap between image and text. > > Meanwhile I understand, what you meant. Both GTK and Carbon had some utterly > wrong default font. I fixed that, but native GTK Table still calculates its > height a bit differently than the styled variant. I'll look at your code to see > what can be done. > > I implemented a primitive cache. Unfortunately it does nothing on GTK, because > the event order is different there from what I expect. On GTK first all > outstanding measureItem events happen, and afterwards the paintItem events. The we should maybe ask one of SWT guys, whether they can fix them. We had issues like that in 3.3 timeframe and could get rid of these different event-orders. If you have a simple snippet we could file a bug against SWT asking for their comment.
Created attachment 73626 [details] Implementation and snippets Sorry, two bugs had to be fixed.
Created attachment 73633 [details] Implementation and snippets ...and of course there is always a third bug. I am really sorry for these frequent updates, next time I'll wait a bit longer before posting.
(In reply to comment #45) > The we should maybe ask one of SWT guys, whether they can fix them. We had > issues like that in 3.3 timeframe and could get rid of these different > event-orders. If you have a simple snippet we could file a bug against SWT > asking for their comment. I did so in Bug 196572. I am curious whether this can be changed.
Created attachment 73818 [details] Implementation and snippets (In reply to comment #39) > - In the JDT UI implementation we paid extra care to make sure that the colored > label is positioned exactly the same as in a native viewer. So for example the > indentation of the image, the size of the gap between image and text. I implemented another style bit, which uses your way of positioning image and text. I made this behaviour switchable because there are usecases (multiline labels, all images placed in the middle of the text) where this does not make sense. I also fixed several bugs concerning pack(). The one which took me most time to figure out was actually introduced by Tom in comment 27: using event.setBounds(textLayout.getBounds()) also resets event.x and event.y. And event.x is used by SWT for width calculation. Meanwhile I also do tests on Motif - this version works there, too... I don't know of outstanding issues any more - I'll wait for feedback.
Created attachment 80884 [details] SimpleStyledCellLabelProvider With the hope that we can finally make some progress on this issue, I had a closer look at Michaels proposal. I think it goes in the right direction. I used his code as a starting point to a reduced proposal, with focus an simple API, performance and with the aim to imitate the native look as much as possible so that normal viewers and colorized viewers can be shown side by side in Eclipse without seeing differences (except for the colors) The 'SimpleStyledCellLabelProvider' does the following: - Single line per item. Image at the front. - Positioning of image and labels and size computation is using native item.getImage/TextBounds - selection and focus box rendering is done either by the native control or using the native coordinates Each platform has different ways of rendering the selection and the focus box. If we would draw it ourselves, it's easy to see the difference. To be able to do this we need to use item.setText and item.setImage with our text and image so we the methods item.getText/ImageBounds and item.getBounds to get the positioning right. That means the label erased by the viewer, but drawn by us. The drawback of this is that we need to use the same font for a whole item. I didn't find a way to tell the tree/table the updated width for an item (you can change the event.width on 'measure', but this doesn't make a difference on how the control draws the selection and what it returns as bounds by item.getTextBound or item.getBound. We should ask SWT for a comment here. For this reason the SimpleStyledCellLabelProvider currently only supports a single font for the whole item: the font settings in the styled ranges are ignored. API: - It's important that a label provider can provide all label information at once as computing label separate from styled ranges would double the work. Michael solution already solves this, but instead of reusing a ViewerLabel, I introduced a new type holding all the information: Reason for this is that ViewerLabel is too big, it holds a lot of information that doesn't apply to our situation. Other reason is that I want to use the label element (called LabelPresentationInfo in my patch) also as a cache object Performance: - Owner draw viewers tend to call 'measure' and 'paint' a lot (control becomes visible, scrolls, gets selection). Native viewers store the evaluated labels and images in the item (item.setText/setImage). We need to keep the styles the same way. Therefore, each item has the LabelPresentationInfo in a data element. - 'measure', 'erase', 'paint' are often called directly after each other, using the same information (e.g. width of the item). It makes sense to keep the computed TextLayout a bit longer. As 'SimpleStyledCellLabelProvider' doesn't implement 'measure', but the code is ready to do this (as soon as we solved the item width issue mentioned above). It would be good if Boris could have a look at a patch and even decide it this is the direction to go and release this. As a next step we should look at other issues related to colored labels a. I think it would be good if OwnerDrawLabelProvider.setUpOwnerDraw(ownerDrawViewer) wouldn't be necessary. The viewer could detect the owner drawn label provider on setLabelProvider. (we would need to introduce a different type than OwnerDrawLabelProvider). It is also necessary that owner draw can be uninstalled again. As a minimum OwnerDrawLabelProvider needs to offer 'uninstalOwnerDraw' b. JFace should provide an implementation of a SimpleStyledCellLabelProvider that wraps a 'normal' label provider. Similar to IFontProvider..., we should offer a IStyledLabelProvider, so that existing label providers can be extended to provide colored labels as well. c. It would be good if JFace could offer something similar like ColoredString, which helps you constructing colored labels (see bug 196128) d. JFace should provide a implementation that wraps a 'DecoratingLabelProvider'. Decorations will then get different colors
Created attachment 80912 [details] Implementation and snippets Hi Martin, thanks for bringing life back to this enhancement request. I added a snippet which was meant to show, that my proposal includes everything you describe. I had to add focus drawing and do a positioning bugfix. Snippet044StyledCellLabelProvider looks exactly the same as yours. I did no performance tests, but both of us cache both the styles and the TextLayout, which I regard as the critical points. But unfortunately I have not noticed until now, that my proposal has serious problems with native selection drawing, when SWT.FULL_SELECTION is not set. These problems do not occur in the usecase of your snippet. But when positioning the image somewhere in the text or using different fonts, I run into the same problems you describe: > you can change the event.width on 'measure', but this doesn't make a > difference on how the control draws the selection and what it returns as > bounds by item.getTextBound or item.getBound So my proposal is not production ready, unless - I remove native selection drawing, if SWT.FULL_SELECTION is not set - the SWT guys do some magic to make this work If this problem did not exist (or could be solved), I would prefer my proposal to yours, simply because it enables the user to do more: inline images, multi-font labels, multi-line labels. Your API is better suited for your usecase, but as my snippet shows, it is far from impossible to do the same with mine. And I never claimed, that the API of my proposal is final. To summarize: I'd like the API to support more than coloring labels, if possible.
I added bug 207711 to ask the SWT guys about our problems with modified width. And I realized, that once I knew of this limitation in my code. Comment #7 says: > - The viewer's control should be created with SWT.FULL_SELECTION on win32 as long as Bug 168807 exists. Native selection and focus drawing were added later, but this correlated bug was already serious back then.
Martin, Michael, I finally had time to look at both patches. My apologies for the long delay and for not making progress on this despite all your help. I need to play with it some more before I can give you an informed opinion, but I hope to have feedback for you on Monday.
Created attachment 82965 [details] Implementation and snippets I have added a workaround for Bug 178044 to my proposal. It adds a rollover (aka tooltip, see Bug #178044 comment #12), if an item is clipped. The implementation is not native, but it mimics XP. The advantage of this approach is, that even GTK gets such rollovers, which doesn't have them natively. But this is the disadvantage as well: it stops looking native. Vista has some gradient background, Carbon does multi-line tooltips instead of rollovers, GTK has no rollovers. I do not emulate them. I'd like to combine this approach with native rollover shells. Maybe if the proposal in Bug #178044 comment #6 to get a native rollover shell gets implemented. Nevertheless I don't think, that this bug is blocked by Bug 178044 anymore. But it is blocked by Bug 168807 in my opinion. As long as there is no solution for both hittest and selection/focus drawing, my implementation only works with SWT.FULL_SELECTION.
Created attachment 83672 [details] updated Martin's patch I have updated Martin's patch to work in a multi-column setting as well. Given our limited resources for anything new in 3.4, and the ongoing maintenance effort that new features such as this require, I would like to use this as a starting point because of its relative simplicity. Michael, I understand that this is not enough for your needs. Also, if we wanted to use the 'use bold for filter matches' convention in other places, it would be good if at least bold and italics styles were supported. Here is what I propose: Lets talk about how SimpleStyledCellLabelProvider could be opened up so that you can reuse most of the code in it but provide more sophisticated painting and/or measuring implementations. I like the fact that SimpleStyledCellLabelProvider can use the native measurements and thus avoids bug 168807 - I am looking for a solution that uses subclassing or delegation rather than implementing everything in one class. To make it easier to discuss improvements over SimpleStyledCellLabelProvider, I have committed the patch to CVS and opened bug 210809 for follow-up discussion. This also makes it possible to track contributions coming from existing Eclipse committers vs. non-committers. I have marked the class as experimental because I am anticipating changes to its API, implementation, and even name.
Martin, for your patch, did you use any code from Michael? Do I have to put Michael's name in any of the copyright headers, perhaps the snippets?
I started from Michael patch and used some of his ideas. So I think its would be good to put his name in the copyright header as well.
For the record: The final patch contains probably 40 - 50 lines from Michael original patch, the rest of the code is new or code comes from JDT/UI's OwnerDrawLabelProvider.
I updated the copyright headers in SimpleStyledCellLabelProvider and the two snippet classes (which I renamed to Snippet049... and Snippet050...).
Verified by running Snippet049 and Snippet050 on Windows XP using I20071211-0010.
Verified by running Snippet049 and Snippet050 on Windows Vista using I20071211-0010.
Reopening. We have a font problem on Linux/GTK. Martin, do you have something like textLayout.setFont(tree.getFont()) in the JDT UI owner draw code? QuickAccessDialog does that...
Fixed in HEAD.
