Page MenuHomePhabricator

elementary: rework Efl.Ui.Factory to have another additional stage during releasing of items.
ClosedPublic

Authored by cedric on Sep 15 2019, 11:38 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.Sep 15 2019, 11:38 PM
cedric requested review of this revision.Sep 15 2019, 11:38 PM
bu5hm4n requested changes to this revision.Sep 16 2019, 1:00 AM
bu5hm4n added inline comments.
src/lib/efl/interfaces/efl_ui_factory.eo
36

@segfaultxavi please bring your seal here, this sounds weird to me.

src/lib/elementary/efl_ui_widget_factory.c
92

This is now a warning.

This revision now requires changes to proceed.Sep 16 2019, 1:00 AM
segfaultxavi requested changes to this revision.Sep 16 2019, 3:55 AM

In general, I do not like method names in gerund form (building, constructing and releasing).
This is explaining what the caller was doing when the method was called, instead of telling the callee what it should do.
This is confusing, and, in fact, the calls to efl_ui_factory_building() are pretty obscure...

I suggest these APIs are replaced with standard infinitive forms (build, construct and release). If this is ambiguos, add a prefix/suffix to indicate what should be built/constructed/released. Maybe build_item?

src/lib/efl/interfaces/efl_ui_factory.eo
36

Remember that lines must be wrapped at 120 columns.

Release an UI object before it is invalidated. After this call, the object can be recycled to another @Efl.Ui.View.
zmike requested changes to this revision.Sep 16 2019, 6:36 AM

Can we consider removing constructing entirely (this should just be a constructor implementation), renaming building to post_finalize, and renaming releasing to recycle or something?

Removing constructing will remove the ability to do things before the construction is finalized without having to reimplement both widget and factory to change constructor property default value.

Xavi the description of this function are exactly what you said. They describe what the caller is doing, because that define the context to the callee. I am not too sure any of the purposes change help as they are more confusing. release imply you would have to release the widget, which is not what the API is doing. The API is already releasing the widget, but is asking the factory to do the cleanup before the effective release of the object.

Arguably all this function could be event like item,releasing on the factory if that would be easier for you.

zmike added a comment.Sep 16 2019, 8:12 AM

Removing constructing will remove the ability to do things before the construction is finalized without having to reimplement both widget and factory to change constructor property default value.

I still don't understand what you mean by this. The custom constructor can just call the super before it does its own setup, can't it? This is more or less how you're using constructing anyway.

Arguably all this function could be event like item,releasing on the factory if that would be easier for you.

An event item, releasing would work perfectly for me. Almost all our current events are emitted after something has happened (and then they use the past tense), but event emission during the act is also common in other places (and then they use the gerund form).
But a method name in gerund form will confuse people for sure.

You have to try to come up with a name that describes what the method implementor should be doing... if it's not item_release then maybe item_cleanup, item_dispose, item_finalize... I really don't know.

cedric planned changes to this revision.Sep 16 2019, 9:54 AM

Ok, will go with an event that would be the less confusing solution.

cedric updated this revision to Diff 25197.Sep 19 2019, 1:15 PM
cedric retitled this revision from elementary: rework factory to have another additional stage during releasing of items. to elementary: rework Efl.Ui.Factory to have another additional stage during releasing of items..

rebase and take into account

cedric added inline comments.Fri, Sep 20, 9:54 AM
src/lib/elementary/efl_ui_widget_factory.c
92

Woot?

cedric updated this revision to Diff 25287.Fri, Sep 20, 3:24 PM

phab!!!

cedric updated this revision to Diff 25414.Mon, Sep 23, 4:12 PM

rebase and take comments into account

bu5hm4n accepted this revision.Tue, Sep 24, 12:08 AM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Sep 24, 12:20 AM
Closed by commit rEFL0a99fb87bb85: elementary: rework Efl.Ui.Factory to have another additional stage during… (authored by cedric, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.