Bug 228397 - Add font support for StyledCellLabelProvider
Summary: Add font support for StyledCellLabelProvider
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.5 M4   Edit
Assignee: Boris Bokowski CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 225351 228376 228695
Blocks: 218605 292198
  Show dependency tree
 
Reported: 2008-04-23 09:10 EDT by Martin Aeschlimann CLA
Modified: 2020-10-16 07:56 EDT (History)
5 users (show)

See Also:


Attachments
patch (5.54 KB, patch)
2008-04-23 09:14 EDT, Martin Aeschlimann CLA
no flags Details | Diff
patch for org.eclipse.jface.snippets (3.84 KB, patch)
2008-04-23 11:03 EDT, Martin Aeschlimann CLA
no flags Details | Diff
patch to make it work on GTK (3.12 KB, patch)
2008-04-24 12:18 EDT, Boris Bokowski CLA
no flags Details | Diff
mylyn/context/zip (5.70 KB, application/octet-stream)
2008-04-24 12:18 EDT, Boris Bokowski CLA
no flags Details
combined patch (11.95 KB, patch)
2008-11-26 14:21 EST, Boris Bokowski CLA
no flags Details | Diff
Updated patch that works on OS X (14.79 KB, patch)
2008-11-27 13:11 EST, Kim Horne CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Aeschlimann CLA 2008-04-23 09:10:47 EDT
20080422-0800

As bug 225351 is fixed, we can remove the restriction on StyledCellLabelProvider about the usage of fonts in stylers.
Comment 1 Martin Aeschlimann CLA 2008-04-23 09:14:34 EDT
Created attachment 97217 [details]
patch

Patch to support different fonts in Stylers.

Note that if bug 228376 could be fixed, the implementation would be simpler and more efficient (see comments in patch)
Comment 2 Boris Bokowski CLA 2008-04-23 10:42:09 EDT
Martin, do you have a snippet for testing this?
Comment 3 Martin Aeschlimann CLA 2008-04-23 11:03:01 EDT
Created attachment 97238 [details]
patch for org.eclipse.jface.snippets
Comment 4 Boris Bokowski CLA 2008-04-23 12:18:57 EDT
Looks good - thanks Martin! Released to HEAD.
Comment 5 Boris Bokowski CLA 2008-04-24 11:44:50 EDT
Reopening - the patch does not work on GTK.
Comment 6 Boris Bokowski CLA 2008-04-24 12:18:01 EDT
Created attachment 97487 [details]
patch to make it work on GTK
Comment 7 Boris Bokowski CLA 2008-04-24 12:18:04 EDT
Created attachment 97488 [details]
mylyn/context/zip
Comment 8 Boris Bokowski CLA 2008-04-24 12:24:27 EDT
I have reverted Martin's patches. We talked about this and agreed that we should postpone font support until 3.5 rather than trying to work around bug 228695 because the workaround has a performance cost. It also makes sense to avoid major changes to how we do owner draw at this late point in the development cycle.
Comment 9 Dani Megert CLA 2008-09-17 09:40:39 EDT
Boris, can you assign a milestone to this? I guess we also need the OK for the blocking SWT bugs before we proceed.
Comment 10 Boris Bokowski CLA 2008-10-01 14:22:22 EDT
(In reply to comment #9)
> Boris, can you assign a milestone to this? I guess we also need the OK for the
> blocking SWT bugs before we proceed.

Yes - at least they are not currently listed on SWT's milestone plan.

Steve, can we expect fixes for bug 228376 and bug 228695 in 3.5, or do we need to defer?
Comment 11 Boris Bokowski CLA 2008-11-26 14:21:38 EST
Created attachment 118828 [details]
combined patch

Combined patch with fix for focus rectangle drawing on Windows XP.
Comment 12 Kim Horne CLA 2008-11-27 10:25:52 EST
(In reply to comment #11)
> Created an attachment (id=118828) [details]
> combined patch
> 
> Combined patch with fix for focus rectangle drawing on Windows XP.
> 

Still testing now, but FYI, those snippets that obtain a display in a static initializer don't work on Mac (Carbon or Cocoa).  The static initializers are run in one thread and the main method is invoked in another so there's no default display.
Comment 13 Steve Northover CLA 2008-11-27 11:51:30 EST
Thanks Apple!
Comment 14 Kim Horne CLA 2008-11-27 13:09:28 EST
(In reply to comment #13)
> Thanks Apple!
> 

You are such a bitter, bitter man.  
Comment 15 Kim Horne CLA 2008-11-27 13:11:23 EST
Created attachment 118939 [details]
Updated patch that works on OS X

This appears to work fine on Carbon and Cocoa.
Comment 16 Boris Bokowski CLA 2008-11-28 16:28:39 EST
Unfortunately, it looks like this will slow down things quite a bit, at least when using my synthetic test case that measures refreshes per second.

Since I don't know how much this matters in practice, I have released the changes to HEAD for now so that we get a couple of builds with the font support (i.e. with the slow down).

Here are my numbers, from running org.eclipse.jface.tests.viewers.interactive.StyledCellLabelProviderTests (in ui.tests), for the case where owner draw is enabled:

                  old code               new code
Windows XP          16 rps                 15 rps
Linux GTK           33 rps                 27 rps
Mac Carbon          20 rps                 13 rps
Mac Cocoa           25 rps                 16 rps

(rps = refresh operations per second)

Now the question is, is the new feature (being able to have multiple fonts in one cell) worth the >30% slowdown for refresh operations on the Mac?

Comment 17 Boris Bokowski CLA 2008-11-28 17:06:42 EST
I just realized that the slowdown could be happening only when actually using multiple fonts, not just by switching to the new code. I updated my test program  with a new option to test this but haven't been able to run it on the platforms yet.
Comment 18 Boris Bokowski CLA 2008-12-03 17:36:38 EST
Here are some new numbers, all units are refresh operations per second, i.e. more is better.

Mac Carbon N20081201-2011

old code, no custom draw               39.5
new code, no custom draw               39.4
old code, custom draw single font      28.2
new code, custom draw single font      20.8
old code, custom draw and bold         n/a
new code, custom draw and bold         18.9

Mac Cocoa I20081202-1812

old code, no custom draw               60.0
new code, no custom draw               60.0
old code, custom draw single font      30.0
new code, custom draw single font      27.1
old code, custom draw and bold         n/a
new code, custom draw and bold         23.3

Linux GTK I20081202-1812

old code, no custom draw               58
new code, no custom draw               58
old code, custom draw single font      35
new code, custom draw single font      30
old code, custom draw and bold         n/a
new code, custom draw and bold         28


Comment 19 Boris Bokowski CLA 2008-12-08 13:03:14 EST
Windows XP N20081206-2000

old code, no custom draw               31
new code, no custom draw               31
old code, custom draw single font      18
new code, custom draw single font      17
old code, custom draw and bold         n/a
new code, custom draw and bold         16
Comment 20 Boris Bokowski CLA 2008-12-08 13:05:48 EST
Marking as fixed. The performance degradation does not seem to be too bad, and I don't think it will matter much overall. Let's see if there is any feedback.
Comment 21 Thomas Schindl CLA 2008-12-09 04:09:27 EST
Boris I saw you updated the Snippet to add this support - now people looking at the snippets under 3.4 get confused because fonts are not working there. Could we add a comment in the snippet to state that Font-Stuff only works in 3.5 M4.
Comment 22 Boris Bokowski CLA 2008-12-09 13:06:21 EST
(In reply to comment #21)
> Could
> we add a comment in the snippet to state that Font-Stuff only works in 3.5 M4.

Done.
Comment 23 Eric Jain CLA 2010-05-11 13:07:59 EDT
font works but fontStyle is ignored?
Comment 24 Dani Megert CLA 2010-05-12 02:25:13 EDT
(In reply to comment #23)
> font works but fontStyle is ignored?
See bug 131988.
Comment 25 Eclipse Genie CLA 2020-10-12 09:33:47 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170638