Uploaded image for project: 'Click'
  1. Click
  2. CLK-79

click.util.Format does not use the request's locale

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: core
    • Labels:
      None

      Description

      Hi,

      Format is a great helper but I think Format has the problem that it does not use the client's locale.

      I'd like to give the current client-locale to Format. I was thinking that Page.getFormat() can be changed to:

      getFormat() {
      if(format == null)

      { format = new Format(getContext().getRequest().getLocale()); }

      return format;
      }

      The problem here is that getFormat() is called before the context is set. Maybe the setting of the Format by click-servlet could be removed at all (including the click-xml entry).

      The configuration of format would than happen through overriding getFormat(). I think with the current trend to no xml this
      would look better. Would make the the whole thing just easier to understand and is also faster than looking up the format in ClickApp and newInstance(). Is there a use-case for having the format specified in click-xml?

      Thanks,
      Christian

        Activity

        Hide
        medgar Malcolm Edgar added a comment -

        Hi Christian,

        I had a related conversation with Ahmend about the Format object a while ago.

        Yes I think it would be good for the Format object to have a Locale variable. This could be provided as a constructor argument or through a setter method. I prefer the constructor argument, as it would require subclasses to explicity set this value.

        Currently the Format object extends Object, where the concept is that you can specify any classname as long as it has a no args construtor. This concept would still apply however your format class must have a Locale arg constructor.

        Regarding the use case, I don't think the current pattern is bad, as you get the best of both worlds. You can set the Format class globally for all your pages (the most common scenario), and you can override this by subclassing Page and override the getFormat() method.

        regards Malcolm Edgar

        Show
        medgar Malcolm Edgar added a comment - Hi Christian, I had a related conversation with Ahmend about the Format object a while ago. Yes I think it would be good for the Format object to have a Locale variable. This could be provided as a constructor argument or through a setter method. I prefer the constructor argument, as it would require subclasses to explicity set this value. Currently the Format object extends Object, where the concept is that you can specify any classname as long as it has a no args construtor. This concept would still apply however your format class must have a Locale arg constructor. Regarding the use case, I don't think the current pattern is bad, as you get the best of both worlds. You can set the Format class globally for all your pages (the most common scenario), and you can override this by subclassing Page and override the getFormat() method. regards Malcolm Edgar
        Hide
        click_christian Christian Essl added a comment -

        Hi Malcolm,

        I also think that the (any) Format should be constructor setup. I don't like setters for essential values.

        I think the Format should take the Page in the constructor and get the Context when needed.. The resons is that when overriding Page.getFormat() the Context is not there when this method is called (by the ClickServlet.initPage()). The Contex is also not available in the constructor to set the Format.

        While I have mostly common super-pages and in 90% of the cases Format is enough for me I agree with you regarding the use-case. It is good to have a central place in case you forget to override in a special page.

        Thanks,
        Christian

        Show
        click_christian Christian Essl added a comment - Hi Malcolm, I also think that the (any) Format should be constructor setup. I don't like setters for essential values. I think the Format should take the Page in the constructor and get the Context when needed.. The resons is that when overriding Page.getFormat() the Context is not there when this method is called (by the ClickServlet.initPage()). The Contex is also not available in the constructor to set the Format. While I have mostly common super-pages and in 90% of the cases Format is enough for me I agree with you regarding the use-case. It is good to have a central place in case you forget to override in a special page. Thanks, Christian
        Hide
        medgar Malcolm Edgar added a comment -

        Changes checked into CVS. Will be available in release 0.19.

        Show
        medgar Malcolm Edgar added a comment - Changes checked into CVS. Will be available in release 0.19.
        Hide
        click_christian Christian Essl added a comment -

        Hi Malcolm,

        Thanks for fixing this in a nice way (Locale in Constructor). I have two (minor) questions mainly about performance:

        Why is there no check anymore wheter the Page has already a format: if(page.getFormat() != null) page.setFormat(...)
        Maybe it would be better to store the Format's contstructor instead of the class in ClickApp.

        Thanks,
        Christian

        Show
        click_christian Christian Essl added a comment - Hi Malcolm, Thanks for fixing this in a nice way (Locale in Constructor). I have two (minor) questions mainly about performance: Why is there no check anymore wheter the Page has already a format: if(page.getFormat() != null) page.setFormat(...) Maybe it would be better to store the Format's contstructor instead of the class in ClickApp. Thanks, Christian

          People

          • Assignee:
            medgar Malcolm Edgar
            Reporter:
            click_christian Christian Essl
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development