Bug 568590 - [win32] Numbers in Spinner control are not vertically centred
Summary: [win32] Numbers in Spinner control are not vertically centred
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.17   Edit
Hardware: PC Windows 10
: P3 normal (vote)
Target Milestone: 4.18 RC1   Edit
Assignee: Alexandr Miloslavskiy CLA
QA Contact: Niraj Modi CLA
URL:
Whiteboard:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2020-11-06 05:16 EST by Phil Beauvoir CLA
Modified: 2020-11-26 05:54 EST (History)
4 users (show)

See Also:
lshanmug: review+


Attachments
Screenshot comparing two versions of Spinner control (7.28 KB, image/png)
2020-11-06 05:23 EST, Phil Beauvoir CLA
no flags Details
Text vs Spinner (4.75 KB, image/png)
2020-11-11 12:23 EST, Alexandr Miloslavskiy CLA
no flags Details
Test snippet's screenshot with patch (14.07 KB, image/png)
2020-11-18 17:16 EST, Alexandr Miloslavskiy CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Beauvoir CLA 2020-11-06 05:16:46 EST
Windows 10 64-bit
Eclipse 4.17 and Eclipse 4.18

In Eclipse 4.15 the numbers in a Spinner control on Windows are centred vertically. On Eclipse 4.17 and 4.18 they are place more at the top of the control.
Comment 1 Phil Beauvoir CLA 2020-11-06 05:23:33 EST
Created attachment 284697 [details]
Screenshot comparing two versions of Spinner control

Running Snippet184 I took some screenshots comparing the Spinner in Eclipse 4.15 and 4.18.


https://git.eclipse.org/c/platform/eclipse.platform.swt.git/plain/examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet184.java
Comment 2 Andrey Loskutov CLA 2020-11-06 15:18:35 EST
What about 4.16? Is this OK or also broken?
Comment 3 Phil Beauvoir CLA 2020-11-11 10:36:16 EST
(In reply to Andrey Loskutov from comment #2)
> What about 4.16? Is this OK or also broken?

No, that version is OK.
Comment 5 Andrey Loskutov CLA 2020-11-11 11:55:54 EST
(In reply to Phil Beauvoir from comment #4)
> Could it be this change?
> 
> https://github.com/eclipse/eclipse.platform.swt/commit/
> ddd5919b91cfc9f0cfb4eef65d19333e495373d1#diff-
> f033e4b8a392b1032f2640db2b9ffd62290c030ad111a8f31c5047587d764b62

Yes, it is. If I revert commit https://github.com/eclipse/eclipse.platform.swt/commit/ddd5919b91cfc9f0cfb4eef65d19333e495373d1 I see the old centered text, so this is a regression from bug 565679 changes.

@Alexandr, could you please take a look?
Comment 6 Alexandr Miloslavskiy CLA 2020-11-11 12:23:26 EST
Created attachment 284736 [details]
Text vs Spinner

In old code, the text's position was in fact a bug caused by border. I fixed that bug.

However, I do agree that it looked better before. It's a kind of a bug that eventually is seen as intentional design, I guess. Also, I have now compared Text to Spinner, and Text's text position is slightly more centered.

The fix should be very simple: Spinner's internal Text needs to be moved a couple pixels right-down.

I'll have a look later. If anyone is willing to do it for me, you're welcome.
Comment 7 Eclipse Genie CLA 2020-11-18 17:14:07 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172482
Comment 8 Alexandr Miloslavskiy CLA 2020-11-18 17:16:39 EST
Created attachment 284811 [details]
Test snippet's screenshot with patch
Comment 9 Alexandr Miloslavskiy CLA 2020-11-18 17:19:20 EST
I have adjusted Spinner's text position to match Text's position. This results in text being moved 2px lower.

While this is still not centered, well this is how SWT's Text has been for years.
Comment 10 Phil Beauvoir CLA 2020-11-18 17:20:59 EST
So, I owe you a beer then, Alex. :-)

I'll test when it gets into an I-build.

Cheers!
Comment 11 Niraj Modi CLA 2020-11-20 05:40:59 EST
(In reply to Eclipse Genie from comment #7)
> New Gerrit change created:
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172482

Change looks good to me.
Fix is localized for Spinner's size adjustment.
Comment 13 Niraj Modi CLA 2020-11-20 06:40:49 EST
(In reply to Eclipse Genie from comment #12)
> Gerrit change
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172482 was
> merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=91c971d35f1e2f4c79104f50feca0cbfa6a4f708

Thanks Alexandr for the fix, resolving now.
Comment 14 Phil Beauvoir CLA 2020-11-21 05:06:32 EST
(In reply to Phil Beauvoir from comment #10)
> So, I owe you a beer then, Alex. :-)
> 
> I'll test when it gets into an I-build.
> 
> Cheers!

Tested on I20201120-1800, looks good. Thanks!
Comment 15 Niraj Modi CLA 2020-11-26 05:54:06 EST
(In reply to Phil Beauvoir from comment #14)
> (In reply to Phil Beauvoir from comment #10)
> > So, I owe you a beer then, Alex. :-)
> > 
> > I'll test when it gets into an I-build.
> > 
> > Cheers!
> 
> Tested on I20201120-1800, looks good. Thanks!

Thanks Phil for the verification & confirmation.