Community
Participate
Working Groups
Created attachment 274175 [details] Screenshot This chevron is custom drawn. Maybe this drawing can be improved so that is looks also good in the dark theme.
Wim, you are a custom widget/ gc expert. Could you help in improving the drawing for both the light and the dark theme?
(In reply to Lars Vogel from comment #1) > Wim, you are a custom widget/ gc expert. Could you help in improving the > drawing for both the light and the dark theme? Thanks for the promotion. I think this is not SWT but must be some stack renderer addon no? WDYT?
Never mind, I think I found it in CTabFolderRenderer#drawChevron
(In reply to Wim Jongman from comment #3) > Never mind, I think I found it in CTabFolderRenderer#drawChevron Master Wim, can you have a look at this? I believe for you it will be trivial and it really affects my dark theme experience.
Resetting target, please re-target as required.
during the analysis of this bug I found another one. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=538456. I also proposed a solution for that bug there.
New Gerrit change created: https://git.eclipse.org/r/128454
New Gerrit change created: https://git.eclipse.org/r/128455
my two gerrits are a proof of concept. I know that the SWT change breaks API (therefore I needed to adapt code in platform.ui) - I just want got get feedback if the general approach is going into the right direction. The chevron does not look better on "normal" res displays (actually it's unchanged there). But is sharp and crisp on high resolution displays.
Created attachment 275630 [details] chevron crisp on high resolution display
Created attachment 275631 [details] chevron crisp unchanged on normal resolution display
(In reply to Matthias Becker from comment #11) > Created attachment 275631 [details] > chevron crisp unchanged on normal resolution display The "white" ugly border comes from the interpolation between the foreground color and the transparent background color. They are still there on the high resolution version but there are much smaller there (because of the higher resolution)
(In reply to Matthias Becker from comment #12) > (In reply to Matthias Becker from comment #11) > > Created attachment 275631 [details] > > chevron crisp unchanged on normal resolution display > > The "white" ugly border comes from the interpolation between the foreground > color and the transparent background color. They are still there on the high > resolution version but there are much smaller there (because of the higher > resolution) I had again a look at this. The code that causes this is marked with a "<<<": Image image = new Image (display, size.x - trim.width, size.y - trim.height); GC gc = new GC (image); RGB transparent; if (button == CTabFolderRenderer.PART_CHEVRON_BUTTON) { transparent = new RGB(0xFF, 0xFF, 0xFF); } else { transparent = new RGB(0xF7, 0, 0); } Color transColor = new Color(display, transparent); gc.setBackground(transColor); gc.fillRectangle(image.getBounds()); "drawing code goes here final ImageData imageData = image.getImageData (DPIUtil.getDeviceZoom ()); imageData.transparentPixel = imageData.palette.getPixel(transparent); "<<< Here an image is created and the background is set to white. After the the chevron is drawn (on the gc on that image) the image data is retrieved and the transparentPixel field is set to the "white" background. This is saying: "All white pixels are transparent". But not all pixels are either "white" or the chevrons foreground color. Due to the interpolation there are pixels that are "almost white". And these are not rendered as transparent and this creates this bade white border around the drawing. For testing purposes I change the code to (chagned are marked with "<<<): Image image = new Image (display, size.x - trim.width, size.y - trim.height); GC gc = new GC (image); RGB transparent; if (button == CTabFolderRenderer.PART_CHEVRON_BUTTON) { transparent = new RGB(67,67,70); "<<< } else { transparent = new RGB(0xF7, 0, 0); } Color transColor = new Color(display, transparent); gc.setBackground(transColor); gc.fillRectangle(image.getBounds()); "drawing code goes here final ImageData imageData = image.getImageData (DPIUtil.getDeviceZoom ()); //imageData.transparentPixel = imageData.palette.getPixel(transparent); "<<< I set the color of the created image to the background of control it is drawn on and do not use transparency at all. I will upload the result as a screenshot in a view moments. The result looks really nice. The white border is gone. But this is just a proof of concept. I got the new RGB(67,67,70) by using a color-picker tool to grab the actual background in the running eclipse. I did not find a way to get that color via code. I also used the CSS-Spy by inspecting the control hierarchy to get the background color but for all the controls (the MPlacHolder, the MArea, the CTabFolder, the toolbar, ...) I get something like RGB(81,86,88). Where can I get the correct background color from? Can somebody pls. give me a hint?
Created attachment 275671 [details] chevron not using transparency but the "correct" background
(In reply to Matthias Becker from comment #13) > color from? Can somebody pls. give me a hint? Pretty cool Matthias. I assume the code is in the Gerrit right? I do not see enough of it here to make a good suggestion. I will take a look at it.
(In reply to Wim Jongman from comment #15) > (In reply to Matthias Becker from comment #13) > > > color from? Can somebody pls. give me a hint? > > Pretty cool Matthias. I assume the code is in the Gerrit right? I do not see > enough of it here to make a good suggestion. I will take a look at it. No the code is not in Gerrit (yet). The attached Gerrits are a prove of concept for enabling drawing in high resolution. But this transparency issue is independent from the other. So I will create an independent gerrit change for this (with my demo-code outlined below).
New Gerrit change created: https://git.eclipse.org/r/128632
(In reply to Matthias Becker from comment #16) > No the code is not in Gerrit (yet). Now it is ;-)
(In reply to Wim Jongman from comment #15) > (In reply to Matthias Becker from comment #13) > > > color from? Can somebody pls. give me a hint? > > Pretty cool Matthias. I assume the code is in the Gerrit right? I do not see > enough of it here to make a good suggestion. I will take a look at it. I added you as a reviewer in gerrit.
New Gerrit change created: https://git.eclipse.org/r/128633
(In reply to Matthias Becker from comment #18) Ok, I got it. How about: Line 729 RGB transparent; if(button == CTabFolderRenderer.PART_CHEVRON_BUTTON){ transparent = chevronItem.getControl().getBackground().getRGB(); } else { transparent = new RGB(0xF7, 0, 0); }
(In reply to Wim Jongman from comment #21) > (In reply to Matthias Becker from comment #18) > > Ok, I got it. How about: > > Line 729 > > RGB transparent; > > if(button == CTabFolderRenderer.PART_CHEVRON_BUTTON){ > transparent = chevronItem.getControl().getBackground().getRGB(); > } else { > transparent = new RGB(0xF7, 0, 0); > } this causes NPEs. ChevronItem may be null, "getControl()" may return null.. If also would like to get rid of that if(button == CTabFolderRenderer.PART_CHEVRON_BUTTON) and would also use the same approach (without ransparency) for all the button images here.
> this causes NPEs. ChevronItem may be null, "getControl()" may return null.. Yes. It was a first glance before I could actually prepare my workspace and run. I figured that you would have seen that. > > If also would like to get rid of that > if(button == CTabFolderRenderer.PART_CHEVRON_BUTTON) > and would also use the same approach (without ransparency) for all the > button images here. Yes but it means we must pass the background color to the method. Which, like you said, requires us to find it in some way from where we are. Did you follow chevronItem.setImage(chevronImage); on line 2573?
(In reply to Wim Jongman from comment #23) > > > this causes NPEs. ChevronItem may be null, "getControl()" may return null.. > > Yes. It was a first glance before I could actually prepare my workspace and > run. I figured that you would have seen that. > > > > > If also would like to get rid of that > > if(button == CTabFolderRenderer.PART_CHEVRON_BUTTON) > > and would also use the same approach (without ransparency) for all the > > button images here. > > Yes but it means we must pass the background color to the method. Which, > like you said, requires us to find it in some way from where we are. > > Did you follow chevronItem.setImage(chevronImage); on line 2573? Even at this call "getControl" returns null. The JavaDoc of ToolItem#getControl says: "Returns the control that is used to fill the bounds of the item when the item is a SEPARATOR". So this does not help. Maybe you can checkout my gerrit change an play with the code by yourself.
Created attachment 275678 [details] Chevron not using transparency (In reply to Matthias Becker from comment #24) > So this does not help. Maybe you can checkout my gerrit change an play with > the code by yourself. Yes, I am playing with it. I found that background = chevronItem.getParent().getBackground().getRGB(); works. I think that we need to pass the background color to this method. In case of null the old code kicks in.
(In reply to Wim Jongman from comment #25) > Created attachment 275678 [details] > Chevron not using transparency > > (In reply to Matthias Becker from comment #24) > > > So this does not help. Maybe you can checkout my gerrit change an play with > > the code by yourself. > > Yes, I am playing with it. I found that > > background = chevronItem.getParent().getBackground().getRGB(); > > works. Not for me. In mac it returns (41,41,41) which is too dark - see screenshot
Created attachment 275680 [details] wrong background color (41,41,41) on mac
Created attachment 275682 [details] screen recording showing toolbar item issue in CTabFolder (In reply to Matthias Becker from comment #27) > Created attachment 275680 [details] > wrong background color (41,41,41) on mac The correct color in macOS dark theme is (70,70,73). And this comes from this dark theme stylesheet. ColorDefinition#org-eclipse-ui-workbench-INACTIVE_UNSELECTED_TABS_COLOR_END { color: #464649; category: '#org-eclipse-ui-presentation-default'; label: url('platform:/plugin/org.eclipse.ui.themes?message=INACTIVE_UNSELECTED_TABS_COLOR_END'); } You can configure this in the preferences. I did change that color to a red color for that screenshot. The screenshot was made with the original code (that still has the transparency issue). As you see in the lower part of the picture we already have an issue with "normal" toolbar items in that area. They have the wrong background color.
Matthias I have troubles adding to the same change set. This works on windows. Can you try on mac? Image createButtonImage(Display display, int button) { GC tempGC = new GC (this); Point size = renderer.computeSize(button, SWT.NONE, tempGC, SWT.DEFAULT, SWT.DEFAULT); tempGC.dispose(); Rectangle trim = renderer.computeTrim(button, SWT.NONE, 0, 0, 0, 0); Image image = new Image (display, size.x - trim.width, size.y - trim.height); GC gc = new GC (image); Color transColor = renderer.parent.getBackground(); gc.setBackground(transColor); RGB transparent = transColor.getRGB(); gc.setBackground(transColor); gc.fillRectangle(image.getBounds()); renderer.draw(button, SWT.NONE, new Rectangle(trim.x, trim.y, size.x, size.y), gc); gc.dispose (); final ImageData imageData = image.getImageData (DPIUtil.getDeviceZoom ()); imageData.transparentPixel = imageData.palette.getPixel(transparent); image.dispose(); image = new Image(display, new AutoScaleImageDataProvider(display, imageData, DPIUtil.getDeviceZoom())); return image; }
(In reply to Wim Jongman from comment #29) > Matthias I have troubles adding to the same change set. > > This works on windows. Can you try on mac? > > Image createButtonImage(Display display, int button) { > GC tempGC = new GC (this); > Point size = renderer.computeSize(button, SWT.NONE, tempGC, SWT.DEFAULT, > SWT.DEFAULT); > tempGC.dispose(); > Rectangle trim = renderer.computeTrim(button, SWT.NONE, 0, 0, 0, 0); > Image image = new Image (display, size.x - trim.width, size.y - > trim.height); > GC gc = new GC (image); > Color transColor = renderer.parent.getBackground(); > gc.setBackground(transColor); > RGB transparent = transColor.getRGB(); > gc.setBackground(transColor); > gc.fillRectangle(image.getBounds()); > renderer.draw(button, SWT.NONE, new Rectangle(trim.x, trim.y, size.x, > size.y), gc); > gc.dispose (); > final ImageData imageData = image.getImageData (DPIUtil.getDeviceZoom ()); > imageData.transparentPixel = imageData.palette.getPixel(transparent); > image.dispose(); > image = new Image(display, new AutoScaleImageDataProvider(display, > imageData, DPIUtil.getDeviceZoom())); > return image; > } I first didn't understand why this looks better as still transparency is used. But now I got it. The artifacts are still there but not less visible in the dark theme because they are also dark. I updated the gerrit patch by removing the testing-code and by adding a detailed commit message. So from my side it's finished. This is working fine on macOS. I tested with all available themes (also the [Windows] and [Linux] once). Can somebody test it on Linux? If it's also working nice there I would like to get it merged early for 4.10.
(In reply to Matthias Becker from comment #30) > This is working fine on macOS. I tested with all available themes (also the > [Windows] and [Linux] once). Can somebody test it on Linux? Looks beautiful under Linux. +1 for early merge.
Created attachment 275693 [details] Fix in dark theme on mac
Created attachment 275694 [details] Fix in light theme on mac
Created attachment 275696 [details] Screenshot
Gerrit change https://git.eclipse.org/r/128632 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=0dd8e12c56f609aa47708bdca09e29969d9fa44e
Service segment update in MANIFEST.MF will be done via https://git.eclipse.org/r/128807 See https://wiki.eclipse.org/Version_Numbering for details.
(In reply to Eclipse Genie from comment #35) > Gerrit change https://git.eclipse.org/r/128632 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=0dd8e12c56f609aa47708bdca09e29969d9fa44e The transparency artefacts are fixed with that change. But the chevron is sitll blurry on high resolution displays. My change https://git.eclipse.org/r/#/c/128454/ shows how this can be fixed. Unfortunately that change breaks API (see https://git.eclipse.org/r/#/c/128455/ what breaks in platform due to this change). When I understand it correctly it is by intention that CTabFolderRenderer can be subclassed to customize the rendering. I tried adding a new methods: protected void draw (int part, int state, Rectangle bounds, GC gc, int zoom) void drawChevron(GC gc, Rectangle chevronRect, int chevronImageState, int zoom) next to protected void draw (int part, int state, Rectangle bounds, GC gc) void drawChevron(GC gc, Rectangle chevronRect, int chevronImageState) So that e.g. CTabRendering in platform.ui does not break. From a compile point of view this works but I get some strange NPE when starting this. Any idea how I should proceed here? Is an incompatible change an option? If yes, what's the correct way of doing it? If not, how could a compatible change look like?
New Gerrit change created: https://git.eclipse.org/r/128813
(In reply to Eclipse Genie from comment #38) > New Gerrit change created: https://git.eclipse.org/r/128813 This are my changes I talked about in comment 37. I don't have a clue why this causes NPEs. Can somebody pls. have a look at it - maybe I am simply blind.
(In reply to Matthias Becker from comment #39) > (In reply to Eclipse Genie from comment #38) > > New Gerrit change created: https://git.eclipse.org/r/128813 > > This are my changes I talked about in comment 37. I don't have a clue why > this causes NPEs. Can somebody pls. have a look at it - maybe I am simply > blind. I don't see the NPEs any more - must have been an artefact locally. I will look into this further.
(In reply to Matthias Becker from comment #37) > Any idea how I should proceed here? Is an incompatible change an option? If > yes, what's the correct way of doing it? If not, > how could a compatible change look like? I think you could add a draw() method that takes and additional zoom parameter and assigns it to a field (which is used where now parameters are passed to) and at the end restores the previous zoom value.
(In reply to Till Brychcy from comment #41) > (In reply to Matthias Becker from comment #37) > > Any idea how I should proceed here? Is an incompatible change an option? If > > yes, what's the correct way of doing it? If not, > > how could a compatible change look like? > > I think you could add a draw() method that takes and additional zoom > parameter and assigns it to a field (which is used where now parameters are > passed to) and at the end restores the previous zoom value. In https://git.eclipse.org/r/#/c/128454/ patch set #5 I've implemented that idea. (In patch set #4 I had some unwanted changes as rebasing was slightly tricky).
(In reply to Till Brychcy from comment #42) > In https://git.eclipse.org/r/#/c/128454/ patch set #5 I've implemented that > idea. > (In patch set #4 I had some unwanted changes as rebasing was slightly > tricky). I think the vertical positioning needs some tuning, the number of tags in the chevron is slightly cut off at the bottom. Matthias, you understand that code better, can you take over again?
(In reply to Till Brychcy from comment #42) > (In reply to Till Brychcy from comment #41) > > (In reply to Matthias Becker from comment #37) > > > Any idea how I should proceed here? Is an incompatible change an option? If > > > yes, what's the correct way of doing it? If not, > > > how could a compatible change look like? > > > > I think you could add a draw() method that takes and additional zoom > > parameter and assigns it to a field (which is used where now parameters are > > passed to) and at the end restores the previous zoom value. > > In https://git.eclipse.org/r/#/c/128454/ patch set #5 I've implemented that > idea. > (In patch set #4 I had some unwanted changes as rebasing was slightly > tricky). Thank you Till. I had a similar version of that locally.
(In reply to Till Brychcy from comment #43) > (In reply to Till Brychcy from comment #42) > > In https://git.eclipse.org/r/#/c/128454/ patch set #5 I've implemented that > > idea. > > (In patch set #4 I had some unwanted changes as rebasing was slightly > > tricky). > > I think the vertical positioning needs some tuning, the number of tags in > the chevron is slightly cut off at the bottom. > Matthias, you understand that code better, can you take over again? I fix the vertical position. Now I really really looks nice on high resolution displays.
Created attachment 275813 [details] with the fix on a retina display
Created attachment 275814 [details] with the fix on a normal resolution display on a normal resolution display it doesn't get more crisp - I just is the same as before.
New Gerrit change created: https://git.eclipse.org/r/129411
@Till: Can you please review my change? @Linux / windows users: Does it also look good on your OS?
(In reply to Lars Vogel from comment #36) > Service segment update in MANIFEST.MF will be done via > https://git.eclipse.org/r/128807 See > https://wiki.eclipse.org/Version_Numbering for details. Next time we'd better create a new bug, no? This bug was solved with this comment.
(In reply to Wim Jongman from comment #50) > (In reply to Lars Vogel from comment #36) > > Service segment update in MANIFEST.MF will be done via > > https://git.eclipse.org/r/128807 See > > https://wiki.eclipse.org/Version_Numbering for details. > > Next time we'd better create a new bug, no? This bug was solved with this > comment. yes. the version of the swt bundle was already increased.
Created attachment 275850 [details] high resolution compared old (left) and new (right) Screenshot shows old and new next to each other. I don't see any alignment differences. I would say this is save to merge. Anyway it would be great if somebody with a linux or windows computer with a high resolution display could test this change.
(In reply to Matthias Becker from comment #52) > Created attachment 275850 [details] > high resolution compared old (left) and new (right) > > Screenshot shows old and new next to each other. I don't see any alignment > differences. > > I would say this is save to merge. > Anyway it would be great if somebody with a linux or windows computer with a > high resolution display could test this change. I'm not aware of a team member with HDPI screen on Linux or Windows. I suggest to go ahead with the merge.
(In reply to Lars Vogel from comment #53) > (In reply to Matthias Becker from comment #52) > I'm not aware of a team member with HDPI screen on Linux or Windows. I > suggest to go ahead with the merge. As the patch replaced some code that I suspected ( as written in the gerrit) was there for HiDPI screens, I think that merging this without testing is a bad idea.
(In reply to Till Brychcy from comment #54) > (In reply to Lars Vogel from comment #53) > > (In reply to Matthias Becker from comment #52) > > I'm not aware of a team member with HDPI screen on Linux or Windows. I > > suggest to go ahead with the merge. > > As the patch replaced some code that I suspected ( as written in the gerrit) > was there for HiDPI screens, I think that merging this without testing is a > bad idea. I agree but I'm not aware of anyone with HDPI. Do we have developers with HDPI on Windows or Linux?
If nobody else can do it, I can do it at least on windows, which should be enough. I have an old eclipse workspace on my bootcamp (= windows on mac) partition, but as I'd first have to update everything, this would probably a few days.
(In reply to Till Brychcy from comment #56) > If nobody else can do it, I can do it at least on windows, which should be > enough. Thanks!
(In reply to Till Brychcy from comment #56) > If nobody else can do it, I can do it at least on windows, which should be > enough. > > I have an old eclipse workspace on my bootcamp (= windows on mac) partition, > but as I'd first have to update everything, this would probably a few days. I have a co-worker that has a windows laptop with a 4k display. I will test with him together and report back.
I am checking this on fedora28 with gnome
(In reply to Lars Vogel from comment #53) I have a HiDPI screen and I am on Windows 10. I will check the patch now.
Seems like EVERYBODY has HDPI screens these days... ;-) I should update my hardware.
(In reply to Wim Jongman from comment #60) > (In reply to Lars Vogel from comment #53) > > > I have a HiDPI screen and I am on Windows 10. I will check the patch now. I'm confused. Which patch set do I need to test? 128454?
(In reply to Lars Vogel from comment #61) > Seems like EVERYBODY has HDPI screens these days... ;-) I should update my > hardware. It is overhyped. I am in 1920 mode most of the time.
(In reply to Wim Jongman from comment #62) > (In reply to Wim Jongman from comment #60) > > (In reply to Lars Vogel from comment #53) > > > > > > I have a HiDPI screen and I am on Windows 10. I will check the patch now. > > I'm confused. Which patch set do I need to test? 128454? didn't you just update your monitor? ;-)
(In reply to Wim Jongman from comment #62) > (In reply to Wim Jongman from comment #60) > > (In reply to Lars Vogel from comment #53) > > > > > > I have a HiDPI screen and I am on Windows 10. I will check the patch now. > > I'm confused. Which patch set do I need to test? 128454? https://git.eclipse.org/r/#/c/128454/
Created attachment 275855 [details] Windows 10 3480x2160 225% scaling It looks a little weird.
(In reply to Wim Jongman from comment #66) > Created attachment 275855 [details] > Windows 10 3480x2160 225% scaling > > It looks a little weird. how does it look like without my patch?
(In reply to Matthias Becker from comment #64 > didn't you just update your monitor? ;-) Co-worker "stole" it while I was in Walldorf. Second one ordered now.
Created attachment 275856 [details] Current SWT Master Windows 10 3480x2160 225% scaling This is the SWT from master.
(In reply to Wim Jongman from comment #69) > Created attachment 275856 [details] > Current SWT Master Windows 10 3480x2160 225% scaling > > This is the SWT from master. It looks fine IMHO. Is it worth the effort to push to perfection?
Created attachment 275858 [details] Screen shots on Kubuntu with 200% scale I am seeing weird behavior. There is a change in the layout with the patch and the number is larger. I think that is not correct.
Created attachment 275859 [details] screenshots with fedora 28 with gnome
thanks for testing. Really really strange what happens on Linux and Windows. I will look into it.
Dear Wim and Sravan, I just uploaded a new patchset that includes some debugging output. Could you re-do your test and paste the console output to this bug? I hope this helps to understand why this behaves so differently on linux and windows.
(In reply to Wim Jongman from comment #70) > (In reply to Wim Jongman from comment #69) > > Created attachment 275856 [details] > > Current SWT Master Windows 10 3480x2160 225% scaling > > > > This is the SWT from master. > > It looks fine IMHO. Is it worth the effort to push to perfection? it interestingly locks not that bad on windows. But compare with my screenshots how it looks on mac - it's really bad there.
(In reply to Matthias Becker from comment #75) I had to rebase your patch with master in order to be synced with the latest binaries. When I resize my editor initially this appeared. createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE Then I resized some more and this appeared: createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE getChevronFont zoom: 100 FontData: 1|Segoe UI|6.0|0|WINDOWS|1|-27|0|0|0|0|0|0|0|1|0|0|0|0|Segoe UI createButtonImage zoom:200 size: Point {48, 32} getChevronFont zoom: 200 FontData: 1|Segoe UI|12.0|0|WINDOWS|1|-27|0|0|0|0|0|0|0|1|0|0|0|0|Segoe UI drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {48, 32} drawChevron chevronImageState: NONE
(In reply to Matthias Becker from comment #74) > Dear Wim and Sravan, > > I just uploaded a new patchset that includes some debugging output. Could > you re-do your test and paste the console output to this bug? > I hope this helps to understand why this behaves so differently on linux and > windows. This is the output I get getChevronFont zoom: 100 FontData: 1|Noto Sans|7.0|0|GTK|1| createButtonImage zoom:200 size: Point {54, 36} getChevronFont zoom: 200 FontData: 1|Noto Sans|14.0|0|GTK|1| drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {54, 36} drawChevron chevronImageState: NONE after resize I get createButtonImage zoom:200 size: Point {54, 36} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {54, 36} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {54, 36} drawChevron chevronImageState: NONE
on macOS it's: getChevronFont zoom: 100 FontData: 1|.SF NS Text|10.0|0|COCOA|1|.SFNSText createButtonImage zoom:100 size: Point {28, 16} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {56, 32} getChevronFont zoom: 200 FontData: 1|.SF NS Text|20.0|0|COCOA|1|.SFNSText drawChevron chevronImageState: NONE createButtonImage zoom:100 size: Point {28, 16} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {56, 32} drawChevron chevronImageState: NONE createButtonImage zoom:100 size: Point {28, 16} drawChevron chevronImageState: NONE createButtonImage zoom:200 size: Point {56, 32} drawChevron chevronImageState: NONE
when I strip out the duplicated lines it boils down to: macOS getChevronFont zoom: 100 FontData: 1|.SF NS Text|10.0 getChevronFont zoom: 200 FontData: 1|.SF NS Text|20.0 createButtonImage zoom:100 size: Point {28, 16} createButtonImage zoom:200 size: Point {56, 32} Linux: getChevronFont zoom: 100 FontData: 1|Noto Sans|7.0 getChevronFont zoom: 200 FontData: 1|Noto Sans|14.0 createButtonImage zoom:200 size: Point {54, 36} Windows: getChevronFont zoom: 100 FontData: 1|Segoe UI|6.0 getChevronFont zoom: 200 FontData: 1|Segoe UI|12.0 createButtonImage zoom:200 size: Point {48, 32} The font sizes look right to me. The sizes given in createButtonImage are the one *after* scaling. The values for the 200 version are also very similar. For mac both buttom image are created for linux and window only the 200 version is created. But this seems to be an implementation detail. I don't have a clue how does wrong here? I thought that the image data provider simply return the image based on the zoom level and SWT / OS then takes care to display the correct one. I looks like as on windows and linux the image is twice as big as it should be. Also I don't understand Wim's screenshot https://bugs.eclipse.org/bugs/attachment.cgi?id=275856 form the state without my patch. Why is the chevron and the number next to is sharp and crisp compare to the situation on macOS https://bugs.eclipse.org/bugs/attachment.cgi?id=275850. I thought that as the image is drawn on 100% it is then auto-scaled up by SWT / OS on all platforms and therefore does get blurry due to that scaling. Can some of the SWT-Experts help understanding this?
(In reply to Matthias Becker from comment #79) > when I strip out the duplicated lines it boils down to: > > macOS > > getChevronFont zoom: 100 FontData: 1|.SF NS Text|10.0 > getChevronFont zoom: 200 FontData: 1|.SF NS Text|20.0 > createButtonImage zoom:100 size: Point {28, 16} > createButtonImage zoom:200 size: Point {56, 32} > > > Linux: > > getChevronFont zoom: 100 FontData: 1|Noto Sans|7.0 > getChevronFont zoom: 200 FontData: 1|Noto Sans|14.0 > createButtonImage zoom:200 size: Point {54, 36} > > > Windows: > > getChevronFont zoom: 100 FontData: 1|Segoe UI|6.0 > getChevronFont zoom: 200 FontData: 1|Segoe UI|12.0 > createButtonImage zoom:200 size: Point {48, 32} > > The font sizes look right to me. The sizes given in createButtonImage are > the one *after* scaling. The values for the 200 version are also very > similar. > For mac both buttom image are created for linux and window only the 200 > version is created. But this seems to be an implementation detail. > > I don't have a clue how does wrong here? I thought that the image data > provider simply return the image based on the zoom level and SWT / OS then > takes care to display the correct one. I looks like as on windows and linux > the image is twice as big as it should be. > Also I don't understand Wim's screenshot > https://bugs.eclipse.org/bugs/attachment.cgi?id=275856 form the state > without my patch. Why is the chevron and the number next to is sharp and > crisp compare to the situation on macOS > https://bugs.eclipse.org/bugs/attachment.cgi?id=275850. > I thought that as the image is drawn on 100% it is then auto-scaled up by > SWT / OS on all platforms and therefore does get blurry due to that scaling. > > Can some of the SWT-Experts help understanding this? I will do a review on Monday
After reviewing here are my observations for creating the chevron. 1. Create a GC 2. Draw chevron scaledup(zoomed up) dimensions. - lines drawn with zoom *coordinates - font size scaled up The issue here is the way we create GC and draw on GC in hidpi mode. On Windows and Linux KDE we do the following 1. Create GC with scaled up surface. 2. All GC operations are scaled up using dpiutil methods. So the image is drawn using scaled up coordinates. On Linux Gnome. 1. Create cairo surface with scaled up size but set the device_scale parameter to the surface to zoom. 2. All GC operations are cairo drawing operations. These are scaled up by cairo automatically On Mac 1. We create GC with 100% representation (ideally we should create a new blank representation with the current zoom level) 2. Draw using the GC methods. With this we get a 100% representation drawn properly and that representation is used for display by scaling up. This causes blurriness in the final display. I hope this helps
(In reply to Sravan Kumar Lakkimsetti from comment #81) > After reviewing here are my observations for creating the chevron. > > 1. Create a GC > 2. Draw chevron scaledup(zoomed up) dimensions. > - lines drawn with zoom *coordinates > - font size scaled up > > The issue here is the way we create GC and draw on GC in hidpi mode. > > On Windows and Linux KDE we do the following > 1. Create GC with scaled up surface. > 2. All GC operations are scaled up using dpiutil methods. So the image is > drawn using scaled up coordinates. > > On Linux Gnome. > 1. Create cairo surface with scaled up size but set the device_scale > parameter to the surface to zoom. > 2. All GC operations are cairo drawing operations. These are scaled up by > cairo automatically > > On Mac > 1. We create GC with 100% representation (ideally we should create a new > blank representation with the current zoom level) > 2. Draw using the GC methods. > > With this we get a 100% representation drawn properly and that > representation is used for display by scaling up. This causes blurriness in > the final display. > > I hope this helps Sravan, thank you for the review and explaining. Is this GC-creation stuff you talk about valid everywhere or just for this place? If it's valid everywhere: Does this mean the GC behaves differently on the different platforms? Shouldn't SWT shield the client code from such differences? What's your proposal to fix this issue?
(In reply to Matthias Becker from comment #82) > (In reply to Sravan Kumar Lakkimsetti from comment #81) > > After reviewing here are my observations for creating the chevron. > > > > 1. Create a GC > > 2. Draw chevron scaledup(zoomed up) dimensions. > > - lines drawn with zoom *coordinates > > - font size scaled up > > > > The issue here is the way we create GC and draw on GC in hidpi mode. > > > > On Windows and Linux KDE we do the following > > 1. Create GC with scaled up surface. > > 2. All GC operations are scaled up using dpiutil methods. So the image is > > drawn using scaled up coordinates. > > > > On Linux Gnome. > > 1. Create cairo surface with scaled up size but set the device_scale > > parameter to the surface to zoom. > > 2. All GC operations are cairo drawing operations. These are scaled up by > > cairo automatically > > > > On Mac > > 1. We create GC with 100% representation (ideally we should create a new > > blank representation with the current zoom level) > > 2. Draw using the GC methods. > > > > With this we get a 100% representation drawn properly and that > > representation is used for display by scaling up. This causes blurriness in > > the final display. > > > > I hope this helps > > Sravan, thank you for the review and explaining. Is this GC-creation stuff > you talk about valid everywhere or just for this place? > If it's valid everywhere: Does this mean the GC behaves differently on the > different platforms? Shouldn't SWT shield the client code from such > differences? > > What's your proposal to fix this issue? Unfortunately this is valid everywhere. I am currently working on fixing this issue on Mac. other platforms we do shield this from the end users.
(In reply to Sravan Kumar Lakkimsetti from comment #83) > Unfortunately this is valid everywhere. I am currently working on fixing > this issue on Mac. other platforms we do shield this from the end users. So would that mean that my fix is no longer needed after you fix? Pls. keep me updated on your fix.
(In reply to Matthias Becker from comment #84) > (In reply to Sravan Kumar Lakkimsetti from comment #83) > > Unfortunately this is valid everywhere. I am currently working on fixing > > this issue on Mac. other platforms we do shield this from the end users. > So would that mean that my fix is no longer needed after you fix? > Pls. keep me updated on your fix. Most likely it may not be required. GC related effort is being tracked in Bug 489451
Gerrit change https://git.eclipse.org/r/129411 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=b2e893fed155f811b92accdc60b3734831b507e6
(In reply to Eclipse Genie from comment #86) > Gerrit change https://git.eclipse.org/r/129411 was merged to [master]. > Commit: > http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=b2e893fed155f811b92accdc60b3734831b507e6 > Added bug number comment to the entry - http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=ff98913a88f2354d653ba4d66b082ffe8a47102d
*** Bug 464230 has been marked as a duplicate of this bug. ***
Matthias, Can you please resolve this bug (as some changes went in for 4.10) and open a new one for the remaining issue?
(In reply to Lakshmi Shanmugam from comment #89) > Matthias, > Can you please resolve this bug (as some changes went in for 4.10) and open > a new one for the remaining issue? I'm marking this as fixed. Please open new bugs as mentioned by Lars.
