Bug 493047 - [HighDPI] Clipboard Example: Image looses it's quality during ImageTransfer
Summary: [HighDPI] Clipboard Example: Image looses it's quality during ImageTransfer
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.6   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 4.6 RC2   Edit
Assignee: Niraj Modi CLA
QA Contact:
URL:
Whiteboard:
Keywords: example
Depends on: 493114
Blocks: 494263
  Show dependency tree
 
Reported: 2016-05-05 02:23 EDT by Niraj Modi CLA
Modified: 2017-03-21 09:51 EDT (History)
5 users (show)

See Also:
sravankumarl: review+
niraj.modi: review? (markus.kell.r)
lshanmug: review+


Attachments
Clipboard_Example_ImageTransfer_150_Zoom (143.70 KB, image/png)
2016-05-05 02:23 EDT, Niraj Modi CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Niraj Modi CLA 2016-05-05 02:23:55 EDT
Created attachment 261476 [details]
Clipboard_Example_ImageTransfer_150_Zoom

Steps to reproduce:
1. Switch to High zoom settings
2. Launch ClipboardExample
3. Perform "Open Image" action you can see an image as per DPI settings.
4. Copy and Paste the image to the widget on the right side.

Current behavior:
Image quality decreases drastically in the ImageTransfer step.

Expected behavior:
Image quality should be maintained in the ImageTransfer step.
Comment 1 Niraj Modi CLA 2016-05-05 04:58:13 EDT
Problem is with ImageTransfer step in Clipboard Example, it doesn't uses new Image APIs meant of High DPI usage.

Will submit a patch shortly, targeting for Neon RC1.
Comment 2 Eclipse Genie CLA 2016-05-05 05:00:42 EDT
New Gerrit change created: https://git.eclipse.org/r/72133
Comment 3 Niraj Modi CLA 2016-05-06 05:16:19 EDT
Hi Sravan,
Neon RC1 bug, for your review ?
Comment 5 Niraj Modi CLA 2016-05-06 08:42:28 EDT
Resolving.
Comment 6 Markus Keller CLA 2016-05-11 15:21:28 EDT
If example code has to read internal properties and duplicate parts of DPIUtil, then you know something is wrong.

The real fix is probably to fix bug 490213 and then make sure the clipboard only transfers the 100% image.
Comment 7 Niraj Modi CLA 2016-05-12 10:10:32 EDT
(In reply to Markus Keller from comment #6)
> If example code has to read internal properties and duplicate parts of
> DPIUtil, then you know something is wrong.
> 
> The real fix is probably to fix bug 490213 and then make sure the clipboard
> only transfers the 100% image.

Hi Markus, 
We don't copy the Image instance on the the clipboard, we actually copy the ImageData(which can be serialized)

What we have fixed in ClipboardExample at HighDPI settings:
1. There is an image A which needs to be copied over using Clipboard.
2. Copy Operation: Instead of copying Image#getImage() to the clipboard which is be scaled down to 100% what's currently in use.. so we now copy Image#getImageDataAtCurrentZoom().
3. Paste Operation: We read the ImageData from the clipboard which is at HighDPI and has to be consumed as-is by target Image B, which can be achieved using ImageDataProvider only, which mandates us to read the System property for deviceZoom.[Yes some cleanup needed now, since change for bug 493462 is in, I can now depend on one System property i.e. deviceZoom]

Do you see a better alternate here ?
Comment 8 Eclipse Genie CLA 2016-05-13 05:10:45 EDT
New Gerrit change created: https://git.eclipse.org/r/72689
Comment 9 Niraj Modi CLA 2016-05-18 04:31:35 EDT
(In reply to Eclipse Genie from comment #8)
> New Gerrit change created: https://git.eclipse.org/r/72689

Other possible alternate is to expose AutoScaleImageDataProvider class as one of the implementation in SWT for client purpose, but any call on new API can be taken only in Oxygen release.
Comment 10 Niraj Modi CLA 2016-05-19 06:15:03 EDT
(In reply to Niraj Modi from comment #9)
> (In reply to Eclipse Genie from comment #8)
> > New Gerrit change created: https://git.eclipse.org/r/72689
> 
> Other possible alternate is to expose AutoScaleImageDataProvider class as
> one of the implementation in SWT for client purpose, but any call on new API
> can be taken only in Oxygen release.

Above gerrit patch just removes the old property that has been removed from SWT in RC1, so for correctness of the ClipboardExample code we should get this change in for Neon RC2 itself.

If AutoScaleImageDataProvider or a new API for getDevicezoom() is available in future, then the 'deviceZoom' system property can be replaced with that API.
Comment 11 Lakshmi P Shanmugam CLA 2016-05-19 06:19:22 EDT
(In reply to Niraj Modi from comment #10)
> (In reply to Niraj Modi from comment #9)
> > (In reply to Eclipse Genie from comment #8)
> > > New Gerrit change created: https://git.eclipse.org/r/72689
> > 
> > Other possible alternate is to expose AutoScaleImageDataProvider class as
> > one of the implementation in SWT for client purpose, but any call on new API
> > can be taken only in Oxygen release.
> 
> Above gerrit patch just removes the old property that has been removed from
> SWT in RC1, so for correctness of the ClipboardExample code we should get
> this change in for Neon RC2 itself.
I agree that we shouldn't leave the old system property "swt.enable.autoScale" in the example code. It could be misleading to developers looking at the example code.
Comment 13 Niraj Modi CLA 2016-05-19 06:57:03 EDT
Resolving.
Comment 14 Niraj Modi CLA 2016-05-23 03:33:41 EDT
Verified fix in Build id: I20160519-1730
Comment 15 Niraj Modi CLA 2016-05-23 04:02:19 EDT
Please note:
1. Raised bug 494263 to make a similar fix in SWT Snippets library
2. Raised bug 494264 to evaluate the need for any of below new API w.r.t. HighDPI:
   - AutoScaleImageDataProvider class
   - or new API for getDevicezoom() method(either in Display or Device class)