Duncan Mac-Vicar P. wrote:
% See attached patches.
%
% They fix
%
% - time localization
% - first day of the week
% - clicking on time picker
% - autoclose
% - move the libraries to separate package
Hello Duncan,
I've reviewed and committed date picker patches. Although I have some
objections I've decided to fix it rather then send it back and forth.
...
% + private String toPhpTimeFormat(String format) {
% + String ret = format.replaceAll("(a)+", "a");
% + ret = ret.replaceAll("(H)\\1+", "H");
% + ret = ret.replaceAll("(H)", "G");
This is wrong. It will replace HH with H and immediately H with G which
is most likely not what you meant.
% + // k (0-24) not supported, convert to the 0-23 format
% + ret = ret.replaceAll("(k)\\1+", "H");
% + ret = ret.replaceAll("(k)", "G");
% + // K (0-11) not supported, convert to the 1-12 format
% + ret = ret.replaceAll("(k)\\1+", "h");
% + ret = ret.replaceAll("(k)", "g");
This should likely be (K) not (k).
% + ret = ret.replaceAll("(h)\\1+", "h");
% + ret = ret.replaceAll("(h)", "g");
Again hh is replaced with h and immediately h with g.
% + ret = ret.replaceAll("(m)+", "i");
% + ret = ret.replaceAll("(s)+", "s");
...
% @@ -159,15 +192,18 @@ public class DateTimePickerTag extends TagSupport {
%
% HtmlTag dateAddon = createInputAddonTag("date", "fa fa-calendar");
For icons there's IconTag which ensures consistent usage of icons.
(So that one won't use fa-calendar while others use fa-calendar-o.)
...
% + /**
% + * Convert day java.util.Calendar constants
% + * (for which we can't assume a fixed value)
% + * to an index usable by the javascript picker.
% + *
% + * @return the equivalent index for the javascript
% + * picker
% + */
% + private int getJavascriptPickerDayIndex(int calIndex) throws
IllegalArgumentException {
% + switch (calIndex) {
% + case Calendar.SUNDAY: return 0;
% + case Calendar.MONDAY: return 1;
% + case Calendar.TUESDAY: return 2;
% + case Calendar.WEDNESDAY: return 3;
% + case Calendar.THURSDAY: return 4;
% + case Calendar.FRIDAY: return 5;
% + case Calendar.SATURDAY: return 6;
% + default: throw new IllegalArgumentException("Invalid day " +
calIndex);
% + }
% + }
I think such switch statement is really overkill. Yes, in theory
calendar constant values may change in the future but in fact they are pretty
solid for couple of years now.
...
% + private String toWeirdDateFormat(String format) {
% + String ret = format.replaceAll("(M)\\1\\1\\1+", "MM");
% + ret = ret.replaceAll("MMM", "M");
% + ret = ret.replaceAll("MM", "mm");
% + ret = ret.replaceAll("M", "m");
Exactly the same problem I described in toPhpTimeFormat().
MMMM is replaced by MM which is then replaced by mm.
MMM is replaced by M which is then replaced by m.
% + ret = ret.replaceAll("(E)\\1\\1\\1+", "DD");
% + ret = ret.replaceAll("E+", "D");
% + ret = ret.replaceAll("(D)\\1+", "dd");
% + ret = ret.replaceAll("D", "d");
And again EEEE to DD to dd.
% + ret = ret.replaceAll("(y)\\1\\1\\1+", "yyyy");
% + ret = ret.replaceAll("y+", "yy");
% + return ret;
% + }
And again.
% + private void writePickerHtml(Writer out) throws IOException {
...
% + HtmlTag timeAddon = createInputAddonTag("time", "fa fa-clock-o");
% +
% + HtmlTag timeInput = new HtmlTag("input");
% + //dateInput.setAttribute("value", data.getDate().toString());
% + timeInput.setAttribute("type", "text");
% + timeInput.setAttribute("class", "form-control");
% + timeInput.setAttribute("id", data.getName() +
"_timepicker_widget_input");
% +
% + HtmlTag tzAddon = new HtmlTag("span");
% + tzAddon.setAttribute("id", data.getName() + "_tz_input_addon");
% + tzAddon.setAttribute("class", "input-group-addon");
% + tzAddon.addBody(
% + data.getCalendar().getTimeZone().getDisplayName(
% + false, TimeZone.SHORT, data.getLocale()));
% +
% + HtmlTag col2 = new HtmlTag("div");
% + col2.setAttribute("class", "col-md-5");
% +
% + group.addBody(dateAddon);
% + group.addBody(dateInput);
% + if (!data.getDisableTime()) {
% + group.addBody(timeAddon);
% + group.addBody(timeInput);
% + group.addBody(tzAddon);
% + }
Whole timeAddon and timeInput should be better placed inside the if block.
So they aren't even generated when they aren't used later.
...
% + private void writeI18NMap(Writer out) throws IOException {
% + // generate i18n for the picker here
% + DateFormatSymbols syms = data.getDateFormatSymbols();
% + out.append("<script type='text/javascript'>\n");
% + out.append("$.fn.datepicker.dates['" + data.getLocale() + "'] =
{\n");
% + out.append("days: [ \n");
% + out.append(String.format(" '%s'",
syms.getWeekdays()[Calendar.SUNDAY]));
% + out.append(String.format(", '%s'",
syms.getWeekdays()[Calendar.MONDAY]));
% + out.append(String.format(", '%s'",
syms.getWeekdays()[Calendar.TUESDAY]));
...
% + out.append(String.format(" '%s'",
syms.getMonths()[Calendar.JANUARY]));
% + out.append(String.format(", '%s'",
syms.getMonths()[Calendar.FEBRUARY]));
% + out.append(String.format(", '%s'",
syms.getMonths()[Calendar.MARCH]));
...
Oops, this is why loops were invented.
% public class HtmlTag extends BaseTag {
%
% + private static final String[] VOID_ELEMENTS = new String[] {
% + "area", "base", "br", "col", "command", "embed", "hr", "img",
% + "input", "keygen", "link", "meta", "param", "source", "track",
"wbr"};
...
% + public boolean isVoidElement() {
% + for (String voidElem : VOID_ELEMENTS) {
% + if (getTag().equals(voidElem)) {
% + return true;
% + }
% + }
% + return false;
% + }
This isn't very efficient. For every non-void tag (i.e. majority of tags) it
loops over the whole array. A simple set which implements member() would
be more appropriate.
% + /**
% + * {@inheritDoc}
% + */
% + @Override
% + public String render() {
% + StringBuilder ret = new StringBuilder();
% + ret.append(renderOpenTag());
% + if (isVoidElement()) {
% + ret.deleteCharAt(ret.length() - 1);
% + ret.append(">");
...
">" should likely be "/>" otherwise it produce opening tags not self
closed tags.
Regards,
--
Michael Mráka
Satellite Engineering, Red Hat
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
