Skip to content
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

PICARD-1000: Show album/cluster cover art #642

Merged
merged 35 commits into from Mar 13, 2017

Conversation

antlarr
Copy link
Contributor

@antlarr antlarr commented Mar 5, 2017

This PR adds support to show album/cluster cover art on the CoverArtBox.
I think it's better shown with an example, so...
When an album is looked up and the cover image has changed (slightly, but it has change) it shows up as http://i.imgur.com/M6dFXna.png
When one track is selected, and a different cover image is dropped for that track: http://i.imgur.com/W6xfS2L.png
Then when the user clicks on the album... http://i.imgur.com/ITX4e3g.png
Another file from another album (which has a different cover art) is added to track 9 of that album: http://i.imgur.com/jAt7ZLJ.png
And then another file is added to another track and other covers are dropped to other tracks on that album: http://i.imgur.com/XSig6BS.png

https://tickets.metabrainz.org/browse/PICARD-1000

picard/album.py Outdated
orig_images = []
for track in self.tracks:
for file in list(track.linked_files):
for image in file.metadata.images:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This same for loop has been repeated 4 times, maybe you can make a private function and call it instead?

self.setPixmap(cover)
pixmap = pixmap.scaled(w, h, QtCore.Qt.KeepAspectRatio, QtCore.Qt.SmoothTransformation)
self.setPixmap(pixmap)
pixmap = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we setting pixmap to None here?

IHMO we can move the if clause below to the top and move the above block to an else.

data = image
break
else:
log.debug("%s using images:" % (self.name), metadata.images)
Copy link
Collaborator

@samj1912 samj1912 Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are adding a debug log, maybe an additional parameter display the display string of the related object will be more beneficial than just "new cover art" and "original cover art".

Also the above changes on each selection update, it might clutter the log a bit too much.

Copy link
Collaborator

@samj1912 samj1912 Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On that, maybe we can cache the pixmap. Since it is updated each time the selection is updated, it can lead to too much wasted processing and I/O. Especially since a call to self.data[i].data involves I/O.

@@ -94,6 +95,19 @@ def __eq__(self, other):
def show(self):
self.set_data(self.data, True)

def decorate_cover(self, pixmap):
offx, offy, w, h = (1, 1, 121, 121)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an unrelated note: I wonder about the future of this (hardcoded values), especially since we want to add high dpi compatibility in 2.x releases.

painter.drawPixmap(x, y, pixmap)
pixmap.loadFromData(self.data[0].data)
else:
stack_width, stack_height = (w + displacements * (len(self.data) - 1), h + displacements * (len(self.data) - 1))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

displacements * (len(self.data) - 1) should be stored in a variable instead of repeating it

data=data
)
except CoverArtImageError as e:
log.warning("Can't load image: %s" % unicode(e))
return
if isinstance(self.item, Album):
album = self.item
album.metadata.append_image(coverartimage)
album.metadata.set_front_image(coverartimage)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this ?

I'm low on coffee, but it seems to set_front_image() is actually removing other images having is_front_image set to True. How does this work when a release has multiple front images (currently only possible with CAA provider).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, when an user drags an image and drops it on the cover art box, I think the natural thing he/she wants to do is to replace the cover art with something different, not add another image to a list of images. Maybe in the context menu (check my cover-art-context-menu branch) I can add a "radio button" where user can select between "Append images on drop" or "Replace images on drop". Would that be better? Still, I think the default should be "replace" since it's more intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of radio buttons in the context menu, maybe an option in the "user interface" section of the options dialog would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the radio buttons on the context menu not to clutter the options dialog with a new option, so this will be addressed by this branch:
https://github.com/antlarr/picard/compare/album-cover-art...antlarr:cover-art-context-menu?expand=1
You can see the result here: http://i.imgur.com/lyI9SNs.png
I'll create the pull request for the cover-art-context-menu branch as soon as this one is merged (the work there is already finished) since the commits in that branch depend on this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, depending on chosen option, the other front images should be kept imho. What about either prepending dropped image, or replacing only the first one ? And keep the rest as is.

data = image
break
else:
log.debug("%s using images:" % (self.name), metadata.images)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line also doesn't work because not all arguments to log.debug are converted during string formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will just remove that line. As samj1912 said, that would clutter the log too much. I just added while I was working on that and thought it would be ok to keep it, but it doesn't really give any information once this works as expected.

@@ -28,6 +28,36 @@
from picard.util import encode_filename, imageinfo


class LRUCache(dict):
Copy link
Collaborator

@samj1912 samj1912 Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this can be used by other modules too, mind moving this to a new utils.cache module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

super(CoverArtThumbnail, self).__init__(active, drops, *args, **kwargs)
self.data = None
self.name = name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any use for the name attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right. Not anymore, so I'll remove it

@antlarr antlarr changed the title Show album/cluster cover art PICARD-1000: Show album/cluster cover art Mar 7, 2017
pixmap.loadFromData(self.data[0].data)
pixmap = self.decorate_cover(pixmap)
else:
stack_width, stack_height = (w + displacements * (len(self.data) - 1), h + displacements * (len(self.data) - 1))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to:

offset = displacements * (len(self.data) - 1)
stack_width, stack_height = (w + offset, h + offset)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -55,6 +55,11 @@ def __nonzero__(self):
def append_image(self, coverartimage):
self.images.append(coverartimage)

def set_front_image(self, coverartimage):
# First remove all front images
self.images[:] = [ img for img in self.images if not img.is_front_image() ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove spaces at start and end of list comprehension:

self.images[:] = [img for img in self.images if not img.is_front_image()]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.

"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment block should be on the class itself, not somewhere before it.

"""


class LRUCache(dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyQt provides a QPixmapCache class, is that something we could use instead of rolling our own? (If we need an lru cache in the future, we can use the one in functools if that future involves python 3).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antlarr I didn't know about this but it seems like the way to go. It has all the functionality we need.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -101,7 +102,7 @@ def __init__(self, obj, parent=None):
self.ui = Ui_InfoDialog()
self.display_existing_artwork = False
if isinstance(obj, File) and isinstance(obj.parent, Track) or \
isinstance(obj, Track):
isinstance(obj, Track) or isinstance(obj, Album):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use isinstance(obj, (File, Track, Album))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I can merge Track and Album but isinstance(obj, File) has an "and" with isinstance(obj.parent, Track), so I would leave it as:

if isinstance(obj, File) and isinstance(obj.parent, Track) or \
        isinstance(obj, (Track, Album)):

Deal?

Copy link
Collaborator

@samj1912 samj1912 Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the "\" for a pair of parentheses and you got it! :P

mineo
mineo previously requested changes Mar 7, 2017
Copy link
Member

@mineo mineo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order in which the covers are stacked seems wrong: the frontmost is the cover of the last track that does not have the same cover as the first track. I would have expected the covers to follow the track order, with the frontmost one being the cover of the first track.

One other thing that I really don't like is how this album-level view of the cover art is different from the album-level view of track metadata: the latter one shows (different across x items), even if x-1 items have the same value for a tag, the cover art now looks as if all tracks of an album have the same y pictures. Is this the direction we want to go?

picard/album.py Outdated
for image in file.orig_metadata.images:
if image not in orig_images:
orig_images.append(image)
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under which circumstances does this raise an AttributeError? Assuming that file is always a File, it will always have orig_metadata and that will always have images.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file is not always a File, but can be a Track too, and tracks don't have orig_metadata. Should I change that to "obj" ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it isn't always a File, yes, better change it to obj.

picard/album.py Outdated

for track in self.tracks:
process_file_images(track)
for file in list(track.linked_files):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is linked_files ever not a list?

picard/album.py Outdated
process_file_images(track)
for file in list(track.linked_files):
process_file_images(file)
for file in list(self.unmatched_files.files):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is unmatched_files.files ever not a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a reason for those, but I forgot... so ok, I'll change it (and the other case)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, there are other previous cases in album.py where linked_files is converted to a list, so I'm not so sure that's not needed

@@ -70,6 +70,9 @@ def __init__(self, data, prefix='picard', suffix=''):
def __eq__(self, other):
return self._hash == other._hash

def hash(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this __hash__?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because IMHO that function doesn't return the hash of that object, but the hash of the CoverArtImage stored in that DataHash object.

@@ -296,6 +297,20 @@ def _display_info_tab(self):
tabWidget.setTabText(tab_index, _("&Info"))
self.tab_hide(tab)

class TrackInfoDialog(InfoDialog):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something's wrong on my end, but it doesn't seem to be possible to access this dialog from the Info button in the toolbar or the context menu of tracks without files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I tested it with zas' example release with 3 images (e5e80fb7-f6b7-3c51-9183-5af44a6c6195) and there it works, but you're right, for single image albums it's not accesible. I'll try to fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed now

@@ -944,6 +948,10 @@ def update_selection(self, objects=None):
}
self.set_statusbar_message(msg, mparms, echo=None,
history=None)
elif isinstance(obj, Album):
obj.update_metadata_images()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really need to be called every time an album is selected? It really feels like this should be done after the images on the metadata objects are updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any way to call an album's update_metadata_images when the images are changed in any track/file related to that album. Do you have any suggestion?

Copy link
Collaborator

@samj1912 samj1912 Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simply recursively update the parent's orig_metadata/new_metadata on addition of a new image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the parent of a file is the track, not the album, right? I think I prefer your previous solution (emitting a signal from track/files and have the album's update_metadata_images called). Would that be ok?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works :)

@antlarr
Copy link
Contributor Author

antlarr commented Mar 8, 2017

The order in which the covers are stacked seems wrong: the frontmost is the cover of the last track that does not have the same cover as the first track. I would have expected the covers to follow the track order, with the frontmost one being the cover of the first track.

It's strange but for me, the current implementation follows track order. Imagine that you have the track's covers in your hands. First, the first track's cover is laid down, then, the second track's cover is laid down (thus over the first one), and so on. Can someone else vote here which one looks more natural for you?

In any case, if everyone else votes to have the first track at the top, I would arrange the stack from bottom-left to top-right instead of the current top-left to bottom-right, so at least the tracks order is somehow "left to right". Although that's probably cultural and some countries with other writing systems would prefer something else :(.

One other thing that I really don't like is how this album-level view of the cover art is different from the album-level view of track metadata: the latter one shows (different across x items), even if x-1 items have the same value for a tag, the cover art now looks as if all tracks of an album have the same y pictures. Is this the direction we want to go?

I guess you have a point there. Would it be ok if we show the album cover art only when all tracks have the same front images and if any is different we show one of the following options?

  1. The default disc shadow with a red tint
  2. A split screen-like pixmap of the different covers (as opposed to the current stack of covers)
  3. The current stack with some symbol in one of the corners

I think I prefer 1 or 3.

@antlarr
Copy link
Contributor Author

antlarr commented Mar 8, 2017

Please review again

@zas
Copy link
Collaborator

zas commented Mar 8, 2017

It's strange but for me, the current implementation follows track order. Imagine that you have the track's covers in your hands. First, the first track's cover is laid down, then, the second track's cover is laid down (thus over the first one), and so on. Can someone else vote here which one looks more natural for you?

First track cover image on top, second under it, etc....

I guess you have a point there. Would it be ok if we show the album cover art only when all tracks have the same front images and if any is different we show one of the following options?

The default disc shadow with a red tint
A split screen-like pixmap of the different covers (as opposed to the current stack of covers)
The current stack with some symbol in one of the corners

I think I prefer 1 or 3.

Adding symbols is a bad idea imho (it complicates things, needs explanations, etc...). So i'll not go for 3.

When all tracks share the same image, just display this image for the release.

When one or more tracks have different images, display the one shared among most tracks or the first one on top, plus a special shadow showing there are more images (like a stack of images).

@antlarr
Copy link
Contributor Author

antlarr commented Mar 8, 2017

When the tracks/files have no common images, a yellow glow (same color as used in the MetadataBox) is added to the front image: http://i.imgur.com/aXF4lUO.png
When there are four images to show, they are shown normally (in this case, with a glow, because there are different images in different tracks): http://i.imgur.com/RU3M4B0.png
When there are five (or more) images to show, only 3 are shown and a "grey stack" is added to represent "there are more": http://i.imgur.com/5TlkS2o.png
I think that should satisfy everyone

pixmap.loadFromData(self.data[0].data)
pixmap = self.decorate_cover(pixmap)
else:
limited = len(self.data) > 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please store this 4 in an uppercased variable (constant).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

for k in range(border_length):
color.setAlpha(255 - k * 255 / border_length)
painter.setPen(color)
painter.drawLine(x, y - k - 1, x + 121 + k + 1, y - k - 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there Qt methods to draw such shadow ? Also i wonder about high dpi scaling of the whole cover art box, not sure such code will give good visual results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, there isn't any method in Qt for that. I think there may be methods in kde frameworks in some style library like breeze, but that's definitely of no use here. About high dpi scaling, that's "not supported" on Qt4, Qt5 brings support for hidpi devices from Qt 5.6.0, but that's definitely something to worry about with picard 2.0 . In the meantime, since the coverartbox is fixed to use 128x128 covers, that means users with hidpi devices will see small covers and small cover stacks (which Qt5 will scale automatically)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With respect to the drawing, I tried saving an image for the glow under the resources directory and just loading/painting it. Since the resolution is fixed, it's doable, but I found my drawing skills suck, so I ended up making the glow in code, mainly because we can experiment with making it bigger, changing color or whatever. But if you think it's better to stick to that glow, it's also possible to store the glow and the cover stack as images and just use them as is done with CoverArtShadow.png . Only note that I would still use code to generate the images instead of having a svg image as source. Since there are not many cases (afaik) with so many cover art images and anyway the generated images are cached, I'm not sure it's worth to change that.

if hasattr(metadata, 'has_common_images'):
text += '<br />'
if has_common_images:
text += _(u'<i>Common images on all tracks</i>')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would exclude tags from translatable string here:

if has_common_images:
    note =  _(u'Common images on all tracks')
else:
    note = _(u'Tracks contain different images')
text += '<br /><i>%s</i>' % note

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

if isinstance(obj, File) and isinstance(obj.parent, Track) or \
isinstance(obj, Track):
if (isinstance(obj, File) and isinstance(obj.parent, Track) or
isinstance(obj, Track)):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While at it, move operators in front:

if (isinstance(obj, File)
    and isinstance(obj.parent, Track)
    or isinstance(obj, Track)):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -73,6 +73,7 @@ def dropEvent(self, event):


class CoverArtThumbnail(ActiveLabel):
MAX_COVERS_TO_STACK = 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to picard.const

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't know about picard.const. I moved it there.

@samj1912
Copy link
Collaborator

samj1912 commented Mar 9, 2017

Please don't include any merge commits in a PR. Always rebase against the master :)

@zas
Copy link
Collaborator

zas commented Mar 9, 2017

since the coverartbox is fixed to use 128x128 covers, that means users with hidpi devices will see small covers and small cover stacks (which Qt5 will scale automatically)

That's exactly my point, current cover art box code (and the one you are adding) are far from being "scalable", with all those fixed numbers, so a complete rewrite will be needed.

To make it clear: 128x128 on a 4k screen is a small icon, not a thumbnail, scaling it up will increase the size but quality will be awful, since we have 500x500 pixels images from CAA we could at least use those without the downscale/upscale process.

I was just wondering if we could avoid to rewrite the code here in one month...

@antlarr
Copy link
Contributor Author

antlarr commented Mar 9, 2017

Yes, that has to be rewritten anyway for Qt5 (just like the previous code in there had to be rewritten too) as everything not managed by layouts has to use "device-independent pixels". I'll try to help with that once the port to Qt5 starts.

@antlarr antlarr force-pushed the album-cover-art branch 3 times, most recently from 7f5dcd5 to fb3be80 Compare March 10, 2017 18:48
@zas
Copy link
Collaborator

zas commented Mar 11, 2017

@antlarr : please fix conflicts and sort out this PICARD-1000 / description / whatever @mineo reported
@mineo : are changes you requested done ? If yes, please approve

This make CoverArtImage objects hashable
This commit implements a LRU cache for generated pixmaps so they're not
generated each time the function is called (saving a lot of I/O and
CPU). The cache is shared by cover_art and orig_cover_art objects
and currently is configured to contain a maximum of 40 pixmaps.
Instead of raising an exception, now it's possible to see all
images of tracks with no linked file.
Retrieve the images from tracks, since an album that doesn't have any linked
file, can still have images defined in its tracks.
Moved the LRUCache class to its own module in utils, in case it's useful
in other parts of the code.
Remove spaces at start/end of braces
And remove an unused variable in TrackInfoDialog
I changed it in e9fa6c8 so a track
is passed to that function some times, so better reflect that in
the variable name too.
In order to fix tracks without files not having the info button
available, I changed it to allow 0, 1 or multiple files.
If the track doesn't have any linked file, only the cover art is shown
(if available). If the track has n files, the info tab shows information
from the all the files.
I forgot to add the changes from this file to the previous commit
(6544900)
Add a signal to Files/Tracks that gets emitted when the images
change (or might have changed), and update the album's metadata images
on such signals instead of each time the album is selected in the
user interface.
Disable/enable the update of images also before/after setting a dropped image
If all tracks/files have a common set of images, show the stack
of cover art images as usual. If any track/file has a different
set of images, the front image is painted with a darkgoldenrod glow
(the same color used for the MetadataBox for tags with different values).

Also, fixed the draw of the stack so the bottom cover is drawn
correctly.
If there are more than 4 images, draw only three and a kind
of "grey stack of covers" that can be easily identified with
"and more covers"
Add a constant to hold the maximum number of covers to draw in a stack.
Extract html tags out of a translated message.
Reformat an if expression to move overators to front.
Remove an unneeded comment.
@antlarr
Copy link
Contributor Author

antlarr commented Mar 11, 2017

I think the only thing reported by mineo I didn't address is that DataHash.hash should be __hash__, but I was waiting for him to agree or disagree with my opinion that it doesn't feel ok that hash(image) would be equal to hash(image.datahash). But if it's necessary, I can of course change that.

@zas zas dismissed mineo’s stale review March 13, 2017 12:30

6 days old, no answer, let's move on

@zas zas merged commit cb1ff1d into metabrainz:master Mar 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants