Forum: Radiant CMS Radiant Core Dev: Page Controller

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
Jacob B. (Guest)
on 2007-06-01 19:05
(Received via mailing list)
Hey,

I just read this post:
http://www.therailsway.com/2007/6/1/railsconf-reca...

And it made me think immediately of the old handle_new_or_edit_post in
page_controller
http://dev.radiantcms.org/radiant/browser/trunk/ra...


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/ra...
and
http://dev.radiantcms.org/radiant/browser/trunk/ra...


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
Sean C. (Guest)
on 2007-06-01 19:10
(Received via mailing list)
We would love a patch, of course!  This will also help in a future
refactoring toward REST which is on the horizon.

Sean
Jacob B. (Guest)
on 2007-06-01 19:32
(Received via mailing list)
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
Sean C. (Guest)
on 2007-06-01 19:59
(Received via mailing list)
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
Loren J. (Guest)
on 2007-06-01 21:24
(Received via mailing list)
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. (Guest)
on 2007-06-01 21:46
(Received via mailing list)
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
John W. Long (Guest)
on 2007-06-01 22:28
(Received via mailing list)
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
John W. Long (Guest)
on 2007-06-01 22:42
(Received via mailing list)
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><a href="<r:url />"><r:title /></a></r:normal>
   <r:here><a class="here" href="<r:url />"><r:title /></a></r:normal>
   <r:selected>
     <a class="selected" href="<r:url />"><r:title /></a></r:selected>
   <r:between> | </r:between>
</r:navigation>

Just a thought.

--
John L.
http://wiseheartdesing.com
Sean C. (Guest)
on 2007-06-01 22:47
(Received via mailing list)
>    </r:items>
>    <r:normal><a href="<r:url />"><r:title /></a></r:normal>
>    <r:here><a class="here" href="<r:url />"><r:title /></a></r:normal>
>    <r:selected>
>      <a class="selected" href="<r:url />"><r:title /></a></r:selected>
>    <r:between> | </r:between>
> </r:navigation>
>
> Just a thought.
>
Much better!

Sean
This topic is locked and can not be replied to.