Page MenuHomePhabricator

efl: add a factory Eina_Error and rename the file to be more on point with its purpose.
ClosedPublic

Authored by cedric on Dec 7 2018, 2:48 PM.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cedric created this revision.Dec 7 2018, 2:48 PM
segfaultxavi requested changes to this revision.Dec 8 2018, 1:17 AM

This is public API. Where are the docs? What does this error mean? Who emits it? And the unit tests.
I see Model errors do not have all this, but we should start doing things correctly at some point... in this case it's not that much work :)

Also, does this patch really belong to this patchset? It makes reviewing difficult, specially if one of the patches fails to build, as in the previous patchset.

This revision now requires changes to proceed.Dec 8 2018, 1:17 AM

shouldn't it be
EFL_MODEL_ERROR_FACTORY_NOT_SUPPORTED_STR?

if this error message is only for factory, I'm not sure it is proper to put it here.

It is only emitted by the Factory, which use Model, but doesn't inherit from it. So I don't think it should be in that namespace.

Arguably, I believe we should have error defined in .eo and so this one would be defined in the efl_ui_factory.eo file. Maybe something to revive.

OK, I am not familiar with EFL factories, models and MVVM, but it hurts my eyes to see a symbol named EFL_FACTORY_* in a file called efl_model_common.c. Are we sure it does not belong somewhere else, or that the symbol shouldn't be named differently? :)

I think actually renaming the file make sense. We started with just model and have evolved with a pattern that use factory. Arguably all the content of this file is related to the MVVM common infrastructure and not just Model. Will correct that and push.

cedric updated this revision to Diff 18023.Dec 20 2018, 11:06 AM
cedric retitled this revision from efl: add a factory Eina_Error. to efl: add a factory Eina_Error and rename the file to be more on point with its purpose..

Rename file.

Shouldn't Efl_Model_Common.h be also renamed to Efl_MVVM_Common.h?

Good point indeed.

cedric updated this revision to Diff 18071.Dec 21 2018, 11:21 AM

Rebase and rename the header.

segfaultxavi accepted this revision.Dec 22 2018, 10:14 AM
This revision is now accepted and ready to land.Dec 22 2018, 10:14 AM
This revision was automatically updated to reflect the committed changes.