Imho, we should put as much logic as possible into the actual plugin. So that the layout only contains layout things. Currently there is quite a big logic block in it.
Personally I would for example the preparing of the images in the plugin and just pass the images as a variable. Templates already can override those images if they want, and if the logic doesn't fit them, they can still do something different in the layout.

I think I'm in favour of having the images in the layout even if these can be overridden. However I do wonder whether the JUri logic should be kept in the plugin and passed into the layout

I think I'm in favour of having the images in the layout even if these can be overridden.

Imho, one could even move the whole image logic (the for stuff) into the plugin. Passing an array which can be used in a foreach. Like this:
Plugin

// Lookup images once in the plugin.
$starImageOn = JHtml::_('image', 'system/rating_star.png', JText::_('PLG_VOTE_STAR_ACTIVE'), null, true);
$starImageOff = JHtml::_('image', 'system/rating_star_blank.png', JText::_('PLG_VOTE_STAR_INACTIVE'), null, true);
$rating = (int) @$row->rating;
$stars = array();

for ($i = 0; $i < $rating; $i++)
{
    $stars[] = $starImageOn;
}

for ($i = $rating; $i < 5; $i++)
{
    $stars[] = $starImageOff;
}

Layout

foreach ($stars as $star) :
    echo $star;
endforeach;

This would also make it much simpler to wrap the images into other elements instead.

However I do wonder whether the JUri logic should be kept in the plugin and passed into the layout

I guess the uri stuff could be made much simpler (not tested):

$url = JUri::getInstance()->getQuery() . '&hitcount=0';

And then just use $url instead of $uri->toString().
I think such a one-liner would be fine in the layout. However moving it to the plugin wouldn't hurt either.

@Bakual that's the wrong concept of "pass all to layouts". It doesn't solve any issue with the layout. In fact is causing more data to be loaded and passed from here to there. it's not needed because if I want to use my custom images I don't need to receive an array with stuff I'm not going to use. Less data loaded = better.

@wilsonge the JUri is questionable but think in a component that wants to override the vote layout. For that is better to load the URL in the layout.

it's not needed because if I want to use my custom images I don't need to receive an array with stuff I'm not going to use. Less data loaded = better.

If you want to use custom images, you can override them without touching code at all because we use JHtml::_('image', ...).
And doing logic stuff in a layout is just wrong if it could be done elsewhere.

the JUri is questionable but think in a component that wants to override the vote layout.

The plugin itself can't be used at all in by another component anyway (due to it return early if not com_content). Thus one could only reuse the layout itself, but that needs to be overriden as well because of the task field which has a fixed task article.vote in it.
You are better of using an own layout in your extension, you don't get any benefit from this plugin layout
In this case here, we don't need to think about making the layout reusable, as it currently isn't reusable anyway.

OK we need to pass in the task into the layout

I think we should aim to make the layout reusable (I don't see why the plugin really needs to be on the other hand)

What benefit is there to making this a JLayout based layout then if it is
bound to a plugin with a specific implementation? This is another case
where I'd just use a template layout in the plugin.

On Friday, February 28, 2014, George Wilson notifications@github.com
wrote:

OK we need to pass in the task into the layout

I think we should aim to make the layout reusable (I don't see why the
plugin really needs to be on the other hand)

Reply to this email directly or view it on GitHub#3201 (comment)
.

@wilsonge @mbabker I had a discussion together with @phproberto where an idea would be to JLayouts eventually become the replacement for existing layouts.
The idea would then be that we have reusable layouts in the /layouts/joomla folder (for example) and extensions specific layouts in /layouts/plugins, /layouts/components, /layouts/libraries and /layouts/modules. This one would be an extension specific one in /layouts/plugins/content/vote.

It probably needs a discussion first within PLT what direction we will go, but it could probably solve the issues with overrides without maintaining two layout systems.

What about this approach?
Plugin

$rating = (int) @$row->rating;
$stars = array();

for ($i = 0; $i < $rating; $i++)
{
    $stars[] = 1;
}

for ($i = $rating; $i < 5; $i++)
{
    $stars[] = 0;
}

Layout

// Lookup images once in the plugin.
$images = array();
$images[0] = JHtml::_('image', 'system/rating_star_blank.png', JText::_('PLG_VOTE_STAR_INACTIVE'), null, true);
$images[1] = JHtml::_('image', 'system/rating_star.png', JText::_('PLG_VOTE_STAR_ACTIVE'), null, true);

foreach ($stars as $star) :
    echo $images[$star];
endforeach;

Logic in plugin - Images in layout.

@mbabker I understand your doubts but with the new PR you will understand better the benefits of using layouts. As an example think in using different layouts for 2.5 & 3.x. Different layout for different languages... or simply because now you will be able to override the getRenderer($layout, $options) to return your own layout class/rendering system.

@Bakual I think you have too much free time. Listen: a new plugin needs a new layout. It's a bad practice to call plugins.content.vote layout from another plugin. Would you do it if it was your commercial plugin? And remember: Frontenders prefer to do things in the layout.

About the logic on the layout who says it's wrong? Logic related to the display is not only good but also recommended to be inside the layout. What is wrong is to add logic related to getting the data.

@Bakual @wilsonge Pass the task? So the universal vote plugin of the world (that uses content vote layout) will be forced to current view? :D No thank you. An universal voting system will probably use a totally different URL. And no. I'm not going to add the URL to the plugin. This makes no sense at all. You can add 32455 useless things in a new PR when this is merged.

This is about making overridable a plugin.

I think you have too much free time. Listen: a new plugin needs a new layout. It's a bad practice to call plugins.content.vote layout from another plugin.

@phproberto I didn't suggest to reuse that layout. In contrary: I fully agree with you

I mainly care about using as less logic as possible in the layout. Imho there is no reason that the calculation for empty/full "stars" need to be in the layout. Each layout would have to do the same anyway, so it would make sense to move that into the plugin to keep the layout as simple as possible.

I may be a bit picky here, but I think because this will be the first plugin using JLayouts, we should make it a good example.