Mailing List Archive

_____________________________
From: David Hampton <mythtv@dhampton.net>
Sent: Thursday, December 22, 2016 4:32 am
Subject: [mythtv] Ignore 'The|A|An' prefixes when sorting video directories
To: <mythtv-dev@mythtv.org>


Hi,

I'm a longtime (10+ year) user of MythTv, but I think this is my first
attempt at submitting a patch. The video library code currently ignores
the 'The|A|An' prefixes when sorting video names, but it doesn't do so
when sorting directory names. This is most noticeable for me in the TV
section of my video library where the files are all grouped into
folders by show name and season. I have a couple of line patch to 0.28
that adds support for ignoring common prefixes on directories. I wanted
to run it past this list to see if its acceptable before creating a
Trac entry. Thanks.

David


diff --git a/mythtv/programs/mythfrontend/videolist.cpp
b/mythtv/programs/mythfrontend/videolist.cpp
index 7bcbda0..926c0a8 100644
--- a/mythtv/programs/mythfrontend/videolist.cpp
+++ b/mythtv/programs/mythfrontend/videolist.cpp
@@ -195,8 +195,9 @@ struct metadata_path_sort
 
     bool sort(const QString &lhs, const QString &rhs)
     {
-        QString lhs_comp(lhs);
-        QString rhs_comp(rhs);
+        const QRegExp prefixes = QRegExp(QObject::tr("^(The |A |An
)"));
+        QString lhs_comp = QString(lhs).remove(prefixes);
+        QString rhs_comp = QString(rhs).remove(prefixes);
         if (m_ignore_case)
         {
             lhs_comp = lhs_comp.toLower();
_______________________________________________
The issue with your patch is that it will only work if you use A or An. Any capitalisations will cause it to not work (eg an allied directory)You need to remove the strings after performing the comp.to.lower operation. 

Well done. A "feature" much desired in my household.


On 22/12/2016 5:01 PM, Jean-Yves Avenard wrote:
>
> _____________________________
> From: David Hampton <mythtv@dhampton.net <mailto:mythtv@dhampton.net>>
> Sent: Thursday, December 22, 2016 4:32 am
> Subject: [mythtv] Ignore 'The|A|An' prefixes when sorting video
> directories
> To: <mythtv-dev@mythtv.org <mailto:mythtv-dev@mythtv.org>>
>
>
> Hi,
>
> I'm a longtime (10+ year) user of MythTv, but I think this is my first
> attempt at submitting a patch. The video library code currently ignores
> the 'The|A|An' prefixes when sorting video names, but it doesn't do so
> when sorting directory names. This is most noticeable for me in the TV
> section of my video library where the files are all grouped into
> folders by show name and season. I have a couple of line patch to 0.28
> that adds support for ignoring common prefixes on directories. I wanted
> to run it past this list to see if its acceptable before creating a
> Trac entry. Thanks.
>
> David
>
>
> diff --git a/mythtv/programs/mythfrontend/videolist.cpp
> b/mythtv/programs/mythfrontend/videolist.cpp
> index 7bcbda0..926c0a8 100644
> --- a/mythtv/programs/mythfrontend/videolist.cpp
> +++ b/mythtv/programs/mythfrontend/videolist.cpp
> @@ -195,8 +195,9 @@ struct metadata_path_sort
>
> bool sort(const QString &lhs, const QString &rhs)
> {
> - QString lhs_comp(lhs);
> - QString rhs_comp(rhs);
> + const QRegExp prefixes = QRegExp(QObject::tr("^(The |A |An
> )"));
> + QString lhs_comp = QString(lhs).remove(prefixes);
> + QString rhs_comp = QString(rhs).remove(prefixes);
> if (m_ignore_case)
> {
> lhs_comp = lhs_comp.toLower();
> _______________________________________________
>
> The issue with your patch is that it will only work if you use A or
> An. Any capitalisations will cause it to not work (eg an allied directory)
> You need to remove the strings after performing the comp.to.lower
> operation.
>
>
>

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

On 22/12/16 06:01, Jean-Yves Avenard wrote:
>
> _____________________________
> From: David Hampton <mythtv@dhampton.net <mailto:mythtv@dhampton.net>>
> Sent: Thursday, December 22, 2016 4:32 am
> Subject: [mythtv] Ignore 'The|A|An' prefixes when sorting video directories
> To: <mythtv-dev@mythtv.org <mailto:mythtv-dev@mythtv.org>>
>
>
> Hi,
>
> I'm a longtime (10+ year) user of MythTv, but I think this is my first
> attempt at submitting a patch. The video library code currently ignores
> the 'The|A|An' prefixes when sorting video names, but it doesn't do so
> when sorting directory names. This is most noticeable for me in the TV
> section of my video library where the files are all grouped into
> folders by show name and season. I have a couple of line patch to 0.28
> that adds support for ignoring common prefixes on directories. I wanted
> to run it past this list to see if its acceptable before creating a
> Trac entry. Thanks.
>
> David
>
>
> diff --git a/mythtv/programs/mythfrontend/videolist.cpp
> b/mythtv/programs/mythfrontend/videolist.cpp
> index 7bcbda0..926c0a8 100644
> --- a/mythtv/programs/mythfrontend/videolist.cpp
> +++ b/mythtv/programs/mythfrontend/videolist.cpp
> @@ -195,8 +195,9 @@ struct metadata_path_sort
>
> bool sort(const QString &lhs, const QString &rhs)
> {
> - QString lhs_comp(lhs);
> - QString rhs_comp(rhs);
> + const QRegExp prefixes = QRegExp(QObject::tr("^(The |A |An
> )"));
> + QString lhs_comp = QString(lhs).remove(prefixes);
> + QString rhs_comp = QString(rhs).remove(prefixes);
> if (m_ignore_case)
> {
> lhs_comp = lhs_comp.toLower();
> _______________________________________________
>
> The issue with your patch is that it will only work if you use A or An.
> Any capitalisations will cause it to not work (eg an allied directory)
> You need to remove the strings after performing the comp.to.lower
> operation.
>
>

There is a modifier you can pass to the QRegExp to make it do
case insensitive regexp's which will solve this issue.


Regards
Stuart

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

On Thu, 2016-12-22 at 11:23 +0000, Stuart Auchterlonie wrote:
> On 22/12/16 06:01, Jean-Yves Avenard wrote:
> >
> > _____________________________
> > From: David Hampton <mythtv@dhampton.net <mailto:mythtv@dhampton.ne
> > t>>
> > Sent: Thursday, December 22, 2016 4:32 am
> > Subject: [mythtv] Ignore 'The|A|An' prefixes when sorting video
> > directories
> > To: <mythtv-dev@mythtv.org <mailto:mythtv-dev@mythtv.org>>
> >
> >
> > Hi,
> >
> > I'm a longtime (10+ year) user of MythTv, but I think this is my
> > first
> > attempt at submitting a patch. The video library code currently
> > ignores
> > the 'The|A|An' prefixes when sorting video names, but it doesn't do
> > so
> > when sorting directory names. This is most noticeable for me in the
> > TV
> > section of my video library where the files are all grouped into
> > folders by show name and season. I have a couple of line patch to
> > 0.28
> > that adds support for ignoring common prefixes on directories. I
> > wanted
> > to run it past this list to see if its acceptable before creating a
> > Trac entry. Thanks.
> >
> > David
> >
> >
> > diff --git a/mythtv/programs/mythfrontend/videolist.cpp
> > b/mythtv/programs/mythfrontend/videolist.cpp
> > index 7bcbda0..926c0a8 100644
> > --- a/mythtv/programs/mythfrontend/videolist.cpp
> > +++ b/mythtv/programs/mythfrontend/videolist.cpp
> > @@ -195,8 +195,9 @@ struct metadata_path_sort
> >  
> >      bool sort(const QString &lhs, const QString &rhs)
> >      {
> > -        QString lhs_comp(lhs);
> > -        QString rhs_comp(rhs);
> > +        const QRegExp prefixes = QRegExp(QObject::tr("^(The |A |An
> > )"));
> > +        QString lhs_comp = QString(lhs).remove(prefixes);
> > +        QString rhs_comp = QString(rhs).remove(prefixes);
> >          if (m_ignore_case)
> >          {
> >              lhs_comp = lhs_comp.toLower();
> > _______________________________________________
> >
> > The issue with your patch is that it will only work if you use A or
> > An.
> > Any capitalisations will cause it to not work (eg an allied
> > directory)
> > You need to remove the strings after performing the comp.to.lower
> > operation. 
> >
> >
>
> There is a modifier you can pass to the QRegExp to make it do
> case insensitive regexp's which will solve this issue.

Thanks. I added the Qt::CaseInsensitive argument, retested, and filed a
ticket.

David

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

On 12/22/2016 01:29 PM, David Hampton wrote:
> On Thu, 2016-12-22 at 11:23 +0000, Stuart Auchterlonie wrote:
>> On 22/12/16 06:01, Jean-Yves Avenard wrote:
>>> _____________________________
>>> From: David Hampton <mythtv@dhampton.net <mailto:mythtv@dhampton.ne
>>> t>>
>>> Sent: Thursday, December 22, 2016 4:32 am
>>> Subject: [mythtv] Ignore 'The|A|An' prefixes when sorting video
>>> directories
>>> To: <mythtv-dev@mythtv.org <mailto:mythtv-dev@mythtv.org>>
>>>
>>>
>>> Hi,
>>>
>>> I'm a longtime (10+ year) user of MythTv, but I think this is my
>>> first
>>> attempt at submitting a patch. The video library code currently
>>> ignores
>>> the 'The|A|An' prefixes when sorting video names, but it doesn't do
>>> so
>>> when sorting directory names. This is most noticeable for me in the
>>> TV
>>> section of my video library where the files are all grouped into
>>> folders by show name and season. I have a couple of line patch to
>>> 0.28
>>> that adds support for ignoring common prefixes on directories. I
>>> wanted
>>> to run it past this list to see if its acceptable before creating a
>>> Trac entry. Thanks.
>>>
>>> David
>>>
>>>
>>> diff --git a/mythtv/programs/mythfrontend/videolist.cpp
>>> b/mythtv/programs/mythfrontend/videolist.cpp
>>> index 7bcbda0..926c0a8 100644
>>> --- a/mythtv/programs/mythfrontend/videolist.cpp
>>> +++ b/mythtv/programs/mythfrontend/videolist.cpp
>>> @@ -195,8 +195,9 @@ struct metadata_path_sort
>>>
>>> bool sort(const QString &lhs, const QString &rhs)
>>> {
>>> - QString lhs_comp(lhs);
>>> - QString rhs_comp(rhs);
>>> + const QRegExp prefixes = QRegExp(QObject::tr("^(The |A |An
>>> )"));
>>> + QString lhs_comp = QString(lhs).remove(prefixes);
>>> + QString rhs_comp = QString(rhs).remove(prefixes);
>>> if (m_ignore_case)
>>> {
>>> lhs_comp = lhs_comp.toLower();
>>> _______________________________________________
>>>
>>> The issue with your patch is that it will only work if you use A or
>>> An.
>>> Any capitalisations will cause it to not work (eg an allied
>>> directory)
>>> You need to remove the strings after performing the comp.to.lower
>>> operation.
>>>
>>>
>> There is a modifier you can pass to the QRegExp to make it do
>> case insensitive regexp's which will solve this issue.
> Thanks. I added the Qt::CaseInsensitive argument, retested, and filed a
> ticket.

Haven't looked myself, but is yours done the same way it's done for the
videos themselves and in Watch Recordings? Would be nice to make sure
they're all the same--even if the same is "wrong"--for the principle of
least surprise.

So, basically, IMHO, the patch to add this feature to should make it
work like the rest. If the rest are case-sensitive matching, a separate
patch should be made to change all of them to be case-insensitive matching.

Mike
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

On Fri, 2016-12-23 at 13:28 -0500, Michael T. Dean wrote:
> On 12/22/2016 01:29 PM, David Hampton wrote:
> > On Thu, 2016-12-22 at 11:23 +0000, Stuart Auchterlonie wrote:
> > > On 22/12/16 06:01, Jean-Yves Avenard wrote:
> > > > _____________________________
> > > > From: David Hampton <mythtv@dhampton.net <mailto:mythtv@dhampto
> > > > n.ne
> > > > t>>
> > > > Sent: Thursday, December 22, 2016 4:32 am
> > > > Subject: [mythtv] Ignore 'The|A|An' prefixes when sorting video
> > > > directories
> > > > To: <mythtv-dev@mythtv.org <mailto:mythtv-dev@mythtv.org>>
> > > >
> > > >
> > > > Hi,
> > > >
> > > > I'm a longtime (10+ year) user of MythTv, but I think this is
> > > > my
> > > > first
> > > > attempt at submitting a patch. The video library code currently
> > > > ignores
> > > > the 'The|A|An' prefixes when sorting video names, but it
> > > > doesn't do
> > > > so
> > > > when sorting directory names. This is most noticeable for me in
> > > > the
> > > > TV
> > > > section of my video library where the files are all grouped
> > > > into
> > > > folders by show name and season. I have a couple of line patch
> > > > to
> > > > 0.28
> > > > that adds support for ignoring common prefixes on directories.
> > > > I
> > > > wanted
> > > > to run it past this list to see if its acceptable before
> > > > creating a
> > > > Trac entry. Thanks.
> > > >
> > > > David
> > > >
> > > >
> > > > diff --git a/mythtv/programs/mythfrontend/videolist.cpp
> > > > b/mythtv/programs/mythfrontend/videolist.cpp
> > > > index 7bcbda0..926c0a8 100644
> > > > --- a/mythtv/programs/mythfrontend/videolist.cpp
> > > > +++ b/mythtv/programs/mythfrontend/videolist.cpp
> > > > @@ -195,8 +195,9 @@ struct metadata_path_sort
> > > >   
> > > >       bool sort(const QString &lhs, const QString &rhs)
> > > >       {
> > > > -        QString lhs_comp(lhs);
> > > > -        QString rhs_comp(rhs);
> > > > +        const QRegExp prefixes = QRegExp(QObject::tr("^(The |A
> > > > |An
> > > > )"));
> > > > +        QString lhs_comp = QString(lhs).remove(prefixes);
> > > > +        QString rhs_comp = QString(rhs).remove(prefixes);
> > > >           if (m_ignore_case)
> > > >           {
> > > >               lhs_comp = lhs_comp.toLower();
> > > > _______________________________________________
> > > >
> > > > The issue with your patch is that it will only work if you use
> > > > A or
> > > > An.
> > > > Any capitalisations will cause it to not work (eg an allied
> > > > directory)
> > > > You need to remove the strings after performing the
> > > > comp.to.lower
> > > > operation.
> > > >
> > > >
> > >
> > > There is a modifier you can pass to the QRegExp to make it do
> > > case insensitive regexp's which will solve this issue.
> >
> > Thanks. I added the Qt::CaseInsensitive argument, retested, and
> > filed a
> > ticket.
>
> Haven't looked myself, but is yours done the same way it's done for
> the videos themselves and in Watch Recordings?  Would be nice to make
> sure they're all the same--even if the same is "wrong"--for the
> principle of least surprise.
>
> So, basically, IMHO, the patch to add this feature to should make it 
> work like the rest.  If the rest are case-sensitive matching, a
> separate patch should be made to change all of them to be case-
> insensitive matching.

Its inconsistent today. There's one instance in libmythmetadata which
conditionally does case sensitive or case insensitive comparisons,
although all of its current callers request case insensitive
comparisons. There are three usages in mythfrontend (playbackbox.cpp,
proglist.cpp and programrecpriority.cpp), all of which are hard coded
to use case sensitive comparisons.

The metadata_path_sort structure in videolist.c that I was extending
already had a m_ignore_case boolean (which I should have used),
although all its current callers specify case insensitive comparisons.
I will update my patch to be case sensitive/insensitive based on the
existing m_ignore_case variable.

In order to be fully consistent, I would need to modify the three
regexps in the frontend code to be case insensitive. It would be nice
if all comparisons could all be keyed off the same variable, but I
can't find a good place for it. There doesn't seem to be any common
parent structure where it could go. The three frontend classes are all
derived from ScheduleCommon, while the callers to the other two
instances are from the VideoListImp class that has no parent. Would it
make more sense to make this a global variable in the frontend code? I
just started looking at globalsettings.h/cpp files which might be a
possible location to put this boolean, although it looks like I'm
drifting into code that creates the settings pages. Does this line of
thought make sense and am I looking in the right direction?

David

P.S. I know there's bug report (#12298) that says that sorting should
never ignore prefixes. I was thinking that an exception list would
solve the reported problem (i.e. don't strip prefixes from the string
"A to Z") while stripping prefixes for the normal case. I was
considering this as a follow-on project, although passing a list of
strings into the various sorting functions seems messier than the
current code that only passes a single boolean.

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

Quoting David Hampton <mythtv@dhampton.net>:

> P.S. I know there's bug report (#12298) that says that sorting should
> never ignore prefixes. I was thinking that an exception list would
> solve the reported problem (i.e. don't strip prefixes from the string
> "A to Z") while stripping prefixes for the normal case. I was
> considering this as a follow-on project, although passing a list of
> strings into the various sorting functions seems messier than the
> current code that only passes a single boolean.
>

I've added a comment to #12298 that outlines a way to fix this.
Basically it punts the problem outside of the MythTV code by adding an
extra field to the database that is used for simple sorting. The extra
field should be copied from the title and then it can be manipulated
by a script to remove the prefixes and all the complexity of what to
remove and what not to remove can be in one place and it only has to
run once per show rather than every time a list is produced.

Cheers,
Tim.


_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

On Fri, 2017-01-06 at 13:02 +0000, mythtv@phipps-hutton.freeserve.co.uk
wrote:
> Quoting David Hampton <mythtv@dhampton.net>:
>
> > P.S. I know there's bug report (#12298) that says that sorting
> > should
> > never ignore prefixes. I was thinking that an exception list would
> > solve the reported problem (i.e. don't strip prefixes from the
> > string
> > "A to Z") while stripping prefixes for the normal case. I was
> > considering this as a follow-on project, although passing a list of
> > strings into the various sorting functions seems messier than the
> > current code that only passes a single boolean.
> >
>
> I've added a comment to #12298

FYI I don't see a comment from you there (last comment is #1 from Gary
2 years ago). Perhaps one more button click required?

> that outlines a way to fix this.  
> Basically it punts the problem outside of the MythTV code by adding an  
> extra field to the database that is used for simple sorting. The extra  
> field should be copied from the title and then it can be manipulated  
> by a script to remove the prefixes and all the complexity of what to  
> remove and what not to remove can be in one place and it only has to  
> run once per show rather than every time a list is produced.

Maybe eventually things like xmltv and the metadata grabbers could
provide this "sort order" field alongside the normal name if the
datasource they are consuming includes it.

Ian.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

On Fri, 2017-01-06 at 13:02 +0000, mythtv@phipps-hutton.freeserve.co.uk
wrote:
> Quoting David Hampton <mythtv@dhampton.net>:
>
> > P.S. I know there's bug report (#12298) that says that sorting
> > should
> > never ignore prefixes. I was thinking that an exception list would
> > solve the reported problem (i.e. don't strip prefixes from the
> > string
> > "A to Z") while stripping prefixes for the normal case. I was
> > considering this as a follow-on project, although passing a list of
> > strings into the various sorting functions seems messier than the
> > current code that only passes a single boolean.
> >
>
> I've added a comment to #12298 that outlines a way to fix this.  
> Basically it punts the problem outside of the MythTV code by adding
> an extra field to the database that is used for simple sorting. The
> extra
> field should be copied from the title and then it can be manipulated 
> by a script to remove the prefixes and all the complexity of what to 
> remove and what not to remove can be in one place and it only has to 
> run once per show rather than every time a list is produced.

I like the idea of having a title and a "sort title" in the database
and that would cleanly handle a number of problems, but it doesn't
address my initial problem of directory sorting. I know I could just
rename my directories to force a sort order, but doing that triggers a
different bug where the renamed directories never show a cover image
(presumably because they don't match anything in the database). I end
up having to add a folder.jpg icon into each renamed directory to get
them to display properly.

David

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org