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

File: Add feature to file module for sorting files vs directories. #1595

Merged
merged 4 commits into from Jun 14, 2017

Conversation

6 participants
@ghost

ghost commented Jan 23, 2017

Add configuration option in [file] section of mopidy.conf
Add documentation for feature with the valid options.

Signed-off-by: caysho@internode.on.net

mopidy-dev
File: Add feature to file module for sorting files vs directories.
Add configuration option in [file] section of mopidy.conf
Add documentation for feature with the valid options.

Signed-off-by: caysho@internode.on.net
@sumpfralle

This comment has been minimized.

Contributor

sumpfralle commented Jan 27, 2017

Thank you for this useful proposal!

I am not too familiar with the mopidy core code, thus take my comments with a pinch of salt.

You are currently using three distinct (and mutually exclusive) flags for the sort order (_sort_mixed, _sort_directories_first and _sort_files_first). I assume, that it could improve the readability of the code to turn this into a single variable with multiple states (mixed, directories_first, files_first)?

Thinking aloud: maybe the configuration approach of the vim regarding its wildmode setting (using a list of sort order flags) could be more future-proof than single distinct keywords?
I could imagine the following:

# default (current behaviour => "mixed")
sort_order: name
# primary: directories before files; secondary: names
# for you: "DirectoriesFirst"
sort_order: directories_before_files, name
# primary: files before directories; secondary: names
# for you: "FilesFirst"
sort_order: !directories_before_files, name

This approach would allow future additional search orders (e.g. timestamps, album_year, ...).

Since python uses a stable sorting algorithm, the implementation would be rather simple:

def __init__(...):
    ...
    self._sort_funcs = []
    sort_order_key_func_map = {
        "directories_before_files": lambda item: 1 if isinstance(item, models.Ref.track) else 0,
        "name": operator.attrgetter('name'),
    }
    for sort_key in self._sort_order.split(","):
        reverse = sort_key.startswith("!")
        key_func = sort_order_key_func_map[sort_key.lstrip("!")]
        self._sort_funcs.append((key_func, reverse))

def browse(...):
    ...
    # going through the sort functions from least to most significant
    # Python's stable sorting guarantees that the less significant sort orders are maintained
    for key_func, reverse in reversed(self._sort_funcs):
        result.sort(key=key_func, reverse=reverse)

Just the schema specification (comma separated list of a set of strings) would probably need some thought.

Anyway: I am sorry for jumping into your train of thoughts. Feel free to ignore my comments.

@ghost

This comment has been minimized.

ghost commented Jan 28, 2017

Note this change is only looking at the File browser. The artist, album etc are not applicable, only the directories and files.

I spent much of the afternoon playing with this (I am just getting into Python). This technique is new to me :) The multiple levels of sorting is neat

Unfortunately, I had no luck with the models.Ref.xxxx part in the lambda.
I am pretty sure the isinstance function is not being passed correct data.

I finally ended up putting in some debug logging:
type(item) returns <class 'mopidy.models.Ref'>
type(item).__name__ returns Ref
type(models.Ref.directory) returns <type 'instancemethod'>
type(models.Ref.directory).__name__ returns instancemethod
type(item.DIRECTORY).__name__ returns unicode

The last one gave me the clue about what was happening.
item.type returns directory or track.

If the lambda is changed to the following:
"directory": lambda item: True if item.type == 'directory' else False,
the sorting then works but it is in reverse when the conf item is listed as directory, name. Using !directory, name gives the DirectoriesFirst result.

Any ideas on what is going on here ?

@sumpfralle

This comment has been minimized.

Contributor

sumpfralle commented Jan 29, 2017

Thank you for joining my train of thoughts! :)

Your observation regarding the object types (directory, track) clarifies my wrong assumption about how to distinguish them. Your approach looks good to me.
I took a look at mopidy/models/__init__.py: here you can find symbol names for the different types. Thus your comparison would be even better with: item.type == mopidy.models.Ref.DIRECTORY.
Regarding the improper sorting that you noticed: this is just based on the order of the boolean values: False < True. That's why I used 1 and 0 in my example (but you are right: this is indeed confusing). Do you have another idea, which sortable items we should use?

@ghost

This comment has been minimized.

ghost commented Jan 29, 2017

Using item.type == models.Ref.DIRECTORY works. The following is the new statement:

'directory': lambda item: 0 if item.type == models.Ref.DIRECTORY else 1

These are the two sortable items for the browsing that I see - the relevant code is at file.library.browse where the os.path.isdir and os.path.isfile are evaluated.

Other sortable items are used by the local backend (presented as Local media), not file. Refer Mopidy-Local.

Based on this, I would prefer having the configuration item as a string instead of a list to reduce validation requirements.
The sort list can be assigned in code:

self._sort_order_list = []
default_sort_order = ['name']
if self._sort_order == 'FilesFirst':
    self._sort_order_list = ['!directory'] + default_sort_order
if self._sort_order == 'DirectoriesFirst':
    self._sort_order_list = ['directory'] + default_sort_order
if not self._sort_order_list:
    self._sort_order_list = default_sort_order
@ismailof

This comment has been minimized.

Contributor

ismailof commented Jan 29, 2017

Just for python clarity I would suggest to reduce the last code lines as follows:

self._sort_order_list = ['name']
if self._sort_order == 'FilesFirst':
    self._sort_order_list.insert(0, '!directory')
elif self._sort_order == 'DirectoriesFirst':
    self._sort_order_list.insert(0, 'directory')
@sumpfralle

This comment has been minimized.

Contributor

sumpfralle commented Jan 29, 2017

Yeah - that looks better now!

Personally I would still lean towards the comma separated configuration style, since it will be easier to extend without changing the configuration specification.

How about using the List schema combined with the choice validator?

from mopidy.config.validators import validate_choice
...
schema['sort_order'] = config.List(optional=True)
...
self._sort_order_list = []
default_sort_order = ['name']
valid_sort_keys = ["directories_before_files", "name"]
# allow the prefix "!" for all sort keys
valid_sort_keys += ["!" + key for key in valid_sort_keys]
for sort_key in self._sort_order:
    validate_choice(sort_key, valid_sort_keys)
    self._sort_order_list.append(sort_key)
if not self._sort_order_list:
    self._sort_order_list = default_sort_order

Anyway: I do not really feel entitled to judge this (I am just the maintainer of a mopidy extension).

Any other opinions?

@ghost

This comment has been minimized.

ghost commented Jan 30, 2017

@ismailof Yes, that was lazy of me. I was thinking in terms of two lists, not list items.

However, I think this last suggestion from @sumpfralle is the way to. I thought the list validation would be more involved.
I expect there will only be the two keys, but I like the idea of being able to reverse the direction of both keys via configuration.
It is something I had not thought of, because I never sort directories in reverse alphabetical order, but others might.

Any further suggestions ?

@adamcik

This comment has been minimized.

Member

adamcik commented Jan 30, 2017

Sorry for being late to the party, but do we needs this to be configurable at all? Why not just make it directories first and sorted alphabetically beyond that?

If someone wants to have more fancy sorting the client could support it, even if that means each the clients redoing the sorting.

Thoughts?

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jan 30, 2017

I think this is a really good addition but I do agree with @adamcik, not everything needs to be configurable. Having too many options just obscures the ones that do matter (getting people to read the docs is hard enough) and can introduce unnecessary code complexity.

@sumpfralle

This comment has been minimized.

Contributor

sumpfralle commented Jan 30, 2017

I would also assume that the sorting ["directories_before_files", "name"] is the expected behaviour.

@tkem

This comment has been minimized.

Member

tkem commented Jan 31, 2017

I agree with @adamcik - after all, this is a default sort order and clients are free to reorder browse results in any way they want, so configuring this on the server may not even have any visible effect for end users (possibly triggering even more questions/issues if this is made configurable).
For clients which display browse results "as-is" (mpc, etc.) I think listing directories first would be desirable behavior.

Caysho
Discussion in mopidy PR 1595: remove sort order configuration item.
Browse/File sort order to be directories first, otherwise all items
alphabetically.
Final sort order to be left to clients.

Change mopidy/file/library.py to sort as described.

Remove changes to:
1. docs/ext/file.rst
2. mopidy/file/__init__.py
3. mopidy/file/ext.conf

Thanks go to sumpfralle for providing the base code for the sort lambda
and stable sort suggestion.

Signed-off-by: Caysho <caysho@internode.on.net>
@ghost

This comment has been minimized.

ghost commented Feb 1, 2017

OK, this makes the changes needed much smaller :)
I have added commit 0e1e703

@tkem

This comment has been minimized.

Member

tkem commented on mopidy/file/library.py in 0e1e703 Feb 1, 2017

Sorting multiple times here is needlessly inefficient - the key function could also return a tuple, for example

def order(item):
    return (item.type != models.Ref.DIRECTORY, item.name)
result.sort(key=order)
@@ -51,3 +51,4 @@ See :ref:`config` for general help on configuring Mopidy.
Number of milliseconds before giving up scanning a file and moving on to
the next file. Reducing the value might speed up the directory listing,
but can lead to some tracks not being shown.

This comment has been minimized.

@adamcik

adamcik Feb 2, 2017

Member

Remove this stray newline and the same in ext.conf? You might have to change your editor setting for this.

else 1]
logger.debug('sort_funcs %s', sort_funcs)
for key_func in sort_funcs:

This comment has been minimized.

@adamcik

adamcik Feb 2, 2017

Member

Doing it like this I you will end up first sorting it just by name, and then resorting it just by type == models.Ref.DIRECTORY. On a side note int(True) -> 1 and int(False) -> 0. With this in mind I think you want something like:

sorted([('directory', 'xyz'), ('directory', 'def'), ('directory', 'abc'), ('track', 'xyz'), ('track', 'def')], key=lambda v: (v[0] != 'directory', v[1]))

[('directory', 'abc'),
 ('directory', 'def'),
 ('directory', 'xyz'),
 ('track', 'def'),
 ('track', 'xyz')]

Which would give you result.sort(sort=lambda ref: (ref.type != models.Ref.DIRECTORY, ref.name)) as the sort function you need.

This comment has been minimized.

@sumpfralle

sumpfralle Feb 3, 2017

Contributor

Doing it like this I you will end up first sorting it just by name, and then resorting it just by type == models.Ref.DIRECTORY.

Yes - this leads to the desired effect (first directories sorted by name, then files sorted by name), due to the python's stable sorting algorithm (not changing the order of "equal" items in successive runs).

On a side note int(True) -> 1 and int(False) -> 0.

Yes, but I think the natural order of boolean values is not as obvious for the reader as the natural order of integer numbers. A matter of taste :)

Merging the successive sorting iterations into a single sorting run is reasonable, since the sorting priorities are not supposed to be configurable anymore.

This comment has been minimized.

@adamcik

adamcik Feb 3, 2017

Member

Doh, thanks for explaining that. Should have kept my original wording with "Doing it like this I think..." :-)

Caysho added some commits Feb 3, 2017

Caysho
mopidy/file/library.py
Improve sort efficiency with tkem's suggestion in PR 1595.

Remove
import operator
because operator.attrgetter() not used now.

mopidy/file/ext.conf
Remove errant new line character introduced in
commit 0e1e703.

Signed-off-by: Caysho <caysho@internode.on.net>
Caysho
docs/etc/file.rst
Remove trailing new line character

Signed-off-by: Caysho <caysho@internode.on.net>
@ghost

This comment has been minimized.

ghost commented Feb 5, 2017

I made the change suggested by @tkem, but I am not clear on where the redundant sorting was occurring.

I have read that lambda's are less efficient than named functions, so I did some basic timings, and there was about a 15% improvement on the previous code.
It was not particularly vigorous testing, but there is a difference.

@tkem

This comment has been minimized.

Member

tkem commented Feb 5, 2017

@Caysho: The issue I commented on was sorting two times, once based on name, and then a second time based on type. Sorting only once using a tuple as key may be more efficient, but will hardly make any difference for most (rather short) directory listings. Efficiency put aside, IMHO this also tends to be more readable and easier to verify than relying on Python's sort algorithm being stable, as other comments have shown ;-)
Whether you use a named order function or a lambda I consider a matter of personal taste/style - I'd doubt that would make any noticeable difference in this case, and I used a named function just for clarity here.

@adamcik adamcik merged commit d0ef2c4 into mopidy:develop Jun 14, 2017

1 check passed

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

@jodal jodal added this to the v2.2 milestone Mar 30, 2018

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