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

Extract loltext plugin to gem #339

Merged
merged 2 commits into from
Apr 16, 2017
Merged

Conversation

matthutchinson
Copy link
Member

@matthutchinson matthutchinson commented Apr 15, 2017

Removes the default loltext plugin and uses the new (just released) lolcommits-loltext plugin gem instead (v0.0.2)

The font file has also been removed and a config test (that's now tested in the plugin gem itself).

The README at lolcommits-loltext explains how to configure text and overlay styling options.

@mroth - I've added you as a gem maintainer for this new plugin at rubygems.org (and for lolcommits-plugin-sample too)

@matthutchinson matthutchinson requested a review from mroth April 15, 2017 15:39
Copy link
Member

@mroth mroth left a comment

Choose a reason for hiding this comment

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

Hadn't even thought about extracting this to a default gem too, clever and modular! This diff looks clean (especially since its mostly just removing code).

@matthutchinson
Copy link
Member Author

Thanks, yes it was pretty straightforward, after this will move onto extracting the others.

@matthutchinson matthutchinson merged commit 834c7b3 into master Apr 16, 2017
@matthutchinson matthutchinson deleted the extract-loltext-plugin-to-gem branch April 16, 2017 16:09
@matthutchinson
Copy link
Member Author

@mroth - I fear we're going to have to make a change to the post/pre capture hooks - since the order of plugin execution will become important - e.g. we'll have plugins in the post capture hook - that annotate or modify the image like loltext, and then other plugins that must run afterward that (not before) e.g. to post to twitter etc.

two options I see are;

  • keep things simple and introduce a third hook point - so something like :precapture, :postcapture, :postimageready
  • OR allow plugins to specify an order number, and sort on that during the pre/post hook execution

I want to keep things simply but flexible going forward, what do you think is best?

@mroth
Copy link
Member

mroth commented Apr 17, 2017

Hmmm... I see the issue here.

Really we do have three phases that are now becoming more apparent with moving loltext into a true plugin:

  1. pre-capture
  2. post-capture, image processing (originally just loltext, but possibly something else in the future)
  3. post-capture, do stuff with images (most of the plugins which do upload)

My sense is introducing/enforcing a third hook point makes the most sense, since third-party plugins might not have knowledge of each other in the future, so getting absolute order numbers to match up cleanly would be difficult.

Additionally, since loltext is "special" (default), it could make sense to ensure it always runs first in it's own hook point, but not sure whether that actually introduces any benefits or just complicates matters.

@matthutchinson
Copy link
Member Author

matthutchinson commented Apr 17, 2017

OK, I agree - so I'll introduce a 3rd hook - we could call the three points something like this;

  • precapture
  • postcapture
  • imageready

Since I haven't released yet (with the new loltext gem plugin) - I can easily rename the existing hook points to use these symbols names. I was planning on releasing a new lolcommits gems today (using the loltext and tranzlate gems) then extract the rest of the plugins.

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

Successfully merging this pull request may close these issues.

2 participants