Page MenuHomePhabricator

efl_ui_layout: check part existence in text_set
ClosedPublic

Authored by zmike on Feb 6 2019, 12:57 PM.

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9385
zmike created this revision.Feb 6 2019, 12:57 PM
zmike requested review of this revision.Feb 6 2019, 12:57 PM

Hum, I have been reading this patch, and I think the correct patch might be requiring more work. I think efl_text_set should be able to return an error code if a part doesn't exist or if it fails. We can make setter return an Eina.Error, make the implementation in src/lib/edje/edje_part_invalid.c return an error code and directly return that here. This would make the API more future proof I think.

cedric requested changes to this revision.Feb 11 2019, 2:41 PM
This revision now requires changes to proceed.Feb 11 2019, 2:41 PM
zmike requested review of this revision.Feb 25 2019, 6:40 AM

Hum, I have been reading this patch, and I think the correct patch might be requiring more work. I think efl_text_set should be able to return an error code if a part doesn't exist or if it fails. We can make setter return an Eina.Error, make the implementation in src/lib/edje/edje_part_invalid.c return an error code and directly return that here. This would make the API more future proof I think.

I'm not opposed to that, but I think it's more useful to restore expected behavior immediately instead of leaving it broken until someone does a major refactor.

bu5hm4n accepted this revision.Feb 27 2019, 12:11 PM
In D7888#143381, @zmike wrote:

Hum, I have been reading this patch, and I think the correct patch might be requiring more work. I think efl_text_set should be able to return an error code if a part doesn't exist or if it fails. We can make setter return an Eina.Error, make the implementation in src/lib/edje/edje_part_invalid.c return an error code and directly return that here. This would make the API more future proof I think.

I'm not opposed to that, but I think it's more useful to restore expected behavior immediately instead of leaving it broken until someone does a major refactor.

I fully agree with this, and therefore accaept this revision.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 27 2019, 1:04 PM
This revision was automatically updated to reflect the committed changes.

@zmike

I am afraid this patch breaks the backward compatibility.
Please check the Popup -> popup-center-text in elementary_test. (text is not displayed.)

I think we should revert this patch for now..

zmike added a comment.Mar 13 2019, 7:06 AM

@zmike

I am afraid this patch breaks the backward compatibility.
Please check the Popup -> popup-center-text in elementary_test. (text is not displayed.)

I think we should revert this patch for now..

That's fine, I've reviewed the new patch and it seems reasonable once the issue I cited is fixed.