Details

    • Sub-task
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 5.0M15
    • Classlib
    • Patch Available

    Description

      Implementation of ordering feature for ImageIO.spi.ServiceRegistry class as discussed with Alexei Fedotov on the dev mailing list. Test case is also provided.

      Attachments

        1. imageio_build_v2.diff
          4 kB
          Lang Yang
        2. imageio_build.diff
          2 kB
          Lang Yang
        3. HARMONY-6507-lang-3.txt
          15 kB
          Lang Yang

        Activity

          leshik Alexei Fedotov added a comment -

          Hello Lang,

          I'm trying to apply your patch and run tests. I got the following problem:

          testGetServiceProviders Failure expected:<org.apache.harmony.x.imageio.plugins.png.PNGImageReaderSpi@29e872c8> but was:<org.apache.harmony.x.imageio.plugins.jpeg.JPEGImageReaderSpi@29e87260>

          Somthing is broken, I cannot commit that. Could you please fix it?

          leshik Alexei Fedotov added a comment - Hello Lang, I'm trying to apply your patch and run tests. I got the following problem: testGetServiceProviders Failure expected:<org.apache.harmony.x.imageio.plugins.png.PNGImageReaderSpi@29e872c8> but was:<org.apache.harmony.x.imageio.plugins.jpeg.JPEGImageReaderSpi@29e87260> Somthing is broken, I cannot commit that. Could you please fix it?
          yanglang Lang Yang added a comment -

          Hello Alexei,

          I have made some changes against the testGetServiceProviders method in the new patach. The ServiceRegistry ordering is a little bit hard to test. There could be serveral correct return results.

          I had passed all the test cases before I submitted the previouse patch. Don't know why it didn't work on your computer. Could you please re-test using the new patch and let me know the result?

          Thank you,

          Lang

          yanglang Lang Yang added a comment - Hello Alexei, I have made some changes against the testGetServiceProviders method in the new patach. The ServiceRegistry ordering is a little bit hard to test. There could be serveral correct return results. I had passed all the test cases before I submitted the previouse patch. Don't know why it didn't work on your computer. Could you please re-test using the new patch and let me know the result? Thank you, Lang
          leshik Alexei Fedotov added a comment -

          Lang, I'm trying to understand the testGetServiceProviders testcase. Why do you add jpegWriter to CATEGORIES[1] category? How does RI run this test if you set test.jre.home to RI directory?

          $ ant -Dbuild.module=imageio -Dtest.jre.home=<path to RI> test

          leshik Alexei Fedotov added a comment - Lang, I'm trying to understand the testGetServiceProviders testcase. Why do you add jpegWriter to CATEGORIES [1] category? How does RI run this test if you set test.jre.home to RI directory? $ ant -Dbuild.module=imageio -Dtest.jre.home=<path to RI> test
          yanglang Lang Yang added a comment -

          Hello Alexei,

          Cause we only have 3 reader spis, so I wanted to add jpegWriter to CATEGORIES[1] to get one more element to be sorted. I had read Harmony's code, and found that the current implementation allows a different type of spis to a certain category. Therefore, I did so.

          After I have tested on RI, I found out that RI doesn't allow this operation. So, I have fixed this issue. Some other bugs have also been fixed accordingly.

          Please test the new patch and let me know if there any problems.

          Thank you,

          Lang

          yanglang Lang Yang added a comment - Hello Alexei, Cause we only have 3 reader spis, so I wanted to add jpegWriter to CATEGORIES [1] to get one more element to be sorted. I had read Harmony's code, and found that the current implementation allows a different type of spis to a certain category. Therefore, I did so. After I have tested on RI, I found out that RI doesn't allow this operation. So, I have fixed this issue. Some other bugs have also been fixed accordingly. Please test the new patch and let me know if there any problems. Thank you, Lang
          yanglang Lang Yang added a comment -

          Hi, I have made some changes against build.xml and make/run-test.xml file (see imageio_build.diff) in order to put supporting classes to classpath when running test cases.

          yanglang Lang Yang added a comment - Hi, I have made some changes against build.xml and make/run-test.xml file (see imageio_build.diff) in order to put supporting classes to classpath when running test cases.
          hindessm Mark Hindess added a comment -

          At first glance the build.xml patch doesn't look quite right. If you have a compile with an include then I'd expect the other compile to exclude the same set of files to avoid compiling them twice?

          hindessm Mark Hindess added a comment - At first glance the build.xml patch doesn't look quite right. If you have a compile with an include then I'd expect the other compile to exclude the same set of files to avoid compiling them twice?
          yanglang Lang Yang added a comment -

          Thanks Mark, you are right.

          I uploaded a new version of build.xml patch. In this patch, I want to keep support files in "src/test/support" folder, api specific tests in "src/test/api/java" folder and impl specific tests in "src/test/impl/java" folder. In run-test.xml, I have created two new targets: "-test-module" is for api specific tests and "-test-module-impl" is for impl specific tests.

          For some reason, the patch doesn't includes the folders I have created. If you think the patch looks ok, could you please create these folders:
          src/test/api/
          src/test/impl/java
          src/test/support/common/java

          and move the "src/test/java" folder under "src/test/api" (as they are all api specific tests)

          Thank you very much, let me know if there any thing wrong.

          Lang

          yanglang Lang Yang added a comment - Thanks Mark, you are right. I uploaded a new version of build.xml patch. In this patch, I want to keep support files in "src/test/support" folder, api specific tests in "src/test/api/java" folder and impl specific tests in "src/test/impl/java" folder. In run-test.xml, I have created two new targets: "-test-module" is for api specific tests and "-test-module-impl" is for impl specific tests. For some reason, the patch doesn't includes the folders I have created. If you think the patch looks ok, could you please create these folders: src/test/api/ src/test/impl/java src/test/support/common/java and move the "src/test/java" folder under "src/test/api" (as they are all api specific tests) Thank you very much, let me know if there any thing wrong. Lang
          leshik Alexei Fedotov added a comment -

          Lang, thank you for the patch. Please, check if the patch is applied correctly at r953413.

          I adjusted the test for RI, renamed "emptyList" to "firstNodes", added few comments and Wikipedia reference about the algorithm, moved localization strings into message.properties. I also removed @SuppressWarnings("unchecked") since I didn't see any warnings. My environment doesn't show warnings, but maybe yours one does.

          leshik Alexei Fedotov added a comment - Lang, thank you for the patch. Please, check if the patch is applied correctly at r953413. I adjusted the test for RI, renamed "emptyList" to "firstNodes", added few comments and Wikipedia reference about the algorithm, moved localization strings into message.properties. I also removed @SuppressWarnings("unchecked") since I didn't see any warnings. My environment doesn't show warnings, but maybe yours one does.
          leshik Alexei Fedotov added a comment -

          ... and yes, I have fixed a small conflict with Tim's commit of HARMONY-6518.

          leshik Alexei Fedotov added a comment - ... and yes, I have fixed a small conflict with Tim's commit of HARMONY-6518 .
          hudson Hudson added a comment -

          Integrated in Harmony-1.5-head-linux-x86_64 #842 (See http://hudson.zones.apache.org/hudson/job/Harmony-1.5-head-linux-x86_64/842/)
          Committed Lang's patch for HARMONY-6507: implemented service registry ordering.

          hudson Hudson added a comment - Integrated in Harmony-1.5-head-linux-x86_64 #842 (See http://hudson.zones.apache.org/hudson/job/Harmony-1.5-head-linux-x86_64/842/ ) Committed Lang's patch for HARMONY-6507 : implemented service registry ordering.
          hudson Hudson added a comment -

          Integrated in Harmony-select-1.5-head-linux-x86_64 #29 (See http://hudson.zones.apache.org/hudson/job/Harmony-select-1.5-head-linux-x86_64/29/)
          Committed Lang's patch for HARMONY-6507: implemented service registry ordering.

          hudson Hudson added a comment - Integrated in Harmony-select-1.5-head-linux-x86_64 #29 (See http://hudson.zones.apache.org/hudson/job/Harmony-select-1.5-head-linux-x86_64/29/ ) Committed Lang's patch for HARMONY-6507 : implemented service registry ordering.

          People

            leshik Alexei Fedotov
            yanglang Lang Yang
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: