Page MenuHomePhabricator

elm_entry: speed up setting text (check if same text is already set), fix setting same text pointer
ClosedPublic

Authored by ali.alzyod on Apr 7 2019, 5:59 AM.

Details

Summary

This patch deal with two cases:

1- Setting text in Entry (elm_entry_entry_set) , as the one get using (elm_entry_entry_get), caused the entry to become empty.

  • becase entry string was free inside the elm_entry_entry_set function, without checking new passed string.

Now we check if same string was passed to the function, then nothing need to be changed.

// Old Behaviour : Entry will become empty
// New Behaviour : Entry will Skip setting same text 
elm_entry_entry_set(app->entry, elm_entry_entry_get(app->entry));

2- Setting text in Entry (elm_entry_entry_set) , with same content string.

  • internally entry will set empty string then set passed string, which will case recalculation and re-render the entry element.

Now we check if same string data that is passed to the function is the same of the entry content, then nothing need to be changed.

// This will be skiped internally since same text is set
elm_entry_entry_set(app->entry, "aaaaa");  
elm_entry_entry_set(app->entry, "aaaaa");  //skipped
Test Plan
#include <Elementary.h>


typedef struct _APP
{
   Evas_Object *win, *box, *entry;
} APP;
APP *app;


EAPI_MAIN int
elm_main(int argc, char **argv)
{


   app = calloc(sizeof(APP), 1);

   elm_policy_set(ELM_POLICY_QUIT, ELM_POLICY_QUIT_LAST_WINDOW_CLOSED);

   
   app->win = elm_win_util_standard_add("Main", "App");
   elm_win_autodel_set(app->win, EINA_TRUE);

   app->box = elm_box_add(app->win);
   app->entry = elm_entry_add(app->box);

   elm_entry_input_panel_enabled_set(app->entry,EINA_TRUE);


   evas_object_size_hint_weight_set(app->box, EVAS_HINT_EXPAND, EVAS_HINT_EXPAND);
   evas_object_size_hint_align_set(app->box, EVAS_HINT_FILL, EVAS_HINT_FILL);

   

   /* TESTING CODE*/
   elm_entry_entry_set(app->entry, "aaaaa");


   /* CASE 1*/
   // This will be skiped internally since same text is set
   elm_entry_entry_set(app->entry, "aaaaa");  


   /* CASE 2*/
   // Old Behaviour : Entry will become empty
   // New Behaviour : Entry will Skip setting same text 
   elm_entry_entry_set(app->entry, elm_entry_entry_get(app->entry));


   evas_object_show(app->entry);
   evas_object_show(app->box);
   elm_box_pack_end(app->box, app->entry);;

   elm_win_resize_object_add(app->win, app->box);
   evas_object_resize(app->win, 320, 480);

   evas_object_size_hint_weight_set(app->entry, EVAS_HINT_EXPAND, EVAS_HINT_EXPAND);
   evas_object_size_hint_align_set(app->entry, EVAS_HINT_FILL, EVAS_HINT_FILL);

   evas_object_show(app->win);
   
   elm_run();

   return 0;
}
ELM_MAIN()

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.
ali.alzyod created this revision.Apr 7 2019, 5:59 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

ali.alzyod requested review of this revision.Apr 7 2019, 5:59 AM
ali.alzyod edited the test plan for this revision. (Show Details)Apr 7 2019, 6:06 AM
ali.alzyod edited the summary of this revision. (Show Details)Apr 7 2019, 6:13 AM
ali.alzyod updated this revision to Diff 21264.Apr 7 2019, 6:17 AM
ali.alzyod edited the test plan for this revision. (Show Details)

update

ali.alzyod edited the summary of this revision. (Show Details)Apr 7 2019, 6:23 AM
ali.alzyod added reviewers: woohyun, bowonryu.
ali.alzyod edited the test plan for this revision. (Show Details)
bu5hm4n requested changes to this revision.Apr 17 2019, 11:13 PM

I think you need a little bit more here. i think you need to delete the append_text_left field, and the append_text_idler. Otherwise the result could be different to what a user expects. Imagine this, text_set("a") text_append("b") text_set("a") after one idler this text displayed entry will say "ab" instead of "a".

src/lib/elementary/elm_entry.c
3331

current_text instead of currentText and move the declaration of the var into the beginning of the function, (like len).

This revision now requires changes to proceed.Apr 17 2019, 11:13 PM

move code, and align with code convention

bu5hm4n requested changes to this revision.Apr 17 2019, 11:50 PM

Now this function will exit without thawing the events.

src/lib/elementary/elm_entry.c
3347

Typo

This revision now requires changes to proceed.Apr 17 2019, 11:50 PM

Okay, now this code block is *after* the actaul text is setted to "elm.text", is that intended ?

add event thaw

@bu5hm4n

It work if we are going to set text in "elm.text", else we do not care inside this function

ali.alzyod marked an inline comment as done.Apr 18 2019, 4:10 AM
ali.alzyod marked an inline comment as done.
raster requested changes to this revision.Apr 18 2019, 5:06 AM
raster added a subscriber: raster.

small change i think ... it's less code and less prone to error.

but interesting you're trying to fix a case that's kind of "invalid" we return a const char * which in so many api cases is the sign of "we return an internal object (ptr to data) of some sort" and passing it back in in a set is kind of... asking for trouble. because you are using a value that, by definition changes when you do a set, and you are using it as input to the thing that changes it. we can guard against it like your patch, but keep in mind that this is guarding against something that just should not be done in so so so so many cases like this.

src/lib/elementary/elm_entry.c
3351

i'd encourage using a goto here - add a "done:" label at the end of the func and goto that. yes - gotos can be good especially for cases like this. less copy & paste code and fewer areas to go wrong. :)

This revision now requires changes to proceed.Apr 18 2019, 5:06 AM
ali.alzyod updated this revision to Diff 21457.Apr 18 2019, 6:06 AM

use goto label

ali.alzyod marked an inline comment as done.Apr 18 2019, 6:07 AM

developer should not do case 1 it is invalid, but it case he did it, we will help him :)

oh i had an idea over lunch... store sd->text in a local var and do the free at the end of the function. then it can't ever be an invalid pointer. the check for it being the same ptr can avoid doing the duplicate and the free to as it does now so store and move the free before the done label and there will be no crash. right?

@raster if I understand your comment well, I think the (sd->text) does not always have the entry text, specially if you append text in-front or back, so condition will be on "elm.text" part to ensure this is entry latest text

Ok I see, I think we can move all the sd->text free before done label, because it is initialize only in entry_text_get function.
I will do a test that it did not called and move it, or save it in local var and free it.

ali.alzyod updated this revision to Diff 21568.Apr 23 2019, 8:46 AM

save sd->text if text did not change

@raster is this what you want to do ?

Can you adjust the commit title here to say something like:

´elm_entry: speed up [...]´

We try to prefix our commit messages with the subsystem the commit is touching ... :)

ali.alzyod retitled this revision from Speed up setting text (check if same text is already set), fix setting same text pointer to elm_entry: speed up setting text (check if same text is already set), fix setting same text pointer.Fri, Apr 26, 9:44 AM
bu5hm4n accepted this revision.Thu, May 2, 10:27 AM
This revision was not accepted when it landed; it landed in state Needs Review.Mon, May 6, 8:08 AM
Closed by commit rEFLbfaec01987d4: elm_entry: speed up setting text (check if same text is already set), fix… (authored by ali.alzyod, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.