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 extension -- file browsing without scanning #1207

Merged
merged 17 commits into from Jul 9, 2015

Conversation

5 participants
@rawdlite
Contributor

rawdlite commented Jun 30, 2015

This time based on branch develop.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jun 30, 2015

There are a lot of debug messages in there, I don't think that many of them are useful. When importing unicode_literals you don't need to prefix your unicode strings with u. The default value for media_dir should not contain paths specific to your system.

EDIT:
Actually I take that back, some looks useful, there is just maybe some logging and debug stuff that should go before merging.

enabled = true
media_dir =
~/:Home
/data/music/music_data:Music

This comment has been minimized.

@adamcik

adamcik Jun 30, 2015

Member

$XDG_MUSIC_DIR is available if you make sure to use mopidy.internal.path.expand_path on the paths. Would make more sense to use this than to have a non-standard location as default.

def __init__(self, backend, config):
super(FilesLibraryProvider, self).__init__(backend)
self._media_dirs = []
# import pdb; pdb.set_trace()

This comment has been minimized.

@adamcik

adamcik Jun 30, 2015

Member

Stray debug line, please remove.

else:
media_dir['name'] = media_dict[0].replace(os.sep, '+')
self._media_dirs.append(media_dir)
logger.debug(self._media_dirs)

This comment has been minimized.

@adamcik

adamcik Jun 30, 2015

Member

Maybe provide more context in the debug logging, or just remove it?

self._scanner = scan.Scanner(
timeout=config['files']['metadata_timeout'])
def browse(self, uri, encoding=sys.getfilesystemencoding()):

This comment has been minimized.

@adamcik

adamcik Jun 30, 2015

Member

Why the extra encoding param? I don't think we will ever call this with anything more than the URI.

elif stat.S_ISDIR(st.st_mode):
result.append(models.Ref.directory(name=name, uri=uri))
elif stat.S_ISREG(st.st_mode) and self._check_audiofile(uri):
# if self._is_playlist(child):

This comment has been minimized.

@adamcik

adamcik Jun 30, 2015

Member

Remove the commented out code and/or add a TODO with what is missing.

This comment has been minimized.

@kingosticks

kingosticks Jun 30, 2015

Member

There's also the subject of the key-value list (aka dictionary) config.
I've seen that mopidy-local-sqlite has a very similar thing but does it
slightly differently (with spaces rather than ':' separator characters. Do
you think it's worth trying to standardise this sort of thing? Through a
new config type or just a convention?
On 30 Jun 2015 20:22, "Thomas Adamcik" notifications@github.com wrote:

In mopidy/files/library.py
#1207 (comment):

  •            logger.warn(u'Not in basedir: %s' % localpath)
    
  •            return []
    
  •        for name in os.listdir(localpath):
    
  •            child = os.path.join(localpath, name)
    
  •            uri = path.path_to_uri(child)
    
  •            name = name.decode('ascii', 'ignore')
    
  •            if self._follow_symlinks:
    
  •                st = os.stat(child)
    
  •            else:
    
  •                st = os.lstat(child)
    
  •            if not self._show_hidden and name.startswith(b'.'):
    
  •                continue
    
  •            elif stat.S_ISDIR(st.st_mode):
    
  •                result.append(models.Ref.directory(name=name, uri=uri))
    
  •            elif stat.S_ISREG(st.st_mode) and self._check_audiofile(uri):
    
  •                # if self._is_playlist(child):
    

Remove the commented out code and/or add a TODO with what is missing.


Reply to this email directly or view it on GitHub
https://github.com/mopidy/mopidy/pull/1207/files#r33612833.

This comment has been minimized.

@adamcik

adamcik Jun 30, 2015

Member

#947 covers this, so yes we probably should. Just nobody has decided on what color to paint the bikeshed.

@rawdlite

This comment has been minimized.

Contributor

rawdlite commented Jul 1, 2015

About the debug messages: Yes there are a few, yet i tried to limit them to the information i considered useful to have when being asked to debug a users setup.

The specific path was an oversight on my part when testing the multiple media dir feature.
$XDG_MUSIC_DIR is a good choice.

@rawdlite

This comment has been minimized.

Contributor

rawdlite commented Jul 3, 2015

$XDG_MUSIC_DIR is not available on all system setups.
Code failed when xdg-utils are not installed. Therefore i now check whether a path can be expanded and exists.

The scanner fails on files that are not ending on .mp3 but on mp3.bak etc.
Not sure whether to consider this a bug or a feature. ;-)

EDIT
This is not true for all the .bak files i have.
Still have to dig deeper why this is happening for some of my files.

EDIT II
Those .bak files were indeed not playable.
Scanner is doing fine.

@jodal jodal added the C-enhancement label Jul 5, 2015

@jodal jodal added this to the v1.1 - Robust core milestone Jul 5, 2015

Mopidy-Files is an extension for playing music from your local music archive.
It is bundled with Mopidy and enabled by default.
It allows you to browse through your local file system.
Only files that are considered playable will be shown

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

Missing punctuation.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

done

.. confval:: files/media_dir
A list of directories to be browsable.
Each Directory path has to be written in a separate line.

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

This isn't strictly true, as our config parser supports both:

media_dir = foo, bar, baz

And:

media_dir =
    foo
    bar
    baz

Also, "Directory" shouldn't be capitalized.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

done

A list of directories to be browsable.
Each Directory path has to be written in a separate line.
Optionally the path can be followed by : and a name that will be shown for that path.

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

Is the choice of : a wise choice if someone wants to run Mopidy on Windows, given Windows' use of : to separate the drive letter and the path? (Running Mopidy on Windows has been done, and is probably still possible for someone dedicated enough.)

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

Good Point. Would '|' be a better choice?

Each Directory path has to be written in a separate line.
Optionally the path can be followed by : and a name that will be shown for that path.
.. confval:: files/show_hidden

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

I think this config becomes more self-describing and less sounding like a double negation if it is renamed to e.g. show_dotfiles.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

done

.. confval:: files/follow_symlinks
Whether to follow symbolic links found in :confval:`files/media_dir`.
Directories and files that are outside the configured media_dirs will not be shown.

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

I think s/media_dirs/directories/ reads better.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

done

Whether to follow symbolic links found in :confval:`files/media_dir`.
Directories and files that are outside the configured media_dirs will not be shown.
Default is false

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

Missing punctuation.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

done

~/:Home
show_hidden = false
follow_symlinks = true
metadata_timeout = 1000

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

Missing newline a the end of the file.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

done

[files]
enabled = true
media_dir =
$XDG_MUSIC_DIR:Music

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

Four space indentation should be enough for everyone ;-)

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

done

import sys
import urllib2

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

A single empty line is enough here.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

done

if not self._media_dirs:
return None
elif len(self._media_dirs) == 1:
localpath = self._media_dirs[0]['path']

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

I think this reads just as well if you inline this on the next line.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

Sorry, not getting it.

try:
local_path = path.expand_path(
media_dict[0].encode(sys.getfilesystemencoding()))
except:

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

except clauses without a specified exception is a no-no. This will swallow attribute errors caused by a typo on the previous two lines. I guess you want to catch UnicodeEncodeError?

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

Actually i wanted to catch $XDG_MUSIC_DIR not being set.
As path.expand_path handles this case without an exception, i think i can get rid of the try statement alltogether.

media_dict[0].encode(sys.getfilesystemencoding()))
except:
pass
if not local_path:

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

If the previous except clause is hit, local_path will not be defined and a NameError will be raised here.

Can you move the if-block here into the except-block? Or does this if-block also catch other cases where local_path is falsy?

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

S.o.
Removed the previous except clause instead.

else:
try:
st = os.stat(local_path)
except:

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

You'll want to catch OSError, and not everything.

try:
st = os.stat(local_path)
except:
logger.warn('Could not open %s' % local_path)

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

Would be nice to have the one-line OSError description included here:

from mopudy.internal import encoding

try:
    # ...
except OSError as exc:
    logger.warn(
        'Could not open %s (%s)',
        local_path, encoding.locale_decode(exc.strerror))

Also, don't use % to interpolate strings in log messages, but simply pass the strings as additional arguments to the log function. That way, some work is saved when the current log configuration causes the log message to never be printed anywhere.

except:
logger.warn('Could not open %s' % local_path)
continue
if not stat.S_ISDIR(st.st_mode):

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

Since the only thing st is used for is this, keep to the simpler os.path.isdir(path).

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

Right, this allows for much cleaner code here.

self._media_dirs = []
for entry in config['files']['media_dir']:
media_dir = {}
media_dict = entry.split(':')

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

The variable name indicates that this is a dict, but it is a list.

This comment has been minimized.

@jodal

jodal Jul 5, 2015

Member

Use entry.split(':', 1) to split at most once and allow : to be used in the name part.

This comment has been minimized.

@rawdlite

rawdlite Jul 6, 2015

Contributor

media_list would sound misleading to me. I go with media_dir_split then.

assert not file_path.endswith(os.sep), (
'File path %s cannot end with a path separator' % file_path)
def is_local_path_inside_base_dir(local_path, base_path):

This comment has been minimized.

@jodal

jodal Jul 6, 2015

Member

I don't think the local_ prefix adds anything here. It just confuses a bit by indicating a link with the local extension.

This comment has been minimized.

@jodal

jodal Jul 6, 2015

Member

Also, it would be a nice bonus with some tests of this function.

This comment has been minimized.

@rawdlite

rawdlite Jul 7, 2015

Contributor

Right. I wanted to express that path could be dir or file. I could just leave it at file_path, yet i think it is a bit confusing when a file_path is a directory path or is it not?

This comment has been minimized.

@jodal

jodal Jul 7, 2015

Member

Just path works for both files and dirs :-)

This comment has been minimized.

@rawdlite

rawdlite Jul 7, 2015

Contributor

path is good as long as we don't use
from os import path which we wouldn't ;-)

BTW
why do we check on local_path.endswith(os.sep) ?

I hope i can do the remaining changes tonight.

local_path = path.uri_to_path(uri)
if local_path == 'root':
return list(self._get_media_dirs_refs())
for dir_entry in os.listdir(local_path):

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Before this for-loop, we should have a check of local_path being inside the media_dirs:

if not self._is_in_basedir(os.path.realpath(local_path)):
    logger.debug(
        'Rejected attempt to browse path (%s) outside dirs defined '
        'in files/media_dirs config.',
        local_path.decode(sys.getfilesystemencoding(), 'ignore'))
    return []

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Make that a warning, not debug.

This comment has been minimized.

@rawdlite

rawdlite Jul 8, 2015

Contributor

I will move the logger call to _is_in_basedir. Less repetion.

scratch that. see comment below.

continue
dir_entry = dir_entry.decode(sys.getfilesystemencoding(),
'replace')

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

We should probably decide on using ignore or replace as the policy throughout the code here. I think replace makes sense, as it then becomes obvious from the returned Unicode string that some character was not understood and was replaced with a replacement char (U+FFFD).

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Also, replacement characters makes it more obvious in clients that somewhere down the stack someone had problems understanding some char encoding. By ignoring unknown byte sequences it will look more like a bug to end-users.

return result
def lookup(self, uri):
logger.debug('looking up uri = %s', uri)

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Something like:

logger.debug('Looking up file URI: %s', uri)
logger.debug('looking up uri = %s', uri)
local_path = path.uri_to_path(uri)
if not self._is_in_basedir(local_path):
logger.warn('Ignoring URI outside base dir: %s', local_path)

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

This should use the same log message as for the same check in browse(). Also, we need to decode this path before logging.

This comment has been minimized.

@rawdlite

rawdlite Jul 8, 2015

Contributor

The logging done in _is_in_basedir should be sufficient. I should delete this line.

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Maybe better to remove the logging from _is_in_basedir() since the caller of it has more context about the serverity of something not being in the basedir and knows better what to log and on what level.

This comment has been minimized.

@rawdlite

rawdlite Jul 8, 2015

Contributor

Hm, ok. Valid argument.

track = utils.convert_tags_to_track(result.tags).copy(
uri=uri, length=result.duration)
except exceptions.ScannerError as e:
logger.warning('Problem looking up %s: %s', uri, e)

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

I guess this will be quite normal, so I suggest lowering this to info level, and changing "Problem looking up" to "Failed looking up".

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Scratch that. This is files on your own system, not Internet streams, so I guess warning level is OK. Change the text, though.

for dir_entry in os.listdir(local_path):
child_path = os.path.join(local_path, dir_entry)
uri = path.path_to_uri(child_path)
printable_path = child_path.decode(sys.getfilesystemencoding(),

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

It's tempting to replace all the sys.getfilesystemencoding() calls with an FS_ENCODING constant defined at top level in this file.

local_path = path.expand_path(
media_dir_split[0].encode(sys.getfilesystemencoding()))
if not local_path:
logger.warn('Could not expand path %s', media_dir_split[0])

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Change to "Failed expanding path (%s) from files/media_dirs config value." ?

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Also, use logger.warning() instead, which is the real function. warn() is just an undocumented alias, I think.

logger.warn('Could not expand path %s', media_dir_split[0])
continue
elif not os.path.isdir(local_path):
logger.warn('%s is not a directory', local_path)

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Change to logger.warning() and the text to something like "Path (%s) in files/media_dirs config value is not a directory." ?

if len(media_dir_split) == 2:
media_dir['name'] = media_dir_split[1]
else:
media_dir['name'] = media_dir_split[0].replace(os.sep, '+')

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

I suspect the replace() here is because MPD don't like / in dir names? If so, this is something we should rewrite in the MPD frontend instead. If I'm right, please add a TODO comment here explaining why this is here and where it should be fixed instead. I can live with doing the replace here for now.

try:
result = self._scanner.scan(uri)
logger.debug('got scan result playable: %s for %s',
result.uri, str(result.playable))

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Change log message to for example:

logger.debug(
    'Scan indicates that file %s is %s.',
    result.uri, result.playable and 'playable' or 'unplayable')
result.uri, str(result.playable))
return result.playable
except exceptions.ScannerError as e:
logger.debug('Could not scan %s: %s', uri, e)

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

"Failed scanning %s: %s"

base_dirs = [mdir['path'] for mdir in self._media_dirs]
for base_dir in base_dirs:
if path.is_path_inside_base_dir(local_path, base_dir):
res = True

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Can we replace this for-loop with this?

is_inside = any(
    path.is_path_inside_base_dir(local_path, base_dir)
    for base_dir in base_dirs)

I think it reads better than keeping track of how res can stay False and how it can become True.

This comment has been minimized.

@jodal

jodal Jul 8, 2015

Member

Another version, eating up one more line from the original implementation:

is_inside = any(
    path.is_path_inside_base_dir(local_path, media_dir['path'])
    for media_dir in self._media_dirs)
@jodal

This comment has been minimized.

Member

jodal commented Jul 8, 2015

I'm quite pleased with the state of this PR now :-)

There's one thing that's on my wishlist, though: I'd love some test coverage of browse() and lookup() and their corner cases. I'm open to merging this first and making an issue for the 1.1 milestone about adding some sanity tests of the files library provider before shipping this.

@jodal jodal changed the title from file-browser: initial commit to files extension -- file browsing without scanning Jul 8, 2015

@tkem

This comment has been minimized.

Member

tkem commented Jul 8, 2015

I'm probably nitpicking, but if this extension handles "file" URIs, shouldn't it be called "file" and not "files"? The "stream" extension isn't called "streams" either. Generally, I think plural is rarely used with extension names.

@rawdlite rawdlite changed the title from files extension -- file browsing without scanning to file extension -- file browsing without scanning Jul 8, 2015

rawdlite added some commits Jul 8, 2015

@jodal jodal merged commit 4e0c114 into mopidy:develop Jul 9, 2015

3 checks passed

Scrutinizer 15 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-1.57%) to 75.46%
Details
@jodal

This comment has been minimized.

Member

jodal commented Jul 9, 2015

Merged! Thanks :-)

I tried it out a bit an tweaked some on the debug logging to avoid it becoming too much. Feel free to comment on my commits.

With this, I consider #1004 fixed.

@rawdlite

This comment has been minimized.

Contributor

rawdlite commented Jul 9, 2015

Perfect!
Just got a new client and will be rather busy now.

@rawdlite rawdlite deleted the rawdlite:feature/file-browsing branch Jul 10, 2015

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