user filter which takes precedence over htmlize-buffer if set #13

Merged
merged 10 commits into from Jan 1, 2014

Conversation

Projects
None yet
3 participants
@danielsz
Contributor

danielsz commented Dec 26, 2013

Added the following interactive functions:

imp-set-user-filter and imp-remove-user-filter

This allows the user to define a custom html-producing function on the current buffer. When set, takes precedence over the htmlize feature and regular html files. When removed, it's back to the previous behavior (no processing for html buffers, htmlize-buffer for non-html buffers).

Usage:

The user may define a basic function like this:

(defun user-function (&rest buffer)
  (princ "<html><body>test</body></html" (current-buffer)))

The original editing buffer is passed along the user function as a parameter, which allows for preprocessing, like so:

(defun user-function (buffer)
  (let ((count 
     (with-current-buffer buffer
           (count-words-region (point-min) (point-max)))))
    (princ (format  "<html><body>%d</body></html" count) (current-buffer))))

The motivation for this PR is live coding goodness. The possibilities are endless. For example, one edits a Clojure buffer where hiccup is used to produce HTML and garden processes the CSS. By setting the user function appropriately (via cider-eval), one is effectively able to live previewing the code in the browser as one edits it.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 30, 2013

Owner

I'm basically ready to merge this as-is. Nice work. I like the API that the user satisfies to implement a filter. The API is certainly sufficient to use htmlize as a filter instead of a special case. I'd like to see that as a code cleanup. Can you convert the htmlize logic to simply be the default filter (check the htmlize enable flag inside the filter.)

Owner

netguy204 commented Dec 30, 2013

I'm basically ready to merge this as-is. Nice work. I like the API that the user satisfies to implement a filter. The API is certainly sufficient to use htmlize as a filter instead of a special case. I'd like to see that as a code cleanup. Can you convert the htmlize logic to simply be the default filter (check the htmlize enable flag inside the filter.)

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

Thank you. Yes, absolutely, I can do that. And you are right, there is an opportunity to clean up some code. One question remains in my mind, though: what should the imp-remove-user-filterdo? As of now, it doesn't do anything useful, and is candidate for removal. But I'll wait to hear what you say. Thank you.

Contributor

danielsz commented Dec 31, 2013

Thank you. Yes, absolutely, I can do that. And you are right, there is an opportunity to clean up some code. One question remains in my mind, though: what should the imp-remove-user-filterdo? As of now, it doesn't do anything useful, and is candidate for removal. But I'll wait to hear what you say. Thank you.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

Owner

I think I'd remove it.

I think the other most reasonable implementation would be to have set-user-filter push the filter onto a per-buffer stack and remove-user-filter pop it from the stack (and then only use the top filter on the stack to process the buffer). But, that's a lot of behavior that yields very little value--thus my recommendation to remove.

Owner

netguy204 commented Dec 31, 2013

I think I'd remove it.

I think the other most reasonable implementation would be to have set-user-filter push the filter onto a per-buffer stack and remove-user-filter pop it from the stack (and then only use the top filter on the stack to process the buffer). But, that's a lot of behavior that yields very little value--thus my recommendation to remove.

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

I've updated the documentation to reflect this PR.

After some thought, I've come to the conclusion that imp-remove-user-filter would restore the default htmlize filter. This seems the most logical, but feel free to discuss.

Contributor

danielsz commented Dec 31, 2013

I've updated the documentation to reflect this PR.

After some thought, I've come to the conclusion that imp-remove-user-filter would restore the default htmlize filter. This seems the most logical, but feel free to discuss.

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

We've been posting at the same time and I'm confused. Can you please read the modified README and confirm that you want imp-remove-user-filter? Thank you.

Contributor

danielsz commented Dec 31, 2013

We've been posting at the same time and I'm confused. Can you please read the modified README and confirm that you want imp-remove-user-filter? Thank you.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

Owner

That's a reasonable approach. I like it: imp-remove-user-filter restores the default htmlize filter.

Owner

netguy204 commented Dec 31, 2013

That's a reasonable approach. I like it: imp-remove-user-filter restores the default htmlize filter.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

Need a closing bracket on </html>.

Need a closing bracket on </html>.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

Closing bracket on </html>

Closing bracket on </html>

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

Great. Anything else? Are we all done for now? Can we call it a PR?

Contributor

danielsz commented Dec 31, 2013

Great. Anything else? Are we all done for now? Can we call it a PR?

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

Oops. Fixed.

Contributor

danielsz commented Dec 31, 2013

Oops. Fixed.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

The ability to toggle htmlize is very useful. I recommend:

(defun imp-toggle-htmlize ()
  (interactive)
  (if (eq imp-user-filter #'default-user-filter)
      (imp-set-user-filter nil)
    (imp-set-user-filter #'default-user-filter)))

Also, recommend changing the name of the default filter to imp--default-filter because it's really imp internal.

The ability to toggle htmlize is very useful. I recommend:

(defun imp-toggle-htmlize ()
  (interactive)
  (if (eq imp-user-filter #'default-user-filter)
      (imp-set-user-filter nil)
    (imp-set-user-filter #'default-user-filter)))

Also, recommend changing the name of the default filter to imp--default-filter because it's really imp internal.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

I believe with-temp-buffer would be more appropriate here. It handles annoying cases like making sure that the buffer doesn't leak if the user's filter function throws an error.

I believe with-temp-buffer would be more appropriate here. It handles annoying cases like making sure that the buffer doesn't leak if the user's filter function throws an error.

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

Yes, you are right about the default filter name. Change is committed.

Regarding imp-toggle-htmlize, if you set the user filter to nil, there will be no output for non-html buffers in the browser. Is this something we want?

Contributor

danielsz commented Dec 31, 2013

Yes, you are right about the default filter name. Change is committed.

Regarding imp-toggle-htmlize, if you set the user filter to nil, there will be no output for non-html buffers in the browser. Is this something we want?

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

Owner

The current use case for imp-toggle-htmlize is toggling the browser between "show me the highlighted HTML" and "show me the rendered HTML". This adaptation still satisfied that use case. I'm open to some kind of generalization that would handle other types of buffers but it's not something I see as required right now.

Owner

netguy204 commented Dec 31, 2013

The current use case for imp-toggle-htmlize is toggling the browser between "show me the highlighted HTML" and "show me the rendered HTML". This adaptation still satisfied that use case. I'm open to some kind of generalization that would handle other types of buffers but it's not something I see as required right now.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

Needs to call (imp--notify-clients) as its last statement so that the browser will get kicked into updating when the user changes filters.

Needs to call (imp--notify-clients) as its last statement so that the browser will get kicked into updating when the user changes filters.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

Needs to call (imp--notify-clients) as its last statement so that the browser will get kicked into updating when the user changes filters.

Needs to call (imp--notify-clients) as its last statement so that the browser will get kicked into updating when the user changes filters.

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

OK, I believe I've addressed all your points. Please let me know if I've missed something or whether there's anything else. Thank you.

Contributor

danielsz commented Dec 31, 2013

OK, I believe I've addressed all your points. Please let me know if I've missed something or whether there's anything else. Thank you.

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Dec 31, 2013

Owner

A friend of mine (and far better Elisp developer than I) is taking a pass through this PR to make sure things are in good shape. We'll proceed once he's put in his $.02

Owner

netguy204 commented Dec 31, 2013

A friend of mine (and far better Elisp developer than I) is taking a pass through this PR to make sure things are in good shape. We'll proceed once he's put in his $.02

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

Sounds great. The more eyeballs the better.

Contributor

danielsz commented Dec 31, 2013

Sounds great. The more eyeballs the better.

@skeeto

This comment has been minimized.

Show comment
Hide comment
@skeeto

skeeto Dec 31, 2013

Contributor

Hi, I'm the friend. I've made some additional changes here:

https://github.com/skeeto/impatient-mode/commits/user-filter

I've reworked things a bit to make it take better advantage of buffer-local variables. Since the default action is to do something (htmlize), imp-user-filter shouldn't have a default value of nil, which means "don't do anything." Disagreeing with netguy204 about hiding imp--default-filter, I renamed it to imp-htmlize-filter so that it can reasonably be seen as the default value for imp-user-filter. It's not an internal function but rather one of the officially provided filters.

Along the same lines I added imp-default-user-filters which is an alist mapping major modes to user filters. By default it lists html-mode and web-mode with a user filter of nil so that these buffers are passed unfiltered. The main idea for having an alist is that a hook isn't required when setting a default user filter for a particular mode.

(push '(my-major-mode . my-user-filter) imp-default-user-filters)

If someone doesn't like the global default of using htmlize, they can change the default value of imp-user-filter to whatever they like (using setq-default).

There were a few other small things. imp-last-state needs to be incremented before calling imp--notify-clients because there's a state change occuring. imp-set-user-filter needs to accept nil ("do nothing"), which wouldn't work with the fboundp, though the interactive prompt still won't allow it.

@netguy204 With my additions I recommend merging the PR.

Contributor

skeeto commented Dec 31, 2013

Hi, I'm the friend. I've made some additional changes here:

https://github.com/skeeto/impatient-mode/commits/user-filter

I've reworked things a bit to make it take better advantage of buffer-local variables. Since the default action is to do something (htmlize), imp-user-filter shouldn't have a default value of nil, which means "don't do anything." Disagreeing with netguy204 about hiding imp--default-filter, I renamed it to imp-htmlize-filter so that it can reasonably be seen as the default value for imp-user-filter. It's not an internal function but rather one of the officially provided filters.

Along the same lines I added imp-default-user-filters which is an alist mapping major modes to user filters. By default it lists html-mode and web-mode with a user filter of nil so that these buffers are passed unfiltered. The main idea for having an alist is that a hook isn't required when setting a default user filter for a particular mode.

(push '(my-major-mode . my-user-filter) imp-default-user-filters)

If someone doesn't like the global default of using htmlize, they can change the default value of imp-user-filter to whatever they like (using setq-default).

There were a few other small things. imp-last-state needs to be incremented before calling imp--notify-clients because there's a state change occuring. imp-set-user-filter needs to accept nil ("do nothing"), which wouldn't work with the fboundp, though the interactive prompt still won't allow it.

@netguy204 With my additions I recommend merging the PR.

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Dec 31, 2013

Contributor

@skeeto Solid advice and nice refinements. Thanks for helping out.
@netguy204 If I understood correctly, you're the one who pulls those commits. If you need something else, please do not hesitate to ask. Thanks.

Contributor

danielsz commented Dec 31, 2013

@skeeto Solid advice and nice refinements. Thanks for helping out.
@netguy204 If I understood correctly, you're the one who pulls those commits. If you need something else, please do not hesitate to ask. Thanks.

netguy204 added a commit that referenced this pull request Jan 1, 2014

Merge pull request #13 from danielsz/user-filter
user filter which takes precedence over htmlize-buffer if set

@netguy204 netguy204 merged commit 6ca4a07 into netguy204:master Jan 1, 2014

@netguy204

This comment has been minimized.

Show comment
Hide comment
@netguy204

netguy204 Jan 1, 2014

Owner

All done! Thanks @danielsz and @skeeto for the great PR.

Owner

netguy204 commented Jan 1, 2014

All done! Thanks @danielsz and @skeeto for the great PR.

@danielsz

This comment has been minimized.

Show comment
Hide comment
@danielsz

danielsz Jan 1, 2014

Contributor

Well done. Thank you, @netguy204 and @skeeto!

Contributor

danielsz commented Jan 1, 2014

Well done. Thank you, @netguy204 and @skeeto!

@danielsz danielsz deleted the danielsz:user-filter branch Jan 1, 2014

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