Facets branch extensibiliy extended

Hey Sean,

(and anybody else interested in facets)

I’ve further refactored facets, and refined my TinyMCE extension (whick
takes advantage of that refactoring).

Instead of:
admin.page.edit_partials.add “preview/preview_button”, :region =>
:buttons

I’ve made it:
Radiant::EditorUI.editors.default.add “preview/preview_button”,
:region => :buttons

My TinyMCE is added via:
Radiant::EditorUI.editors.add(TinymceEditorUI.new)

Where TinymceEditorUI is a subclass of Radiant::EditorUI::DefaultEditor

I’ve zipped up the entire thing and made it available at:

http://jotapajaro.com/dump/facetsv2.zip

Please check it out, let me know what you think. Let me know if you
think I’m going in the right direction. Let me know if you have any
questions.

What’s the best way to get in touch with the Radiant core team directly?
I’d like to discuss my ideas.

thanks,
Jacob

I’ve made it:
Radiant::EditorUI.editors.default.add “preview/preview_button”,
:region => :buttons

My TinyMCE is added via:
Radiant::EditorUI.editors.add(TinymceEditorUI.new)

And now that I’ve got that working. (You page preview extension works
properly) I could probably change it to:

admin.editors.default.add “preview/preview_button”, :region => :buttons

and:

admin.editors.add(TinymceEditorUI.new)

I just had to make it a seperate class because I was getting confused
with PageAdmin being defined inside of AdminUI. I’ve defined a seperate
file editor_ui.rb that sort of copies what admin_ui.rb does for admin
tabs to enable the user-selection of page editor.

I was also thinking I might make something like this work:

admin.editors.for_all_editors{ |editor| editor.add
“preview/preview_button”, :region => :buttons }

Where we use a block so we can save the block until all extensions have
loaded and added whatever editors are going to be added. And then once
it’s all loaded we can execute all the blocks that were passed via
“for_all_editors”.

With some exception catching. (In the above example if an editor has
already had it’s :buttons region removed we can’t add
“preview/preview_button” and we should just fail silenty, unless perhaps
there are no editors that this block executes successfully for)

As I said, it’s a basic start, and definitely not as stable as mental.
Could you provide me a diff of the changes to the core of ‘facets’?
(pastie or private attachment preferable). I’d like to review them and
work them into facets.

Sean

Hey Sean,

Here you go… : )

http://pastie.caboo.se/41244

(also at: http://jotapajaro.com/dump/facets.diff)

I implemented the “for_all_editors” I was talking about aboce…

Here’s the example of usage from page preview extension;

  • Radiant::ExtensionLoader::do_after_activate_extensions do
  •  Radiant::PageEditorUI.each_editor{ |editor| editor.add 
    

“preview/preview_button”, :region => :buttons }

  • end

Let me know if you have any questions,
I’m eager to see the changes I’m now relying on “officialy supported”

Jacob


And for anybody else who’s interested the latest version of my changes
along with TinyMCE and FCKEditor extensions is available at
http://jotapajaro.com/dump/facetsv4.zip

I finally got the chance to have a quick look at your changes.

I’ve only looked at the code, not run it, but in summary it looks like
what you want is to have a variety of different interfaces with which to
edit a page - potentially you’d want to look at the same page using two
different editors? Is that an accurate description?

The reason for you doing this is so that you can use fck or tinymce to
edit page parts. I think that a much better mechanism for doing so would
be to implement an fck/tinymce filter for the page and then insert a
partial directly after the edit_page_parts partial that inserts
javascript to listen for changes to the filter on the part and adjusts
the textarea accordingly.

Even better I would think would be to have an extension that completely
replaces the edit_page_parts partial with one that implements fck or
tinymce - I don’t really see a reason why you’d want to switch between
fck / tinymce / textile / markdown at runtime.

I find the interface for editing partials that’s currently in facets is
much cleaner and easier to use than your implementation (though I am
biased because I helped with the design) - the integration of editors is
a very specific usecase, and while your interface makes that specific
task more straightforward, I think it complicates the simple usecases
unneccesarily.

Dan.

How have I specifically hurt the simple use cases? Is there some
compromise we could make that would still give me a hook to have
multiple editors?

My code may be a little less elegant, but I don’t think it’s obtrusive.

The editor you didn’t see in my original submissions (but is in v5) is
the “content” editor. This is an editor with some features cut out
which is specifically setup for a “content” person to use. It’s only
supposed to show the partials that use can edit, and it hides things
like layout and page type. You can look at this editor in my v5
attachment.

I tried your suggested approach of having fck or tinyMCE load directly
into the current page structure. The problem with this is that it’s too
slow to use in practice. Both WYSIWYG editors take significant time
and bandwidth to load, and reloading them each time you tab between page
parts was unacceptably slow. Additionally, I couldn’t get tinyMCE to
work in Safari within the TabControl currently being used. It has to be
done the way I did it. (One WYSIWYG loads and the contents of the parts
are swapped in and out of it by the tabs)

The other reason for making facets support multiple editors is for
compatability. If there starts to be multiple extensions that alter the
page editor they can start to conflict. The easiest way to avoid
conflict is for each to define it’s own editor. I think it makes
writting extension a little more elegant too because you define all of
your page editor modification in your class which subclasses
DefaultEditor instead of having to define these changes in “activate”

Looking to the future, imagine very specific page types. A “calendar”
page type for instance. You don’t want to edit the calendar page with
the current editor, you want to edit it with a customized editor from
the calendar extension. And you don’t want all your page editors to be
calendar page editors. And your calendar editor should still re-use
components from the standard page editor, and still post to the
page_controller. This is the more “long term vision” problem I am
attempting to solve.

I’d like to further extend my work so that handle_new_or_edit_post in
PageController makes a call to some method of the editor_ui as a hook
for doing saving or whatever on whatever your editor has added to the
page. (In my case it’s page_attributes, and currently I’m just
overriding handle_new_or_edit_post)

In my latest version of facets, it’s possible to set
flash[:content_editor_ui]. If it is set, then it will use the editor
specified just for the current page edit. (And revert back to whatever
editor you have saved in your session for other pages)

I realize that:
Radiant::PageEditorUI.editors.default
isn’t as elegant as:
admin.page.edit_partials

I had envisioned doing something like:
admin.page_editors.default
This would be more consistend with the currently accepted admin.tabs.
method of adding tabs, But my ability to understand and debug
dependencies loading failed me.

What else are you objecting to?

Please suggest how I can improve my code to support multiple editors
without complicating the simple usecases.

On a side note,
I’ve found Radiant::ExtensionLoader::do_after_activate_extensions do end
to be usefull for more than just setting up multiple page editors. I’ve
found that getting dependencies to load in the proper order can be very
tricky, and this provides another hook for fiddling with the loading
order of things. (In my case, I had to load my extensions to
PageController in this block or I would get loading problems)

How have I specifically hurt the simple use cases? Is there some
compromise we could make that would still give me a hook to have
multiple editors?

My code may be a little less elegant, but I don’t think it’s
obtrusive.

I think it’s mainly because I can’t see the thought processes behind
most of what you’ve done. Hopefully I’ll get some time this weekend to
sit down and try and replicate your integration of the wysiwyg editors
and see if I can find the same problems with the current facets code.

I’m in total agreement on needing to have different interfaces for
different page types - but I would see the flow of page creation being:

New Page --> Select Page Type --> Display specific editor

Rather than have a selection of editors available directly on a page.

I’ve currently worked around this in my ‘groggy’ extension by overriding
the new method to change the page type on creation if it’s a child of a
certain page and then wrapping my partials in if statements. It’s
workable, but not extensable or easy.

I’d like to further extend my work so that handle_new_or_edit_post in
PageController makes a call to some method of the editor_ui as a hook
for doing saving or whatever on whatever your editor has added to the
page. (In my case it’s page_attributes, and currently I’m just
overriding handle_new_or_edit_post)

I originally saw this as a need when I first started working on my
admin_page_parts extension, but then realised that I could do everything
that I wanted with adding extra methods to the page to accept the new
attributes (by getting them through page.attributes = params[:page]),
and dumped all the code for it because my extensions never found a
usecase for it and it complicated things unnecessarily. Do you have an
example of where you’ve found it useful?

Dan.

Another dump of my “current” radiant setup is available for those who
are curious

http://jotapajaro.com/dump/facetsv5.zip

Waiting on Sean to react to my first round of facets changes before I
dump any more changes on him…

I welcome all critiques of my code.

Daniel et al,

Let me know if you want to look at working on facets this weekend. I
would like to complete it and prep it for merging with mental, pending
core team review, of course.

Sean

I know this is a little late (email from last month)… but in regard
to caching, isn’t the major problem with implementing sessions in
Radiant that the headers are cached as well? This seems to be over
optimisation… Just cache the page text and let the headers be
generated. Surely there isn’t that large a hit in performance, and
the advantage of using individual user sessions is great. Or make it
an option at least, since most user ACL type systems require sessions…

My use case for needing more than just a hook on page.attributes= is for
when page has_many

My extension adds:
has_many :page_attributes, :class_name => ‘PageAttribute’,
:dependent => :destroy

I need to capture each page_attribute that was added or deleted, similar
to the way page parts are managed. (But as I said, I’ve just overriden
handle_new_or_edit_post for now…)

Also, I think I can understand not wanting to add
_edit_select_editor.rhtml to the core. As long as you maintain the
usage of page_helper’s:

def editor_ui
if(flash[:page_editor_ui])
to_return = Radiant::PageEditorUI.editors[flash[:page_editor_ui]]
end
unless to_return and to_return != nil
to_return =
Radiant::PageEditorUI.editors[session[:page_editor_ui]]
end
if to_return.nil?
Radiant::PageEditorUI.editors.default
else
to_return
end
end

I can always make _edit_select_editor a part of my extension and add to
every editor during extension loading.

And most extensions can set flash[:page_editor_ui] for “simple” usecase.

to the way page parts are managed. (But as I said, I’ve just
overriden
handle_new_or_edit_post for now…)

Now I remember. I had the same problem with attachments - though in that
case when I changed around my interface to be more like sean’s it was no
longer a problem (removal and creation were done with an ajax calls) -
this has the problem that removals from the list take affect before the
user hits ‘save’, but there’s probably a decent workaround for that.

perhaps:

before_save :remove_page_attributes

def removed_page_attributes_id=(ids)
@removed_attributes = ids
end

private
def remove_page_attributes
page_attributes = page_attributes.select {|x|
!@removed_attributes.include?(x.id) } if @removed_attributes
end

And have javascript in your view so that a hidden
page[removed_attribute] input is added whenever an attribute is removed.

Not tested, but seems plausable.

Dan.

If you guys would like to keep up with my “usecase” for the facets
changes I proposed, you can now take a look at:

http://dev.eyebeam.org/projects/radiant-partatts/browse

Thank you to Jaime for setting this up.

Hopefully this will eventually morph into a the repository for Just the
partatts extension. (And supplemental TinyMCE and FCKEditor extensions)

Currently, it maintains a full version of radiant (modified facets) as
well.

I am more interested in making the Radiant a powerful platform, than I
am in simply getting my changes checked in to core. If you guys can
come up with a better API for supporting multiple page editors, I’ll be
happy to port my extension over to use it.

Using my API as a base, and improving it. Is also a possibility I
encourage.

Also… perhaps we should come up with a convention for extension
including static files. It’s no big deal to have to copy the fckeditor
javacripts into public to get the extension to work, but perhaps there
should be an official standard.

public/extensions//etc…

for example.

Jacob

I know this is a little late (email from last month)… but
in regard
to caching, isn’t the major problem with implementing sessions in
Radiant that the headers are cached as well? This seems to be over
optimisation… Just cache the page text and let the headers be
generated. Surely there isn’t that large a hit in performance, and
the advantage of using individual user sessions is great. Or make it
an option at least, since most user ACL type systems require
sessions…

To serve a page currently, the following steps are performed:

a) check for the existance of /cache/.yml
b) check for the validity of /cache/.yml
if file exists and is valid:
c) send /cache/.data

else
d) Call Page.find_by_url to find the page
e) call the page.headers method to set the page headers
f) call the page.render method to render the page
content

A quick experiment to see the performance impact of making a
Page.find_by_url call on every request - using my current database
containing a few hundred pages, I’ll pick a random deep page.

With the current caching mechanism, running:

ab2 -n 1000 -c 2
http://localhost/articles/2006/04/13/2006-melbourne-international-comed
y-festival-reviews/levelland/’

I ran it first using the current radiant caching mechanism, and then
again modifying the site_controller to always do a Page.find_by_url
regardless of whether the page is cached or not.

Original cache : 160 requests / second
Modified cache : 4 requests / second

So, “Surely there isn’t that large a hit in performance”… unless two
orders of magnitude doesn’t constitute a large hit for you, I hope you
see why the cache behaves as it does.

Dan.

Jacob,

Is there a public URL to check that out?

I found this link referenced for svn https://dev.eyebeam.org/svn/
radiant-partatts but it requires svn user to authenticate.

Apologies, I forgot to update the wiki to note that you should use
user/pass = tester/tester to checkout “anonymously.” We use LDAP to
authenticate and it’s unhappy with genuinely anonymous users.

http://dev.eyebeam.org/projects/radiant-partatts/wiki/radiant-partatts

@John-- should we move this branch-branch to radiantcms.org? I set
this up largely because it only takes 15 seconds

-jamie

Jamie M. Wilkinson wrote:

user/pass = tester/tester to checkout “anonymously.” We use LDAP to
authenticate and it’s unhappy with genuinely anonymous users.

I tried tester/tester for the user/pass, and got a 403 forbidden.

Worse, even after hitting the browser (Mozilla, recent) refresh button
several times, it doesn’t offer me a login screen to use for trying
again.

??

Thx.

B.


This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

On Feb 23, 2007, at 1:14 PM, Jacob B. wrote:

If you guys would like to keep up with my “usecase” for the facets
changes I proposed, you can now take a look at:

http://dev.eyebeam.org/projects/radiant-partatts/browse

Thank you to Jaime for setting this up.

Jacob,

Is there a public URL to check that out?

I found this link referenced for svn https://dev.eyebeam.org/svn/
radiant-partatts but it requires svn user to authenticate.

dm

David Minor wrote:

Brian,

the user/pass is for an svn checkout from:
https://dev.eyebeam.org/svn/radiant-partatts

and I just did a co with the user/pass that worked fine.

I mis-read his mail, and thought it was a Wiki.

Sorry for the noise. I’ll go back in my hole.

B.


This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

On Feb 25, 2007, at 1:20 PM, Brian Capouch wrote:

Brian,

the user/pass is for an svn checkout from:
https://dev.eyebeam.org/svn/radiant-partatts

and I just did a co with the user/pass that worked fine.

dm

What I was suggesting wasn’t that Page.find_by_url be called on
every page request… The original email was about changes to the
caching mechanism…
My suggestion, working from your outline:
a) check for the existance of /cache/.yml
b) check for the validity of /cache/.yml
if file exists and is valid:
c) call the page.headers method to set the page headers
d) send /cache/.data

else
e) Call Page.find_by_url to find the page
f) call the page.headers method to set the page headers
g) call the page.render method to render the page content

There’s a flaw in your logic - You can’t call page.headers until after
you’ve called Page.find_by_url.

Dan.