Comments

Conditional setter? Never seen that before. Should throw exception or create template if not already created.
I understand why it's here, but it's a just a very specific usecase.

Copy link

Author

@holantomas holantomas Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if you got it right. Whole idea of this PR is to be able setup file without creating template. If we throw exception or create template there is no PR needed any more, cause it will work just like shortcut for $control->getTemplate()->setFile().

But I thought about it if I should add this condition. Because it's just for specific usecase as you said when user try to set file after template was created to prevent "WTF?" situations and let setter work like user expect.

Copy link

Contributor

@mabar mabar Dec 10, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, I understand. But imagine you don't know that code, just api. It looks exactly like a shortcut, but acts absolutely different.

Copy link

Author

@holantomas holantomas Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, I'm not saying you're not right, but I think user don't care about it, he just want to set template file/view and this did that without unnecessary loads.

And I really hate setting template in $onAnchor event. Or do you know way how to do it without event in control factory or in createComponent<name>() method?

Copy link

Contributor

@mabar mabar Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could configure template file through it's factory definition

fooControlFactory:
 implement IFooControlFactory
 setup:
 - getTemplate()->setFile('/path/to/file')

But I never used that approach. I have ITemplate with setView method and to change path to these views I only need change pattern in configuration.

Copy link

Author

@holantomas holantomas Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will end with InvalidStateException or new template instance. Anyway I don't like to setup file template in config, there are situations where you want to setup view by condition, or you have one control with factory and want to setup many different view for actual page.

@holantomas

Copy link

Author

holantomas commented Dec 10, 2018 •

edited

Loading

@mabar I'm OK with that 👍 If more people will consider it as good way I can try to implement it.

@mabar

Copy link

Contributor

mabar commented Dec 10, 2018

I think you cannot as it requires more than ITemplate interface

@milo

Copy link

Member

milo commented Dec 11, 2018

IMHO, template file is internal thing of Control, thing of encapsulation. Control may use more templates too, that's why you usualy set template file in render() method.

Why do you need set template file outside of Control?

@holantomas

Copy link

Author

holantomas commented Dec 11, 2018 •

edited

Loading

IMHO, template file is internal thing of Control, thing of encapsulation. Control may use more templates too, that's why you usualy set template file in render() method.

Why do you need set template file outside of Control?

It's shown in forum topic. User usign 3rd party control like datagrid or paginator but HTML not equal to his UI for example.

encapsulation:
Control::getTemplate(): ITemplate is public
ITemplate::setFile() is public
This doesn't break anything.

And I'm not sure but settings template file in render() is to prevent unnecessary creating template or prevent creating template before Control is attached to presenter I think. Both doing my PR too.