New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use sort tags in paned browser; union tied tags in paned browser #1796

Merged
merged 16 commits into from Feb 14, 2016

Conversation

Projects
None yet
2 participants
@pfps
Contributor

pfps commented Jan 21, 2016

Code changes:

1/ list_separate and list_sort (new) in quodlibet/formats/_audio.py
Only uses are in the paned browser

a) Union all results for tied tags instead of concatenating them in list_separate. (This is
a change, but it only affects the paned browser.)

b) Return display,sort pairs when a tag has an associated sort key with
different values.

2/ list_separate and format_list in quodlibet/pattern/_pattern.py
Only uses are in the paned browser

Handle display,sort pairs appropriately.

3/ paned browser

Use sort value for sorting if present.

User-visible changes:

1/ Paned browser uses sort tags when present.
2/ Paned browser unions results for tied tags.

Caveats:

1/ If a tag value has different sort values (including no sort value) in
different songs then in panes using the tag songs with this tag value will
sort to one of the sort values at random and may have more than one entry.
2/ If multiple tag values have the same sort value then in panes using the
tag sorting of these tag values will be non-deterministic and there may
be multiple entries for some of the tag values. (This problem already
existed.)
3/ Some changes to documentation are indicated.

@pfps pfps referenced this pull request Jan 21, 2016

Closed

sorting in paned browser #1785

In case of tied tags the result will be unicode, otherwise
it returns the same as list()
def list_sort(self, key):

This comment has been minimized.

@lazka

lazka Jan 22, 2016

Member

Please add a test for list_sort()

This comment has been minimized.

@pfps

pfps Feb 2, 2016

Contributor

Done

if len(display) > 0 and len(sort) > 0 and display != sort:
return [((d if d != "" else s, s) if s != "" else d)
for d, s in izip_longest(display, sort, fillvalue="")]
else:

This comment has been minimized.

@lazka

lazka Jan 22, 2016

Member

I think it would be nicer to just always return a list of tuples even if there are no sort values. Also throw out sort values if there are too many.

This comment has been minimized.

@pfps

pfps Jan 22, 2016

Contributor

I was trying to be somewhat efficient, i.e., if there are no useful sort
values, then don't go through the bother of creating both the display and sort
versions. The consuming code already handled the mixed situation, because
that is what comes out of the pattern code, so there was no extra downstream
work (except a little bit at the very end) to do things this way.

I also wanted to distinguish between the situation where there is a sort value
and where there isn't so that a subsequent sort value (from a different song,
like where "The Beatles" is sometimes missing the "Beatles, The" sort value)
could be used in the paned browser.

As far as using extra sort values, I was thinking that extra information
should not be dropped. If they are truely unneeded then they can be removed
from the data.

If you don't think that either of these are useful just let me know and I'll
make the changes. (Both should be easy.)

peter

PS: I noticed that using izip doesn't work in Python 3. Should I include
the compatability magic to make later migration to Python 3 easier?

On 01/22/2016 10:00 AM, Christoph Reiter wrote:

In quodlibet/quodlibet/formats/_audio.py
#1796 (comment):

     """
  •    display = decode_value(key, self.**call**(key))
    
  •    display = display.split("\n") if display else []
    
  •    sort = []
    
  •    if key in TAG_TO_SORT:
    
  •        sort = decode_value(TAG_TO_SORT[key],
    
  •                            self.**call**(TAG_TO_SORT[key]))
    
  •        # it would be better to use something that doesn't fall back
    
  •        # to the key itself, but what?
    
  •        sort = sort.split("\n") if sort else []
    
  •    if len(display) > 0 and len(sort) > 0 and display != sort:
    
  •        return [((d if d != "" else s, s) if s != "" else d)
    
  •                for d, s in izip_longest(display, sort, fillvalue="")]
    
  •    else:
    

I think it would be nicer to just always return a list of tuples even if there
are no sort values. Also throw out sort values if there are too many.


Reply to this email directly or view it on GitHub
https://github.com/quodlibet/quodlibet/pull/1796/files#r50568841.

This comment has been minimized.

@lazka

lazka Jan 22, 2016

Member

I was trying to be somewhat efficient, i.e., if there are no useful sort
values, then don't go through the bother of creating both the display and sort
versions. The consuming code already handled the mixed situation, because
that is what comes out of the pattern code, so there was no extra downstream
work (except a little bit at the very end) to do things this way.

I'd prefer the pattern to output tuple lists as well. Makes things more readable and less likely to make errors in handling the output. If we have a use case were only display versions are needed we can add another method just for that in the future.

I also wanted to distinguish between the situation where there is a sort value
and where there isn't so that a subsequent sort value (from a different song,
like where "The Beatles" is sometimes missing the "Beatles, The" sort value)
could be used in the paned browser.

If this isn't needed in this case I'd ignore it and take empty strings as "no sort tag".

As far as using extra sort values, I was thinking that extra information
should not be dropped. If they are truely unneeded then they can be removed
from the data.

If we need it in the future we can easily change the code then. Maybe add the limitations regarding different value count in the docstring.

If you don't think that either of these are useful just let me know and I'll
make the changes. (Both should be easy.)

peter

PS: I noticed that using izip doesn't work in Python 3. Should I include
the compatability magic to make later migration to Python 3 easier?

Sure, why not.

This comment has been minimized.

@pfps

pfps Feb 2, 2016

Contributor

Done

"""
display = decode_value(key, self.__call__(key))

This comment has been minimized.

@lazka

lazka Jan 22, 2016

Member

note that self.__call__() is the same as self()

This comment has been minimized.

@pfps

pfps Jan 22, 2016

Contributor

OK, fixed (in two places).

peter

On 01/22/2016 10:00 AM, Christoph Reiter wrote:

In quodlibet/quodlibet/formats/_audio.py
#1796 (comment):

     """
  •    display = decode_value(key, self.**call**(key))
    

note that |self.call()| is the same as |self()|


Reply to this email directly or view it on GitHub
https://github.com/quodlibet/quodlibet/pull/1796/files#r50568908.

@lazka

This comment has been minimized.

Member

lazka commented Feb 14, 2016

Any update on this?

@pfps

This comment has been minimized.

Contributor

pfps commented Feb 14, 2016

Is the ball in my court? The last change I made was to always return pairs and ignore extra sort values, as requested. This didn't change any observable behaviour.

I have been running with the modification, with no problems. The effects of the changes are confined to the paned browser (unless add-ons can access this far down into quodlibet). I think that the changes are ready to go.

If the change is made, there probably should be some updates to the documentation. I'm willing to propose some updates. (How would this be done, by the way?)

@lazka

This comment has been minimized.

Member

lazka commented Feb 14, 2016

I assumed it's not done since I saw some isinstance(tuple) checks. i'll have another look.

@pfps

This comment has been minimized.

Contributor

pfps commented Feb 14, 2016

Hmm. Some of these are defensive, just in case a non-pair leaks through. However, format_list might need some work. I'll take a look.

@pfps

This comment has been minimized.

Contributor

pfps commented Feb 14, 2016

The instance(..., tuple) checks in models.py are defensive. If a non-tuple
leaks through, these checks do the right thing with it.

Some checks in format_list are needed. The output from a pattern execution
contains non-pairs for literal strings, which have to be handled correctly.
format_list did not turn these strings into pairs. I have added a new patch
which does the conversion so that format_list always returns pairs. This
simplified the logic in format_list, but does consume some extra resources (to
build up the pairs when they would not otherwise be needed). I added an
explicit test for a pattern with just a literal string, and adjusted the
other test where a non-pair was being generated.

The special list_separate function inside the pattern function in
util/init.py is used to generate column headings. This does not
generate display,sort pairs, but that's OK as it is never used in a setting
where pairs can happen.

@pfps

This comment has been minimized.

Contributor

pfps commented Feb 14, 2016

The failure appears to be that some resource is not currently available. This should be temporary. Is there a way to rerun the test?

@lazka

This comment has been minimized.

Member

lazka commented Feb 14, 2016

@pfps

This comment has been minimized.

Contributor

pfps commented Feb 14, 2016

Aah.

The reason is running the results of format_list through _post. Should this be done for format_list at all?
I was only doing that for the display string, not the sort string. I will put in a patch to _post for both display and sort, which should "fix" the testing, but someone who understands what _post is supposed to be doing to figure out whether _post is a good idea for format_list, even before the changes in this pull request.

@lazka

This comment has been minimized.

Member

lazka commented Feb 14, 2016

_post on both seems good.

@pfps

This comment has been minimized.

Contributor

pfps commented Feb 14, 2016

OK, _post is on both display and sort, and all the required tests look good. I'm running the new patch locally and the results look fine. I have lots of songs with both sort values and without and the separating and sorting all looks correct even for complex patterns in the paned browser.

lazka added a commit that referenced this pull request Feb 14, 2016

Merge pull request #1796 from pfps/master
use sort tags in paned browser; union tied tags in paned browser

@lazka lazka merged commit 6f013c5 into quodlibet:master Feb 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lazka

This comment has been minimized.

Member

lazka commented Feb 14, 2016

Thanks!

@pfps

This comment has been minimized.

Contributor

pfps commented Feb 14, 2016

Yeah, well, I did this mostly to fix something that was bugging me!

I'll take a look at the documentation and suggest changes there. Should I just open an issue for that?

@lazka

This comment has been minimized.

Member

lazka commented Feb 14, 2016

Whatever is easier for you.

@pfps

This comment has been minimized.

Contributor

pfps commented Feb 14, 2016

It looks as if I can just do a pull request. I'll try that.

(I should have made the doc change part of this pull request, but I guess it's too late now.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment