Page MenuHomePhabricator

entry: move a point to do 'auto_save' to another place
ClosedPublic

Authored by akanad on Mar 14 2019, 9:46 PM.

Details

Summary

By reworking on efl_file, logic flow for entry has been changed.
and it causes autosave making a file that is passed to elm_entry_file_set empty.

Test Plan
  1. call elm_entry_file_set for a file.
  2. check the file is not empty after calling the function.

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.
akanad created this revision.Mar 14 2019, 9:46 PM
akanad requested review of this revision.Mar 14 2019, 9:46 PM
zmike requested changes to this revision.Mar 15 2019, 7:32 AM

I don't think this can possibly be right? efl_file_set is not supposed to have any functional effect other than setting the file property. elm_entry_file_set is a wrapper function around both file_set and file_load, so any issues with elm_entry_file_set should probably just be fixed in that function?

This revision now requires changes to proceed.Mar 15 2019, 7:32 AM

I am not sure you have checked previous code, have you?
previously, save do was called before setting file path.
and that is the reason why save do would not be called.

but after yout rewoking on efl.file, behavior of not only entry but also photocam, layout, evas_image have been changed.

would you please make a sample app for testing steps described above.

juat add entry under window and pass a existing file to efl entry file set and you will get a problem.

zmike added a comment.Mar 15 2019, 8:26 AM

I'm not going to write yet another sample app, if you have interest in enforcing legacy behavior then you may consider creating a unit test to accompany the patch.

I see that point that you're making, but the patch you've proposed is also wrong since there is no guarantee that after efl_file_set is called on the object efl_file_load will also be called. This is why I suggested adding the save directly into elm_entry_file_set since this would guarantee the legacy behavior that you've described.

akanad updated this revision to Diff 20948.Mar 26 2019, 12:00 AM

I got a point of yours. then how about this?

autosave should be invoked while unloading something loaded before.
so that I added filepath for loaded one to save loaded content on it once it's getting unloaded

akanad updated this revision to Diff 20949.Mar 26 2019, 12:01 AM

this is a missed file for testing

zmike requested changes to this revision.Mar 26 2019, 6:13 AM

I understand what this revision is attempting, but it seems to me like this is making the code more complex (and slower) for no real gain? Consider:

  • You've now added another stringshare pointer which duplicates the purpose of the existing file pointer
    • the only reason file exists is to handle legacy behavior of elm_entry_file_get
  • load now directly calls unload, triggering extra operations

If the test case that you've provided is the behavior that you want to preserve, what about this patch:

diff --git a/src/lib/elementary/elm_entry.c b/src/lib/elementary/elm_entry.c
index ed7035fd90..bed9f87890 100644
--- a/src/lib/elementary/elm_entry.c
+++ b/src/lib/elementary/elm_entry.c
@@ -4982,7 +4982,12 @@ _elm_entry_file_text_format_set(Eo *obj EINA_UNUSED, Elm_Entry_Data *sd, Elm_Tex
 EAPI Eina_Bool
 elm_entry_file_set(Evas_Object *obj, const char *file, Elm_Text_Format format)
 {
+   Elm_Entry_Data *sd;
    Eina_Bool ret;
+
+   sd = efl_data_scope_safe_get(obj, MY_CLASS);
+   ELM_SAFE_FREE(sd->delay_write, ecore_timer_del);
+   if (sd->auto_save) _save_do(obj);
    elm_obj_entry_file_text_format_set(obj, format);
    ret = efl_file_simple_load(obj, file, NULL);
    return ret;
@@ -5003,8 +5008,6 @@ _elm_entry_efl_file_load(Eo *obj, Elm_Entry_Data *sd)
    if (efl_file_loaded_get(obj)) return 0;
    err = efl_file_load(efl_super(obj, MY_CLASS));
    if (err) return err;
-   ELM_SAFE_FREE(sd->delay_write, ecore_timer_del);
-   if (sd->auto_save) _save_do(obj);
    return _load_do(obj);
 }

It passes the test and is much simpler. If we really wanted to enforce auto-saving behavior (i.e., when using efl_file_set/load directly) then a simple solution would be to just block using those functions outside of elm_entry_file_set like how elm_win has handlers for when legacy methods are called after eo methods. This would involve something like adding a "locking" bool flag to elm_entry_file_set which triggers a CRI any time the file methods are called when the flag is not set.

This revision now requires changes to proceed.Mar 26 2019, 6:13 AM

Hi. I got things confused after having a look of your comment.

In D8362#153159, @zmike wrote:

I understand what this revision is attempting, but it seems to me like this is making the code more complex (and slower) for no real gain? Consider:

  • You've now added another stringshare pointer which duplicates the purpose of the existing file pointer
    • the only reason file exists is to handle legacy behavior of elm_entry_file_get

elm_entry_file_get has nothing to do with file member. it just passes a result of efl_file_get to a caller, like below.

e7b6d38af3 (Vitor Sousa 2015-07-01 18:30:23 +0100> |5022 elm_entry_file_get(const Evas_Object *obj, const char **file, Elm_>
e7b6d38af3 (Vitor Sousa 2015-07-01 18:30:23 +0100> |5023 {
326e18b3fb (Mike Blumenkrantz 2019-02-27 13:17:09 -0500>|5024 if (file) *file = efl_file_get(obj);

  • load now directly calls unload, triggering extra operations
  1. behavior of elm_entry_file_set has been changed.

user could call elm_entry_file_set multiple time for one entry object, and it would load a new file(passed one).
but now it doesn't, we should unload it before calling elm_entry_file_set right?

  1. that is the reason why I added unload logic in load logic.

if you think users who use efl_file_** should call load/unload explicitly. then let me know that I will post another changes.

  1. by the way, I don't get your point about extra operations.

you are saying that I should pick some lines from unloading logic and copy it rather than calling efl_file_unload to minimize executing instructions?
or you didn't get my intention as described on 1~2 above?

If the test case that you've provided is the behavior that you want to preserve, what about this patch:

diff --git a/src/lib/elementary/elm_entry.c b/src/lib/elementary/elm_entry.c
index ed7035fd90..bed9f87890 100644
--- a/src/lib/elementary/elm_entry.c
+++ b/src/lib/elementary/elm_entry.c
@@ -4982,7 +4982,12 @@ _elm_entry_file_text_format_set(Eo *obj EINA_UNUSED, Elm_Entry_Data *sd, Elm_Tex
 EAPI Eina_Bool
 elm_entry_file_set(Evas_Object *obj, const char *file, Elm_Text_Format format)
 {
+   Elm_Entry_Data *sd;
    Eina_Bool ret;
+
+   sd = efl_data_scope_safe_get(obj, MY_CLASS);
+   ELM_SAFE_FREE(sd->delay_write, ecore_timer_del);
+   if (sd->auto_save) _save_do(obj);
    elm_obj_entry_file_text_format_set(obj, format);
    ret = efl_file_simple_load(obj, file, NULL);
    return ret;
@@ -5003,8 +5008,6 @@ _elm_entry_efl_file_load(Eo *obj, Elm_Entry_Data *sd)
    if (efl_file_loaded_get(obj)) return 0;
    err = efl_file_load(efl_super(obj, MY_CLASS));
    if (err) return err;
-   ELM_SAFE_FREE(sd->delay_write, ecore_timer_del);
-   if (sd->auto_save) _save_do(obj);
    return _load_do(obj);
 }

It passes the test and is much simpler. If we really wanted to enforce auto-saving behavior (i.e., when using efl_file_set/load directly) then a simple solution would be to just block using those functions outside of elm_entry_file_set like how elm_win has handlers for when legacy methods are called after eo methods. This would involve something like adding a "locking" bool flag to elm_entry_file_set which triggers a CRI any time the file methods are called when the flag is not set.

this is the second one that I don't get, what is a reason that we consider this for?
did I understand wrongly ?
user can call elm_entry_file_set and also efl_fle_set and efl_file_load for a entry? right?
and in both ways, they expect auto-save feature will work.

akanad updated this revision to Diff 20977.Mar 26 2019, 10:16 PM

right, this patch looks more simple.

zmike accepted this revision.Mar 27 2019, 7:17 AM

Hi. I got things confused after having a look of your comment.

In D8362#153159, @zmike wrote:

I understand what this revision is attempting, but it seems to me like this is making the code more complex (and slower) for no real gain? Consider:

  • You've now added another stringshare pointer which duplicates the purpose of the existing file pointer
    • the only reason file exists is to handle legacy behavior of elm_entry_file_get

elm_entry_file_get has nothing to do with file member. it just passes a result of efl_file_get to a caller, like below.

Right, my point is that this is wrong since legacy expects that strings like "~/somefile.txt" will be resolved whereas efl_file_get would instead return "/home/zmike/somefile.txt". The internal file string saves the original passed string in order to preserve this behavior, otherwise that struct member would not need to exist.

e7b6d38af3 (Vitor Sousa 2015-07-01 18:30:23 +0100> |5022 elm_entry_file_get(const Evas_Object *obj, const char **file, Elm_>
e7b6d38af3 (Vitor Sousa 2015-07-01 18:30:23 +0100> |5023 {
326e18b3fb (Mike Blumenkrantz 2019-02-27 13:17:09 -0500>|5024 if (file) *file = efl_file_get(obj);

  • load now directly calls unload, triggering extra operations
  1. behavior of elm_entry_file_set has been changed. user could call elm_entry_file_set multiple time for one entry object, and it would load a new file(passed one). but now it doesn't, we should unload it before calling elm_entry_file_set right?

It still does load the new file though...your D8483 patch tests this and it passes. There is no need to explicitly call unload here.

  1. that is the reason why I added unload logic in load logic. if you think users who use efl_file_** should call load/unload explicitly. then let me know that I will post another changes.

I will put up a patch to block this, people should not be using eo apis on legacy objects...

  1. by the way, I don't get your point about extra operations. you are saying that I should pick some lines from unloading logic and copy it rather than calling efl_file_unload to minimize executing instructions? or you didn't get my intention as described on 1~2 above?

No, my point was that by calling unload explicitly then this goes through the entire call chain for unload on the entry object. This should already be implicitly handled whenever an object loads a new file--implementors of efl.file should be clearing out references to the previously-loaded file prior to loading the new one. In the case of entry specifically, this also ends up doing the entire call chain for text_set on the entry object, which we should try to avoid when possible.

If the test case that you've provided is the behavior that you want to preserve, what about this patch:

diff --git a/src/lib/elementary/elm_entry.c b/src/lib/elementary/elm_entry.c
index ed7035fd90..bed9f87890 100644
--- a/src/lib/elementary/elm_entry.c
+++ b/src/lib/elementary/elm_entry.c
@@ -4982,7 +4982,12 @@ _elm_entry_file_text_format_set(Eo *obj EINA_UNUSED, Elm_Entry_Data *sd, Elm_Tex
 EAPI Eina_Bool
 elm_entry_file_set(Evas_Object *obj, const char *file, Elm_Text_Format format)
 {
+   Elm_Entry_Data *sd;
    Eina_Bool ret;
+
+   sd = efl_data_scope_safe_get(obj, MY_CLASS);
+   ELM_SAFE_FREE(sd->delay_write, ecore_timer_del);
+   if (sd->auto_save) _save_do(obj);
    elm_obj_entry_file_text_format_set(obj, format);
    ret = efl_file_simple_load(obj, file, NULL);
    return ret;
@@ -5003,8 +5008,6 @@ _elm_entry_efl_file_load(Eo *obj, Elm_Entry_Data *sd)
    if (efl_file_loaded_get(obj)) return 0;
    err = efl_file_load(efl_super(obj, MY_CLASS));
    if (err) return err;
-   ELM_SAFE_FREE(sd->delay_write, ecore_timer_del);
-   if (sd->auto_save) _save_do(obj);
    return _load_do(obj);
 }

It passes the test and is much simpler. If we really wanted to enforce auto-saving behavior (i.e., when using efl_file_set/load directly) then a simple solution would be to just block using those functions outside of elm_entry_file_set like how elm_win has handlers for when legacy methods are called after eo methods. This would involve something like adding a "locking" bool flag to elm_entry_file_set which triggers a CRI any time the file methods are called when the flag is not set.

this is the second one that I don't get, what is a reason that we consider this for?
did I understand wrongly ?
user can call elm_entry_file_set and also efl_fle_set and efl_file_load for a entry? right?
and in both ways, they expect auto-save feature will work.

I don't think we should expect eo methods to be called on legacy objects. We already have code in place to add compatibility for legacy behavior, and it becomes a bit more difficult to preserve this behavior when we have to consider new methods being called on the object. I will put up a patch for it so you can see.

This patch looks good now, thanks for sticking with it!

This revision is now accepted and ready to land.Mar 27 2019, 7:17 AM
This revision was automatically updated to reflect the committed changes.