I don't know much about elm_map.
But Here is my review.
1. It would be better if your patches were generated based on the previous ones.
Because reviewer needs to see the only changes by each patch files.
2. missed exception handling.
+ for (zoom = wd->src->zoom_min; zoom <= wd->src->zoom_max; zoom++)
+ {
+ g = calloc(1, sizeof(Grid));
+ g->zoom = zoom;
please check g not to have segfault.
3. Maybe there looks same pattern scattered to find the current zoom level
grid.
(I found 4 times at least)
+ Eina_List *l;
+ Grid *g;
..........
+ EINA_LIST_FOREACH(wd->grids, l, g)
+ {
+ if (wd->zoom == g->zoom) break;
+ }
This code could be one inline function.
ex)
Grid *_get_current_zoom_level_grid(Widget_Data *wd)
{
Eina_List *l;
Grid *g;
EINA_LIST_FOREACH(wd->grids, l, g)
if (wd->zoom == g->zoom) return g;
}
4. in second patch
the name of have field is really abstract.
at least it could be "file_have" or something more detailed one.
@@ -254,7 +255,7 @@ struct _Grid_Item
} src, out;
-
+ Eina_Bool have : 1;
Ecore_File_Download_Job *job;
------------------------------------
-Regards, Hermet-
-----Original Message-----
From: "Bluezery"<ohpo...@gmail.com>
To: "Enlightenment developer
list"<enlightenment-devel@lists.sourceforge.net>
Cc:
Sent: 11-12-13(화) 09:43:21
Subject: Re: [E-devel] [Patch][elm_map] Change grid management
Dear, all
I am waiting for a week.
But no one may be interested in map issues.. ;-(
It will be my pleasure if someone reviews this patch.... :-)
2011/12/6 Bluezery <ohpo...@gmail.com>:
> Hello,
>
> I separate my path into 4 patches.
> It should be patched sequentially
>
> (1) Grid Management
> There are no caching mechanism in current elm_map.
> So, too many network resources are wasted because elm_map keeps only
> two grids and already downloaded images are downloaded again and
> again. (This also slows the map loading speed)
> I have changed this grid management policy.
>
> I have done followings.
> 1. Create all grids (all zoom levels) when elm_map_add() is called (No
> memory overhead because of sparse matrix)
> 2. Clear all grids when map object is deleted.
> 3. Loads necessary grids and unloads unused grids when zoom level is changed.
>
> Changed grid management have one weakness that memory and tmp size can
> grow bigger while map object is live.
> I think it may need API such as elm_map_cache_size_set().
>
> (2) Revoking gi->have
> I restore the gi->have values. The removal of this is my mistake.
> gi->have is needed because I cannot know whether this file is
> downloading (just opened and not written) or downloaded state even if
> image file exists.
>
> (3) Simplify grid_load()
> There exists 3 state for grid items. i.e., downloaded, downloading, none
> states.
> grid_load() does followings:
> 1. if a grid item does not exist, create it.
> 2. update a tile (if downloaded) or queues for download (if none) or
> nothing (if downloading)
>
> (4) Merging foreach
> Two foreach loop can be merged.
>
> Please review this again.
>
> 2011년 12월 5일 오후 9:41, Daniel Juyung Seo <seojuyu...@gmail.com>님의 말:
>> Yeah as Hermet mentioned, this is one big blob of patch which makes
>> reviewers hard to review.
>> Please separate your patch into a couple for patches for each feature.
>> That will help reviewing and debugging.
>> Thanks.
>>
>> Daniel Juyung Seo (SeoZ)
>>
>> 2011/12/5 ChunEon Park <her...@naver.com>:
>>> please make one shot one kill.
>>> Final change should not be included here.
>>> ------------------------------------
>>> -Regards, Hermet-
>>>
>>> -----Original Message-----
>>> From: "Bluezery"<ohpo...@gmail.com>
>>> To: "Enlightenment developer
>>> list"<enlightenment-devel@lists.sourceforge.net>
>>> Cc:
>>> Sent: 11-12-05(월) 19:38:24
>>> Subject: [E-devel] [Patch][elm_map] Change grid management
>>> Hi,
>>> There are no caching mechanism in current elm_map.
>>> So, too many network resources are wasted because elm_map keeps only
>>> two grids and already downloaded images are downloaded again and
>>> again. (This also slows the map loading speed)
>>> I have changed this grid management policy.
>>> I have done followings.
>>> 1. Create all grids (all zoom levels) when elm_map_add() is called (No
>>> memory overhead because of sparse matrix)
>>> 2. Clear all grids when map object is deleted.
>>> 3. Loads necessary grids and unloads unused grids when zoom level is
>>> changed.
>>> Changed grid management have one weakness that memory and tmp size can
>>> grow bigger while map object is live.
>>> I think it may need API such as elm_map_cache_size_set().
>>> Finally, I restore the gi->have values. The removal of this is my mistake.
>>> gi->have is needed because I cannot know whether this file is
>>> downloading (just opened and not written) or downloaded state even if
>>> image file exists.
>>> Please review this patch.
>>> Thanks
>>> --
>>> BRs,
>>> Kim.
>>> ------------------------------------------------------------------------------
>>> All the data continuously generated in your IT infrastructure
>>> contains a definitive record of customers, application performance,
>>> security threats, fraudulent activity, and more. Splunk takes this
>>> data and makes sense of it. IT sense. And common sense.
>>> http://p.sf.net/sfu/splunk-novd2d_______________________________________________
>>> enlightenment-devel mailing list
>>> enlightenment-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>>> ------------------------------------------------------------------------------
>>> All the data continuously generated in your IT infrastructure
>>> contains a definitive record of customers, application performance,
>>> security threats, fraudulent activity, and more. Splunk takes this
>>> data and makes sense of it. IT sense. And common sense.
>>> http://p.sf.net/sfu/splunk-novd2d
>>> _______________________________________________
>>> enlightenment-devel mailing list
>>> enlightenment-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>>
>> ------------------------------------------------------------------------------
>> All the data continuously generated in your IT infrastructure
>> contains a definitive record of customers, application performance,
>> security threats, fraudulent activity, and more. Splunk takes this
>> data and makes sense of it. IT sense. And common sense.
>> http://p.sf.net/sfu/splunk-novd2d
>> _______________________________________________
>> enlightenment-devel mailing list
>> enlightenment-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
>
>
> --
> BRs,
> Kim.
--
BRs,
Kim.
------------------------------------------------------------------------------
Systems Optimization Self Assessment
Improve efficiency and utilization of IT resources. Drive out cost and
improve service delivery. Take 5 minutes to use this Systems Optimization
Self Assessment. http://www.accelacomm.com/jaw/sdnl/114/51450054/
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
------------------------------------------------------------------------------
Systems Optimization Self Assessment
Improve efficiency and utilization of IT resources. Drive out cost and
improve service delivery. Take 5 minutes to use this Systems Optimization
Self Assessment. http://www.accelacomm.com/jaw/sdnl/114/51450054/
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
