Pivot
  1. Pivot
  2. PIVOT-713

Consider making some inner classes non-final

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.1
    • Component/s: wtk
    • Labels:
      None

      Description

      I am implementing a groovy-builder-like DSL for pivot using scala. In the DSL, I need subclass of each component/container that retains original component/container behavior but is aware of the DSL context as well. All goes well except GridPane.Row.

      1. patch_remove_final_from_all_public_static_final_class.patch
        9 kB
        Sandro Martini
      2. patch.patch
        2 kB
        Sandro Martini
      3. pivot_final_class_grep.txt
        7 kB
        Sandro Martini
      4. pivot_public_static_final_class_grep.txt
        2 kB
        Sandro Martini

        Activity

        Hide
        Sandro Martini added a comment -

        Mark as improvement, because in standard cases this works good, but currently it's not overridable.

        Show
        Sandro Martini added a comment - Mark as improvement, because in standard cases this works good, but currently it's not overridable.
        Hide
        Chris Bartlett added a comment -

        Before undertaking this change, it might be worth performing a quick review of similar public final classes to see if there are other inconsistencies.

        If GridPane.Row will be made non-final, should it's private 'cells' field also be changed to protected or a protected getter added? (And then also for TablePane.Row to be consistent?)

        Show
        Chris Bartlett added a comment - Before undertaking this change, it might be worth performing a quick review of similar public final classes to see if there are other inconsistencies. If GridPane.Row will be made non-final, should it's private 'cells' field also be changed to protected or a protected getter added? (And then also for TablePane.Row to be consistent?)
        Hide
        Greg Brown added a comment -

        What is the use case for extending this class? I'm not suggesting there isn't one - I'd just like to understand what it is.

        Show
        Greg Brown added a comment - What is the use case for extending this class? I'm not suggesting there isn't one - I'd just like to understand what it is.
        Hide
        xiefei added a comment -

        I am implementing a groovy-builder-like DSL for pivot using scala. In the DSL, I need subclass of each component/container that retains original component/container behavior but is aware of the DSL context as well. All goes well except GridPane.Row.

        Show
        xiefei added a comment - I am implementing a groovy-builder-like DSL for pivot using scala. In the DSL, I need subclass of each component/container that retains original component/container behavior but is aware of the DSL context as well. All goes well except GridPane.Row.
        Hide
        Greg Brown added a comment -

        Renaming issue to more accurately capture the requirement.

        Show
        Greg Brown added a comment - Renaming issue to more accurately capture the requirement.
        Hide
        Greg Brown added a comment -

        FYI, I copied your comment into the description field so that it is easier to understand the motivation behind this issue.

        Show
        Greg Brown added a comment - FYI, I copied your comment into the description field so that it is easier to understand the motivation behind this issue.
        Hide
        Sandro Martini added a comment -

        removed final from GridPane.Row, and add protected method there and even in TablePane.Row, as requested

        Show
        Sandro Martini added a comment - removed final from GridPane.Row, and add protected method there and even in TablePane.Row, as requested
        Hide
        Sandro Martini added a comment -

        Hi all,
        with the patch here in attach I think all the changes required are done, tell me if they are Ok (and maybe if something other here or in other related classes is missing), so please try the fix. If there aren't objections, next week I'll commit the patch.

        Bye,
        Sandro

        Show
        Sandro Martini added a comment - Hi all, with the patch here in attach I think all the changes required are done, tell me if they are Ok (and maybe if something other here or in other related classes is missing), so please try the fix. If there aren't objections, next week I'll commit the patch. Bye, Sandro
        Hide
        Greg Brown added a comment -

        There may be a few others as well. Did you search for "public static final class"?

        Show
        Greg Brown added a comment - There may be a few others as well. Did you search for "public static final class"?
        Hide
        Sandro Martini added a comment -

        No, at the moment I've only fixed the required class and add method, as requested here.
        If there are other issues I'm not against removing the final qualifier, but probably we should see what real cases are requiring it. But I think it's safe to commit this small patch at the moment.

        If you think it's useful, I can try to search all "public static final class" under the skin and remove the final ... what do you think ?

        Show
        Sandro Martini added a comment - No, at the moment I've only fixed the required class and add method, as requested here. If there are other issues I'm not against removing the final qualifier, but probably we should see what real cases are requiring it. But I think it's safe to commit this small patch at the moment. If you think it's useful, I can try to search all "public static final class" under the skin and remove the final ... what do you think ?
        Hide
        Greg Brown added a comment -

        > If there are other issues I'm not against removing the final qualifier, but probably we should see what real cases are requiring it.

        The use case is the builder pattern. A similar issue came up a couple months back when a Java-based builder pattern was suggested. Unless all of these classes are made non-final, such patterns cannot be universally employed.

        I suggest that you generate a list of static final classes and attach it to this issue for review.

        Show
        Greg Brown added a comment - > If there are other issues I'm not against removing the final qualifier, but probably we should see what real cases are requiring it. The use case is the builder pattern. A similar issue came up a couple months back when a Java-based builder pattern was suggested. Unless all of these classes are made non-final, such patterns cannot be universally employed. I suggest that you generate a list of static final classes and attach it to this issue for review.
        Hide
        Sandro Martini added a comment -

        this is the list of Pivot sources (I think updated to last week) containing "final class" inside

        Show
        Sandro Martini added a comment - this is the list of Pivot sources (I think updated to last week) containing "final class" inside
        Hide
        Sandro Martini added a comment -

        just put in attach a txt containing the output of a grep "final class" on all Pivot sources.
        In my opinion could be good to remove final from all wtk-terra and maybe even wtk classes, but for core and other classes I don't know, we have to look better ... Greg and Chris what do you think ?

        xiefei, you think it would be enough for your Scala code to work ?

        Bye,
        Sandro

        Show
        Sandro Martini added a comment - just put in attach a txt containing the output of a grep "final class" on all Pivot sources. In my opinion could be good to remove final from all wtk-terra and maybe even wtk classes, but for core and other classes I don't know, we have to look better ... Greg and Chris what do you think ? xiefei, you think it would be enough for your Scala code to work ? Bye, Sandro
        Hide
        Greg Brown added a comment -

        You should search for public static final classes only - private or non-static inner classes should probably remain as-is.

        Show
        Greg Brown added a comment - You should search for public static final classes only - private or non-static inner classes should probably remain as-is.
        Hide
        Sandro Martini added a comment -

        list of Pivot sources containing "public static final class".
        Note that I have to verify what FindBugs say after removing the final keyword (on performances etc) in some of them, just for test.

        Show
        Sandro Martini added a comment - list of Pivot sources containing "public static final class". Note that I have to verify what FindBugs say after removing the final keyword (on performances etc) in some of them, just for test.
        Hide
        Sandro Martini added a comment -

        Patch to remove final from all public static final class in Pivot sources.

        Tell me if it's Ok to apply.

        Show
        Sandro Martini added a comment - Patch to remove final from all public static final class in Pivot sources. Tell me if it's Ok to apply.
        Hide
        Greg Brown added a comment -

        Actually, now that I see the list, I think the only ones that should be non-final are GridPane.Row and TableView.Column. The others are not as likely to be subclassed.

        Show
        Greg Brown added a comment - Actually, now that I see the list, I think the only ones that should be non-final are GridPane.Row and TableView.Column. The others are not as likely to be subclassed.
        Hide
        Sandro Martini added a comment -

        Ok, so in next days I'll revert and apply only the first patch (patch.patch, here in attach).

        Later if some other change will be required, be free to reopen the ticket and add some info.

        Bye,
        Sandro

        Show
        Sandro Martini added a comment - Ok, so in next days I'll revert and apply only the first patch (patch.patch, here in attach). Later if some other change will be required, be free to reopen the ticket and add some info. Bye, Sandro
        Hide
        Greg Brown added a comment -

        No, the first patch is wrong too. The only classes that should be made non-final are GridPane.Row and TableView.Column.

        Show
        Greg Brown added a comment - No, the first patch is wrong too. The only classes that should be made non-final are GridPane.Row and TableView.Column.
        Hide
        Sandro Martini added a comment -

        Ok, I'll change only those definitions.

        But what do you think on this comment by Chris ?
        >If GridPane.Row will be made non-final, should it's private 'cells' field also be changed to protected or a protected getter added? (And then also for TablePane.Row to be consistent?)
        So my previous addition even of a protected getCells() method in GridPane and TablePane.
        Tell me if this is needed.

        And last:
        wtk\src\org\apache\pivot\wtk\GridPane.java
        00260: public static class Filler extends Component {
        wtk\src\org\apache\pivot\wtk\TablePane.java
        00622: public static final class Filler extends Component {
        I think it would be better to put GridPane.Filler final, ok ? Tell me.

        Bye,
        Sandro

        Show
        Sandro Martini added a comment - Ok, I'll change only those definitions. But what do you think on this comment by Chris ? >If GridPane.Row will be made non-final, should it's private 'cells' field also be changed to protected or a protected getter added? (And then also for TablePane.Row to be consistent?) So my previous addition even of a protected getCells() method in GridPane and TablePane. Tell me if this is needed. And last: wtk\src\org\apache\pivot\wtk\GridPane.java 00260: public static class Filler extends Component { wtk\src\org\apache\pivot\wtk\TablePane.java 00622: public static final class Filler extends Component { I think it would be better to put GridPane.Filler final, ok ? Tell me. Bye, Sandro
        Hide
        Greg Brown added a comment -

        No, cells should not have a protected accessor. GridPane.Row implements Sequence<Component> and wraps the cell list, so its contents are already accessible. Further, providing a protected accessor would allow subclasses to circumvent the event processing performed by the base class, which would be bad.

        The use case for making these classes non-final is simply to support builders - it shouldn't be viewed as an excuse to break encapsulation.

        Show
        Greg Brown added a comment - No, cells should not have a protected accessor. GridPane.Row implements Sequence<Component> and wraps the cell list, so its contents are already accessible. Further, providing a protected accessor would allow subclasses to circumvent the event processing performed by the base class, which would be bad. The use case for making these classes non-final is simply to support builders - it shouldn't be viewed as an excuse to break encapsulation.
        Hide
        Sandro Martini added a comment -

        Ok, now all is clear (I hope ).

        Thank you very much

        Show
        Sandro Martini added a comment - Ok, now all is clear (I hope ). Thank you very much
        Hide
        Sandro Martini added a comment -

        resolved, as discussed with Greg and others

        Show
        Sandro Martini added a comment - resolved, as discussed with Greg and others

          People

          • Assignee:
            Sandro Martini
            Reporter:
            xiefei
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development