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

Added support for custom image loaders #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schoentoon
Copy link
Contributor

Took a bit longer than I expected. I ended up going for a whole interface around it, which you can implement yourself in any way you'd like.

Rather than letting the library directly read from files and io.Readers backed by http
it now has support for custom loaders. You will have to implement the ImageLoader() (image.Image, error) interface
from now on. This allows you to be in control yourself over where you load your image from, how you cache it, etc.
For GIF support you'll have to implement the Gif() (*gif.GIF, error) interface next to the ImageLoader interface.
For backwards compatibility I added a couple of loaders already, the FileImage, HttpImage. The main program should
still function as originally.
@trashhalo
Copy link
Owner

@muesli since you are the first 3rd party consumer of this API could you review this PR to confirm it meets what your use case for glow?

@muesli
Copy link
Contributor

muesli commented Nov 8, 2020

I think I would have just gone for an io.Reader. That's all we need to load data. The wrappers for http / file loaders around it can be tiny then and we remove a lot of duplicated code.

@schoentoon
Copy link
Contributor Author

Now that I look at it again I should probably have made the HttpImage accept a custom http.Client anyway. I mostly just figured that the file and http loaders were gonna be needed to keep the command line tool working anyway. So wouldn't make a huge difference whether it would be exposed in the library or just in the command line tool imo.

@muesli
Copy link
Contributor

muesli commented Nov 8, 2020

Understood. I'll quickly whip up an example with a simple io.Reader approach for comparison.

@muesli
Copy link
Contributor

muesli commented Nov 8, 2020

Here it is: #15

@muesli
Copy link
Contributor

muesli commented Nov 8, 2020

Just as a heads up: I skipped the other improvements @schoentoon introduced. Nice work there, I'm all for them!

@trashhalo
Copy link
Owner

@schoentoon You got some gnarly conflicts at the moment. For this to get merged. Changes I'd like to see:

  1. Obvi conflicts addressed
  2. Lib code moved back to root OR follow go standard layout

I have some bandwidth this weekend to push this forward if you are swamped.

@schoentoon
Copy link
Contributor Author

TIL /lib is non-standard. I'll move it to /pkg later today and address the conflicts.

@muesli
Copy link
Contributor

muesli commented Nov 18, 2020

@schoentoon Yeah, it's all conventions, but I guess it makes sense to stick to the established ones. Sorry for causing the merge conflicts here, but I hope you can see why using the existing io.ReadCloser interface is sufficient in this case: it enables compatibility with an entire ecosystem of existing Go packages that all support this very interface (or the easily NilCloser wrapped io.Reader).

For this reason I think we should try not to introduce an ImageLoader interface here unless we got a really good reason for it.

@schoentoon
Copy link
Contributor Author

I would argue that being able to provide a image.Image directly (or a gif.GIF for that matter) would be nice to have as well. Take for example wanting to apply certain filters over an image somewhere within your program. You'll probably already have a image.Image in such a case. Not having to encode it to later on decode it again seems like a benefit to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants