Mosaic plugin #2352
Mosaic plugin #2352
Conversation
There was a problem hiding this comment.
Great job! Welcome to beets!
You should probably set your editor a bit to work with python in general and the beets codebase in particular.
We follow pep8 relatively closely. At a first glance, you should at least have:
- Autoconvert tabs to 4 spaces
- Autoremove trailing whitespace
- Max line length = 80
If possible, the best (and easiest!) option would be to use a pep8 linter, from CLI or IDE.
beetsplug/mosaic.py
Outdated
| covers = list(); | ||
|
|
||
| for album in albums: | ||
| self._log.info(u'#{}#', album.artpath) |
There was a problem hiding this comment.
Here album.artpath is displayed before checking if it is not None. Is this a concern?
beetsplug/mosaic.py
Outdated
| if not os.path.exists(album.artpath): | ||
| continue | ||
|
|
||
| covers.append(album.artpath) |
There was a problem hiding this comment.
Maybe this could be more elegantly written as
if album.artpath and os.path.exists(album.artpath):
#cover list and logging
else:
#is some form of logging needed here?
beetsplug/mosaic.py
Outdated
| cmd = ui.Subcommand('mosaic', help=u"create mosaic from coverart") | ||
|
|
||
| def func(lib, opts, args): | ||
| self._generate_montage(lib, lib.albums(ui.decargs(args)), u'mos.png') |
There was a problem hiding this comment.
Should the mosaic filename be chosen by the user, at least optionally? It makes sense from a ux point of view.
Also, this would make the code more sensible, as for now _generate_montage has the output_fn parameter which is always hardcoded to u'mos.png'. That made me scratch my head a little bit.
embedart for example could be an inspiration for this. Moreover, embedart also does not let the user choose the extension, similarly to this plugin, so you could use the exact same convention and ux.
|
This looks awesome and very pretty! I'll give it a run on linux asap. Looking forward to seeing the docs and finished ux! Do you think this plugin is fit for some testing? Is there some business logic to isolate? |
There was a problem hiding this comment.
Wow; this is awesome! I'm also excited to give this a try. ✨
One little style note: in standard PEP8 style, the recommendation is to use 4 spaces for indentation (as opposed to real "tab" characters).
beetsplug/mosaic.py
Outdated
| @@ -0,0 +1,105 @@ | |||
| # -*- coding: utf-8 -*- | |||
| # This file is part of beets. | |||
| # Copyright 2015-2016, Ohm Patel. | |||
There was a problem hiding this comment.
You might want to put your own name here. 😃
beetsplug/mosaic.py
Outdated
|
|
||
|
|
||
| class MosaicCoverArtPlugin(BeetsPlugin): | ||
| col_size = 4 |
There was a problem hiding this comment.
It looks like col_size might be unused?
beetsplug/mosaic.py
Outdated
|
|
||
| def _generate_montage(self, lib, albums, output_fn): | ||
| fullwidth=0; | ||
| fullheight=0; |
There was a problem hiding this comment.
Python doesn't need semicolons. 🦆
|
It took a bit of bulldozing, but still, congrats!! 🥇 My observations:
Some more minor comments:
To solve those last two creatively, I'd suggest (just throwing that out there, maybe I could it myself if I really want it) to play with the overlap. This would give a more "natural mosaic" look while allowing to cheat on numerous constraints! |
|
Couldn't one use this with bpd or the play command to create wallpapers and statuses that change as you change things? |
|
BTW the code with the minimal changes for a working plugin, which I used to do the little tests is at nathdwek/beets@d5f1d5b . @SusannaMaria you could maybe pull from there to get going? |
|
Thanks guys for the reply. I really want to follow the pep rules, and for sure I want to consider the issues related to ux. |
|
|
I think the two first points are perfectly reasonable, my idea would be sort of a bonus if anything. |
|
Some ideas regarding config, inspired by http://www.imagemagick.org/Usage/montage/#montage |
|
I added a blending of a picture to the whole mosaic in this example I used Maybe it's interesting to have something like a watermark in it. Maybe the github-user icon. |
|
I've implemented the same thing (beside a lot of other things) some time ago in my arttools plugin with the |
|
Very cool! It looks like this is passing our style checker now too. Woohoo! I think all this needs is some plugin documentation before it's mergeable. |
|
I have a bug with processing the command-option. Right now you can not pass option to the beet mosaic command but the query. mosaic: I will work on the documentation maybe you can give me some hint, what I'm doing wrong in the |
|
Cool! There's actually an easy way to handle command-line arguments that override defaults from the configuration file. Here are the relevant docs: Calling something like |
|
Is it possible that there are some miscalculation of the mosaic size? It seems that my bigger mosaics are systematically missing what seems like a last row? |
|
Added a creation of dummy cover if the artpath is not given. I need to add a font therefor I moved the plugin into a subfolder and changed the manifest.in. Guess I need some days more to check for bugs and documentation... |
|
Hmm… including assets authored by other people can cause weird licensing headaches, especially for the people who package beets for OS package managers. Maybe we can make the plugin automatically download FreeSans when it first needs it? (Or use an installed font if one is available?) |
|
@sampsyo yes I understood, i have commented the font code out. Because I want to have a feature to label the cover however it's better to plan a solution which cause no issues. I will start with documentation and code comments tomorrow. Hope I get the bug with command-line fixed. |
|
That a weirdo bug. cause to fail all checks. |
|
OK, sounds good! That's weird about the test failure. I don't see your plugin is getting imported during the tests (since there are no tests yet). If we do want to add tests, we'll of course have to instruct the test harness to install PIL/Pillow. But that shouldn't be a problem yet… I'll have to look a little closer. Sorry about that! |
|
Hmm… I'm not sure when this changed, but Travis has stopped complaining about the missing dependency! It does have some style suggestions, though. |
beetsplug/mosaic.py
Outdated
|
|
||
| from PIL import Image | ||
| import math | ||
| from parse import * |
There was a problem hiding this comment.
It's usually best to avoid "star imports," which make it difficult to tell where names come from.
beetsplug/mosaic.py
Outdated
| 'geometry'].get(str) | ||
| else: | ||
| geometry = opts.background | ||
| albums = lib.albums(ui.decargs(args)) |
There was a problem hiding this comment.
Since you've already used self.config.set_args, you don't need all these if/else blocks for fallbacks. For example, this suffices just fine:
geometry = self.config['geometry'].get(str)In fact, this is probably what you want:
geometry = self.config['geometry'].as_str()No need to test whether the value is present; just use it.
There was a problem hiding this comment.
Of course, you don't even need to pass around stuff like geometry as a parameter—the code that needs it can just use self.config['geometry'].as_str() right there.
| To use the ``mosaic`` plugin, first enable it in your configuration (see | ||
| :ref:`using-plugins`). Then, install the `Pillow`_ library by typing:: | ||
|
|
||
| pip install Pillow |
There was a problem hiding this comment.
Looks like this plugin also uses a library called parse, so you probably want to mention that here. (Or just use regular expressions instead.)
Fix misuse of flags in re.sub() calls
Add info about overlaying configs, Resolves beetbox#2084
|
Maybe you can give me some advise because of unit testing? Right now I have only one test implemented. which just generates a 2x2 mosaic with size 359x359. I compare the image size at the end. Ideas
|
|
https://tympanus.net/Tutorials/ThumbnailGridExpandingPreview/ |
|
Very cool! I actually think a small test is probably fine in this case. If problems come up with strange parameters like very large numbers of albums, we can add tests for those too, but just getting the basic functionality covered is a great place to be for a new plugin. The web thing is a neat idea too! Especially if there were still a way to flatten to a standalone image… |
|
Just added a second usecase and faced a problem with png-Watermark. Any change to put this plugin into master after QA and cleaning with static mosaic creation, or should I continue merging into the web-frontend as HTML5 with canvas? |
|
Very cool! Yes, I'd be happy to merge this into master whenever you think it's ready. Maybe we can hit the green button after your next round of refactoring? |
|
added option to download and use font: |
|
Nice. Would you mind mentioning the new font option in the documentation so people know how to use it? Also, I’m not 100% sure what’s going on, but the GitHub pull request view is for some reason showing a bunch of unrelated changed. Maybe it’s a strange consequence of rebasing? I could dig a little deeper to find out what’s wrong, but maybe it’s clearer from your side. |
|
Because of the long break of 6 months I merged the master into the feature branch before I continued 3 days ago. Sorry because of the mess. |
|
Ah, no problem. It makes the diff display look weird, but I think it will still merge correctly on the command line. |
|
Okay that's it from my side for the mosaic plugin for now. Unfortunately I'm unable to understand the Problem with https://travis-ci.org/beetbox/beets/jobs/255377806 Because of similarity: guess it's best to use a conservative approach to store relations with Flexible Field Types. Lets see, but more or less I want to explore the best way to visualize the graph. Okay offtoic for this pull request :-) |
|
Wow; that Travis failure is indeed really weird. I suspect it may be unrelated to your new plugin… I’ll look into it a bit more deeply. Thanks for all your effort on this! ✨ |
|
Maybe its the download of the font. I never asked the question, where I have to store the font permanently? so its stored at the same location of the plugin. Feeling guilty ... |
|
"The deceased were taken from the grave" ... I never was satisfied with the static generation of one file, but I have found pyqtgraph today, very neat lib.
I really don't know if exploring 3000 albums by coverart makes really sense. But maybe within 99 years I come up with some implementation which brings value. |
It's really fun to learn python within beets. :-)
Some months ago I wrote a simple java-application which created a mosaic of my cover art from beet lib. I decided yesterday to implement a very first plugin which covers this feature.
Here is the result fetching my whole lib, took ~1 minute. I resized the result with irfanview because it is 50 MB.
I will continue on the develop add documentation and meaningful configuration ... and code-documentation blush
Right now it's only tested on windows10 with python2.7