Hi Jiř,
Here are my comments about your changes on the unref branch.
Please reinstate view->priv instead of GET_PRIVATE in
aa04b92d25592044c31775f16027f7c10349fa7e. Accessing a pointer is faster
than the G_TYPE_INSTANCE_GET_PRIVATE macro. The priv pointer cannot be
read outside of champlain-view.c because the definition of the struct is
undefied outsite it (see the typedefs).
Inheriting from ClutterGroup is not a good solution: the Clutter devs
have said we should rather implement a ClutterActor that implements the
ClutterContainer interface that does the layouting.
The rest of the changes in this commit are good.
in 7cdde727495633976ffd210c45903ab5f9aeb7c2, the coding style we use
(Telepathy's) define headers this way:
+static void
+fade_in_completed (ClutterAnimation *animation,
+ ChamplainTile *self)
This may be odd, but that's this way for now every where in the code :)
In 2efa6379de623c173d825bc404e92b4ceaac1c58, if you are removeing the
{}, you need to move the var declaration at the top of the function :) I
lazily accepted the commit as is because it had the curly brackets :)
In 98ab7c94afd0edcc854102d75db57e2d0cdacf43: cool addition! :D It'd be
even nicer with a go_to ;-)
Regarding the comment in ad4686924e08d024c0208652706fa38e176867a5:
viewport_pos_changed_cb has a view_load_visible_tiles, it should
download the tiles. But I like your change anyway, it's cleaner.
In 0f32705faafff12c439c5b4bd3eb83504ac6e98d, this condition was meant to
save a lot of looping. This callback is called really often with the
same values. I think it should stay.
In 2942103542c53a93ac0351183cb93250f81ce05c, these calls will make
features like "show the currently cached tiles on zoom out" hard to
do :)
In 26fa47ea332c5e3261671778f2d860eb0c6eca80, I believe a more static
array for tile_map would save a lot of CPU. It could be resized when
needed, but not reallocated on every run of that function, which is
quite often.
In 1bd883f7c19432f59818e1b63927f03409e0030b, it used to be in a idle
callback until the animation-marker demo was created. The animations
seems to eat the idle's time and tiles were sometimes not loaded. Did
you experience that?
In 9ef262a7c43e47cc8d9b66435e57762bfc9770f1, it is indeed a wtf
moment... It seems that TidyViewport returns invalid values when we
update the Adjustments. Could be worth investigating. But your later
commits that block the callback might be a solution, did you retry to
remove that fix again after?
All in all, those a huge improvements! And the widget really feels
faster and more responsive. I've looked into fixing those memory fixes
many many times, throwing hours at it but I was locked in my mental
model of the problem. I am still experiencing a memory leak, but it
seems to be much more contained than before (and as previously thought,
it could be a leak in my 3d driver). You've also neatly simplified the
code! :)
My only complain about the widget now is something that I introduced in
0.4.3: before that, there never used to be a "black out" between zoom
levels. It'd be nice if the previously visible tiles were still visible
while the new ones are loaded. But that can be fixed after we merged
all this.
Thanks for your work.
Pierre-Luc
Attachment:
signature.asc
Description: This is a digitally signed message part