Community
Participate
Working Groups
The Combo#addSegmentListener(SegmentListener) API was dropped at some point in bug 230854. For bug 407865, we'd need that.
We need it fixed for Bug 178081 Daniel
Created attachment 236840 [details] Combo win32 patch Initial version of win32 patch. Should be in sync with what we did in bug 230854 for Text, plus contain changes specific to list items of a Combo.
Created attachment 236841 [details] Test case
Please note that some functionality/code common to Text and Combo can be extracted to some shared location and reused by both.
Any comments on the patch? Lakshmi? Markus?
Sorry, I'm completely absorbed by Java 8 work, and I'm also not a master of SWT/win32 internals. CCing Arun.
Thanks, Markus. Hi Arun, The proposed API changes are only: - addSegmentListener (SegmentListener listener) - removeSegmentListener (SegmentListener listener) ========== And I just noticed a problem here: https://bugs.eclipse.org/bugs/attachment.cgi?id=236840&action=diff#a/bundles/org.eclipse.swt/Eclipse_sec10 It was supposed to be: if (hooks (SWT.Segments) || filters (SWT.Segments)) return items [index]; (logical OR, not AND) Thanks!
Created attachment 240334 [details] JUnit tests Hi Arun, The new JUnit test I am uploading fails with the last Combo patch at Combo#removeAll, since OS.CB_RESETCONTENT should also reset the internal item list. I have fixed this problem locally and will reupload the Combo patch. I think it should include any other fixes that would be necessary.
Hi Lina, Thanks for coming up with those very comprehensive unit tests! I'll run these against the new patch once you upload it. As for testing the RTL orientation of the Combo itself, the test_setOrientationI() test probably covers it already and we would not need to add anything new, isn't it? Regarding the changes that are in the currently uploaded patch, with my limited knowledge of the SWT win32 code, I could not find any major issues as such. There are a few minor comments I wanted to add though. The API javadoc should be modified as it states that the API is available on Windows and GTK which is not the case as of now. Is it okay to remove the line of commented code in applyListSegments () ? //if ((style & SWT.H_SCROLL) != 0) setScrollWidth (buffer, true); At https://bugs.eclipse.org/bugs/attachment.cgi?id=236840&action=diff#a/bundles/org.eclipse.swt/Eclipse_sec25 - the else check can probably be removed from case OS.EM_CANUNDO. At https://bugs.eclipse.org/bugs/attachment.cgi?id=236840&action=diff#a/bundles/org.eclipse.swt/Eclipse_sec26 - there seems to be some indentation problem at line number 2636. In the switch case code that is introduced in multiple places, it would be better to indent the case statements so that it is more consistent with the existing switch case statements. This is not a must fix, so feel free to ignore if you think its alright. Like I said, all these changes are mostly cosmetic, the actual patch itself seems to be in very good shape already and seems to work as expected too. So, I'm fine if you want to go ahead and commit the patch with these minor modifications without another review. However, if you want me to take another look at a new patch, please let me know. Thanks!
Hi Lina, Let me know when you're planning to upload/push your new patch. We should try to get it done by end of Friday as next week would be the M6 milestone week...
Hi Arun, thank you so much for your review! I hope I addressed all your comments. I am doing some more manual testing with various |SegmentListener|s and will upload/push the new patch by Sunday. Thanks!
(In reply to Arun Thondapu from comment #9) > As for testing the RTL orientation of the Combo itself, the > test_setOrientationI() test probably covers it already and we would not need > to add anything new, isn't it? Yes, I also think so. (However, I added setOrientation to the manual test to make sure SegmentListener behaves correctly in both of the orientations.)
Done: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=d05349824e51056345d3e4d57c688edd41637c64
Hi Arun, do you have any outstanding comments ? If not, can you please deliver this patch ? For functionality provided in this bug to be properly leveraged we must extend additional API in BidiUtil (for more details please see bug 427794 ). Plugins (Eclipse components in general) are going to leverage functionality from this bug only through this API - applyBidiProcessing. Due to this dependency it would help a lot if the current patch is delivered asap (unless you have outstanding comments of course). Many thank in advance for your help and support.
Hi Tomer, the changes are delivered already.
Sorry for the ignorance :-)) Many thanks Lina & Arun.
(In reply to Lina Kemmel from comment #15) > Hi Tomer, the changes are delivered already. Yep, it would be a good idea to test them in the latest available I-build here - http://download.eclipse.org/eclipse/downloads/drops4/I20140302-2000/ The newly added unit test has failed on Mac and Linux as expected, I'll add some guarding code to the test to make sure it executes only on Windows. Lina, we need to include this new API in the New & Noteworthy for M6. Can you please provide me a brief summary of the API that I can add to the N&N? Thanks in advance!
(In reply to Arun Thondapu from comment #17) > Yep, it would be a good idea to test them in the latest available I-build > here - http://download.eclipse.org/eclipse/downloads/drops4/I20140302-2000/ > I will test that, thanks. > The newly added unit test has failed on Mac and Linux as expected, I'll add > some guarding code to the test to make sure it executes only on Windows. > Yes, it fails at listenerCalled checks, since a SegmentEvent is not sent and the listener is not called on those platforms. Sorry, I had to take this into account. if (!SwtTestUtil.isWindows) { // TODO Fix GTK and Cocoa failure. if (SwtTestUtil.verbose) { System.out .println("Excluded test_consistency_Segments(org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Text)."); } return; } > Lina, we need to include this new API in the New & Noteworthy for M6. Can > you please provide me a brief summary of the API that I can add to the N&N? > Thanks in advance! Yes sure.
(In reply to Lina Kemmel from comment #18) > Yes, it fails at listenerCalled checks, since a SegmentEvent is not sent and > the listener is not called on those platforms. Sorry, I had to take this > into account. Yes, this is the guarding code I was meaning to add too. Are you going to commit the change? We can verify in the next I-build's test runs. > > if (!SwtTestUtil.isWindows) { > // TODO Fix GTK and Cocoa failure. > if (SwtTestUtil.verbose) { > System.out > .println("Excluded > test_consistency_Segments(org.eclipse.swt.tests.junit. > Test_org_eclipse_swt_widgets_Text)."); This should be changed to Combo. > } > return; > } > Thanks!
Disabled the test for GTK+ and Cocoa - http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=c750b3a5a941178a0d38e5a0532e6c1b936c0c50 Marking the bug as resolved.
(In reply to Arun Thondapu from comment #17) > Yep, it would be a good idea to test them in the latest available I-build > here - http://download.eclipse.org/eclipse/downloads/drops4/I20140302-2000/ > It seems to behave properly in the I20140302-2000 build. Marking as Verified. Thanks, Arun, for all your help and guidance.
(In reply to Lina Kemmel from comment #21) > (In reply to Arun Thondapu from comment #17) > > > Yep, it would be a good idea to test them in the latest available I-build > > here - http://download.eclipse.org/eclipse/downloads/drops4/I20140302-2000/ > > > > It seems to behave properly in the I20140302-2000 build. Marking as Verified. > Thanks, Arun, for all your help and guidance. You're welcome Lina! Do you have the content for the N&N ready yet? We need to submit it by tomorrow.
Created attachment 240587 [details] Addition for N&N Arun, please let me know if it's okay for N&N. I would also like to add a snippet referenced in the N&N.
(In reply to Lina Kemmel from comment #23) > Created attachment 240587 [details] > Addition for N&N > > Arun, please let me know if it's okay for N&N. > I would also like to add a snippet referenced in the N&N. Looks good to me. Please push the snippet as well as the N&N content (along with the screenshot) directly to the git repo. Thanks!
Thank you, done!
