Community
Participate
Working Groups
Example stack: Exception in thread "main" java.lang.IllegalArgumentException: Argument not valid at org.eclipse.swt.SWT.error(SWT.java:4850) at org.eclipse.swt.SWT.error(SWT.java:4784) at org.eclipse.swt.SWT.error(SWT.java:4755) at org.eclipse.swt.graphics.GC.drawImage(GC.java:870) at org.eclipse.swt.graphics.GC.drawImage(GC.java:854) at org.eclipse.swt.custom.CLabel.onPaint(CLabel.java:577) <...> I will provide more information soon.
Created attachment 284710 [details] Reproducing snippet
This is a regression from Bug 564097. Paul, can you please have a look?
Yep, looking into it.
Hello. During my time working on the hidpi issues for icons I also stumbled along this check, which is throwing the IAE. //rounding error correction for hidpi if (srcX + srcWidth > imgWidth + 1 || srcY + srcHeight > imgHeight + 1) { SWT.error(SWT.ERROR_INVALID_ARGUMENT); } Question is why is it doing this check, and what does it have to do with the "simple" parameter passed into the drawImage (line 856 of GC) function. When I comment out this check I don't see any problems in Control Example, or Eclipse. The comment attached with the function has to do with the addition +1 to allow for error correction but nothing to do with why the arguments are invalid. What I found from debugging was that during the switch of screen DPI is due to the image updating its size and width to accommodate for the new 200% DPI, the imageRects are then updated to the new sizes (this passes the check, srcWidth > imgWidth). However, drawImage is called again after, however this time the image is reverted back to its own DPI, and now srcWidth > imgWidth is triggered due to the first call. I believe the reason the image resets to the old DPI is due to my second monitor. Now what I'm wondering is if you also have two monitors, so I can confirm my reasoning.
According to my understanding, the assertion verifies that source image has the requested rect. For example, for a 10x10 image it's wrong to try to copy 10 pixels from pixel 5 - that would mean that 5 pixels on the right of the copy rect are outside the source image. I do not have 2 monitors. However, user who reported it does have two monitors. I understand that it's irrelevant.
(In reply to Soraphol (Paul) Damrongpiriyapong from comment #4) > Hello. During my time working on the hidpi issues for icons I also stumbled > along this check, which is throwing the IAE. > > //rounding error correction for hidpi > if (srcX + srcWidth > imgWidth + 1 || srcY + srcHeight > imgHeight + 1) { > SWT.error(SWT.ERROR_INVALID_ARGUMENT); > } > > Question is why is it doing this check, and what does it have to do with the > "simple" parameter passed into the drawImage (line 856 of GC) function. When > I comment out this check I don't see any problems in Control Example, or > Eclipse. The comment attached with the function has to do with the addition > +1 to allow for error correction but nothing to do with why the arguments > are invalid. > > What I found from debugging was that during the switch of screen DPI is due > to the image updating its size and width to accommodate for the new 200% > DPI, the imageRects are then updated to the new sizes (this passes the > check, srcWidth > imgWidth). However, drawImage is called again after, > however this time the image is reverted back to its own DPI, and now > srcWidth > imgWidth is triggered due to the first call. I believe the reason > the image resets to the old DPI is due to my second monitor. > > Now what I'm wondering is if you also have two monitors, so I can confirm my > reasoning. In the drawImage user will copy from a location from srcImage. the srcX, srcY points to starting position and srcWidth and srcHeight determines how much width and height that needs to be copied. Now srcX+srcWidth should not exceed srcImage.width other wise you'll be accessing a non existent memory location(IAE). Same case with height as well. During HiDpi work our first implementation depended on image manipulation is SWT itdelf. basically when required we are scaling up and scaling down the images. We had trouble when we had fractional scaling factors. Since we don't support fractional scaling we don't need +1 in that error check.
It would be really nice to have this fixed. This problem is one of the larger sources of unexpected exceptions for us since we updated to the newer SWT.
Yes, I'm looking into it, I will try my best to get it out by end of this week.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172538
A patch has been made. Hopefully we will be able to have it in within the next Eclipse release.
Thanks! I tried to review it, but figured that I don't quite understand how auto-scaling works. I'll try again on Monday.
I have now studied how auto-scaling works. To my understanding, it can be explained in very simple words: -------------------------------------------------------------- C1) Every Cairo surface has associated DPI. C2) When transferring between surfaces, Cairo automatically scales transfer to match DPI of destination surface. This is how I would expect SWT to work: --------------------------------------- S1) Have 'internal_dpi' for coordinates. This is the DPI to which coords are converted when interacting with SWT user via methods like 'Image.getBounds()', 'Control.getBounds()' etc. S2) Have 'screen_dpi' for actual DPI of current screen. S3) Every 'Image' should have its own 'image_dpi'. This is similar to 'cairo_surface_set_device_scale()' in Cairo. S4) S1+S2 means that for 'Control', SWT automatically scales coords: (internal_dpi -> screen_dpi) S5) S1+S3 means that for 'Image', SWT automatically scales coords: (internal_dpi -> image_dpi -> screen_dpi) The second conversion is not needed if SWT managed to call 'cairo_surface_set_device_scale()', because in this case Cairo will perform it. Alternatively, SWT could tell Cairo to use 'internal_dpi' for 'image_dpi'. Finally, let's compare the plan (S1-S5) above to what's actually happening in SWT. I will only highlight discrepancies that I noticed. Correct me if I misunderstood something in code, because I didn't spend much time on it. Also, I didn't test anything and only judge from my understanding. I noticed the following discrepancies: -------------------------------------- D1) 'internal_dpi' is hardcoded as '100' or '100f' in code, instead of having a dedicated variable for that. D2) With 'swt.autoScale' property set to 'false', SWT uses 'DPIUtil.deviceZoom=100'. I understand that this means 'screen_dpi=100%' in terms of (S1-S5) plan. This is incorrect; SWT shall instead set 'internal_dpi=screen_dpi'. This has the biggest effect in (S5), where the conversion formula is broken. See also (D1). D3) 'Image.getBounds()' shall not test for 'useCairoAutoScale', because regardless of it, the formula should be: (image_dpi -> internal_dpi). I understand that this is a consequence of (D2). D4) Private 'GC.drawImage()' incorrectly receives coords scaled as: (internal_dpi -> screen_dpi). This formula is missing intermediate 'image_dpi' step, which probably causes even more weirdness later. I think that it's a good idea to get rid of this pre-scaling and instead to (S5) inside the private function. D5) 'SWT.error()' in 'GC.drawImage()', from which discussion has started, should map 'src*' coordinates to DPI of the Image before making the comparison: (internal_dpi -> image_dpi). D6) I would expect 'Image.width', 'Image.height' to be expressed in DPI of the Image, that is, in pixels. Otherwise, code could become quite a surprise to anyone who doesn't have experience. D7) SWT shall never calculate magnification factor (DPI_1/DPI_2) as a single 'float', because it leads to rounding errors. One example of such problems is probably '+1' part of condition for 'SWT.error()' in 'GC.drawImage()'. Instead, all conversion functions shall accept two int's and perform integer arithmetics. I see the following plan as reasonable: --------------------------------------- P1) Fix exception in 'GC.drawImage()'. It understand that current suggested patch unfortunately strays even further from (S1-S5). Instead, (D4+D5) could be a sufficient fix. P2) Fix other problems in (D1-D7), and the ones I didn't notice, in a larger patch for next release. I suggest to make separate patches when fixing (D1-D6) etc, otherwise it's hard to review. For example, getting rid of float values is a good standalone patch. I would be happy if someone else takes care of it, because my TODO list is overflowing badly at the moment :(
Please also consider that the screen dpi value can be changed by the user while the application runs.
(In reply to Alexandr Miloslavskiy from comment #12) > I have now studied how auto-scaling works. > > To my understanding, it can be explained in very simple words: > -------------------------------------------------------------- > C1) Every Cairo surface has associated DPI. > C2) When transferring between surfaces, Cairo automatically scales > transfer to match DPI of destination surface. > Actually there is no need to hold screen dpi or internal dpi. We can directly deal with user level coordinates(100%). Only thing is we need to set appropriate device_scale when ever we are creating a new surface. Only for this purpose we need dpi. There is another complication there are about 4 classes where we use pixbuf. Pixbug doesn't have capability of handling hidpi. at this place we need to hold dpi at which the pixbuf is created in the image object. This way we can still have the image dimensions using 100% co-ordinates. If you look at the autoscaleup and autoscaledown methods they return same value in case of cairo scaling. Actually we should get rid of scaleup/scaledown completely. This is still there because cairo scaling doesn't work on KDE or unity.
(In reply to Thomas Singer from comment #13) > Please also consider that the screen dpi value can be changed by the user > while the application runs. Actually this is the usecase that caused current bug.
Holding 'internal_dpi' and 'screen_dpi' is inevitable if you want things such Control positions/sizes to be scaled. This would be true even if SWT didn't have 'Image' class at all. Just now one value is explicit, and the other is hardcoded in many places as '100'. It doesn't really change the fact that there are two values. As for Image, even with Cairo auto-scaling, it's still visually better to have different images for different DPI to avoid blurriness. It is currently implemented with 'ImageDataProvider', which creates images for specified DPI on demand. But then, how do you specify a range of pixels in such an image? Let's make an example: consider a 20x20 image at 200%. 10x10 - SWT size of image, as if it was scaled to 100% 20x20 - real size of image, as Cairo sees it. Cairo is also told that image has 200% DPI. If I understand you correctly, you're suggesting to forget 200% and forget 20x20, only storing 10x10. This lets you answer to 'Image.getBounds()', but how do you give image size to Cairo? As I understand it, it's inevitable that you must know the Image's scaling also.
(In reply to Alexandr Miloslavskiy from comment #16) > Holding 'internal_dpi' and 'screen_dpi' is inevitable if you want > things such Control positions/sizes to be scaled. This would be true > even if SWT didn't have 'Image' class at all. Just now one value is > explicit, and the other is hardcoded in many places as '100'. It > doesn't really change the fact that there are two values. > I know this question, I had similar question when I started work on Hidpi. It turned out we never set sizes. We provide desired size and its gtk which does layout for us. So no need to do scaleup or scaledown here. All the co-ordinates are in user level co-ordinates provided to GTK. It is GTK that handles dpi manipulation. > As for Image, even with Cairo auto-scaling, it's still visually better > to have different images for different DPI to avoid blurriness. It is > currently implemented with 'ImageDataProvider', which creates images > for specified DPI on demand. > > But then, how do you specify a range of pixels in such an image? > Let's make an example: consider a 20x20 image at 200%. > 10x10 - SWT size of image, as if it was scaled to 100% > 20x20 - real size of image, as Cairo sees it. Cairo is also told that > image has 200% DPI. > > If I understand you correctly, you're suggesting to forget 200% and > forget 20x20, only storing 10x10. This lets you answer to > 'Image.getBounds()', but how do you give image size to Cairo? > For the Image the idea was is to store the dimensions in user level co ordinates i.e 10X10, Now when eclipse is running on 200% monitor cairo surface is created with 20X20, with device_scale as 2. Please take a look at https://git.eclipse.org/c/platform/eclipse.platform.swt.git/tree/bundles/org.eclipse.swt/Eclipse%20SWT/gtk/org/eclipse/swt/graphics/Image.java?id=5b6474909194a96accf81dc255ae51cd4c018c34#n701 In this constructor we are fetching imageData as required scaling factor via DPIUtil.validateAndGetImageDataAtZoom. If we have data(determined by found[0] flag, we create surface required scaling factor in init(data) method. If required imagedata is not found we scaleup 100% data to required scaling factor before passing on to init(data). in case of cairo surface you can get surface dimensions using cairo_image_surface_get_width and cairo_image_surface_get_height. but getting device_scale for the surface will provide us the scalefactor at which this particular surface is created. In case of pixbuf we don't have this ability so we store the scalefactor at which the pixbuf is created in the image object. > As I understand it, it's inevitable that you must know the Image's > scaling also. We do have this information in Image object. There is another thing we do Image.refreshImage(), this recreates Image object at the current scalefactor. this gets called whenever we draw a Image.
> It is GTK that handles dpi manipulation. Thanks for clarification. Indeed, I didn't expect that cairo auto scaling also affects GTK controls. My updated understanding is that for Image, your plan is: P1) only deal with 100% coordinates in SWT and let Cairo handle the rest P2) when user switches screen DPI, images are updated to new DPI P3) there is some additional code for pixbuf, because it doesn't support DPI on its own. Is that correct?
Created attachment 284877 [details] Reproducing snippet Here is an updated test snippet. Please add it to the patch. It still detects problems, namely, why does .getBoundsInPixels() now return 10x10?
(In reply to Alexandr Miloslavskiy from comment #18) > > It is GTK that handles dpi manipulation. > > Thanks for clarification. Indeed, I didn't expect that cairo auto scaling > also affects GTK controls. > > My updated understanding is that for Image, your plan is: > P1) only deal with 100% coordinates in SWT and let Cairo handle the rest > P2) when user switches screen DPI, images are updated to new DPI > P3) there is some additional code for pixbuf, because it doesn't support > DPI on its own. > > Is that correct? Yes that is correct.
(In reply to Alexandr Miloslavskiy from comment #19) > Created attachment 284877 [details] > Reproducing snippet > > Here is an updated test snippet. Please add it to the patch. > > It still detects problems, namely, why does .getBoundsInPixels() now return > 10x10? getBoundsInPixels is an internal method, not supposed to be used by end users. We made it public as we needed to use this in SWT packages. that is also one of the reasons why it is deprecated. Actually with cairo scaling this will always return 10X10 as gtk and cairo are handling the dpi management. If you see Image.getBounds(), you'll see autoScaleDown method. This method checks whether we are using cairo scaling and if cairo scaling is used, the sizes are multiplied with a scalefactor of 1(meaning returns the same values as the input) Most of the confusion arises from the DPIUtil methods. Most of these are not useful anymore(They are used in Windows).
Created attachment 284887 [details] Reproducing snippet
Created attachment 284888 [details] Screenshot comparing various combinations of patches and os When I see two methods, one of which is named "getBounds()" and the other "getBoundsInPixels", and the second does NOT return pixels, that's when I know that code shouldn't be that way. Honestly, I still can't decode the design behind current calculations. In my perception, it's a combination of lots of variables and branches that I still can't fit into a single picture. So, I have grown suspicious and went on a hunt for the case of cairo auto scaling disabled. With the help of Bug 536542 I arrived at testing KUbuntu 18.04. And things are really weird there, see screenshot: 1) Without the current patch, it just crashes. Well, that's why we are discussing things. 2) With current patch, both image positions and sizes are scaled for 100% instead of 200%. 3) I then checked out SWT at commit for Bug 536542. It looks good to me. 4) I found that 'setUseCairoAutoScale()' is broken since Bug 539392. The simplest way to revert it is to delete (GTK > 3.22) code from 'Device.getDeviceZoom()'. Without this change, 'setUseCairoAutoScale()' is true even on Ubuntu 18.04. 5) With 536542 reverted, there are two problems: * drawImage(simple=false) only draws 1/4 of image * Something is wrong with image positioning - notice no gaps between images 6) With (5) and patch applied, it's even worse: * For all images only 1/4 of image is drawn * drawImage(simple=false) are also scaled incorrectly * image positions are wrong To me, it's an indication that not only me, but also you are not understanding the permutations of hacks currently involved. And when authors themselves are confused by the code, that's when I know that it needs to be changed. I once again suggest to adopt the logic where there are {internal_dpi, image_dpi, device_dpi} and a function to transform between any two of these. This will allow to remove various hacks. Note that since last iteration, I replaced 'screen_dpi' with 'screen_dpi', because nowhere in GTK we're dealing with screen coords. Examples: --------- In case of (useCairoAutoScale = true): * Set (device_dpi = 100%), because GTK's "device" is expressed in 100% coordinates and handles transformations itself. In case of "swt.autoScale=false": * Set 'internal_dpi' equal to 'device_dpi'. This means that SWT gives direct access to device coordinates. In case of 'Image': * Keep cairo sizes (that is, expressed in 'image_dpi') * 'Image.getBounds()' will transform these to 'internal_dpi' In case of 'GC.drawImage()': * source and dest coords are expressed in 'internal_dpi' * AFTER image is updated for new DPI, source/dest coords need to be transformed into 'image_dpi'. This will automatically fix condition for 'SWT.error()'. With such approach, all conversions are really simple, you just need to know: * which type of coordinates you have * which type of coordinates you want * 3 numbers {internal_dpi, image_dpi, device_dpi}
Let me try to put it with a different words for easier understanding. * Forget about current code for a moment. * Is it correct that every thing, such as Image, screen, GTK, SWT, Controls, Widgets - all have some secret DPI in which they express their coordinates? My answer is "yes". * Is it correct that we can learn that secret DPI for every thing? My answer is "yes". * Is it correct that if we know DPI for every thing, and also know which coords belong to which thing, it's only a matter of simple transformation to convert coords? My answer is "yes". * Note that I didn't have to notice any edge cases, such as cairo scaling, because they are not really edge cases anymore in such view. All such edge cases are will merely affect secret DPI for things, but not the transformation formula. In my understanding, current code has the following problems: * It assigns secret DPIs incorrectly * It doesn't correctly know which coords belong to which DPI * It has multiple transofmation formulas - for example, some consider cairo scaling and others don't. It desperately tries to fix first two problems by adjusting transformation formulas for every case. Good example is code that does "if (cairo scaling) {image.getBounds()} else {image.getBoundsInPixels()}"
(In reply to Alexandr Miloslavskiy from comment #23) > > To me, it's an indication that not only me, but also you are not > understanding the permutations of hacks currently involved. And when > authors themselves are confused by the code, that's when I know that > it needs to be changed. > You are not wrong As the original author of the this feature, I can confirm hidpi doesn't work on KDE. The logic used in that path is really trouble some(KDE path uses same logic as windows). Logic used on the Gnome path is very much similar to Mac. Many times I feel to remove kde code path completely and reimplement hidpi for that path. Atleast the performance would greatly improve.
I did a test on the following 1. Gnome on X11 2. Gnome on Wayland 3. KDE plasma on X11 4. Ubuntu Unity on X11 5. Ubuntu Unity on Wayland Cairo scaling is available on all platforms now. We can remove the logic for when cairo scaling not available. One thing I noticed is dynamic switching of dpi(changind from 100 to 200% or the other way) is not possible on KDE plasma. this requires a restart of GUI(restart of eclipse is not sufficient).
(In reply to Sravan Kumar Lakkimsetti from comment #26) > I did a test on the following > > 1. Gnome on X11 > 2. Gnome on Wayland > 3. KDE plasma on X11 > 4. Ubuntu Unity on X11 > 5. Ubuntu Unity on Wayland > > Cairo scaling is available on all platforms now. We can remove the logic for > when cairo scaling not available. > > One thing I noticed is dynamic switching of dpi(changind from 100 to 200% or > the other way) is not possible on KDE plasma. this requires a restart of > GUI(restart of eclipse is not sufficient). raised bug 569233 to address removal of autoscaling logic
Comment 23 gives screenshot for what is happening on KUbuntu 18.04, where cairo scaling is NOT available. At least not according to the older code that tried to detect it.
(In reply to Alexandr Miloslavskiy from comment #28) > Comment 23 gives screenshot for what is happening on KUbuntu 18.04, where > cairo scaling is NOT available. At least not according to the older code > that tried to detect it. Actually we are not detecting availability of cairo scaling properly. It is available. I have a created a patch utilizing only cairo scaling https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172943 If it possible for you to test it? please note Image.getBoundsInPixels will not work yet. but the remaining problems should be fixed.
I will give it a try today or tomorrow.
(In reply to Alexandr Miloslavskiy from comment #30) > I will give it a try today or tomorrow. Thanks Alexandr. I would prefer to merge bug 568641 and bug 569233(along with fix for Image.getBoundsInPixels()) at the start of next week as soon as master is opened for 4.19 development
Unfortunately you didn't explain what combination of patches you are expecting me to test. Patches for Bug 569233 and Bug 568641 have merge conflicts. When testing the new Bug 569233 alone, I observe the same visual results as with Bug 568641 alone. Specifically, on KUbuntu 18.04 with 'System Settings | Display and Monitor | Scale Display = 2', all images are 20x20, when 40x40 is expected. You can see that in Comment 23 screenshot, image #4 labeled "KUbuntu 18.04; 2x scaling; patch applied". Note that correct images are #2 and #5.
The values on said KUbuntu 18.04 are: 1 = gdk_monitor_get_scale_factor() 1 = cairo_surface_get_device_scale() 192 = gdk_screen_get_resolution() Note that 'cairo_surface_get_device_scale()' does NOT report 2x surface DPI there.
(In reply to Alexandr Miloslavskiy from comment #32) > Unfortunately you didn't explain what combination of patches you are > expecting me to test. Patches for Bug 569233 and Bug 568641 have merge > conflicts. > > When testing the new Bug 569233 alone, I observe the same visual results as > with Bug 568641 alone. Specifically, on KUbuntu 18.04 with 'System Settings > | Display and Monitor | Scale Display = 2', all images are 20x20, when 40x40 > is expected. > > You can see that in Comment 23 screenshot, image #4 labeled "KUbuntu 18.04; > 2x scaling; patch applied". Note that correct images are #2 and #5. Actually bug 569233 has 568641 included. So testing with 569233 is sufficient. Thank you for testing this out. Let me try to setup kubuntu and try it out.
> Note that correct images are #2 and #5. Typo; I meant #1 and #5. > Let me try to setup kubuntu and try it out. That's what I did myself for this Bug. Remember to use 18.04, because 20.04 is different. I'm still studying what exactly happens there, will share later.
I did some more testing on KUbuntu 18.04. The important numbers are ------------------------- KUbuntu 18.04; 1x scaling 96 = gdk_screen_get_resolution() 1 = gdk_monitor_get_scale_factor() 1 = gdk_screen_get_monitor_scale_factor() 1 = cairo_surface_get_device_scale() KUbuntu 18.04; 2x scaling 192 = gdk_screen_get_resolution() 1 = gdk_monitor_get_scale_factor() 1 = gdk_screen_get_monitor_scale_factor() 1 = cairo_surface_get_device_scale() Ubuntu 20.04; 100% DPI 96 = gdk_screen_get_resolution() 1 = gdk_monitor_get_scale_factor() 1 = gdk_screen_get_monitor_scale_factor() 1 = cairo_surface_get_device_scale() Ubuntu 20.04; 200% DPI 96 = gdk_screen_get_resolution() 2 = gdk_monitor_get_scale_factor() 2 = gdk_screen_get_monitor_scale_factor() 2 = cairo_surface_get_device_scale() I have studied GTK sources and it seems that the last 3 numbers should match for windows: * 'gdk_screen_get_monitor_scale_factor()' is a trivial wrapper for 'gdk_monitor_get_scale_factor()' * windows inherit scale from monitor and save them via 'gdk_window_get_scale_factor()' * windows use 'cairo_surface_set_device_scale()' for their surfaces, see for example 'gdk_x11_ref_cairo_surface()' * For non-window surfaces, such as SWT image, we might still want to 'cairo_surface_set_device_scale()' explicitly. This narrows the discussion to two numbers: (A) 192 or 96 = gdk_screen_get_resolution() (B) 2 or 1 = gdk_monitor_get_scale_factor() Ubuntu 20.04 scales things by increasing (B) with (A) unchanged. KUbuntu 18.04 does the opposite. The meaning of these numbers ---------------------------- gdk_screen_get_resolution () is documented as follows: Sets the resolution for font handling on the screen. This is a scale factor between points specified in a PangoFontDescription and cairo units. The default value is 96, meaning that a 10 point font will be 13 units high. (10 * 96. / 72. = 13.3). As for 'gdk_monitor_get_scale_factor()', the scale factor is used throughout GTK sources to scale every things here and there. I went further and also measured 'setBounds(50, 50, 20, 20)' on a screenshot: KUbuntu 18.04; 1x scaling Good: ( 50px, 50px, 20px, 20px) KUbuntu 18.04; 2x scaling BAD: ( 50px, 50px, 20px, 20px) Ubuntu 20.04; 100% DPI Good: ( 50px, 50px, 20px, 20px) Ubuntu 20.04; 200% DPI Good: (100px, 100px, 40px, 40px) Summary: -------- It's not really about "cairo auto scaling". I understand that cairo scaling is available anywhere on GTK3. However, KUbuntu simply doesn't set screen to 200% and instead *only* doubles font sizes, but not Control positions or Images. However, applications on KUbuntu mostly behave as if the setting meant "scale everything". Some examples are: * Firefox doubles the size of icons on toolbar * Intellij IDEA doubles the size of icons on toolbar * KDE's Start menu doubles the size of images I think that SWT shall also understand the setting as "double everything".
(In reply to Alexandr Miloslavskiy from comment #36) > Summary: > -------- > It's not really about "cairo auto scaling". I understand that cairo scaling > is > available anywhere on GTK3. However, KUbuntu simply doesn't set screen to > 200% > and instead *only* doubles font sizes, but not Control positions or Images. > > However, applications on KUbuntu mostly behave as if the setting meant > "scale everything". Some examples are: > * Firefox doubles the size of icons on toolbar > * Intellij IDEA doubles the size of icons on toolbar > * KDE's Start menu doubles the size of images > > I think that SWT shall also understand the setting as "double everything". Thank you doing a comprehensive test and debugging. Kubuntu 18.04 is based on Qt. All the settings are set for Qt framework. Kubuntu 20.04 sets the GTK parameters as well. In this situation we went ahead with "double everything" for the first implementation. We could not get a consistent way to detect scaling in Qt environment and gtk environment. Please note: Bug 569233 removes the above implementation. The above implementation also causes performance hit. When gtk and cairo came up with device scale parameter, we went ahead with second implementation which utilizes cairo's device_scale parameter. This implementation does not This works in Ubuntu 20.04 with KDE, Unity and Gnome. I tested on my system with all three desktop environments installed. I hope I explained the current situation. My current reasoning for removal of "double everything" implementation is 1. Most developers stay on latest LTS versions. That means majority will be using ubuntu 20.04, where they don't use "double everything" implementation 2. using device_scale in cairo images, implementation will be quite simple and delegating scaling algorithms to cairo. 3. Potential to support fractional scaling. 4. Scaling using Qt settings never worked/broken from quite sometime till now. Lets go forward by providing proper scaling using cairo and device scale.
> We could not get a consistent way to detect scaling in Qt > environment and gtk environment. With current knowledge, it sounds simple enough. gdk_monitor_get_scale_factor() - apply to Image's DPI and coords (gdk_screen_get_resolution() / 96.0) - apply to Image's coords and Control coords > Please note: Bug 569233 removes the above implementation. The above > implementation also causes performance hit. Which performance hit? Mere multiplication of coords in SWT shouldn't be a problem. > 1. Most developers stay on latest LTS versions. Sounds like an overkill do declare 2-year old 18.04 as unsupported. Also, it's called LTS for a reason which is exactly opposite to declaring it unsupported. > 2. using device_scale in cairo images, implementation will be quite simple > and delegating scaling algorithms to cairo. I can sympathize with the desire to make SWT code simpler. Still, I'm not sure if it's worth the negative effects of not supporting a recent OS. > 3. Potential to support fractional scaling. Fractional scaling is not a problem on 18.04 as well. In fact, the "Scale monitor" setting is configured in 0.1x increments. > 4. Scaling using Qt settings never worked/broken from quite sometime till > now. Lets go forward by providing proper scaling using cairo and device > scale. It was broken mostly due to oversight. Again, I can understand the desire to fix problem by calling it unsupported OS, but is it really worth it?
(In reply to Alexandr Miloslavskiy from comment #38) > > We could not get a consistent way to detect scaling in Qt > > environment and gtk environment. > > With current knowledge, it sounds simple enough. > > gdk_monitor_get_scale_factor() - apply to Image's DPI and coords > (gdk_screen_get_resolution() / 96.0) - apply to Image's coords and Control > coords > > > Please note: Bug 569233 removes the above implementation. The above > > implementation also causes performance hit. > > Which performance hit? Mere multiplication of coords in SWT shouldn't be a > problem. > > > 1. Most developers stay on latest LTS versions. > > Sounds like an overkill do declare 2-year old 18.04 as unsupported. Also, > it's called LTS for a reason which is exactly opposite to declaring it > unsupported. > > > 2. using device_scale in cairo images, implementation will be quite simple > > and delegating scaling algorithms to cairo. > > I can sympathize with the desire to make SWT code simpler. Still, I'm not > sure if it's worth the negative effects of not supporting a recent OS. > > > 3. Potential to support fractional scaling. > > Fractional scaling is not a problem on 18.04 as well. In fact, the "Scale > monitor" setting is configured in 0.1x increments. > > > 4. Scaling using Qt settings never worked/broken from quite sometime till > > now. Lets go forward by providing proper scaling using cairo and device > > scale. > > It was broken mostly due to oversight. Again, I can understand the desire to > fix problem by calling it unsupported OS, but is it really worth it? I am not calling Ubuntu 18.04 as unsupported. It is supported with one limitation hidpi may not work with Qt based desktop environments at higher scalefactors. Please note scaling works if you are using Gnome on ubuntu 18.04. It is the KDE and Unity that has problem. By the way Gnome is the default desktop environment in Ubuntu 18.04 https://wiki.ubuntu.com/BionicBeaver/ReleaseNotes/18.04#Other_highlights_since_16.04_LTS My concern is fixing this is really worth the effort. By the time we fix the issues with "double everything" method, we may well be at the end of 2021(almost 4 years after ubuntu 18.04 is released). To me it seems to be waste of time to pursue this path when you have an easy workaround of using Gnome or using at scalefactor 1 or upgrade to Ubuntu 20.04. There are multiple important bugs like Bug 534422, Bug 537441 and Bug 535065 which needs attention. Another problem is dynamic switching of dpi, which is not possible with "scale everything" method.
I would prefer existing code to be fixed and things to just work, but fine, I do understand your reasons. I understand that Bug 569233 is the continuation of this problem, then? I'll continue the discussion there.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172538 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=4cdab725877fe83f22cf2bdc137e985b4b774e41
