Add Search to Palette Editor #71

Open
dimmus wants to merge 4 commits from dimmus/enlightenment:devs/dimmus/paledit into master
First-time contributor

Added text entry to search or sort color list item(s) in Palette Editor.

Added text entry to search or sort color list item(s) in Palette Editor.
dimmus added 2 commits 2024-06-04 07:08:17 -07:00
dimmus changed title from devs/dimmus/paledit to Add Search to Palette Editor 2024-06-04 07:27:24 -07:00
raster requested review from Owners 2024-06-05 10:19:49 -07:00
raster reviewed 2024-06-05 10:29:07 -07:00
@ -206,0 +214,5 @@
/* XXX: We found exact item, so focus it.
* But not shure that it's usefull, may be even annoying.
* It throught errors when you filter from exact to exact imidiately:
* Ex.: enter ':bg-dim', then select '-dim' part and delete it.
* You get exact ':bg' and focus_manager error in stdout ;( */
Owner

if its "focusing/selecting" as you type - i think this might be better done with a timer + delay. so as you type and keep typing it might filter but not select as it assumes you are still in the middle of typing. then after e.g. 0.2 sec or 0.5 sec of no more changing of the filter string then if there is an exact match, select it. it's easy - just add a timer every change of filter string and delete the old timer if there was one (or do a timer reset on the timer). set timer handle to null if timer goes off/expires etc. - common technique to use timers to guess when the user is done...

if its "focusing/selecting" as you type - i think this might be better done with a timer + delay. so as you type and keep typing it might filter but not select as it assumes you are still in the middle of typing. then after e.g. 0.2 sec or 0.5 sec of no more changing of the filter string then if there is an exact match, select it. it's easy - just add a timer every change of filter string and delete the old timer if there was one (or do a timer reset on the timer). set timer handle to null if timer goes off/expires etc. - common technique to use timers to guess when the user is done...
Author
First-time contributor

Not exactly so.
Here we make a selection only when your search entry text is exactly equal to the string in the list. In the middle of typing we have no selection.

But i think about timer. Nice suggestion.

Not exactly so. Here we make a selection only when your search entry text is exactly equal to the string in the list. In the middle of typing we have no selection. But i think about timer. Nice suggestion.
Owner

a timer certainly reduces "sluggishness". if you have to walk and rebuild lists every char you type ... that can impact responsiveness. if you wait for the typing to pause/stop - it is better :)

a timer certainly reduces "sluggishness". if you have to walk and rebuild lists every char you type ... that can impact responsiveness. if you wait for the typing to pause/stop - it is better :)
dimmus marked this conversation as resolved
raster reviewed 2024-06-05 10:31:23 -07:00
@ -521,0 +541,5 @@
_search_changed_cb(void *data, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED)
{
Search_Event_Data * event_data = (Search_Event_Data *)data;
const char *str = elm_entry_entry_get(event_data->en_obj);
Owner

formatting - no blank line above this const char * - keep variables in block at top of scope :)

formatting - no blank line above this const char * - keep variables in block at top of scope :)
dimmus marked this conversation as resolved
raster requested changes 2024-06-05 10:32:07 -07:00
raster left a comment
Owner

very minor thing - and a comment on the selecting immediately thing - perhaps do the timer delay?

very minor thing - and a comment on the selecting immediately thing - perhaps do the timer delay?
dimmus added 1 commit 2024-06-09 02:07:13 -07:00
Author
First-time contributor

@raster, it got me wondering, do we need to restart timer on each entry change?

@raster, it got me wondering, do we need to restart timer on each entry change?
Author
First-time contributor

Another question.
What will be the difference between

static void
_cb_search_changed(void *data, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED)
{
   ecore_timer_add(0.3, _cb_items_sort, data);
}

and this

static void 
_cb_search_changed(void *data, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED)
{
   if (sort_timer) ecore_timer_del(sort_timer);
   sort_timer = ecore_timer_add(0.3, _cb_items_sort, data);
}

For the former, what will happen with timer on the second and further call? Will it restart?

Another question. What will be the difference between ``` static void _cb_search_changed(void *data, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED) { ecore_timer_add(0.3, _cb_items_sort, data); } ``` and this ``` static void _cb_search_changed(void *data, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED) { if (sort_timer) ecore_timer_del(sort_timer); sort_timer = ecore_timer_add(0.3, _cb_items_sort, data); } ``` For the former, what will happen with timer on the second and further call? Will it restart?
dimmus added 1 commit 2024-06-09 05:21:07 -07:00
Author
First-time contributor

It seems good now.

It seems good now.
dimmus requested review from raster 2024-06-09 05:24:10 -07:00
raster requested changes 2024-06-17 00:16:16 -07:00
raster left a comment
Owner

see inline comments :) good work btw ... still more massaging to do.

see inline comments :) good work btw ... still more massaging to do.
@ -3,0 +6,5 @@
struct _Search_Event_Data
{
Evas_Object *en_obj;
Evas_Object *win_obj;
};
Owner

any reason to have sort time global but en_obj and win_obj not?

any reason to have sort time global but en_obj and win_obj not?
@ -521,0 +552,5 @@
static void
_cb_search_changed(void *data, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED)
{
if (sort_timer) return;
Owner

hmm no - perhaps it should be if (sort_timer) ecore_timer_del(sort_timer); .... because if i keep typing it should probably just wait until i stop for a bit. you can reduce the timeout then to like 0.2 or 0.3 or something to make it appear to respond faster, but while i keep typing it should hold back... right?

hmm no - perhaps it should be if (sort_timer) ecore_timer_del(sort_timer); .... because if i keep typing it should probably just wait until i stop for a bit. you can reduce the timeout then to like 0.2 or 0.3 or something to make it appear to respond faster, but while i keep typing it should hold back... right?
@ -594,7 +635,24 @@ palcols_add(Evas_Object *win)
elm_object_content_set(btn, o);
evas_object_show(o);
o = elm_genlist_add(win);
bx_entry = elm_box_add(win);
Owner

why put a horizontal box into the vertical box and only have one item (the entry)?? kind of pointless extra widget...

why put a horizontal box into the vertical box and only have one item (the entry)?? kind of pointless extra widget...
@ -598,0 +644,5 @@
en = o = elm_entry_add(win);
elm_entry_single_line_set(o, EINA_TRUE);
elm_entry_scrollable_set(o, EINA_TRUE);
elm_object_part_text_set(o, "guide", "Type item's name here to search.");
Owner

This guide text might be a little long... :) It might not even fully fit? It might be better as "Search" or "Type to search" or something short.

This guide text might be a little long... :) It might not even fully fit? It might be better as "Search" or "Type to search" or something short.
@ -621,0 +680,5 @@
event_data->en_obj = en;
event_data->win_obj = win;
evas_object_smart_callback_add(en, "changed,user", _cb_search_changed, (void*)event_data);
evas_object_event_callback_add(gl, EVAS_CALLBACK_FREE, _cb_search_del, (void*)event_data);
Owner

you probably want to use EVAS_CALLBACK_DEL not EVAS_CALLBACK_FREE :). Also in the _cb_search_del just to be sure - delete the _cb_search_changed callback. :) You might also want to delete the sort timer too here... :) (and set it to NULL)

you probably want to use EVAS_CALLBACK_DEL not EVAS_CALLBACK_FREE :). Also in the _cb_search_del just to be sure - delete the _cb_search_changed callback. :) You might also want to delete the sort timer too here... :) (and set it to NULL)
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b dimmus-devs/dimmus/paledit master
git pull devs/dimmus/paledit

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff dimmus-devs/dimmus/paledit
git push origin master
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: enlightenment/enlightenment#71
No description provided.