Hey,
I just read this post:
http://www.therailsway.com/2007/6/1/railsconf-recap-skinny-controllers
And it made me think immediately of the old handle_new_or_edit_post in
page_controller
http://dev.radiantcms.org/radiant/browser/trunk/radiant/app/controllers/admin/page_controller.rb?rev=57
But now I go and look at the latest version, and I see it’s been
cleaned up a bit, and there a new AbstractModelController seperating
the logic a bit.
http://dev.radiantcms.org/radiant/browser/trunk/radiant/app/controllers/admin/page_controller.rb
and
http://dev.radiantcms.org/radiant/browser/trunk/radiant/app/controllers/admin/abstract_model_controller.rb
But still… you have a lot of logic in the controllers. And a lot of
that logic, if moved to the model, would be a lot easier to override
and modify in extensions.
For instance, I’d like to see all the part-creating/arranging logic
moved into the Page model.
96 parts_to_update = {}
97 (params[:part]||{}).each {|k,v| parts_to_update[v[:name]] = v
}
98
99 parts_to_remove = []
100 @page.parts.each do |part|
101 if(attrs = parts_to_update.delete(part.name))
102 part.attributes = part.attributes.merge(attrs)
103 else
104 parts_to_remove << part
105 end
106 end
107 parts_to_update.values.each do |attrs|
108 @page.parts.build(attrs)
109 end
110 if result = @page.save
111 new_parts = @page.parts - parts_to_remove
112 new_parts.each { |part| part.save }
113 @page.parts = new_parts
114 end
So, my question to the core devs is… how open would you be to patch
coming from me, that addresses some of these things.
I must admit, my agenda is based on making my own extensions more
elegantly integrated with Radiant. But I’m hoping to appeal to any
skinny-controller philosophy-followers as well.
thoughts?
Jacob
We would love a patch, of course! This will also help in a future
refactoring toward REST which is on the horizon.
Sean
Thanks Sean.
Looks like posting on the mailing list gets a much faster reply that
posting on Trac.
So then a follow-up question would be, what’s wrong with these
patches, why havn’t they been replied-to?
http://dev.radiantcms.org/radiant/ticket/504
http://dev.radiantcms.org/radiant/ticket/502
Jacob
I don’t know whether John has gone out of town yet or not, so I’ll do my
best to reply in his stead.
In general, the mailing list is faster because it’s more in-your-face.
Too, I think it’s a good principle to discuss something thoroughly on
the mailing list, then create a patch or ticket on Trac that addresses
the issue.
That said, I don’t often apply patches submitted via Trac; I let John
make the decision. However, as a general rule, patches that provide
small, incremental, well-tested changes that don’t break or
fundamentally alter the functionality of Radiant are more likely to be
accepted. Regarding #502, I think it’s a great idea, one I’ve been
thinking about myself, but I have not tested it yet (for various
reasons), and it changes a lot of core functionality. For #504, the
<r:navigation> tag has been a sore spot for a long time and is unlikely
to change in an incremental fashion. Also, just like the Rails core, if
we don’t it useful that way, it’s just not likely to make it in. #504
would be ripe for an extension, though – one of the reasons extensions
exist, in fact.
If you look at the roadmap, there are also tickets on there that have
existed for nearly a year – things that are important but have not
found a solution that is satisfactory to the core team. We’re only
human, and have a limited attention span and limited time to work on
Radiant. We appreciate contributions partly because they save us time,
and partly because they come from people who are passionate about the
feature they want to implement or bug they want to fix. Sometimes the
end result is not always exactly what was submitted, but open source is
about the communication and participation.
Moral of the story – don’t be discouraged if your patch or ticket gets
ignored. Discuss it on the mailing list and we’ll work it out together.
Cheers,
Sean
This is actually pretty old and the big thing missing from it is
backwards-compatibility. One thing I personally would like to maintain
about the navigation tag is that it is independent of the site
structure. Anyway, here’s the pastie of my original idea:
http://pastie.caboo.se/26340
Sean
Jacob,
Your patch adding the level tag in <r:navigation> is quiet tidy. In
my own Swiss Army tag library extension which I dump into nearly
every Radiant site I put up I have a similar tag <r:navigation_level
level=“2”>. Your version is cleaner. My tag gets use in combined with
an <r:level /> tag which returns the current depth/level in the tree.
Speaking to the bigger picture of core changes to the <r:navigation>
tag I think that Sean is right to suggest a more drastic refactoring
of the tag is in order. If I understand right Sean’s been playing
with some of these more drastic revisions himself.
Sean, do I have you wrong? Can we have a peek?
Loren
Sean C. wrote:
One thing I personally would like to maintain
about the navigation tag is that it is independent of the site
structure.
Agreed. I’m not entirely sure I like the syntax of the new navigation
tag. I wonder if there is a way to simplify it:
<r:navigation>
<r:items>
Web Log: /weblog/
External Link: http://externalsite.com/
<r:children:each>
<r:title />: <r:url />
</r:children:each>
</r:items>
<r:normal><r:title /></r:normal>
<r:here><r:title /></r:normal>
<r:selected>
<r:title /></r:selected>
<r:between> | </r:between>
</r:navigation>
Just a thought.
–
John L.
http://wiseheartdesing.com
Sean C. wrote:
In general, the mailing list is faster because it’s more in-your-face.
Too, I think it’s a good principle to discuss something thoroughly on
the mailing list, then create a patch or ticket on Trac that addresses
the issue.
In our defense I’ve asked that people talk about changes and problems on
the mailing list before submitting a patch or ticket:
http://dev.radiantcms.org/radiant/wiki#Contributions
That being said, we’ve done a poor job at keeping up with the tickets on
Trac and need to do a better at it.
That said, I don’t often apply patches submitted via Trac; I let John
make the decision.
Sean, feel free to apply patches on Trac if you feel they are well
tested and address the core goals of Radiant.
Moral of the story – don’t be discouraged if your patch or ticket gets
ignored. Discuss it on the mailing list and we’ll work it out together.
Yup, yup. Good stuff.
–
John L.
http://wiseheartdesign.com