Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #509648 +++ Bug 384906 provides an implementation and API for setting the background color of table headers on Windows with stubs for the other platforms. Bug 509648 added the implementation for GTK. This bug is for the Mac (Cocoa) implementation.
New Gerrit change created: https://git.eclipse.org/r/94167
(In reply to Eclipse Genie from comment #1) > New Gerrit change created: https://git.eclipse.org/r/94167 Basic implementation for Cocoa. Known limitations (for possible improvement in a later patch): 1) No little separator lines between the header column. This is where you can click to resize the columns, so it would be nice to have them. We could try to draw our own. 2) The sort indicator is always black, which is a bit hard to see in the current "dark" design. I haven't found a way to style it. A possible solution might be to just use NSTableHeaderCell.sortIndicatorRectForBounds for the expected position and somehow draw our own sorting indicator (in the foreground color? with some alpha) where currently NSTableHeaderCell.drawSortIndicatorWithFrame is called. 3) There are still light grey horizontal lines above and below the header from the existing "native" little shadows of the header. I haven't found a way to style them, but I think the current solution is acceptable. (I guess they are drawn by NSTableHeaderCell drawWithFrame:inView: before it invokes drawInteriorWithFrame:inView:, but reimplementing that seems overkill)
(In reply to Till Brychcy from comment #2) > 1) No little separator lines between the header column. This is where you > can click to resize the columns, so it would be nice to have them. We could > try to draw our own. Done in patch set 2
(In reply to Till Brychcy from comment #2) > 2) The sort indicator is always black, which is a bit hard to see in the > current "dark" design. I haven't found a way to style it. A possible > solution might be to just use NSTableHeaderCell.sortIndicatorRectForBounds > for the expected position and somehow draw our own sorting indicator (in the > foreground color? with some alpha) where currently > NSTableHeaderCell.drawSortIndicatorWithFrame is called. Done in patch set 2). All of this could probably be some improved a up a bit, e.g. by extracting methods for the replicated code. Also the patches for 1) and 2) only kick in if headerForeground is set, too. I currently have no idea how the access the proper foreground color and headerForeground will probably be always set together witll headerBackground. Still I anybody has improvement suggestions...
(In reply to Till Brychcy from comment #4) > Done in patch set 2). I meant patch set 3. (In reply to Till Brychcy from comment #2) > 3) There are still light grey horizontal lines above and below the header > from the existing "native" little shadows of the header. I haven't found a > way to style them, but I think the current solution is acceptable. (I guess > they are drawn by NSTableHeaderCell drawWithFrame:inView: before it invokes > drawInteriorWithFrame:inView:, but reimplementing that seems overkill) I have actually experimented with this, but stopped when I noted that highlight:withFrame:inView: is used for drawing selected row headers, so we'd have to override that too. I couldn't do that with the existing native methods, so I stop here. I'd propose somebody looks at this as possible improvement for a later release.
Created attachment 267654 [details] Snippet to show the problems Till, thanks for providing the patch! I've tested the patch and it works well for most cases. Here are a few things that needs to be fixed: 1. Header background is set, header foreground is not set, then the header separator lines are not drawn. 2. Header background is not set, header foreground is set, the sort indicator doesn't use the header foreground color. 3. Header background is set and header is visible, but Table/Tree is empty, the header background is not applied. It's applied when a column is added.
(In reply to Lakshmi Shanmugam from comment #6) > 1. Header background is set, header foreground is not set, then the header > separator lines are not drawn. > > 2. Header background is not set, header foreground is set, the sort > indicator doesn't use the header foreground color. True, this is what i meant in comment 4 by "Also the patches for 1) and 2) only kick in if headerForeground is set,..." Do you have an suggestion how to access to proper foreground color to use in this case? > > 3. Header background is set and header is visible, but Table/Tree is empty, > the header background is not applied. It's applied when a column is added. Hmm I guess we may have to override a method in NSTableHeaderView to fix this. BTW, I've continued with my experiment for 3) from comment 2 (Thanks to the work of Gunnar Wagenknecht in Bug 506092 and Bug 514191 I was able to locally compile native bindings added with MacGeneratorUI) - I'll probably upload the result this evening or at least within the next few days.
(In reply to Till Brychcy from comment #7) > (In reply to Lakshmi Shanmugam from comment #6) > > 1. Header background is set, header foreground is not set, then the header > > separator lines are not drawn. Done in patch set 4 > > > > 2. Header background is not set, header foreground is set, the sort > > indicator doesn't use the header foreground color. Done in patch set 4 > [...] > Do you have an suggestion how to access to proper foreground color to use in > this case? To answer this myself: Just leave the current stroke color alone. > > > > > 3. Header background is set and header is visible, but Table/Tree is empty, > > the header background is not applied. It's applied when a column is added. Cocoa seems to draw a dummy column in this case, that takes over the header drawing, so our callbacks are not called. I think we can live without a workaround for this, as a table without columns doesn't seem useful to me at all. > BTW, I've continued with my experiment for 3) from comment 2 (Thanks to the > work of Gunnar Wagenknecht in Bug 506092 and Bug 514191 I was able to > locally compile native bindings added with MacGeneratorUI) - I'll probably > upload the result this evening or at least within the next few days. Also done in patch set 4!
(In reply to Till Brychcy from comment #5) > (In reply to Till Brychcy from comment #4) > > Done in patch set 2). > > I meant patch set 3. > > (In reply to Till Brychcy from comment #2) > > 3) There are still light grey horizontal lines above and below the header > > from the existing "native" little shadows of the header. I haven't found a > > way to style them, but I think the current solution is acceptable. (I guess > > they are drawn by NSTableHeaderCell drawWithFrame:inView: before it invokes > > drawInteriorWithFrame:inView:, but reimplementing that seems overkill) > > I have actually experimented with this, but stopped when I noted that > highlight:withFrame:inView: is used for drawing selected row headers, > so we'd have to override that too. Thanks for the updated patch. I'm not sure we need to implement highlight:withFrame:inView: & drawWithFrame:inView:. I didn't find any advantages of using them. I've restored your earlier patch and modified it to fix the gaps above and below the header. Please test the patch & let me know if you see any problems.
(In reply to Lakshmi Shanmugam from comment #9) > (In reply to Till Brychcy from comment #5) > > (In reply to Till Brychcy from comment #4) > > > Done in patch set 2). > > > > I meant patch set 3. > > > > (In reply to Till Brychcy from comment #2) > > > 3) There are still light grey horizontal lines above and below the header > > > from the existing "native" little shadows of the header. I haven't found a > > > way to style them, but I think the current solution is acceptable. (I guess > > > they are drawn by NSTableHeaderCell drawWithFrame:inView: before it invokes > > > drawInteriorWithFrame:inView:, but reimplementing that seems overkill) > > > > I have actually experimented with this, but stopped when I noted that > > highlight:withFrame:inView: is used for drawing selected row headers, > > so we'd have to override that too. > > Thanks for the updated patch. I'm not sure we need to implement > highlight:withFrame:inView: & drawWithFrame:inView:. I didn't find any > advantages of using them. I've restored your earlier patch and modified it > to fix the gaps above and below the header. Please test the patch & let me > know if you see any problems. It works. It just feels a bit awkward to me because drawInteriorWithFrame_inView now draws not just the interior and especially it draws outside the passed in cellRect. Maybe a comment at the locations where the background is drawn to point out that this is done and why would be helpful for future readers, e.g.: NSBezierPath.fillRect(headerRect); // note headerRect is larger than cellRect - drawing over already drawn borders in system colors Another thing: I don't know the conventions in the swt code base are, but in general I think it is better to avoid modifying passed in values (like in cellRect.height = headerRect.height) - it would feel cleaner if a fresh NSRect was created for this.
Gerrit change https://git.eclipse.org/r/94167 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=2a1c5d2e847d7f16904b853a1f87ec4ee70653f6
Follow-up commit to use new NSRect --> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a4fa067bc4382ed0b0d81af2ddab3089e4cb80ad
(In reply to Till Brychcy from comment #10) > (In reply to Lakshmi Shanmugam from comment #9) > > (In reply to Till Brychcy from comment #5) > > > (In reply to Till Brychcy from comment #4) > > > > Done in patch set 2). > > > > > > I meant patch set 3. > > > > > > (In reply to Till Brychcy from comment #2) > > > > 3) There are still light grey horizontal lines above and below the header > > > > from the existing "native" little shadows of the header. I haven't found a > > > > way to style them, but I think the current solution is acceptable. (I guess > > > > they are drawn by NSTableHeaderCell drawWithFrame:inView: before it invokes > > > > drawInteriorWithFrame:inView:, but reimplementing that seems overkill) > > > > > > I have actually experimented with this, but stopped when I noted that > > > highlight:withFrame:inView: is used for drawing selected row headers, > > > so we'd have to override that too. > > > > Thanks for the updated patch. I'm not sure we need to implement > > highlight:withFrame:inView: & drawWithFrame:inView:. I didn't find any > > advantages of using them. I've restored your earlier patch and modified it > > to fix the gaps above and below the header. Please test the patch & let me > > know if you see any problems. > Thanks for the feedback & testing! > It works. It just feels a bit awkward to me because > drawInteriorWithFrame_inView now draws not just the interior and especially > it draws outside the passed in cellRect. > > Maybe a comment at the locations where the background is drawn > to point out that this is done and why would be helpful for future readers, > e.g.: > > NSBezierPath.fillRect(headerRect); // note headerRect is larger than > cellRect - drawing over already drawn borders in system colors > I've added comments in the relevant places. > Another thing: I don't know the conventions in the swt code base are, but in > general I think it is better to avoid modifying passed in values (like in > cellRect.height = headerRect.height) - it would feel cleaner if a fresh > NSRect was created for this. I used cellRect directly as we return after that. Changed this to use new NSRect.
Awesome Tim and Lakshmi! Thanks a bunch for keeping consistency over the different platforms. Please update the existing N&N entry, so far we only speak about Win and Linux.
Thank you for this implementation. After trying with SmartGit on OS X 10.11 I have found following problem: we draw a border around tables (e.g. the Files view). Because the default table header has some kind of darker line at the top, we don't draw the top border of the rectangle on OS X. Unfortunately, with a set header background color, there is no such light line at the top and hence it looks strange. To make it look right, we would have to draw the full rectangle with a dark theme and don't paint the top border of the rectangle with the default look - the problem is that the border code does not know anything about a theme. My request: could the table header rendering please be made consistent regarding the top border - no matter whether the default look is used or a header color is set?
(In reply to Thomas Singer from comment #16) > Thank you for this implementation. After trying with SmartGit on OS X 10.11 > I have found following problem: we draw a border around tables (e.g. the > Files view). Because the default table header has some kind of darker line > at the top, we don't draw the top border of the rectangle on OS X. > Unfortunately, with a set header background color, there is no such light > line at the top and hence it looks strange. To make it look right, we would > have to draw the full rectangle with a dark theme and don't paint the top > border of the rectangle with the default look - the problem is that the > border code does not know anything about a theme. > > My request: could the table header rendering please be made consistent > regarding the top border - no matter whether the default look is used or a > header color is set? IIRC, I had some slightly different colored lines drawn in place of the existing borders in my patch set 4 (instead of simply drawing the background completely "flat" as in the resulting patch) Lakshmi didn't see the advantage, so I thought I was probably too perfectionist, but maybe this could be reactivated.
Created attachment 268160 [details] Sample to reproduce the top border problem
Created attachment 268161 [details] Screenshot illustrating the top-border problem
BTW, if I would have the choice, I would the default header not have the border, too, if the table is created without the SWT.BORDER flag. Then we'd not need a special handling for the OS X table.
(In reply to Thomas Singer from comment #20) > BTW, if I would have the choice, I would the default header not have the > border, too, if the table is created without the SWT.BORDER flag. Then we'd > not need a special handling for the OS X table. You could try to set the header background also for "light" designs, then you'd get the same flat behaviour as in your dark example. Would that help you?
Which color should I set? Maybe this: table.setHeaderBackground(table.getBackground()); table.setHeaderForeground(table.getForeground()); ?
(In reply to Thomas Singer from comment #22) > Which color should I set? Maybe this: > > table.setHeaderBackground(table.getBackground()); > table.setHeaderForeground(table.getForeground()); > > ? I guess that table.getBackground() would be null, so that wouldn't do anything. I thought of something like table.setHeaderBackground(parent.getDisplay().getSystemColor(SWT.COLOR_WHITE)); or maybe table.setHeaderBackground(parent.getDisplay().getSystemColor(SWT.COLOR_LIST_BACKGROUND)); (or maybe there is even something more appropriate) NOTE: You'll also loose the little separator line between the header and the table body like in the dark design. BTW: The dark theme in eclipse uses a slightly different dark gray as background for the header than for the body, maybe that would help with your design problem, too?
(In reply to Till Brychcy from comment #23) > (In reply to Thomas Singer from comment #22) > > Which color should I set? Maybe this: > > > > table.setHeaderBackground(table.getBackground()); > > table.setHeaderForeground(table.getForeground()); > > > > ? > > I guess that table.getBackground() would be null, so that wouldn't do > anything. IMHO it never returns null, but setting null causes to restore the default. > I thought of something like > > table.setHeaderBackground(parent.getDisplay().getSystemColor(SWT. > COLOR_WHITE)); > > or maybe > > table.setHeaderBackground(parent.getDisplay().getSystemColor(SWT. > COLOR_LIST_BACKGROUND)); Yes, this could be an option. > NOTE: You'll also loose the little separator line between the header and the > table body like in the dark design. I've noticed this as well. > BTW: The dark theme in eclipse uses a slightly different dark gray as > background for the header than for the body, maybe that would help with your > design problem, too? I'll play with the (dark theme) colors a little bit.
New Gerrit change created: https://git.eclipse.org/r/96599
Gerrit change https://git.eclipse.org/r/96599 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=f4e85592a048d6ae778ca231c07fde3713c1333f
(In reply to Lars Vogel from comment #15) > Awesome Tim and Lakshmi! Thanks a bunch for keeping consistency over the > different platforms. I'm glad you like it. A little detail: I am "Till", not "Tim"... > > Please update the existing N&N entry, so far we only speak about Win and > Linux. I don't think it is a good idea to change the N&N for M6 now. I think that should be done when the combined N&N for Oxygen is created. (In reply to Eclipse Genie from comment #26) > Gerrit change https://git.eclipse.org/r/96599 was merged to [master]. > Commit: > http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/ > ?id=f4e85592a048d6ae778ca231c07fde3713c1333f I've added a few words mentioning the mac support to the M7 N&N entry for bug 514387.
(In reply to Thomas Singer from comment #16) > My request: could the table header rendering please be made consistent > regarding the top border - no matter whether the default look is used or a > header color is set? I verified that the border around the header is not drawn when the header color is set. This behavior is same on Windows and looks fine when the SWT.BORDER style is not set on the Table. But with the SWT.BORDER style, the border around the Table (including the top line) should be drawn. This works as expected on Windows bit doesn't work on Mac. I'll open a separate bug to track this.
Verified the Table & Tree APIs in I20170509-2000.
(In reply to Lakshmi Shanmugam from comment #28) > But with the SWT.BORDER style, the border around the Table (including the > top line) should be drawn. This works as expected on Windows bit doesn't > work on Mac. I'll open a separate bug to track this. Opened Bug 516873 to track this.
With Bug 516873, these changes have been made to the initial fix. - When the header background color is set, a separator line between header and first row is drawn for Tree & Table. - With SWT.BORDER style, the complete border for the Table & Tree is drawn. When SWT.BORDER style is not set, no border is drawn.
Unless my snippet looks good for both cases (no set background, set background), we can't use the table header coloring for OS X.
(In reply to Thomas Singer from comment #32) > Unless my snippet looks good for both cases (no set background, set > background), we can't use the table header coloring for OS X. Can you pls try out the latest build and see if there are any problems?
Created attachment 268582 [details] Screenshot with latest changes As you can see from the attached screenshot, the problem is NOT resolved. Either the default table rendering (without top border) looks OK or the one with all borders drawn (themed). Why not get rid of the top header border if SWT.BORDER is not provided and no header color is set?
