Closed Bug 444827 Opened 12 years ago Closed 12 years ago

Add binding for calendar captions

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(1 file)

Attached patch Caption - v1Splinter Review
We have a label and a separator that shows up often as a header. This looks like something that could be consolidated into a binding.
Attachment #329135 - Flags: review?(Berend.Cornelius)
Comment on attachment 329135 [details] [diff] [review]
Caption - v1

patch works fine and consolidates the code as advertised. Some thoughts:

If you set the "orient" attribute at the content tag you can just as well extend to xul:hbox

I am wondering if it would not make sense to consolidate even more: In all four client code snippets you have following elements like
..
    <box orient="horizontal">
      <spacer class="default-spacer"/>
..
These elements could be added to your binding, too and the next element, that is not the same in these four cases could be included with the <children/> tag.
Also the enveloping vertical box around your binding is very similar in all cases. So why not include also this into your binding?

I would then also consider to place the binding into base/content/calendar-item-bindings.xml.

These are all just suggestions. Maybe I have missed something and they don't work out at all. r=berend.
Attachment #329135 - Flags: review?(Berend.Cornelius) → review+
Isn't creating a xbl binding to just consolidate two simple xul elements exaggerated? From my understanding xbl is useful if you want to consolidate behavior of certain elements but that doesn't is the case here. What about performance? Is performance of xbl in xul better as xul in xul? What about extensibility? Is is possible for extensions to overlay the code?
As far as I know xbl always means a certain performance loss, but I doubt that it you can really feel the difference in this case where you just open a dialog. I find that my proposal offered a good extensibility because it allows to add any kind of child elements at a certain position within the binding.
There might be a certain performance loss, but if it at some point becomes a major bottleneck, I think this is something that should rather be fixed in core. I think xbl is also useful to group certain elements that are commonly used. This caption is currently only used in the summary dialog, but in the alarm dialog there will also be a such caption.

Toolkit also sometimes uses bindings to group things, see for example the checkbox bindings, where a binding is solely used to include a stylesheet.

(In reply to comment #1)
> (From update of attachment 329135 [details] [diff] [review])
> patch works fine and consolidates the code as advertised. Some thoughts:
> 
> If you set the "orient" attribute at the content tag you can just as well
> extend to xul:hbox
Good idea, done.

> I am wondering if it would not make sense to consolidate even more: In all four
> ...
I'd like to keep this binding flexible in case we use it in other places. We might not always want to indent things or have the same spacing before/after the line.

> 
> I would then also consider to place the binding into
> base/content/calendar-item-bindings.xml.
Done
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
You need to log in before you can comment on or make changes to this bug.