Bug 349112 - CCombo: allow setting alignment SWT.RIGHT
Summary: CCombo: allow setting alignment SWT.RIGHT
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 3.6.2   Edit
Hardware: PC All
: P3 enhancement (vote)
Target Milestone: 4.14 RC1   Edit
Assignee: Paul Pazderski CLA
QA Contact: Niraj Modi CLA
URL:
Whiteboard:
Keywords: api, noteworthy, plan
Depends on:
Blocks: 553544
  Show dependency tree
 
Reported: 2011-06-11 05:32 EDT by Andrei Diaconu CLA
Modified: 2019-12-03 00:54 EST (History)
7 users (show)

See Also:
akurtakov: pmc_approved+
sravankumarl: review+


Attachments
CCombo with alignments (10.59 KB, image/png)
2019-11-14 05:12 EST, Paul Pazderski CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Diaconu CLA 2011-06-11 05:32:47 EDT
It would really help me to be able to specify the alignment for the text component inside a CCombo. 

When using numbers it is much easier to read text that is right-aligned. The Text component supports alignments, but unfortunately the list does not. But even if the list remains left-aligned, fixing the text is a huge improvement for me.
Comment 1 Lars Vogel CLA 2019-11-14 03:48:56 EST
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

If the bug is still relevant, please remove the "stalebug" whiteboard tag.
Comment 2 Andrei Diaconu CLA 2019-11-14 04:29:44 EST
This is an API request, and nothing changed. I would still like to align text to right when dealing with numbers in a CCombo.
Comment 3 Eclipse Genie CLA 2019-11-14 05:09:49 EST
New Gerrit change created: https://git.eclipse.org/r/152650
Comment 4 Paul Pazderski CLA 2019-11-14 05:12:26 EST
Created attachment 280640 [details]
CCombo with alignments

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

This applies alignment on the text field. As you already noticed the list does not yet support alignment.
Comment 5 Andrei Diaconu CLA 2019-11-14 08:45:02 EST
Thank you for the fix.
Just a small observation: if you want to make this official and part of the specification, shouldn't you also update the "Styles" paragraph? After your change, it reads:
 * <dt><b>Styles:</b>
 * <dd>BORDER, READ_ONLY, FLAT</dd>
 * <dt><b>Events:</b>
Comment 6 Paul Pazderski CLA 2019-11-14 10:05:46 EST
(In reply to Andrei Diaconu from comment #5)
> Just a small observation: if you want to make this official and part of the
> specification, shouldn't you also update the "Styles" paragraph?

Yes, missed that part. Thank you!
Comment 7 Niraj Modi CLA 2019-11-15 07:01:21 EST
(In reply to Paul Pazderski from comment #4)
> Created attachment 280640 [details]
> CCombo with alignments
> 
> (In reply to Eclipse Genie from comment #3)
> > New Gerrit change created: https://git.eclipse.org/r/152650
> 
> This applies alignment on the text field. As you already noticed the list
> does not yet support alignment.

Hi Paul,
Thanks for the patch, tried it. IMO we should evaluate below items as well:
1. Possibility for supporting list alignment ?
2. Similar to CLabel.java we should also support below two APIs in CCombo.java:
    int getAlignment()
    void setAlignment(int align)
Comment 8 Paul Pazderski CLA 2019-11-15 07:10:38 EST
(In reply to Niraj Modi from comment #7)
> Hi Paul,
> Thanks for the patch, tried it. IMO we should evaluate below items as well:
> 1. Possibility for supporting list alignment ?
> 2. Similar to CLabel.java we should also support below two APIs in
> CCombo.java:
>     int getAlignment()
>     void setAlignment(int align)

1. the SWT List widget does not support alignment. For Windows I did a bit research and the two options I found where some dirty hacks with LTR vs RTL (which wouldn't support centered) or owner drawing.
So IMO list align would be a separate ticket and Andrei is already happy with text align.

2. Agree. Will update the change.
Comment 9 Niraj Modi CLA 2019-11-15 07:31:52 EST
(In reply to Paul Pazderski from comment #8)
> (In reply to Niraj Modi from comment #7)
> > Hi Paul,
> > Thanks for the patch, tried it. IMO we should evaluate below items as well:
> > 1. Possibility for supporting list alignment ?
> > 2. Similar to CLabel.java we should also support below two APIs in
> > CCombo.java:
> >     int getAlignment()
> >     void setAlignment(int align)
> 
> 1. the SWT List widget does not support alignment. For Windows I did a bit
> research and the two options I found where some dirty hacks with LTR vs RTL
> (which wouldn't support centered) or owner drawing.
> So IMO list align would be a separate ticket and Andrei is already happy
> with text align.
Fine with separate bugzilla entry, so it can be tracked/evaluated in future.
 
> 2. Agree. Will update the change.
Thanks!
Comment 10 Paul Pazderski CLA 2019-11-15 08:12:49 EST
(In reply to Niraj Modi from comment #9)
> > 2. Agree. Will update the change.
> Thanks!

I added setAlignment but now I'm less convinced we should add it. The text widget does not support alignment change and my workaround to recreate the text widget is a bit ugly.
Comment 11 Niraj Modi CLA 2019-11-19 03:34:49 EST
(In reply to Paul Pazderski from comment #10)
> (In reply to Niraj Modi from comment #9)
> > > 2. Agree. Will update the change.
> > Thanks!
> 
> I added setAlignment but now I'm less convinced we should add it. The text
> widget does not support alignment change and my workaround to recreate the
> text widget is a bit ugly.

Behavior wise it's working fine, targeting for 4.14 M3 as it's has API changes.
Comment 13 Niraj Modi CLA 2019-11-19 03:39:31 EST
Marking as resolved, suggest to add an N&N entry for 4.14 M3.
Comment 14 Eclipse Genie CLA 2019-11-19 04:19:05 EST
New Gerrit change created: https://git.eclipse.org/r/152944
Comment 16 Niraj Modi CLA 2019-11-20 08:35:44 EST
Reopening, found one issue while testing.
Steps to reproduce:
1. Launch CustomControlExample.java
2. CCombo Tab >> Enable Popup Menu
3. Right click on the CCombo text area, you should see "Sample popup menu item", click on it.
4. Now try to change CCombo's alignment setting, we get below exception:
Exception in thread "main" java.lang.IllegalArgumentException: Argument not valid
	at org.eclipse.swt.SWT.error(SWT.java:4701)
	at org.eclipse.swt.SWT.error(SWT.java:4635)
	at org.eclipse.swt.SWT.error(SWT.java:4606)
	at org.eclipse.swt.widgets.Widget.error(Widget.java:452)
	at org.eclipse.swt.widgets.Control.setMenu(Control.java:3605)
	at org.eclipse.swt.custom.CCombo.createText(CCombo.java:216)
	at org.eclipse.swt.custom.CCombo.setAlignment(CCombo.java:1542)
	at org.eclipse.swt.examples.controlexample.CComboTab.setExampleWidgetAlignment(CComboTab.java:149)
	at org.eclipse.swt.examples.controlexample.AlignableTab.lambda$0(AlignableTab.java:69)
	at org.eclipse.swt.events.SelectionListener$1.widgetSelected(SelectionListener.java:84)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:252)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4175)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1057)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3988)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3587)
	at org.eclipse.swt.examples.controlexample.CustomControlExample.main(CustomControlExample.java:58)
Comment 17 Niraj Modi CLA 2019-11-20 08:36:14 EST
(In reply to Niraj Modi from comment #16)
> Reopening, found one issue while testing.
> Steps to reproduce:
> 1. Launch CustomControlExample.java
> 2. CCombo Tab >> Enable Popup Menu
> 3. Right click on the CCombo text area, you should see "Sample popup menu
> item", click on it.
> 4. Now try to change CCombo's alignment setting, we get below exception:
> Exception in thread "main" java.lang.IllegalArgumentException: Argument not
> valid
> 	at org.eclipse.swt.SWT.error(SWT.java:4701)
> 	at org.eclipse.swt.SWT.error(SWT.java:4635)
> 	at org.eclipse.swt.SWT.error(SWT.java:4606)
> 	at org.eclipse.swt.widgets.Widget.error(Widget.java:452)
> 	at org.eclipse.swt.widgets.Control.setMenu(Control.java:3605)
> 	at org.eclipse.swt.custom.CCombo.createText(CCombo.java:216)
> 	at org.eclipse.swt.custom.CCombo.setAlignment(CCombo.java:1542)
> 	at
> org.eclipse.swt.examples.controlexample.CComboTab.
> setExampleWidgetAlignment(CComboTab.java:149)
> 	at
> org.eclipse.swt.examples.controlexample.AlignableTab.lambda$0(AlignableTab.
> java:69)
> 	at
> org.eclipse.swt.events.SelectionListener$1.widgetSelected(SelectionListener.
> java:84)
> 	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:252)
> 	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
> 	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4175)
> 	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1057)
> 	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3988)
> 	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3587)
> 	at
> org.eclipse.swt.examples.controlexample.CustomControlExample.
> main(CustomControlExample.java:58)

Solution is to add isDisposed() check before setting the Menu instance on the text instance. Will share a gerrit shortly.
Comment 18 Eclipse Genie CLA 2019-11-20 08:38:07 EST
New Gerrit change created: https://git.eclipse.org/r/153059
Comment 19 Niraj Modi CLA 2019-11-20 08:45:32 EST
(In reply to Niraj Modi from comment #17)
> (In reply to Niraj Modi from comment #16)
> Solution is to add isDisposed() check before setting the Menu instance on
> the text instance. Will share a gerrit shortly.

On GTK's Control.java there is disposed check for other SWT resources in text.setFont(), text.setForeground() and text.getBackground() methods, so will need disposed check for all these resources as well.

(In reply to Eclipse Genie from comment #18)
> New Gerrit change created: https://git.eclipse.org/r/153059
In above patch have added  disposed check for above scenarios.
Comment 20 Paul Pazderski CLA 2019-11-20 08:48:17 EST
Thank you Niraj. I tested align change while the combo list is open but not this case.
Comment 22 Niraj Modi CLA 2019-11-20 09:15:14 EST
(In reply to Eclipse Genie from comment #21)
> Gerrit change https://git.eclipse.org/r/153059 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=60423cd8832a3044b9715c664f8fc1a567aedaac

Resolving, will verify it in next I-Build.
Comment 23 Niraj Modi CLA 2019-11-21 02:39:55 EST
Verified in Build id: I20191120-1800 on Win7.
Comment 24 Eclipse Genie CLA 2019-11-25 09:43:18 EST
New Gerrit change created: https://git.eclipse.org/r/153312
Comment 25 Sebastian Ratz CLA 2019-11-25 09:44:12 EST
The refactoring into the createText() method introduced a problem with focus handling.

checkStyle() in CCombo constructor adds SWT.NO_FOCUS.

This must not be passed along to Text().
Comment 26 Paul Pazderski CLA 2019-11-25 10:37:46 EST
Indeed, my mistake. Thank you Sebastian.

Need approval for RC1.
Comment 28 Niraj Modi CLA 2019-11-26 02:21:56 EST
(In reply to Eclipse Genie from comment #27)
> Gerrit change https://git.eclipse.org/r/153312 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=afcabfabee4e29a0ea94f88a3c634a6ecf6313ef

test_setFocus() is failing on MAC, please check if this can be fixed for RC1.
Comment 29 Niraj Modi CLA 2019-11-26 02:22:42 EST
(In reply to Niraj Modi from comment #28)
> (In reply to Eclipse Genie from comment #27)
> > Gerrit change https://git.eclipse.org/r/153312 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> > ?id=afcabfabee4e29a0ea94f88a3c634a6ecf6313ef
> 
> test_setFocus() is failing on MAC, please check if this can be fixed for RC1.

Link to the JUnit test results:
https://download.eclipse.org/eclipse/downloads/drops4/I20191125-1800/testresults/html/org.eclipse.swt.tests_ep414I-unit-mac64-java8_macosx.cocoa.x86_64_8.0.html
Comment 30 Alexander Kurtakov CLA 2019-11-26 02:33:08 EST
IMHO Mac test failures has to be bringed to ZERO ASAP. It has been multiple releases where we haven't seen a clean mac build so everyone just stopped paying attention to results on Mac. 
If it means disabling currently failing tests on Mac better that way so at least we don't see the number increase.
Comment 31 Niraj Modi CLA 2019-11-26 03:36:55 EST
(In reply to Alexander Kurtakov from comment #30)
> IMHO Mac test failures has to be bringed to ZERO ASAP. It has been multiple
> releases where we haven't seen a clean mac build so everyone just stopped
> paying attention to results on Mac. 
> If it means disabling currently failing tests on Mac better that way so at
> least we don't see the number increase.

Thanks Alex, will disable this particular JUnit sub test-case for MAC.
Comment 32 Eclipse Genie CLA 2019-11-26 03:39:31 EST
New Gerrit change created: https://git.eclipse.org/r/153378
Comment 34 Niraj Modi CLA 2019-11-26 05:41:58 EST
(In reply to Eclipse Genie from comment #33)
> Gerrit change https://git.eclipse.org/r/153378 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=ff08024ac90c3b167589711afec56c2d6cc903ab

(In reply to Niraj Modi from comment #28)
> (In reply to Eclipse Genie from comment #27)
> > Gerrit change https://git.eclipse.org/r/153312 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> > ?id=afcabfabee4e29a0ea94f88a3c634a6ecf6313ef
> 
> test_setFocus() is failing on MAC, please check if this can be fixed for RC1.

Hi Sebastian,
For now we have disabled the JUnit from RC1 point of view, still appreciate if you take a look into why it fails on MAC. Thanks!
Comment 35 Niraj Modi CLA 2019-11-27 10:06:14 EST
(In reply to Niraj Modi from comment #34)
> (In reply to Eclipse Genie from comment #33)
> > Gerrit change https://git.eclipse.org/r/153378 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> > ?id=ff08024ac90c3b167589711afec56c2d6cc903ab
> 
> (In reply to Niraj Modi from comment #28)
> > (In reply to Eclipse Genie from comment #27)
> > > Gerrit change https://git.eclipse.org/r/153312 was merged to [master].
> > > Commit:
> > > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> > > ?id=afcabfabee4e29a0ea94f88a3c634a6ecf6313ef
> > 
> > test_setFocus() is failing on MAC, please check if this can be fixed for RC1.
> 
> Hi Sebastian,
> For now we have disabled the JUnit from RC1 point of view, still appreciate
> if you take a look into why it fails on MAC. Thanks!

I have raised bug 553544 to track above part. Marking this bug resolved.
Comment 36 Eclipse Genie CLA 2019-12-03 00:30:16 EST
New Gerrit change created: https://git.eclipse.org/r/153678
Comment 38 Eclipse Genie CLA 2019-12-03 00:46:06 EST
New Gerrit change created: https://git.eclipse.org/r/153680