Mailing List Archive

>> At least on our proxied modperl installation, VHOST_SERVER_NAME is set
>> to *, so using that doesn't work. I can't use javascript to get it,
>> since I can't get around the cross domain issues of going the other
>> direction of passing bric app domain => preview domain (we would need
>> to put a static file on the preview server to be used as a proxy for
>> the data).
>
> Bric::Util::ApacheReq->instance->hostname

Yeah, that's what I'm using, but it gets a little ugly:

if (not PREVIEW_LOCAL and $ar) {
$url .= (index($url, '?') == -1 ? '?' : '&') . 'bric_app_uri=';
my ($proto, $port);
if (SSL_ENABLE && ALWAYS_USE_SSL) {
$proto = 'https://';
$port = (SSL_PORT eq '*' or SSL_PORT == 443) ? '' : ':' . SSL_PORT;
}
else {
$proto = 'http://';
$port = (LISTEN_PORT eq '*' or LISTEN_PORT == 80) ? '' : ':' .
LISTEN_PORT;
}
$url .= $proto . $ar->hostname . $port;
}

Even then, this doesn't quite work on our proxied modperl installation,
since Bricolage runs internally http, but is running https on the
outside (SSL_ENABLE = false).

>> - First, create an empty iframe on the preview page. Change the
>> contents of that empty iframe to be a form that submits all the data
>> needed to modify the field. This is where we use the Bricolage
>> server's uri that was passed along with the redirected preview page.
>>
>> - After submitting the form, the preview frame beings polling its own
>> url for changes (more on this in a bit). It times out after around 30
>> seconds.
>
> Hrm. Why wouldn't the iFrame just wait for the response from its submit
> to Bricolage?

The iframe does wait for the response, but the iframe is now on the
bricolage app domain and it now can't communicate with the preview frame
directly. After the iframe loads (now on the app domain), it can grab
the redirected preview uri and update the preview frame's url to change
the fragment identifier. That's what the preview frame is polling
itself for.

>> - The previous form was posted to the inline_edit.html page. It does
>> its thing, and returns the status. However, because this page is
>> loaded inside an iframe on the preview page, it loses the ability to
>> communicate directly with the preview page (cross-domain). We can do
>> stuff with the frameset/top window (and any other page on the same
>> domain).
>
> Can the preview page poll the iFrame?

Nope. After the iframe posts to the bricolage domain, the preview page
loses its permission to access the iframe.

>> I hope this all explains the weirdness in case someone else needs to
>> do something with this code in the future.
>
> Be sure to add comments about all this to the code, as no one editing
> this stuff in the future will look for explanations on the mail list! :-)

Yup, I'll try get that done when I get a chance. Ideally, I'd like to
have a central location for this overview. Not sure which file would be
the best place.

Adrian

On Feb 26, 2009, at 11:46 AM, Matt Rolf wrote:

>> The Bricolage UI is probably its single weakest point, despite all
>> the work that Marshall has done to improve it (it used to be so
>> much worse!).
>
> This is true. Marshall has laid a very strong foundation (css,
> prototype, etc) for future improvements.

Marshall++

>> I would be surprised, though, to hear of one that improves upon
>> what GS is doing to save clicks for WHO. Except maybe…
>>
>> This all might be boo magical and confusing, frankly.
>
> Yes, that is the other option that was running through my head, and
> I agree that it has the same draw back. But take a look at this
> (just a white board drawing, unfortunately).
>
> http://personal.denison.edu/~rolfm/story_edit.jpg
>
> This is what I was talking about with tabs in the Story profile
> place. File would be a drop down with things like "Check in
> and . . .", "Revert . . . " "Delete" etc. Edit is the default tab.
> You click preview and it opens the preview in that tab (would
> probably drop down for multiple OCs). Details is Info, categories,
> etc.

Yeah, I like the way you're thinking. The interface needs a radical
rethinking, so I'm glad someone is actually doing it.

> So this idea is maybe not even half baked, but maybe it can
> illustrate what I mean when I talk about, what does preview mean.

Sure.

>> Oh, I'm sure they are. But they won't be able to do all their work
>> in the preview window. You can't add elements or fields, for
>> example, just edit existing ones.
>
> And I think they will want to. It will be "easier" to do all the
> editing in the preview mode. So I think there will be pressure to
> make that the main interface.

I'm not sure that's a bad idea. If people want it, and it seems
natural, perhaps we should do it. And if we shouldn't, then we should
figure out a better way to give people the relief they needs. Your
users hate Bricolage for very good reasons. So how do we make them
love it? If it's inline editing, then so what? WYSIWYG took off in the
80s and 90s for a reason.

>>> Which reminds me, I did a survey on our implementation at the end
>>> of last year. I should share some of the information I got from
>>> that with the list.
>>
>> Yeah, that'd be cool.
>
> I need to get permission from people to post comments. I'll be back.

Of course.

>> You're right, I am one BAMF. So don't F with me, MF, Or I'll FYU,
>> shure.
>
> It was only yesterday that I realized how little slang I typically
> use on this list, and how much I use elsewhere on the internets.

Word up, biatch.

Best,

David

On Feb 26, 2009, at 12:03 PM, Adrian Yee wrote:

>> Bric::Util::ApacheReq->instance->hostname
>
> Yeah, that's what I'm using, but it gets a little ugly:
>
> if (not PREVIEW_LOCAL and $ar) {
> $url .= (index($url, '?') == -1 ? '?' : '&') . 'bric_app_uri=';
> my ($proto, $port);
> if (SSL_ENABLE && ALWAYS_USE_SSL) {
> $proto = 'https://';
> $port = (SSL_PORT eq '*' or SSL_PORT == 443) ? '' : ':' .
> SSL_PORT;
> }
> else {
> $proto = 'http://';
> $port = (LISTEN_PORT eq '*' or LISTEN_PORT == 80) ? '' :
> ':' . LISTEN_PORT;
> }
> $url .= $proto . $ar->hostname . $port;
> }
>
> Even then, this doesn't quite work on our proxied modperl
> installation, since Bricolage runs internally http, but is running
> https on the outside (SSL_ENABLE = false).

This should be added as a method somewhere, clearly, as we use
essentially the same code in a few places.

>> Hrm. Why wouldn't the iFrame just wait for the response from its
>> submit to Bricolage?
>
> The iframe does wait for the response, but the iframe is now on the
> bricolage app domain and it now can't communicate with the preview
> frame directly. After the iframe loads (now on the app domain), it
> can grab the redirected preview uri and update the preview frame's
> url to change the fragment identifier. That's what the preview
> frame is polling itself for.

Okay.

>> Can the preview page poll the iFrame?
>
> Nope. After the iframe posts to the bricolage domain, the preview
> page loses its permission to access the iframe.

What a PITA.

>> Be sure to add comments about all this to the code, as no one
>> editing this stuff in the future will look for explanations on the
>> mail list! :-)
>
> Yup, I'll try get that done when I get a chance. Ideally, I'd like
> to have a central location for this overview. Not sure which file
> would be the best place.

Bric::Util::Burner, I should think.

Best,

David

On Feb 26, 2009, at 12:35 PM, David E. Wheeler wrote:

>> Even then, this doesn't quite work on our proxied modperl
>> installation, since Bricolage runs internally http, but is running
>> https on the outside (SSL_ENABLE = false).
>
> This should be added as a method somewhere, clearly, as we use
> essentially the same code in a few places.

Okay, I've refactored the code into a new method,
Bric::Util::ApacheReq and checked it in. With that, your code now
becomes:

if (not PREVIEW_LOCAL and $ar) {
$url .= (index($url, '?') == -1 ? '?' : '&') . 'bric_app_uri=';
$url = Bric::Util::ApacheReq->( uri => $url, ssl => 1 );
}

Best,

David

On Feb 25, 2009, at 1:34 PM, Adrian Yee wrote:

> Hi,

Hi, finally reviewing this. :-)

> Here's the latest patch. Some technical notes about implementation.
> There are some, imho, gross hacks to get around cross-domain issues
> when you're using remote preview:
>
> - Modified Bric::App::Callback::Publish::preview() to pass the
> Bricolage server's uri to the redirected preview url. If you look
> at the patch, this bit of code isn't pretty. Is there a nicer way
> to get Bricolage's uri?

Already covered. :-)

> At least on our proxied modperl installation, VHOST_SERVER_NAME is
> set to *, so using that doesn't work. I can't use javascript to get
> it, since I can't get around the cross domain issues of going the
> other direction of passing bric app domain => preview domain (we
> would need to put a static file on the preview server to be used as
> a proxy for the data).

Okay.

> - When the preview loads, it creates an iframe that loads
> preview_uri.html along with it's own redirect uri (more on why
> later). This page loads a bit of javascript to set the
> redir_preview_uri variable in the frameset/top page.

Fine.

> That handles the initial set up. Then there are more when we save:
>
> - First, create an empty iframe on the preview page. Change the
> contents of that empty iframe to be a form that submits all the data
> needed to modify the field. This is where we use the Bricolage
> server's uri that was passed along with the redirected preview page.

Sounds right.

> - After submitting the form, the preview frame beings polling its
> own url for changes (more on this in a bit). It times out after
> around 30 seconds.

Great.

> - The previous form was posted to the inline_edit.html page. It
> does its thing, and returns the status. However, because this page
> is loaded inside an iframe on the preview page, it loses the ability
> to communicate directly with the preview page (cross-domain). We
> can do stuff with the frameset/top window (and any other page on the
> same domain).
>
> We do two things:
> 1) Tell the preview frame the status of the operation. We use the
> fragment identifier hack to do this. However, IE7 patched this up
> so you can't directly modify window.location.hash anymore. This is
> why we need the preview frame's redirected uri.
> 2) If the frameset's opener is still around (probably the story edit
> page), then we use it to update the value of the field.
>
> I hope this all explains the weirdness in case someone else needs to
> do something with this code in the future.

I think I follow it. Now, the code.

> Index: lib/Bric/Biz/Element/Field.pm
> ===================================================================
> --- lib/Bric/Biz/Element/Field.pm (revision 8416)
> +++ lib/Bric/Biz/Element/Field.pm (working copy)
> @@ -731,12 +731,6 @@
>
> =cut
>
> -sub get_max_length {
> - my $self = shift;
> - return $self if $self->_get('max_length');
> - return;
> -}
> -
> ##############################################################################
>
> =item my $sql_type = $data->get_sql_type

I don't think you meant to do that, did you?

> Index: lib/Bric/Util/Burner.pm
> ===================================================================
> --- lib/Bric/Util/Burner.pm (revision 8416)
> +++ lib/Bric/Util/Burner.pm (working copy)
> @@ -2142,6 +2142,37 @@
>
> ##############################################################################
>
> +=item my $classes = $burner->preview_class($story, $element,
> $just_classes);
> +
> + <p<% $burner->preview_class($story, $element) %>><% $element-
> >get_value %></p>
> + <p class="paragraph<% $burner->preview_class($story, $element, 1)
> %>"></p>
> +
> +Returns the classes used by the in-place editor. To only return
> the class
> +names rather than the whole class="..." attribute, pass in a true
> value for the
> +third argument ($just_classes).
> +
> +=cut

I think we can do away with the story argument here, and just use
$burner->get_story.

> +
> +sub preview_class {
> + my ($self, $story, $element, $just_classes) = @_;
> +
> + return '' unless $self->get_mode == PREVIEW_MODE && $story-
> >get_checked_out;

I'm so sick of seeing this kind of code over and over again. I just
checked in new methods, so you can now do this:

+ return '' unless $self->previewing && $story->get_checked_out;

Also, you need to make sure that the previewing user is the same as
the user who has the story checked-out. So it should probably be:


return '' unless $self->previewing && $story->get_checked_out
&& $story->get_user_id == get_user_id;

> +
> + my $ret = 'bricInlineEdit bricInlineEditId' . $element->get_id .
> + ' bricInlineEditWidgetType' . $element->get_widget_type .
> + ' bricInlineEditMaxLength' . ($element->get_max_length || 0);
> +
> + if ($just_classes) {
> + $ret = " $ret ";
> + }
> + else {
> + $ret = qq| class="$ret" |;
> + }
> + return $ret;

Trinary logic!

return $just_classes ? " $ret " : qq{ class="$ret" };

> --- /dev/null 1969-12-31 16:00:00.000000000 -0800
> +++ comp/media/css/inline_edit.css 2009-02-21 22:50:43.000000000 -0800
> @@ -0,0 +1,45 @@
> +/**
> + * Styles for inline editing
> + */
> +
> +/* An editable element */
> +.bricInlineEdit {
> + cursor: pointer;
> +}
> +
> +/* Hovering over an editable element */
> +.bricInlineEditHover {
> + background-color: #fffed2 !important;
> +}
> +
> +/* In edit mode */
> +.bricInlineEditEditor {
> + padding: 5px;
> + background-color: #fffed2;
> + font-family: Verdana, Helvetica, Arial, sans-serif;
> + font-size: 10px;
> + font-weight: normal;
> +}
> +
> +.bricInlineEditEditor
> textarea, .bricInlineEditEditor .bricInlineEditInputText {
> + width: 99.5% !important;
> +}
> +.bricInlineEditEditor textarea {
> + height: 100px !important;
> +}
> +
> +/* The submit and cancel buttons */
> +.bricInlineEditButtonSubmit, .bricInlineEditButtonCancel {
> + font-size: 11px !important;
> + margin-right: 3px;
> +}
> +
> +.bricInlineEditSaving {
> +/* /comp/media/images/busy_indicator.gif */
> + background-image: url(data:image/
> gif;base64,R0lGODlhEAAQAKIHAMnBsdSTJMS0k7+mdb+PNrqXVsPBvf///yH/
> C05FVFNDQVBFMi4wAwEAAAAh
> +QQFAAAHACwAAAAAEAAQAAADQ3i6R8SQORdXCG2eUuC92cZ11seMZBlxjGFUyzAcrgvLcv1W+GzDB1lrxxAIILqi8bhIAgCHJbP5ej6j04gVClxcIwkAIfkEBQAABwAsAgAAAA4AEAAAAz54qhH7rzWojDnSEbKsxdrGHd61iCNpPhvlHkXxLnE8w/WNKwBADYNFr/cAAnnDg0BwMAaRyiXzSJEyb0tXAgAh+QQFAAAHACwAAAAAEAAQAAADPni6Z8aQORfjfCHURXPejKeBy7cAAEkQB4pu6+qmVcy+4MoIQlQUEB6P8fvthIfB4FAEHpPKpXETXZIUykoCACH5BAUAAAcALAAAAgAQAA4AAAM+eAqgfsa8w9iL8dXmsNTc9SmCME1BcJTlqaQpa7qw2rqv6gzDSRATHu/x+z2EvULhUAQek8qlERdd4h5KVwIAIfkEBQAABwAsAAAAABAAEAAAAz54ugfAkDkXlxBt1nNv3lwHMt54DMNoGCeaVuvqvlHMoiq7FEUUBBAej/H7MYQ9AuFQBB6TyqVxE12aDspKAgAh+QQFAAAHACwAAAAADgAQAAADPHi6J8JQORfPGG0eANa92cZ11seMZBlxSlFUjOvCrUzXN0MQkWEsux3D51MEeYHAgfgzIpPKYgSqvCUhCQAh+QQFAAAHACwAAAAAEAAQAAADPni6N8OQORdXKW0eIeC92cZ11seMZBlxDEFUCwAcrgvLcv1W+GzDBxkjEAAOiUXjAVk0GIzEg9OplE6r1koCACH5BAUAAAcALAAAAAAQAA4AAAM7eLpXxZA5FxchbZ4x4L3ZxnXWx4xkGXFMEFSLIByuC8ty/Vb4bMMHGcNgiAAAECKRcTwOlcxmsghxKhIAOw==);
> + background-position: top left;
> + background-repeat: no-repeat;
> + height: 16px;
> + padding-left: 20px;
> + *padding-left: 0; /* embedding image data isn't supported in IE */
> +}

So is this styling just used for the modal dialog that pops up when a
user edits a paragraph on the preview server? Could the image not be a
regular image? Why embed it in the CSS?

This reminds me: How do you handle the editing of fields that display
as radio buttons or a dropdown list or a code select? Is the full
field displayed just like in the Bricolage UI?

> --- /dev/null 1969-12-31 16:00:00.000000000 -0800
> +++ comp/media/js/inline_edit.js 2009-02-22 16:30:17.000000000 -0800

Eyes glazing over here…it's new code, more important to actually try
it out.

I assume that this all works when Bricolage is doing previews, yes? If
so, we might want to update the default templates to take advantage of
the feature.

> @@ -0,0 +1,321 @@
> +// What happens when they completely erase everything?

Check for an empty submit?

> +my $find_field;
> +$find_field = sub {

You can do that on one line:

my $find_field = sub {

>
> + my ($element, $id) = @_;
> + return unless $id;
> +
> + if (!$element->is_container) {
> + return $element->get_id == $id ? $element : undef;
> + }
> +
> + for ($element->get_elements) {
> + if (!$element->is_container) {
> + return $_ if $_->get_id == $id;
> + next;
> + }

Probably better to use a named iterator variable here instead of $_:

for my $elem ($element->get_elements { ... }

> + my $locate_element = $find_field->($_, $id);
> + return $locate_element if $locate_element;
> + }
> +};
> +
> +my $js_escape = sub {
> + my $text = shift;
> + $text =~ s/'/\\'/g;
> + $text =~ s/\r?\n/\\n/g;
> + return $text;
> +};
> +</%once>
> +<%init>
> +pop_page();
> +
> +my $error = '';
> +my $field;
> +my $story = get_state_data('story_prof', 'story');
> +
> +if (!$story) {
> +# The story state data hasn't already been loaded, look it up.
> +
> + ERROR: {
> + $field = Bric::Biz::Element::Field->lookup({ id =>
> $field_id, object_type => 'story' });
> + unless ($field) {
> + $error = 'Invalid field ID.';
> + last;
> + }
> +
> + $story = (Bric::Biz::Asset::Business::Story-
> >list({ version_id => $field->get_object_instance_id }))[0];
> + unless ($story) {
> + $error = "Can't find the story the field belongs to.";
> + last;
> + }
> +
> + if (!$story->get_checked_out or $story->get_user__id !=
> get_user_id) {
> + $error = "Can't edit this field unless you have checked
> out the story.";
> + $story = undef;
> + last;
> + }
> + }
> +
> + set_state_data('story_prof', 'story', $story) if $story and not
> $error;
> +}
> +
> +if ($story) {
> + $field ||= $find_field->($story->get_element, $field_id);
> + if ($field) {
> + $field->set_value($value);
> + }
> + else {
> + $error = qq|Could not find the field you attempted to
> edit. Did you start editing another story?|;
> + }
> +}
> +</%init>

Good error checking. Maybe I missed it, but where do you actually show
the user the errors?

Also, most, or all, of this code should go into a callback method,
probably in Bric::App::Callback::ContainerProf. We've been trying to
get away from doing this kind of processing in Mason code, and do it
instead in modules, and just use Mason for display. I know that it
doesn't really look that way, but only for hysterical reasons.

Thanks, this is looking promising!

Best,

David

> Hi, finally reviewing this. :-)

Thanks for the feedback! I'm currently in the process of implementing
some of the suggestions you made in previous e-mails, and I'll add these
ones to the list.

>> Index: lib/Bric/Biz/Element/Field.pm
>> ===================================================================
>> --- lib/Bric/Biz/Element/Field.pm (revision 8416)
>> +++ lib/Bric/Biz/Element/Field.pm (working copy)
>> @@ -731,12 +731,6 @@
>>
>> =cut
>>
>> -sub get_max_length {
>> - my $self = shift;
>> - return $self if $self->_get('max_length');
>> - return;
>> -}
>> -
>> ##############################################################################
>>
>>
>> =item my $sql_type = $data->get_sql_type
>
> I don't think you meant to do that, did you?

Actually I did. That's required to properly get the field length in
Bric::Util::Burner::preview_class().

>> Index: lib/Bric/Util/Burner.pm
>> ===================================================================
>> --- lib/Bric/Util/Burner.pm (revision 8416)
>> +++ lib/Bric/Util/Burner.pm (working copy)
>> @@ -2142,6 +2142,37 @@
>>
>> ##############################################################################
>>
>>
>> +=item my $classes = $burner->preview_class($story, $element,
>> $just_classes);
>> +
>> + <p<% $burner->preview_class($story, $element) %>><%
>> $element->get_value %></p>
>> + <p class="paragraph<% $burner->preview_class($story, $element, 1)
>> %>"></p>
>> +
>> +Returns the classes used by the in-place editor. To only return the
>> class
>> +names rather than the whole class="..." attribute, pass in a true
>> value for the
>> +third argument ($just_classes).
>> +
>> +=cut
>
> I think we can do away with the story argument here, and just use
> $burner->get_story.

Yup, got rid of it.

>> +sub preview_class {
>> + my ($self, $story, $element, $just_classes) = @_;
>> +
>> + return '' unless $self->get_mode == PREVIEW_MODE &&
>> $story->get_checked_out;
>
> I'm so sick of seeing this kind of code over and over again. I just
> checked in new methods, so you can now do this:

Yup, saw that commit and changed to use that :)

> + return '' unless $self->previewing && $story->get_checked_out;
>
> Also, you need to make sure that the previewing user is the same as the
> user who has the story checked-out. So it should probably be:

I didn't feel this was a big deal, since the javascript/modifying code
checks, but I'll add in the check.

> Trinary logic!
>
> return $just_classes ? " $ret " : qq{ class="$ret" };

Duh. Changed.

>> --- /dev/null 1969-12-31 16:00:00.000000000 -0800
>> +++ comp/media/css/inline_edit.css 2009-02-21 22:50:43.000000000 -0800
>> @@ -0,0 +1,45 @@
>> +/**
>> + * Styles for inline editing
>> + */
...
>> +.bricInlineEditSaving {
>> +/* /comp/media/images/busy_indicator.gif */
>> + background-image:
>> url(data:image/gif;base64,R0lGODlhEAAQAKIHAMnBsdSTJMS0k7+mdb+PNrqXVsPBvf///yH/C05FVFNDQVBFMi4wAwEAAAAh+QQFAAAHACwAAAAAEAAQAAADQ3i6R8SQORdXCG2eUuC92cZ11seMZBlxjGFUyzAcrgvLcv1W+GzDB1lrxxAIILqi8bhIAgCHJbP5ej6j04gVClxcIwkAIfkEBQAABwAsAgAAAA4AEAAAAz54qhH7rzWojDnSEbKsxdrGHd61iCNpPhvlHkXxLnE8w/WNKwBADYNFr/cAAnnDg0BwMAaRyiXzSJEyb0tXAgAh+QQFAAAHACwAAAAAEAAQAAADPni6Z8aQORfjfCHURXPejKeBy7cAAEkQB4pu6+qmVcy+4MoIQlQUEB6P8fvthIfB4FAEHpPKpXETXZIUykoCACH5BAUAAAcALAAAAgAQAA4AAAM+eAqgfsa8w9iL8dXmsNTc9SmCME1BcJTlqaQpa7qw2rqv6gzDSRATHu/x+z2EvULhUAQek8qlERdd4h5KVwIAIfkEBQAABwAsAAAAABAAEAAAAz54ugfAkDkXlxBt1nNv3lwHMt54DMNoGCeaVuvqvlHMoiq7FEUUBBAej/H7MYQ9AuFQBB6TyqVxE12aDspKAgAh+QQFAAAHACwAAAAADgAQAAADPHi6J8JQORfPGG0eANa92cZ11seMZBlxSlFUjOvCrUzXN0MQkWEsux3D51MEeYHAgfgzIpPKYgSqvCUhCQAh+QQFAAAHACwAAAAAEAAQAAADPni6N8OQORdXKW0eIeC92cZ11seMZBlxDEFUCwAcrgvLcv1W+GzDBxkjEAAOiUXjAVk0GIzEg9OplE6r1koCACH5BAUAAAcALAAAAAAQAA4AAAM7eLpXxZA5FxchbZ4x4L3ZxnXWx4xkGXFMEFSLIByuC8ty/Vb4bMMHGcNgiAAAECKRcTwOlcxmsghxKhIAO
w==);
>>
>> + background-position: top left;
>> + background-repeat: no-repeat;
>> + height: 16px;
>> + padding-left: 20px;
>> + *padding-left: 0; /* embedding image data isn't supported in IE */
>> +}
>
> So is this styling just used for the modal dialog that pops up when a
> user edits a paragraph on the preview server? Could the image not be a
> regular image? Why embed it in the CSS?

That's right. It's an embedded image because I didn't want to require
files to be put on the preview server and in the CSS, we don't know the
url to the application server.

IE doesn't support embedding images like this, but imho, it's not a big
deal, since it's just a indicator that it's doing something. In most
cases, it doesn't show up, since the request completes first. When it
does, IE just gets the 'Saving...' text.

> This reminds me: How do you handle the editing of fields that display as
> radio buttons or a dropdown list or a code select? Is the full field
> displayed just like in the Bricolage UI?

Currently, there's no support for these fields, just text and textarea
widget types. I'm not sure how we'll handle those cases, since we would
need much more info (more than I'd want to pass using the CSS class hack).

>> --- /dev/null 1969-12-31 16:00:00.000000000 -0800
>> +++ comp/media/js/inline_edit.js 2009-02-22 16:30:17.000000000 -0800
>
> Eyes glazing over here…it's new code, more important to actually try it
> out.
>
> I assume that this all works when Bricolage is doing previews, yes? If
> so, we might want to update the default templates to take advantage of
> the feature.

Yup, probably a good idea.

>> @@ -0,0 +1,321 @@
>> +// What happens when they completely erase everything?
>
> Check for an empty submit?

What I meant is should we allow the user to erase everything in a field?
It's just that if they set the field value to an empty string, they
probably won't be able to edit it in the inline editor since there won't
be anything to click on.

>> +my $find_field;
>> +$find_field = sub {
>
> You can do that on one line:
>
> my $find_field = sub {

It's a recursive code ref, so it's needed.

>> + for ($element->get_elements) {
>> + if (!$element->is_container) {
>> + return $_ if $_->get_id == $id;
>> + next;
>> + }
>
> Probably better to use a named iterator variable here instead of $_:

Done.

> Good error checking. Maybe I missed it, but where do you actually show
> the user the errors?

We return it as a javascript string, which gets passed back to the
preview window, and it alert()'s it.

> Also, most, or all, of this code should go into a callback method,
> probably in Bric::App::Callback::ContainerProf. We've been trying to get
> away from doing this kind of processing in Mason code, and do it instead
> in modules, and just use Mason for display. I know that it doesn't
> really look that way, but only for hysterical reasons.

Okay, I'll work on moving that code out.

Another thing I've been working on is getting your suggestion of adding
shorthand of <%pc $e> working. However, the argument to it means I have
to do some parsing of Perl code (ughh). I can keep it simple and only
allow a variable as the single argument, but then it sucks when you have
something a little more complex like wanting to do:

<%pc $element->get_field('my_field')>

Suggestions?

Adrian

> Also, most, or all, of this code should go into a callback method,
> probably in Bric::App::Callback::ContainerProf. We've been trying to get
> away from doing this kind of processing in Mason code, and do it instead
> in modules, and just use Mason for display. I know that it doesn't
> really look that way, but only for hysterical reasons.

What's the typical method of passing data back to mason? Just do a
save_state_data() in the callback and then get_state_data() in the mason
template (passing the error back)?

Adrian

On Mar 6, 2009, at 4:58 PM, Adrian Yee wrote:

>>>
>>> -sub get_max_length {
>>> - my $self = shift;
>>> - return $self if $self->_get('max_length');
>>> - return;
>>> -}
>>> -
>>> ##############################################################################
>>>
>>> =item my $sql_type = $data->get_sql_type
>> I don't think you meant to do that, did you?
>
> Actually I did. That's required to properly get the field length in
> Bric::Util::Burner::preview_class().

Hrm. I wonder why it was there, then, returning a boolean instead of a
value. Looks like I did it, here:

http://viewsvn.bricolage.cc/?view=revision&revision=6699

Given that it looks like it should return a value, not a boolean,
you're probably correct to change it. But I wonder why no one ever
noticed that before? Do all tests pass, BTW?

> I didn't feel this was a big deal, since the javascript/modifying
> code checks, but I'll add in the check.

Good, thanks. I think it's best to be thorough.

> That's right. It's an embedded image because I didn't want to
> require files to be put on the preview server and in the CSS, we
> don't know the url to the application server.

Why can't it point to the URL for Bricolage? Hrm…I guess because the
CSS is static file, and so doesn't know what server it's served from.
But, honestly, I think that relative URLs in CSS are relative to the
server they're served from, not from the page they're displaying.

> IE doesn't support embedding images like this, but imho, it's not a
> big deal, since it's just a indicator that it's doing something. In
> most cases, it doesn't show up, since the request completes first.
> When it does, IE just gets the 'Saving...' text.

Sounds like a decent compromise.

>> This reminds me: How do you handle the editing of fields that
>> display as radio buttons or a dropdown list or a code select? Is
>> the full field displayed just like in the Bricolage UI?
>
> Currently, there's no support for these fields, just text and
> textarea widget types. I'm not sure how we'll handle those cases,
> since we would need much more info (more than I'd want to pass using
> the CSS class hack).

Yeah, starting with the text and textarea fields is a good idea. Just
document it in the class method in Bric::Util::Burner, would you?

>> Eyes glazing over here…it's new code, more important to actually
>> try it out.
>> I assume that this all works when Bricolage is doing previews, yes?
>> If so, we might want to update the default templates to take
>> advantage of the feature.
>
> Yup, probably a good idea.

Thank you. :-_

>> What I meant is should we allow the user to erase everything in a
>> field? It's just that if they set the field value to an empty
>> string, they probably won't be able to edit it in the inline editor
>> since there won't be anything to click on.

If we allow them to delete fields this way, it would be the same
thing. Best to leave it, then, and see what cow paths we get.

>> You can do that on one line:
>> my $find_field = sub {
>
> It's a recursive code ref, so it's needed.

Ah! /me likes those kinds of hacks.

>> Good error checking. Maybe I missed it, but where do you actually
>> show the user the errors?
>
> We return it as a javascript string, which gets passed back to the
> preview window, and it alert()'s it.

Good enough.

>> Also, most, or all, of this code should go into a callback method,
>> probably in Bric::App::Callback::ContainerProf. We've been trying
>> to get away from doing this kind of processing in Mason code, and
>> do it instead in modules, and just use Mason for display. I know
>> that it doesn't really look that way, but only for hysterical
>> reasons.
>
> Okay, I'll work on moving that code out.

Great, thanks. Let me know if you need an overview of how the
callbacks work.

> Another thing I've been working on is getting your suggestion of
> adding shorthand of <%pc $e> working. However, the argument to it
> means I have to do some parsing of Perl code (ughh). I can keep it
> simple and only allow a variable as the single argument, but then it
> sucks when you have something a little more complex like wanting to
> do:
>
> <%pc $element->get_field('my_field')>
>
> Suggestions?

Would you just transparently change whatever is after the `<%pc` bit
and before the closing `%>` bit to the full method call? That's what
the filters for the <%preview> and <%publish%> tags do.

Best,

David

On Mar 6, 2009, at 6:18 PM, Adrian Yee wrote:

> What's the typical method of passing data back to mason? Just do a
> save_state_data() in the callback and then get_state_data() in the
> mason template (passing the error back)?

Yeah, that's pretty standard, although I hate it. I also stick stuff
in $r->pnotes(), which makes me feel slightly less dirty.

Best,

David

> Hrm. I wonder why it was there, then, returning a boolean instead of a
> value. Looks like I did it, here:
>
> http://viewsvn.bricolage.cc/?view=revision&revision=6699
>
> Given that it looks like it should return a value, not a boolean,
> you're probably correct to change it. But I wonder why no one ever
> noticed that before? Do all tests pass, BTW?

Ahh, the issue of tests. I inherited a pre-installed copy of Bricolage on
our servers that apparently required a bit of work to get working on our
servers. I've tried doing a make test on here and it fails miserably (a lot
of compilation fail errors), so I haven't been able to properly test things.

t/Bric/Test/Runner....Error loading Bric::Biz::OutputChannel::Test: Unable
to register field names: Can't locate class method
'Bric::Biz::Asset::Business::ACCESS' via package
'Bric::Biz::Asset::Business' at lib/Bric.pm line 752

(and more).

> Why can't it point to the URL for Bricolage? Hrm...I guess because the
> CSS is static file, and so doesn't know what server it's served from.
> But, honestly, I think that relative URLs in CSS are relative to the
> server they're served from, not from the page they're displaying.

Ah, yes, you're right. Changed.

> > Currently, there's no support for these fields, just text and
> > textarea widget types. I'm not sure how we'll handle those cases,
> > since we would need much more info (more than I'd want to pass using
> > the CSS class hack).
>
> Yeah, starting with the text and textarea fields is a good idea. Just
> document it in the class method in Bric::Util::Burner, would you?

Will do.

> > Another thing I've been working on is getting your suggestion of
> > adding shorthand of <%pc $e> working. However, the argument to it
> > means I have to do some parsing of Perl code (ughh). I can keep it
> > simple and only allow a variable as the single argument, but then it
> > sucks when you have something a little more complex like wanting to
> > do:
> >
> > <%pc $element->get_field('my_field')>
> >
> > Suggestions?
>
> Would you just transparently change whatever is after the `<%pc` bit
> and before the closing `%>` bit to the full method call? That's what
> the filters for the <%preview> and <%publish%> tags do.

You suggested using the syntax <%pc $e> in a previous e-mail. The problem
is that a regex like:

s/<%pc\s+([^>]+)>/<% \$burner->preview_class($1) %>/gi

Fails on more complex code (eg. example above). I guess if I use the syntax
you just suggested of <%pc $e%>, it's less likely to break, but it's still a
possibility.

Adrian

On Mar 6, 2009, at 9:09 PM, Adrian Yee wrote:

> You suggested using the syntax <%pc $e> in a previous e-mail. The
> problem
> is that a regex like:
>
> s/<%pc\s+([^>]+)>/<% \$burner->preview_class($1) %>/gi

It occurs to me that we could make it much simpler by just defining a
`pc` function and exporting it to the template package. Then it should
just work.

Best,

David

On Mar 6, 2009, at 9:09 PM, Adrian Yee wrote:

>> æAhh, the issue of tests. I inherited a pre-installed copy of
>> Bricolage on
> our servers that apparently required a bit of work to get working on
> our
> servers. I've tried doing a make test on here and it fails
> miserably (a lot
> of compilation fail errors), so I haven't been able to properly test
> things.
>
> t/Bric/Test/Runner....Error loading Bric::Biz::OutputChannel::Test:
> Unable
> to register field names: Can't locate class method
> 'Bric::Biz::Asset::Business::ACCESS' via package
> 'Bric::Biz::Asset::Business' at lib/Bric.pm line 752

I suggest you run it on your local box, using `make dev` to build it
as needed.

Best,

David

Working on the inline editor stuff again, and have some questions.

David E. Wheeler wrote:
> On Mar 6, 2009, at 9:09 PM, Adrian Yee wrote:
>
>> You suggested using the syntax <%pc $e> in a previous e-mail. The
>> problem
>> is that a regex like:
>>
>> s/<%pc\s+([^>]+)>/<% \$burner->preview_class($1) %>/gi
>
> It occurs to me that we could make it much simpler by just defining a
> `pc` function and exporting it to the template package. Then it should
> just work.

Where would I do this?

Another thing you mentioned previously was to add inline editing support
to the default templates. Where are they stored?

Adrian

On Tue, 14 Apr 2009, Adrian Yee wrote:
> Working on the inline editor stuff again, and have some questions.
>
> David E. Wheeler wrote:
>> On Mar 6, 2009, at 9:09 PM, Adrian Yee wrote:
>>
>>> You suggested using the syntax <%pc $e> in a previous e-mail. The problem
>>> is that a regex like:
>>>
>>> s/<%pc\s+([^>]+)>/<% \$burner->preview_class($1) %>/gi
>>
>> It occurs to me that we could make it much simpler by just defining a `pc`
>> function and exporting it to the template package. Then it should just
>> work.
>
> Where would I do this?

In lib/Bric/App/Handler.pm there's a block for
package HTML::Mason::Commands , which is the namespace
of the templates. You can see a bunch of exporting there, like

use Bric::Util::ApacheUtil qw(escape_uri);

Look in lib/Bric/Util/ApacheUtil.pm for a simple example (escape_uri).
Presumably you'd want `pc` to be in lib/Bric/Util/Burner.pm .
Then with it exported to HTML::Mason::Commands, the template developer
can just call it wherever they want in the templates.


> Another thing you mentioned previously was to add inline editing support to
> the default templates. Where are they stored?

In data/burn/comp/oc_1/
and sql/Pg/Bric/Biz/Asset/Template.val
and sql/mysql/Bric/Biz/Asset/Template.val

:/

On Apr 15, 2009, at 1:34 AM, Scott Lanning wrote:

> In lib/Bric/App/Handler.pm there's a block for
> package HTML::Mason::Commands , which is the namespace
> of the templates. You can see a bunch of exporting there, like
>
> use Bric::Util::ApacheUtil qw(escape_uri);

I don't think that's used under bric_queued, is it? That's just for
stuff in the Mason name space for the UI.

> Look in lib/Bric/Util/ApacheUtil.pm for a simple example (escape_uri).
> Presumably you'd want `pc` to be in lib/Bric/Util/Burner.pm .
> Then with it exported to HTML::Mason::Commands, the template developer
> can just call it wherever they want in the templates.

ApacheUtil isn't under bric_queued, either. Where you want to put it
is in this code in Bric::Util::Burner:

if (my $code = delete $Bric::Config::INC{PERL_LOADER}) {
my $pkg = TEMPLATE_BURN_PKG;
eval qq{
package $pkg;
use Bric::Util::DBI qw(:junction);
$code;
};
# Just die if there was an error.
die $@ if $@;
}

>> Another thing you mentioned previously was to add inline editing
>> support to the default templates. Where are they stored?
>
> In data/burn/comp/oc_1/
> and sql/Pg/Bric/Biz/Asset/Template.val
> and sql/mysql/Bric/Biz/Asset/Template.val

Yeah, too much duplication…

Best,

David

> ApacheUtil isn't under bric_queued, either. Where you want to put it is
> in this code in Bric::Util::Burner:
>
> if (my $code = delete $Bric::Config::INC{PERL_LOADER}) {
> my $pkg = TEMPLATE_BURN_PKG;
> eval qq{
> package $pkg;
> use Bric::Util::DBI qw(:junction);
> $code;
> };
> # Just die if there was an error.
> die $@ if $@;
> }

Is it possible to get the burner object here?

Adrian

On Apr 15, 2009, at 4:27 PM, Adrian Yee wrote:

>> if (my $code = delete $Bric::Config::INC{PERL_LOADER}) {
>> my $pkg = TEMPLATE_BURN_PKG;
>> eval qq{
>> package $pkg;
>> use Bric::Util::DBI qw(:junction);
>> $code;
>> };
>> # Just die if there was an error.
>> die $@ if $@;
>> }
>
> Is it possible to get the burner object here?

Yeah, it's just $burner.

Best,

David

David E. Wheeler wrote:
> On Apr 15, 2009, at 4:27 PM, Adrian Yee wrote:
>
>>> if (my $code = delete $Bric::Config::INC{PERL_LOADER}) {
>>> my $pkg = TEMPLATE_BURN_PKG;
>>> eval qq{
>>> package $pkg;
>>> use Bric::Util::DBI qw(:junction);
>>> $code;
>>> };
>>> # Just die if there was an error.
>>> die $@ if $@;
>>> }
>>
>> Is it possible to get the burner object here?
>
> Yeah, it's just $burner.

I tried doing:

eval qq{
package $pkg;
use Bric::Util::DBI qw(:junction);
$code;

sub pc {
\$burner->preview_class(\@_);
}
};

and that doesn't work because we're in a new package. Not quite sure
how all the Mason stuff is set up, so I've got no clue what I'm doing
here :(

Adrian

On Apr 16, 2009, at 3:06 PM, Adrian Yee wrote:

> David E. Wheeler wrote:
>> On Apr 15, 2009, at 4:27 PM, Adrian Yee wrote:
>>>> if (my $code = delete $Bric::Config::INC{PERL_LOADER}) {
>>>> my $pkg = TEMPLATE_BURN_PKG;
>>>> eval qq{
>>>> package $pkg;
>>>> use Bric::Util::DBI qw(:junction);
>>>> $code;
>>>> };
>>>> # Just die if there was an error.
>>>> die $@ if $@;
>>>> }
>>>
>>> Is it possible to get the burner object here?
>> Yeah, it's just $burner.
>
> I tried doing:
>
> eval qq{
> package $pkg;
> use Bric::Util::DBI qw(:junction);
> $code;
>
> sub pc {
> \$burner->preview_class(\@_);
> }
> };
>
> and that doesn't work because we're in a new package. Not quite
> sure how all the Mason stuff is set up, so I've got no clue what I'm
> doing here :(

Try this:

if (my $code = delete $Bric::Config::INC{PERL_LOADER}) {
my $pkg = TEMPLATE_BURN_PKG;
eval qq{
package $pkg;
use Bric::Util::DBI qw(:junction);
sub pc { \$burner->preview_class(\@_); }
$code;
};
# Just die if there was an error.
die $@ if $@;
}

Best,

David

> Try this:
>
> if (my $code = delete $Bric::Config::INC{PERL_LOADER}) {
> my $pkg = TEMPLATE_BURN_PKG;
> eval qq{
> package $pkg;
> use Bric::Util::DBI qw(:junction);
> sub pc { \$burner->preview_class(\@_); }
> $code;
> };
> # Just die if there was an error.
> die $@ if $@;
> }

Same thing:

Global symbol "$burner" requires explicit package name at (eval 1033)
line 4.

Adrian

On Apr 16, 2009, at 5:02 PM, Adrian Yee wrote:

>> Same thing:
>
> Global symbol "$burner" requires explicit package name at (eval
> 1033) line 4.

Bah! You'll have to do it for every call to burn_one(), then.
Something like:

Index: Burner.pm
===================================================================
--- Burner.pm (revision 8500)
+++ Burner.pm (working copy)
@@ -1512,6 +1512,9 @@
# Construct the burner and do it!
my $burner = $self->_get_subclass($story, $oc);

+ # Make a pc() function available.
+ eval qq{${TEMPLATE_BURN_PKG}\::pc = sub { \$self-
>preview_class(\@_); }};
+
# Never use the local user's preferences during a burn.
my $use_user = Bric::Util::Pref->use_user_prefs;
Bric::Util::Pref->use_user_prefs(0) if $use_user;

Best,

David

> --- Burner.pm (revision 8500)
> +++ Burner.pm (working copy)
> @@ -1512,6 +1512,9 @@
> # Construct the burner and do it!
> my $burner = $self->_get_subclass($story, $oc);
>
> + # Make a pc() function available.
> + eval qq{${TEMPLATE_BURN_PKG}\::pc = sub { \$self-
> >preview_class(\@_); }};
> +
> # Never use the local user's preferences during a burn.
> my $use_user = Bric::Util::Pref->use_user_prefs;
> Bric::Util::Pref->use_user_prefs(0) if $use_user;

Ahh, poking around and checking out the module docs, it turns out I just need
to hack my way around it by adding a "use vars qw/$burner/;" in there, since
Mason does some magic to add it into that package:

--- Burner.pm (revision 8576)
+++ Burner.pm (working copy)
@@ -2290,6 +2321,10 @@
my $pkg = TEMPLATE_BURN_PKG;
eval qq{
package $pkg;
+
+ use vars qw/\$burner/;
+ sub pc { \$burner->preview_class(\@_); }
+
use Bric::Util::DBI qw(:junction);
$code;
};

Is this okay? Is this going to do the right thing for the other Burners?

Adrian

On Apr 17, 2009, at 12:46 AM, Adrian Yee wrote:

> --- Burner.pm (revision 8576)
> +++ Burner.pm (working copy)
> @@ -2290,6 +2321,10 @@
> my $pkg = TEMPLATE_BURN_PKG;
> eval qq{
> package $pkg;
> +
> + use vars qw/\$burner/;
> + sub pc { \$burner->preview_class(\@_); }
> +
> use Bric::Util::DBI qw(:junction);
> $code;
> };
>
> Is this okay? Is this going to do the right thing for the other
> Burners?

I think so; try it!

Best,

David

David E. Wheeler wrote:
> On Apr 17, 2009, at 12:46 AM, Adrian Yee wrote:
>
>> --- Burner.pm (revision 8576)
>> +++ Burner.pm (working copy)
>> @@ -2290,6 +2321,10 @@
>> my $pkg = TEMPLATE_BURN_PKG;
>> eval qq{
>> package $pkg;
>> +
>> + use vars qw/\$burner/;
>> + sub pc { \$burner->preview_class(\@_); }
>> +
>> use Bric::Util::DBI qw(:junction);
>> $code;
>> };
>>
>> Is this okay? Is this going to do the right thing for the other Burners?
>
> I think so; try it!

I've tested it and I know it works, just worried about if it's the
correct thing to do and whether it's going to break the other burners.

Adrian

On Apr 17, 2009, at 11:54 AM, Adrian Yee wrote:

>>> Is this okay? Is this going to do the right thing for the other
>>> Burners?
>> I think so; try it!
>
> I've tested it and I know it works, just worried about if it's the
> correct thing to do and whether it's going to break the other burners.

I'm pretty sure that it will be fine, as long as all tests pass. In
fact you should add a test for it by adding a call to pc to a test
template, such as t/Bric/Util/Burner/Mason/page.mc, and then check
that it outputs the correct info in the test in Burner::DevTest (if
it's Mason).

Best,

David