Page MenuHomePhabricator

efl_ui_alert_popup: add title part in parts block
ClosedPublic

Authored by herb on Nov 4 2019, 11:06 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.
herb created this revision.Nov 4 2019, 11:06 PM
herb requested review of this revision.Nov 4 2019, 11:06 PM
herb updated this revision to Diff 26679.Nov 5 2019, 4:04 AM

modified the codes

herb updated this revision to Diff 26680.Nov 5 2019, 4:11 AM

add @beta tag

Jaehyun_Cho requested changes to this revision.Nov 5 2019, 4:53 AM
Jaehyun_Cho added inline comments.
src/lib/elementary/efl_ui_alert_popup.eo
49

Since Efl.Ui.Alert_Popup_Part_Title is @beta class, @beta is required here.

This revision now requires changes to proceed.Nov 5 2019, 4:53 AM
herb updated this revision to Diff 26696.Nov 5 2019, 6:26 PM

update the codes

herb added a reviewer: zmike.Nov 5 2019, 6:28 PM

@zmike

In 1.23 release, we opened Efl.Ui.Alert_Popup without @beta but we still kept Efl.Ui.Alert_Popup_Part with @beta.

Is it OK if Efl.Ui.Alert_Popup_Part is changed to Efl.Ui.Alert_Popup_Part_Title since it was @beta?

Jaehyun_Cho accepted this revision.Nov 6 2019, 8:38 PM
This revision is now accepted and ready to land.Nov 6 2019, 8:38 PM
This revision was automatically updated to reflect the committed changes.

(I know this is already merged, however, i am not really happy with this.)

The change itself (in terms of @beta'ness) is IMO completly fine, as we never exposed the part ... :)

However, i would prefer if we use more of the macros that we have, instead of handwriting the part things.

What wonders me also quite a bit here, is there a specific reason we have pd->title_text in the popup itself ? It feels to me that we could just rely on the layout implementation here, and not needing a implementation at all. Additionally, the signal that is emitted in the popup there is also emitted from the layout in a similar manner (when a text gets set, a signal for setting this text is emitted).

Long story short: I dont think we need this title part at all. We can just use Efl.Ui.Layout_Part_Text.

src/lib/elementary/efl_ui_alert_popup.c
274

I would rather prefer to adjust ELM_PART_OVERRIDE_PARTIAL to handle this name, instead of hand writing the macro here.

283

This is just handwritten what we have ELM_PART_OVERRIDE_TEXT_TEXT_GET_FULL for. (Maybe also check ELM_PART_OVERRIDE_TEXT_SET)

292

This is just handwritten what we have ELM_PART_OVERRIDE_TEXT_TEXT_GET_FULL for.(Maybe also check ELM_PART_OVERRIDE_TEXT_GET)