Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 480639

Summary:

[HiDPI][API] Provide monitor-specific DPI scaling / zoom level

Product:

[Eclipse Project] Platform

Reporter:

Thomas Singer <eclipse>

Component:

SWT

Assignee:

Lakshmi P Shanmugam <lshanmug>

Status:

VERIFIED FIXED

QA Contact:

Severity:

enhancement

Priority:

CC:

akurtakov, arunkumar.thondapu, daniel_megert, ericwill, gautier.desaintmartinlacaze, Lars.Vogel, lshanmug, lufimtse, markus.kell.r, ned.twigg, niraj.modi, peter, s.schulz, sravankumarl

Version:

4.5

Flags:

Lars.Vogel: pmc_approved+

Target Milestone:

4.8 M7

Hardware:

All

OS:

All

See Also:

Whiteboard:

Bug Depends on:

533138

Bug Blocks:

509534, 514307

Attachments:

Description	Flags
(partial) fix	none

Description Thomas Singer

2015-10-26 08:46:43 EDT

Please take a look at

<https://msdn.microsoft.com/en-us/library/windows/desktop/dd464646(v=vs.85).aspx>

Since Windows 10 each monitor can have its own scaling factor. While you are at implementing HiDPI support, better do it right from current state of the art. Currently, Monitor lacks a getDpi() function.

Comment 1 Markus Keller

2017-03-28 13:50:58 EDT

I agree we need monitor-specific scaling support, but I think this would be a better API in the Monitor class:

int getZoom()

The returned value would be the zoom level that is used in ImageDataProvider#getImageData(int) callbacks on that monitor. As long as SWT or the underlying platform doesn't support per-monitor zoom levels, this API would return the internal DPIUtil#deviceZoom for all monitors.

We already have a "logical" DPI value at Display#getDPI() that never returned actual dots per inch, and that has been kept stable when the HiDPI Image constructors were added. I wouldn't use the term "DPI" in new APIs.

Comment 2 S. Schulz

2018-01-16 02:10:14 EST

Created attachment 272273 [details]
(partial) fix

- added a field "zoom" to the Device and used that to implement Transform, GC and FontMetrics
- Since the device zoom cannot be a static field anymore, the DPIUtil was changed drastically (not sure if API-compatibility is necessary for internal classes, though)
- the scaling for a Printer should always be 100 (which means getDeviceZoom() should probably be abstract in Device and only implemented in its subclasses, but yet again I was going for API-compatibility)

Comment 3 Niraj Modi

2018-02-01 08:02:18 EST

(In reply to S. Schulz from comment #2)
> Created attachment 272273 [details]
> (partial) fix
> 
> - added a field "zoom" to the Device and used that to implement Transform,
> GC and FontMetrics
> - Since the device zoom cannot be a static field anymore, the DPIUtil was
> changed drastically (not sure if API-compatibility is necessary for internal
> classes, though)
> - the scaling for a Printer should always be 100 (which means
> getDeviceZoom() should probably be abstract in Device and only implemented
> in its subclasses, but yet again I was going for API-compatibility)

Thanks for proposing this patch. Since Monitor doesn't extend from Device this patch won't solve the use-case of per monitor specific zoom use-case.

Also, please share a real use-case of Monitor#getZoom() API, so it will help us in coming with a better designed API.

Comment 4 S. Schulz

2018-02-05 02:59:18 EST

(In reply to Niraj Modi from comment #3)
> (In reply to S. Schulz from comment #2)
> > Created attachment 272273 [details]
> > (partial) fix
> > 
> > - added a field "zoom" to the Device and used that to implement Transform,
> > GC and FontMetrics
> > - Since the device zoom cannot be a static field anymore, the DPIUtil was
> > changed drastically (not sure if API-compatibility is necessary for internal
> > classes, though)
> > - the scaling for a Printer should always be 100 (which means
> > getDeviceZoom() should probably be abstract in Device and only implemented
> > in its subclasses, but yet again I was going for API-compatibility)
> 
> Thanks for proposing this patch. Since Monitor doesn't extend from Device
> this patch won't solve the use-case of per monitor specific zoom use-case.
> 
> Also, please share a real use-case of Monitor#getZoom() API, so it will help
> us in coming with a better designed API.

That's true, it's only for multiple Devices, not Monitors. However I'm not sure how to implement that on a per-monitor-basis. Everything that uses the Device zoom  (Image, GC & Transform at last) only ever use the device, the class Monitor is hardly ever used (hence why I thought the class Display was representing a single monitor). 

I assume you guys have a better understanding of the API, but IMHO the zoom should not be used to create any resources at all. Only when the controls / images are displayed should the zoom factor in. But that's an enormous change...

I'm willing to work on this issue, but I don't know how to proceed. Any advice?

Comment 5 Niraj Modi

2018-02-16 05:49:36 EST

(In reply to S. Schulz from comment #4)
> (In reply to Niraj Modi from comment #3)
> > (In reply to S. Schulz from comment #2)
> > > Created attachment 272273 [details]
> > > (partial) fix
> > > 
> > > - added a field "zoom" to the Device and used that to implement Transform,
> > > GC and FontMetrics
> > > - Since the device zoom cannot be a static field anymore, the DPIUtil was
> > > changed drastically (not sure if API-compatibility is necessary for internal
> > > classes, though)
> > > - the scaling for a Printer should always be 100 (which means
> > > getDeviceZoom() should probably be abstract in Device and only implemented
> > > in its subclasses, but yet again I was going for API-compatibility)
> > 
> > Thanks for proposing this patch. Since Monitor doesn't extend from Device
> > this patch won't solve the use-case of per monitor specific zoom use-case.
> > 
> > Also, please share a real use-case of Monitor#getZoom() API, so it will help
> > us in coming with a better designed API.
> 
> That's true, it's only for multiple Devices, not Monitors. However I'm not
> sure how to implement that on a per-monitor-basis. Everything that uses the
> Device zoom  (Image, GC & Transform at last) only ever use the device, the
> class Monitor is hardly ever used (hence why I thought the class Display was
> representing a single monitor). 
> 
> I assume you guys have a better understanding of the API, but IMHO the zoom
> should not be used to create any resources at all. Only when the controls /
> images are displayed should the zoom factor in. But that's an enormous
> change...
Ok, we were more interested on the exact use-case(s) that you are targeting.
To have a better understanding of the requirement, you can also share SWT Snippets or JUnit test cases that you see is breaking and shall work once we have your patch changes in.

> I'm willing to work on this issue, but I don't know how to proceed. Any
> advice?
Thanks for you interest, currently we use Gerrit for code sharing/review.
Once we have clear understanding on the use-case front we can short it out.

Comment 6 Eclipse Genie

2018-03-27 05:51:11 EDT

New Gerrit change created: https://git.eclipse.org/r/120240

Comment 7 Alexander Kurtakov

2018-03-29 06:29:39 EDT

gdk_monitor_get_scale_factor  in GTK 3.22 should satisfy this need.

Comment 8 Alexander Kurtakov

2018-03-29 06:30:10 EDT

Eric, would you please handle the GTK side?

Comment 9 Lakshmi P Shanmugam

2018-03-29 06:39:25 EDT

Thanks Alex! I've a WIP progress patch for GTK with the new APIs, but still requires testing with the HiDPI monitor. Will post the patch shortly.

Comment 10 Eclipse Genie

2018-04-04 07:06:32 EDT

New Gerrit change created: https://git.eclipse.org/r/120695

Comment 11 Lakshmi P Shanmugam

2018-04-04 09:45:56 EDT

(In reply to Eclipse Genie from comment #10)
> New Gerrit change created: https://git.eclipse.org/r/120695

The gtk patch uses gdk_monitor_get_scale_factor for GTK >= 3.22 and gdk_screen_get_monitor_scale_factor for older versions, but both the APIs return scale factor 1 even for a HiDPI monitor with 200% scaling.

In OS.java, the GDK_SCALE is set to 1. If GDK_SCALE is set to 2 in OS.java, then the GTK APIs return the scale factor as 2. But, removing the code doesn't have any effect. Looks like GDK_SCALE is getting the correct value automatically.

I think the launcher code also sets GDK_SCALE=1, but removing that too doesn't have effect on the GTK APIs.

Alex/Eric any ideas on how to get the correct scale factor?

Comment 12 Eclipse Genie

2018-04-06 08:00:12 EDT

New Gerrit change created: https://git.eclipse.org/r/120847

Comment 13 Lakshmi P Shanmugam

2018-04-09 04:37:47 EDT

(In reply to Eclipse Genie from comment #6)
> New Gerrit change created: https://git.eclipse.org/r/120240

This patch has the Windows and Mac implementation of the Monitor.getZoom() API. 

To enable dynamic DPI on Windows 10 and above, dpiAwareness setting has to be enabled using java.exe.manifest as mentioned here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=533135#c5

Comment 14 Lakshmi P Shanmugam

2018-04-09 04:47:59 EDT

Snippet to test the new API - https://bugs.eclipse.org/bugs/attachment.cgi?id=273491

Comment 15 Eclipse Genie

2018-04-10 13:25:58 EDT

Gerrit change https://git.eclipse.org/r/120240 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=13e0a92eb06604985b73e89050f2814cadcbf876

Comment 16 Lakshmi P Shanmugam

2018-04-12 07:30:04 EDT

(In reply to Lakshmi Shanmugam from comment #11)
> (In reply to Eclipse Genie from comment #10)
> > New Gerrit change created: https://git.eclipse.org/r/120695
> 
> The gtk patch uses gdk_monitor_get_scale_factor for GTK >= 3.22 and
> gdk_screen_get_monitor_scale_factor for older versions, but both the APIs
> return scale factor 1 even for a HiDPI monitor with 200% scaling.
> 
> In OS.java, the GDK_SCALE is set to 1. If GDK_SCALE is set to 2 in OS.java,
> then the GTK APIs return the scale factor as 2. But, removing the code
> doesn't have any effect. Looks like GDK_SCALE is getting the correct value
> automatically.
> 
> I think the launcher code also sets GDK_SCALE=1, but removing that too
> doesn't have effect on the GTK APIs.
> 
> Alex/Eric any ideas on how to get the correct scale factor?

Hi Alex, any ideas?
What I'm trying to find out is, does gdk_monitor_get_scale_factor() return the correct scale factor? I tested the patch with Fedora 27 and it doesn't give scale factor 2 on HiDPI monitor.

Comment 17 Alexander Kurtakov

2018-04-13 07:54:48 EDT

(In reply to Lakshmi Shanmugam from comment #16)
> (In reply to Lakshmi Shanmugam from comment #11)
> > (In reply to Eclipse Genie from comment #10)
> > > New Gerrit change created: https://git.eclipse.org/r/120695
> > 
> > The gtk patch uses gdk_monitor_get_scale_factor for GTK >= 3.22 and
> > gdk_screen_get_monitor_scale_factor for older versions, but both the APIs
> > return scale factor 1 even for a HiDPI monitor with 200% scaling.
> > 
> > In OS.java, the GDK_SCALE is set to 1. If GDK_SCALE is set to 2 in OS.java,
> > then the GTK APIs return the scale factor as 2. But, removing the code
> > doesn't have any effect. Looks like GDK_SCALE is getting the correct value
> > automatically.
> > 
> > I think the launcher code also sets GDK_SCALE=1, but removing that too
> > doesn't have effect on the GTK APIs.
> > 
> > Alex/Eric any ideas on how to get the correct scale factor?
> 
> Hi Alex, any ideas?
> What I'm trying to find out is, does gdk_monitor_get_scale_factor() return
> the correct scale factor? I tested the patch with Fedora 27 and it doesn't
> give scale factor 2 on HiDPI monitor.

Look at the latest patch. After changing Display scale to 200% in gnome settings I can see getZoom properly giving me 200%. Seems to be working fine to me.

Comment 18 Alexander Kurtakov

2018-04-13 07:55:59 EDT

Just for the record in the default case it gives me 100.

Comment 19 Lakshmi P Shanmugam

2018-04-16 09:15:56 EDT

(In reply to Alexander Kurtakov from comment #17)
> Look at the latest patch. After changing Display scale to 200% in gnome
> settings I can see getZoom properly giving me 200%. Seems to be working fine
> to me.
Thanks for looking into this, Alex.

This is strange, I tried the latest patch but getZoom() still gives me 100.
I tried Fedora 27 with both setups: single standard resolution monitor and standard + hidpi monitor. I changed the scale factor from the command line using:
gsettings set org.gnome.desktop.interface scaling-factor 2

What is your setup? how do you set the scale factor?

Comment 20 Alexander Kurtakov

2018-04-16 09:31:05 EDT

(In reply to Lakshmi Shanmugam from comment #19)
> (In reply to Alexander Kurtakov from comment #17)
> > Look at the latest patch. After changing Display scale to 200% in gnome
> > settings I can see getZoom properly giving me 200%. Seems to be working fine
> > to me.
> Thanks for looking into this, Alex.
> 
> This is strange, I tried the latest patch but getZoom() still gives me 100.
> I tried Fedora 27 with both setups: single standard resolution monitor and
> standard + hidpi monitor. I changed the scale factor from the command line
> using:
> gsettings set org.gnome.desktop.interface scaling-factor 2
> 
> What is your setup? how do you set the scale factor?

Fedora 28

1. setting app
2. Devices
3. Display
4. Change Scale to 200%

Comment 21 Lakshmi P Shanmugam

2018-04-17 01:29:04 EDT

Sravan, can you please test the patch on your linux box?

Comment 22 Lakshmi P Shanmugam

2018-04-17 08:39:05 EDT

(In reply to Lakshmi Shanmugam from comment #21)
> Sravan, can you please test the patch on your linux box?
I tested on Sravan's VM with Fedora 27 and I get scale factor as 2.
May be the difference is because the snippet is running on X11 on his setup.
On my system it doesn't seem to run on X11 (OS.isX11() returns false) even if I set GDK_BACKEND=x11.

Comment 23 Alexander Kurtakov

2018-04-18 06:57:54 EDT

(In reply to Lakshmi Shanmugam from comment #22)
> (In reply to Lakshmi Shanmugam from comment #21)
> > Sravan, can you please test the patch on your linux box?
> I tested on Sravan's VM with Fedora 27 and I get scale factor as 2.
> May be the difference is because the snippet is running on X11 on his setup.
> On my system it doesn't seem to run on X11 (OS.isX11() returns false) even
> if I set GDK_BACKEND=x11.

I run on wayland and still see correct value.

Comment 24 Lakshmi P Shanmugam

2018-04-23 07:34:46 EDT

(In reply to Alexander Kurtakov from comment #23)
> (In reply to Lakshmi Shanmugam from comment #22)
> > (In reply to Lakshmi Shanmugam from comment #21)
> > > Sravan, can you please test the patch on your linux box?
> > I tested on Sravan's VM with Fedora 27 and I get scale factor as 2.
> > May be the difference is because the snippet is running on X11 on his setup.
> > On my system it doesn't seem to run on X11 (OS.isX11() returns false) even
> > if I set GDK_BACKEND=x11.
> 
> I run on wayland and still see correct value.

Ok, I'm not sure what is wrong in my setup.

Eric/Leo, can one of you please try out the new API? I just want to make sure the code works on another system before I commit the changes and that the problem is specific to my setup.

Comment 25 Leo Ufimtsev

2018-04-24 14:17:40 EDT

I tried the snippet attached to this bug with Alex's latest patch (v14) on Gtk/Wayland, works well.

Environment:
- 4 monitor setup
- Fedora 27, Gtk3.22  Wayland 
- 1 monitor has 200% HiDpi

The snippet reports 200-100-100-100 correctly and when I move the snippet onto the screen with hiDpi, it makes it self bigger. When I move it back onto a regular screen, it makes itself smaller again. 

Kinda cool. This is similar to what gnome does with it's native apps as well.

Made a little video (a bit blurry unfortunately, but you get the idea):
https://www.youtube.com/watch?v=Xixj1UoLslg&feature=youtu.be

Comment 26 Eclipse Genie

2018-04-24 14:48:27 EDT

Gerrit change https://git.eclipse.org/r/120695 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=ecb072664278dc6cfba0ce56ea06e669e0675959

Comment 27 Eclipse Genie

2018-04-24 15:06:45 EDT

Gerrit change https://git.eclipse.org/r/120847 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=d3da5c5ceec7fff7568841d9eb69a77433b7dac6

Comment 28 Eclipse Genie

2018-04-25 03:59:27 EDT

New Gerrit change created: https://git.eclipse.org/r/121709

Comment 29 Eclipse Genie

2018-04-26 08:54:34 EDT

Gerrit change https://git.eclipse.org/r/121709 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=cdc67cbff3fb701108f19e6dbec01306872e31dc

Comment 30 Niraj Modi

2018-04-26 08:56:48 EDT

(In reply to Eclipse Genie from comment #29)
> Gerrit change https://git.eclipse.org/r/121709 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=cdc67cbff3fb701108f19e6dbec01306872e31dc

Thanks Nikita for the patch, it refactors the code related to new API and primarily simplifies the Windows implementation.

Comment 31 Niraj Modi

2018-05-09 04:39:26 EDT

Resolving.

Comment 32 Niraj Modi

2018-05-09 04:55:54 EDT

Verified in I20180507-2205 on Win10.