Menu
▾
▴
Re: [Gtkpod-devel] Patch to write MHOD type 53 data objects
|
From: Ian S. <ian...@oz...> - 2008-05-24 22:49:40
|
Christophe Fergeau wrote:
> Hi,
>
> 2008/5/24 Ian Stewart <ian...@oz...>:
>
>> I've fixed the Æ problem, attached is the revised patch.
>>
>
> Which I finally reviewed. All in all it's good, I made some changes
> which are either cosmetic (to be a bit more consistent to the coding
> style used in the code around your changes), or some minor changes to
> get the code more readable.
> * in the struct MhodData, I got rid of the union {} data2; and union
> {}data3;, they seem totally useless.
> * I reworked jump_table_letter to avoid using lots of g_utf8_strlen
> calls (which are probably really expensive) and I changed it to use
> g_ucs4_to_utf16 instead of going from UCS4 to utf8 and then to
> utf16. These changes are totally untested, I hope I didn't break
> everything :-/
> * I removed free_mhod53_list since it can be rewritten in 2 lines of
> code (g_list_foreach (.., g_free, NULL); + g_list_free);
> * I split your mk_52and53_mhod function in 2 separate functions, dunno
> if that really makes sense. Now that these functions exist, I'm
> starting to think that the code related to mhod52 and mhod53 in
> mk_mhod could be moved here as well, dunno if you have a feeling about
> that ?
>
> I think it might be possible to simplify the loop creating the
> mhod53_entry : currently, there is that code
>
> + if (sortkey != lastsortkey) {
> + m53 = g_new0 (struct mhod53_entry, 1);
> + m53->letter = lastsortkey;
> + m53->start = mhod53start;
> + m53->count = mhod53count;
> + mhod->data3.mhod53_list = g_list_append (mhod->data3.mhod53_list,
> + lastsortkey = sortkey; mhod53start = mhod53index; mhod53count = 0;
> + }
>
> in the loop when a new letter is found, and that code :
>
> + m53 = g_new0 (struct mhod53_entry, 1);
> + m53->letter = lastsortkey;
> + m53->start = mhod53start;
> + m53->count = mhod53count;
> + mhod->data3.mhod53_list = g_list_append (mhod->data3.mhod53_list, m53)
>
> when going out of the loop to make sure we don't miss the last entry.
> It might be possible to create the mhod53_entry struct when we
> encounter a new letter, and to use that structure to account for the
> use of that letter instead of creating that structure once we have all
> the information about the previous letter (dunno if I'm making sense).
> For example, let's say we are handling letter G, here is what is
> currently happening :
> (this is the 1st string starting with G we encounter)
> we get a string starting with G, increment local var storing G count
> we get a string starting with G, increment local var storing G count
> we get a string starting with H, create a mhod53_entry for G, fill
> it with the local variables we have for that purpose, append it to the
> list, reset the local variables
>
> What I'm suggesting is doing this instead :
> (this is the 1st string starting with G we encounter)
> create a mhod53_entry for G, init it, set G count to 0
> we get a string starting with G, increment G count in the mhod53_entry
> we get a string starting with G, increment G count in the mhod53_entry
> we get a string starting with H, append the mhod53_entry for G to
> the list, create a new mhod53_entry for H
>
> if everything goes well, this could eliminate the need for the 4 or 5
> local state vars (which would make the code easier to read) and the
> need for the duplicated code block when going out of the loop. But
> maybe that's not possible for some reason, I haven't tried it :)
>
> If this approach works, and if you agree that this makes the code a
> bit better, this would be good if you could update your patch to do
> that.
>
> Finally, one last thought that I had while reading the code : the code
> would probably more readable (and more easy to modify) if the various
> sorted lists and jump letter tables were calculated in a function
> called from write_playlist (where mk_mhod52 and mk_mhod53 are called)
> instead of doing it while writing the mhod (with the implicit
> assumption that we won't try to write a mhod53 before the
> corresponding mhod52 for example). It would be cleaner if all those
> lists which are generated in mk_mhod were generated separately.
> But it's just a random thought wrt code readability and is not totally
> related to your patch. That won't prevent it from going in ;)
>
> Thanks again for the patch, and for the quick bugfixes,
>
> Christophe
>
>
>
>> As far as I can tell, the result of this is that the jump table should
>> follow the sort order, no matter how it was generated. This is obviously not
>> quite the case from what you have seen.
>>
>
> I think we can investigate that issue after having landed the 1st part
> of your patch in svn. We might get more eyes on the problem once it's
> committed :)
>
>
>> Regards,
>> Ian.
>>
>> -------------------------------------------------------------------------
>> This SF.net email is sponsored by: Microsoft
>> Defy all challenges. Microsoft(R) Visual Studio 2008.
>> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
>> _______________________________________________
>> Gtkpod-devel mailing list
>> Gtk...@li...
>>
>> https://lists.sourceforge.net/lists/listinfo/gtkpod-devel
>>
>>
>>
Hi,
Here is the updated version of the patch.
One of the g_utf8_strlen calls is back. I found that I still need to
test for a zero length string at the start of jump_letter_table,
otherwise it loops forever. The ones within the loop through the string
are gone for good though.
The g_ucs4_to_utf16 change is good. This is really what I wanted to do
in the first place, but at the time I counld't find the function in the
glib doco on www.gtk.org. It is there, I just must have missed it.
I've made the change suggested to the loop building up the mhod53_entry
list. I didn't like having that extra bit of code after exiting the
loop, but couldn't think of another way of doing it.
I haven't make you last change about generating the tables in functions
called from write_playlist. I will look at this, but probably won't have
time this weekend.
Regards,
Ian.
|