Page MenuHomePhabricator

entry: move a point to do 'auto_save' to another place
Needs RevisionPublic

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

Details

Reviewers
zmike
bu5hm4n
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
Branch
arcpatch-D8362
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10563
Build 8289: arc lint + arc unit
akanad created this revision.Thu, Mar 14, 9:46 PM
akanad requested review of this revision.Thu, Mar 14, 9:46 PM
zmike requested changes to this revision.Fri, Mar 15, 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.Fri, Mar 15, 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.Fri, Mar 15, 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.Tue, Mar 26, 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.Tue, Mar 26, 12:01 AM

this is a missed file for testing

zmike requested changes to this revision.Tue, Mar 26, 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.Tue, Mar 26, 6:13 AM