Skip to content

Updated coverer plugins #101

Closed
wants to merge 16 commits into from

2 participants

@nperrier
Collaborator
nperrier commented May 8, 2012

Please ignore the latest commit, it's just whitespace fixes.

The commit of interest is the penultimate one:

nperrier@7155863

nperrier added some commits Oct 2, 2011
@nperrier nperrier Fixed unit tests that were failing when run on windoze 7307b56
@nperrier nperrier Revert "Fixed unit tests that were failing when run on windoze"
This reverts commit 7307b5695020c5858b3554d19847925815ce3028.
08352fb
@nperrier nperrier Fixed tests to be platform independent 0b13ec1
@nperrier nperrier Add the ability to extract and cache album/track cover art from tags …
…on scan. Must be enabled from GUI (default is disabled) Also added a few constants for application folders and moved common utilities for cover art to single class
94e4311
@nperrier nperrier Split CoverArtUtils into CoverArt and CoverArtCache. Created CoverArt…
…Indexer and move some of the logic from DBCollectionManager to there.
4b1039f
@nperrier nperrier Merge branch 'master' of git://github.com/rodnaph/sockso a6393b3
@nperrier nperrier Added tests for CoverArt and CoverArtCache 9baa9de
@nperrier nperrier Merge git://github.com/rodnaph/sockso
Conflicts:
	src/com/pugh/sockso/music/Collection.java
	src/com/pugh/sockso/music/DBCollectionManager.java
	src/com/pugh/sockso/web/action/FileServer.java
	test/com/pugh/sockso/music/CoverArtCacheTest.java
	test/com/pugh/sockso/music/CoverArtTest.java
b62b76a
@nperrier nperrier Fixing bad merge made by newbie. Fine: 10 lashes f7d0bbe
@nperrier nperrier Merge https://github.com/rodnaph/sockso be4f870
@nperrier nperrier Merge https://github.com/rodnaph/sockso
Conflicts:
	src/com/pugh/sockso/web/action/FileServer.java
64d9808
@nperrier nperrier Coverer plugin refactoring
Added more tests for RemoteCoverer plugin
Created ImageCache inteface for CoverArtCache
Bound CoverSearch interface to AmazonCoverSearch class
Refactored DefaultCoverer a bit
7155863
@nperrier nperrier Fixing whitespace (s/tabs/spaces/g) ce2aef9
@rodnaph
Owner
rodnaph commented May 8, 2012

Oh and lots of whitespace changes... :-/

@rodnaph
Owner
rodnaph commented May 8, 2012

Further to using the Cache interface - you could extend this new TimedCache class...

18fee9b

With a FileCache implementation, and use that for the cover cache.

@nperrier
Collaborator

Have you looked into using a WeakHashMap instead of HashMap for the ObjectCache?

See http://stackoverflow.com/questions/154724/when-would-you-use-a-weakhashmap-or-a-weakreference

@nperrier
Collaborator

I'm trying to understand what your thinking is here for the refactor:

Rename CoverArtCache -> FileCache and extend TimedCache instead of implementing ImageCache?

So the TimedCache is an "in memory" cache, in that, the image data should not be cached on the file system anymore? Or do you mean to leave the file system cache alone and supplement it with a faster in memory cache that has an expiration?

@nperrier
Collaborator

I see the point of putting the images in memory, as reading from disk is expensive and it might make the cover art quite a bit snappier on the client side. Just trying to figure out how to separate file system caching and in memory caching...

@rodnaph
Owner
rodnaph commented May 10, 2012

No, sorry I should have explained a bit more. The TimedCache is an abstract class that just requires implementing readRaw() and writeRaw() for data storage. This is then extended by ObjectCache that uses a simple HashMap to provide an in-memory cache.

What you can do is create a another class called CoverArtCache that extends TimedCache, and just implements readRaw() and writeRaw() and what you can do here is read/write the images to/from the file system. So you get all the benefits of TimedCache, and the same interface via Cache. (i said FileCache to be more generic, but it might make sense for now to call it CoverArtCache)

Does that make sense?

I haven't heard of the WeakHashMap no - I'll take a look thanks!

@nperrier
Collaborator

OK, I thought you were trying to create a hybrid cache class for the coverart that read/writes to the file system and also keeps the images in memory...I was a bit confused how that would work :)

@nperrier
Collaborator

I would assume I need to extend CoverArt from CachedObject now? If I move the code from getCoverArt() to readRaw(), then call readRaw() from the body of getCoverArt(), since getCoverArt() returns a CoverArt, I can't covert a CachedObject (which readRaw() returns) to a CoverArt object, correct?

Trying to imagine what your idea was for implementing this correctly...

@nperrier
Collaborator

Nevermind, I just realized CachedObject.getValue() is for that very purpose.

@nperrier
Collaborator

Should TimedCache.readRaw()/writeRaw() throw IOException's? The problem I'm running into is that the CoverArtCache needs to do IO to read/write images, but doing that could cause an IOException to be raised. If I implement this in the read/writeRaw ops, I can't re-throw it up the stack - the only option I see is returning a null CachedObject or an empty one. (see TODO's)

Thoughts?

@rodnaph
Owner
rodnaph commented May 15, 2012

Ah yes I didn't think of Exception's - yes please just alter the interface so those methods throw Exceptions (because it might not just be IOExceptions that implementors need to throw).

@nperrier
Collaborator

Is that a good idea? Now methods of ObjectCache throw Exceptions unnecessarily (since it extends TimedCached which implements Cache interface) and require client code to catch them. I think maybe the Cache interface is a bit too generic for both the CoverArtCache (which throws IOExceptions) and an ObjectCache (which throws nothing).

@rodnaph
Owner
rodnaph commented May 16, 2012

Yeah don't worry about it, throwing exceptions is fine! :)

@nperrier
Collaborator

I decided a CacheException class would be best rather than throw Exception everywhere. Let me know what you think.

@nperrier
Collaborator

What do you think about adding auth token functionality for mobile apps to the api (see soundcloud doc for example: http://developers.soundcloud.com/docs/api/authentication#authorization-code-flow)

Currently, the only way to do this is via a cookie, which is kinda awkward to program on android :\

@rodnaph
Owner
rodnaph commented May 23, 2012

Hmm... Not sure about token auth, maybe... but we can deal with that in a separate pull request.

Is this cache feature complete now then? I'll check it out later and pull it to master if it's done.

@nperrier
Collaborator

Yes, it's completed and all tests passed.

@rodnaph rodnaph closed this in 0ae383a May 26, 2012
@rodnaph
Owner
rodnaph commented May 26, 2012

Ok I've merged this to master now, thanks!

Few things I noticed when merging though, firstly all the whitespace changes - I think these were introduced ages ago so maybe not to worry about now. But I had to manually unstage them all before commit (git reset -p). If your IDE keeps doing this then you can use interactive add to make sure they don't get committed (git add -i).

Also, I always make variables final (unless impossible), this just keeps me on top of reducing mutation as much as possible, and usually results in cleaner less buggy code I find.

Looking forward to your token auth pull request ;)

@nperrier
Collaborator

Arrrgh, I thought I fixed the whitespace issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.