OFBiz
  1. OFBiz
  2. OFBIZ-3519

Slightly modified Visual Theme Resource Enums for better readability

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: SVN trunk
    • Component/s: ALL COMPONENTS
    • Labels:
      None

      Description

      Minor enhancement to the visual theme resource enumId so that a more verbose name of the resource or template file can be specified.

      The suffix STYLESHEET and JAVASCRIPT in the enum impose a restriction on specifying a meaningful name of the enumId field which is max 20 chars in size.

      The suffix in the enum contains following changes.

      1. STYLESHEET is replaced with CSS
      2. JAVASCRIPT is replaced with JS
      3. LOC is completely removed from TMPLT, since it is anyway specify a template location.

      With all above points the total char size of the enum decreased and it gives an opportunity to specify better names and hence more readability.

      For example VT_HDR_TMPLT_LOC becomes VT_HEADER_TMPLT

      1. enum.patch
        57 kB
        Vikas Mayur

        Activity

        Hide
        Jacques Le Roux added a comment -

        Yes, I agree with Rishi, Scott's proposition is the better... if someone is happy to implement it. Don't forget comments above removing.

        Thanks

        Show
        Jacques Le Roux added a comment - Yes, I agree with Rishi, Scott's proposition is the better... if someone is happy to implement it. Don't forget comments above removing. Thanks
        Hide
        Rishi Solanki added a comment -

        +1 for Scott's suggestion, as the new ID's are giving more verbose meaning (IMO). Later we can remove the old one as dependencies resolve.

        Show
        Rishi Solanki added a comment - +1 for Scott's suggestion, as the new ID's are giving more verbose meaning (IMO). Later we can remove the old one as dependencies resolve.
        Hide
        Scott Gray added a comment -

        Well we've had a couple of suggestions:

        • Support both the new IDs and the old (me)
        • Increase the field length to allow for more verbose names (Adrian)
        • Do nothing (Jacques, Hans)
        • Provide a migration service (Jacques)
        Show
        Scott Gray added a comment - Well we've had a couple of suggestions: Support both the new IDs and the old (me) Increase the field length to allow for more verbose names (Adrian) Do nothing (Jacques, Hans) Provide a migration service (Jacques)
        Hide
        Rishi Solanki added a comment -

        Any conclusion?

        Show
        Rishi Solanki added a comment - Any conclusion?
        Hide
        Adrian Crum added a comment -

        Why not increase the length of the field and keep the seed data as it is?

        Show
        Adrian Crum added a comment - Why not increase the length of the field and keep the seed data as it is?
        Hide
        Jacques Le Roux added a comment -

        Then sorry, I don't think it's worth it. But maybe Vikas can explain more? Because Anil seems to really need this feature.

        Show
        Jacques Le Roux added a comment - Then sorry, I don't think it's worth it. But maybe Vikas can explain more? Because Anil seems to really need this feature.
        Hide
        Scott Gray added a comment -

        My guess is that it is to allow more specific ids in the future and still provide naming consistency.

        Show
        Scott Gray added a comment - My guess is that it is to allow more specific ids in the future and still provide naming consistency.
        Hide
        Jacques Le Roux added a comment -

        Is it only to be able to change ids from (for instance) VT_HDR_TMPLT_LOC to VT_HEADER_TMPLT?

        Show
        Jacques Le Roux added a comment - Is it only to be able to change ids from (for instance) VT_HDR_TMPLT_LOC to VT_HEADER_TMPLT?
        Hide
        Scott Gray added a comment -

        I just pointed out a potential difficulty that should be taken into consideration, I never said I don't think we should proceed. This could be overcome by checking both keys during rendering and maybe we can create a theme worker class that allows the complexities to be moved away from the templates?

        Show
        Scott Gray added a comment - I just pointed out a potential difficulty that should be taken into consideration, I never said I don't think we should proceed. This could be overcome by checking both keys during rendering and maybe we can create a theme worker class that allows the complexities to be moved away from the templates?
        Hide
        Hans Bakker added a comment -

        i completely agree, for minor readability improvement we are not going the change key seed data.

        Please revert this change.

        Regards,
        Hans.

        Show
        Hans Bakker added a comment - i completely agree, for minor readability improvement we are not going the change key seed data. Please revert this change. Regards, Hans.
        Hide
        Anil K Patel added a comment -

        This puts us in difficult spot.

        In Ofbiz community code gets put in so quickly that others don't get enough time to review and sometimes feedback is does not get heard. If I don't like the piece of code then What Am I supposed to do?

        I think we should be allowed to fix the issue here. In order to not break old installations, we can keep the old data and not use it.

        Show
        Anil K Patel added a comment - This puts us in difficult spot. In Ofbiz community code gets put in so quickly that others don't get enough time to review and sometimes feedback is does not get heard. If I don't like the piece of code then What Am I supposed to do? I think we should be allowed to fix the issue here. In order to not break old installations, we can keep the old data and not use it.
        Hide
        Jacques Le Roux added a comment -

        Sorry Vikas,

        But as Scott pointed out, I don't think it's worth it...

        Show
        Jacques Le Roux added a comment - Sorry Vikas, But as Scott pointed out, I don't think it's worth it...
        Show
        Jacques Le Roux added a comment - I can see nothing apart this http://cwiki.apache.org/confluence/display/OFBTECH/Revisions+Requiring+Data+Migration
        Hide
        Scott Gray added a comment -

        Does anyone know if we have a policy for changing seed data ids? I'm guessing this change will cause problems for anyone upgrading.

        Show
        Scott Gray added a comment - Does anyone know if we have a policy for changing seed data ids? I'm guessing this change will cause problems for anyone upgrading.
        Hide
        Vikas Mayur added a comment -

        I would like to commit this patch if no one have any objection.

        Show
        Vikas Mayur added a comment - I would like to commit this patch if no one have any objection.

          People

          • Assignee:
            Unassigned
            Reporter:
            Vikas Mayur
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development