[mythtv] [mythtv-commits] Ticket #1569: mythvideo grab-bag 'o crunchy goodness

[Michael T. Dean]

On 03/25/06 16:39, George Nassas wrote:

>On 25-Mar-06, at 3:01 PM, Michael T. Dean wrote:
>
>>>5: allow multiple videos to have the same title
>>>      
>>>
>>Isn't that significantly more complex (and resource-intensive) than 
>>the fix in http://svn.mythtv.org/trac/ticket/1354 ?  You're using the 
>>same hack, only your approach needlessly scans the list a ton of 
>>times.
>>    
>>
>Ah yes, the "files with square brackets" in the title made me think he 
>was fixing an issue with files that have square brackets so I didn't 
>review the patch.
>  
>
Yeah.  The ticket's summary isn't quite right (part of the problem with 
people (not you--just in general) going directly to Trac instead of 
posting questions on the list first :).  Perhaps I should have fixed the 
summary, but I wasn't sure the devs would agree with my interpretation 
of what is and is not a bug in the original report, so I left it as it was.

>I added this change late in the cycle and didn't notice any slowdown. 
>If my puny 1GHz almost-but-not-quite-a-pentium epia can tear through 
>that loop in no time then there isn't a problem for other machines.
>
>What I was thinking with appending "+" characters is it keeps the sort 
>order deterministic based on where the video appears in the filesystem. 
>With IDs it depends on when you added each video which seems random 
>unless you've got a rainman type memory.
>
>These are hardly major issues but personally I feel applications look 
>more professional when they behave in a predictable way.
>  
>
I would argue that the video's ID is equally deterministic (the video's 
ID doesn't change).  And, if I were to have same-titled videos in 
multiple directories, I can't "intuit" the order they will appear even 
if the order is "deterministic based on where the video appears in the 
filesystem"--I'd actually have to dig into some code to see how the 
filesystem is being read--especially since the filesystem often returns 
results in the order in which files were created instead of 
alphabetically.  (To prove it to yourself, move all the searchplugins 
from your Firefox install to another directory and move them back in a 
weird order--that being the undocumented approach for sorting FF search 
plugins.)

However, I don't think your patch is actually sorting based on where the 
video appears in the filesystem since it's appending plus(es) to each 
duplicate title in the order returned by the metaptrs list.  The list is 
populated with the query results (not by scanning the filesystem), and, 
since the Myth sorting is only occuring when the user chooses 
VideoFilterSettings::kOrderByTitle (all other sorts are done by the DB), 
the query has the videos ordered by title (based on the "ORDER BY title" 
clause)--where title is identical, thus the original issue.  So my best 
guess is that MySQL returns the like-titled videos in the order they 
were added to the DB--which itself determines the video's ID.

Which means that if my patch did result in sorting like-titled videos by 
intid, our patches would have identical results.  However, my patch 
results in their being sorted by ID ascii-betically (really, 
Unicode-betically--technically, lexically--not alphanumerically), so if 
videos with ID 2 and 10 had identical titles, the one with ID 10 would 
come first in MythVideo's UI.

To make my patch give identical results to yours, we could change it to 
use an sprintf() instead of the string concat of the number() and format 
the ID with sufficient leading zeros.  This would provide the same 
functionality as yours, but without scanning the list at least once for 
each video (in the call to contains()) (assuming we never have more than 
2 videos with the same title--those that repeat 3 or more times would 
require additional scans as we find that "title+" exists (and then 
"title++" and ...)).

Let me know what you think, and unless I'm way off base with my 
analysis, I'll update my patch to use sprintf() so we have a somewhat 
intuitive sort order (based off when the video was added to MythVideo).

Oh, and I'm not even mentioning the Unicode sorting issues (i.e. not 
doing localeAwareCompare()'s) with the hack that was in there before 
both of our patches.  :)

Chris Pinkham assigned himself to my patch, so I'm still waiting to see 
if he even wants the hack-on-a-hack fix or wants me to do a proper 
sort--as described in my comment with my patch at 
http://svn.mythtv.org/trac/ticket/1354.

>>posting these changes as separate patches
>>    
>>
>I thought about that but didn't want to go back to a clean tree and 
>recreate each change and incrementally diff
>
So, you left that work for the (already too-busy) devs?  ;)  (Of course, 
I can't say that my 23-line comment for a 1-line patch is any better. :)

> so the options were one big 
>patch or a hand edit of the diff into smaller ones. Hand editing has 
>obvious opportunities for mistakes so I attached the whole thing. I 
>figured if the committer was overwhelmed they'd speak up and I'd either 
>annotate the diff or just explain whatever part they were unsure about.
>  
>
The main reason I mentioned it is because I almost didn't realize that 
your patch fixed an issue for which I had already posted a patch.  I 
glanced over your ticket, but didn't notice the fix for the issue until 
the second time through--and I only scanned it a second time because of 
your reference to the patch in the comment you made in #679.  I figure 
since I only have a few patches in the system to worry about, it must be 
much more challenging for the devs.

And, since several of your changes look useful, I'd really like to see 
them go in sooner rather than later (or rather than not going in because 
the patch is too much work for the devs to deal with).

Mike