REQUEST: Theme configuration settings
|
Serge D
Joined: 2009-06-11
Posts: 2139 |
Posted: Fri, 2009-11-27 19:56
|
|
Can we allocate some dev time to make theme config settings accessible from admin console, please? I would think something like this would be easy implement since it is already supported for modules: Admin module to check current theme folder for controllers/admin_config.php, helpers/admin_config_event.php, views/admin_config.html.php, etc and apply logic in the same way as it is done for modules. a) I know about the time constrains, but feature would be very important for theme developers even with initial version. |
|
Posts: 7934
Something like this actually available now, in fact I'm updating the 3nids contrib theme to demonstrate how to do it. Stay tuned!
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 358
Thumb and I have been having discussions on theming, but I'm not sure where that will end up. I've also got so hacky code that pushes the administration of theme values into the theme. But that approach didn't quite give us the extensible approach that I thought it would, so that ended up as diff file on my pc as opposed an actual commit.
Tim
http://www.timalmdal.com
Posts: 2139
@bharat: please let me know when you have it available
Posts: 7934
Ok, I've taken a shot at it. Right now, the three_nids theme in gallery3-contrib is an example of how it works:
http://github.com/gallery/gallery3-contrib/tree/master/themes/three_nids/
NOTE: talmdal has not yet reviewed this and signed off on it! Even though I've submitted it, he may still shoot it down. So let's comment on this and please tinker with it, but give it a day or two to gel before you fully commit to it. Play with the three_nids theme in contrib to get an idea of what works and doesn't work.
Basically it works like this:
1) Your theme can have its own set of controllers, views, helpers, etc. Three_nids adds an item to the admin menu in themes/three_nids/helpers/three_nids_event.php with the admin_menu() function. This means that you can create your own controller to handle theme settings (yay!). You can also intercept any other events.
2) However, when we're in admin mode, your theme is not active. This sucks because Gallery won't find your controller and send it the request. So I've created the ability for themes to have their own admin mini-module in the "admin" subdirectory. themes/three_nids/admin is a mini-module. It's got its own controller and view for managing its settings. You can have whatever varied/diverse settings you want there without difficulty. So themes/three_nids/admin/controllers/admin_three_nids.php is accessible via gallery3/index.php/admin/three_nids.php (note that our naming convention for admin controllers is that it must start with "admin_" but it's routed via "admin/" -- this gives us a blanket level of security because we won't route anything under "admin/" if you're not an admin).
The one gotcha to this is that you want to modify the menu both in the regular and admin modes, this means that right now you have to duplicate the event listening code both in themes/xxx/helpers and themes/xxx/admin/helpers. I'm looking for ways to make this better.
Thoughts?
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 358
I haven't looked at the theme code, but I did look at the changes to the core to make this work. I'm not overly comfortable with this approach. In all honesty, I'm not sure why, but maybe I feel that we are trying to bend something here. I've gone around in circles a couple of times on whether the theme should have controllers. I can see the the use of having them.
My thinking on this is:
1) I've always said that themes were second class citizens in the hierarchy and as bharat pointed out, this was by design. I think once themes have controllers and event handlers directly, we might as well bag the concept of the theme directory and just make themes reside in the module directory. Let themes be first class citizen. It might take some work to clean up so that two active themes (admin and non-admin) don't clash.
2) If we don't do 1), I think we can take a similar approach to my efforts in doing this a couple of weeks ago. Don't have a theme admin directory. Have Gallery provide a single menu item by default "Theme options". It could then load the theme helper file that contains the views used to load the content pane of the admin page. The corresponding controller could load a theme helper that processed the input.
3) Revert this entire change, and step back and really give some thought and discussion to how we want to do this. This now the third variation on a "theme" (/me just breaks himself up). The first being the original, the second being my attempt a few weeks ago, the third is this one. The other approach to theming I've been giving a lot of thought to is making themes driver implementations. I really like the Kohana's driver paradigm and it really worked well for the identity management. I think that it has a lot to offer from a theming perspective. I just haven't had time to sit down and put thoughts "to paper"
So my suggestion is this: Bharat, take a diff of the changes and save them so the code's not lost and revert the changes for now so people don't get attached to them. Lets have some discussions on how best to do this (i.e. what can we learn from other CMS's). Theming is too important to the overall acceptability and usability of Gallery3 to back into an architecture with out taking into account all of the consideration. (i.e. embedding in the next release)
My $.02
Tim
http://www.timalmdal.com
Posts: 358
Another couple of cents
I think what I'm saying here, is that my approach was what I needed for playing with a theme. Bharat's approach is based on what was required for his playing with 3nids. I think we need to step back and decide what the correct approach is not for a particular theme, but for the Gallery3 product and then go forward.
http://www.timalmdal.com
Posts: 7934
@talmdal: I'm happy to revert my changes, but I'm not sure I understand the thrust of your argument yet. In both solutions, we're basically taking allowing the theme to possess some code. The question is: how much code is the theme *forced* to have? The more code we force the theme to have, the greater the burden on the themer. My philosophy is: a theme should not be required to have *any* code, but it can take advantage of the full freedom and flexibility of the Gallery3 API and the Kohana Cascading Filesystem if it wants to.
In the solution you had, each theme was required to provide its own configuration form. I argue that many themes will not need this, so I didn't want them to be forced to provide it. In your approach, we also had the problem that we were only loading up a few files from the theme module which meant that a theme was very limited in what it could do -- you couldn't use the Kohana cascading filesystem to access a view in your theme without doing something unusual. This felt like a weird subset to me: "in admin mode, this file is accessible, but only this file.. you can't get to anything else in the theme unless you do something outside of the norm".
We can fix this by making the theme a first class citizen but then
a) we blur the distinction between a theme and a module which will be confusing
b) it'll be harder to differentiate the needs of themes (themes have flavors.. site vs. admin) from the needs of modules (modules operate in both realms) and
c) themes will wind up living in the modules directory and it'll be harder to find themes among the modules.
Either way, we don't expect the site theme to be active in admin mode, so we have the strange situation of a module that's conditionally active.
My approach basically says: "this theme comes with its own bundled mini-module". This approach meshes nicely with Kohana's modular approach and it also nicely sidesteps Gallery3's concept of modules which means that for a trivial amount of code it works and it doesn't bend any rules. Individual themes don't have to have *any* code which means that our default theme is still a great example of what you can do without writing any PHP (outside of what we do in views). Anything in the mini-module is available in admin mode, and anything outside of it is available in regular mode.
I took the current approach of having a separate controller only because that's what the 3nids theme was already doing. But it's trivially easy to change this code such that we put the settings directly into the Theme Options page, as you were doing with your change. We just need to expose an event in Theme Options and have the theme/three_nids/admin/helpers/three_nids_event.php code piggyback on the form. I woke up this morning thinking that I should rewrite the theme to do that so that we don't have to bundle unnecessary controllers/views with the theme. This allows us to do pretty much everything your approach was taking, but it doesn't bend any of the rules and (I think) gives us the UI that you're looking for in option #2 above.
Further thoughts?
Edit: I updated the three_nids theme to get rid of the extra admin controller and instead roll it into the Admin > Appearance > Theme Options page. I believe this is now very similar to what you were doing in your change Tim, except that a) it's using events to modify the theme form and b) we're using the Kohana cascading filesystem to access the extra code.
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 2139
@to all: why should we mix modules and themes? we do not even need controllers...
Let's start from the begining:
to have theme settings we need ability to:
a) specify stored settings
b) build Theme settings page
c) persist settings from the page into DB
All said, it actually may be "easier" to have just one factory function and one inheritable page (in the theme forlder)
Function would allow define the setting: name, default value, if empty value allowed, description, error message, if applicable, and type of the field (text, textarea, checkbox).
Theme file would be used just to call this function so many time how many params is needed.
Something similar to graphics rules we have today.
After that everything is handled inside "Theme options" page: if theme has a file, collect the handled settings
Build the page content off the provided settings...
I think this would allow flexibility and provide much needed support for theme configuration.
Posts: 358
@bharat: not having an admin mini-module or an extraneous views and controllers in the theme works for me
http://www.timalmdal.com
Posts: 7934
@Serge D: you'll quickly find that the approach you're suggesting won't provide you enough flexibility. You'll want to have more complex validation and other features available as part of your theme settings, and having only one config file won't allow for that. This will force you to step outside of our normal patterns to try to load more code, etc. For example, the 3nids theme has a clear need for an extra controller to provide some additional ajax functionality. With the approach I've got in the code now, you still only need one file to add new content to the theme options form, but it's 2 directory levels deeper and these directory levels give you the option for a *lot* more flexibility. Again, look at how the three_nids theme is laid out in the contrib repo.
@talmdal: there's still an admin mini-module, but there are no admin related views and controllers in the theme. There's still an extra controller in themes/three_nids/controllers that the regular site interface needs to do some Ajax-related work.
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 2139
@bharat: any kind of validation is possible with JS as well as enabling some ajax functionality
yes, there are complex scenarios around but for theme configuration, would you really need to do so much of the code?
Ok, let's allow link of side JS files. Other then that from what I saw in most of the cases, simple is better.
I have tried to get your theme from git, followed readme and activated it. I have not found any additional settings anywhere. What should happen with admin page?
Posts: 7934
JS based validation is only good up to a point. For example, it will have difficulty properly localizing error messages. Is it necessary? Unclear.. but we're not relying on JS for validation elsewhere and we don't have a clear pattern for injecting JS validation into our current code.
If you're using the latest code from Git and you get the latest version of the three_nids theme, once you switch to the theme you should see extra settings appear in Admin > Appearance > Theme Options. Are you still not seeing it?
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 2139
@bharat: I have looked at provided SS. Main objection I have is that it keeps main theme settings intact. In my case I may not need header/footer text at all. Then I do not have ability to restrict it.
I think theme settings should be in full managed by the theme and not inherit any settings by default.
Posts: 7934
@SergeD: If you'd like to go that route, your theme can completely override theme settings by creating its own admin_theme_options controller. Ie, copy modules/gallery/controllers/admin_theme_options.php to themes/greydragon/admin/controllers/admin_theme_options.php and hack it up in any way that you'd like.
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 358
@SergeD, I ended up converting all the CRLF to LF in the -contrib version of the theme.
http://www.timalmdal.com
Posts: 2139
@bharat: will try to do that
@talmdal: yes, I have noticed. Changed Git setting on my end
Posts: 7934
@SergeD: if you have troubles merging talmdal's change back in, let us know and we'll help.
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 2139
@bharat: I have not found a way to merge some of the changes into fork, so I just recreated one...
Another thing: after syncing to latest git and trying suggestion for theme admin above, I only see a way to replace the persistance. Page layout is actually in another file called themes.php. If I am copy that logic into controller, I am getting desired layout, but when I am trying persist the setting it is not picked up from the form or persisted properly... see attached file
Posts: 7934
@SergeD: You almost had it right. On line 61, change:
$form = theme::get_edit_form_admin();to:
$form = self::get_edit_form_admin();That way the code that sanitizes input is working from the same form that you used to generate the output.
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 2139
Ah-ha! Thanks.
Will be nice also to move layout definition out of themes.php into controller...
Posts: 7934
We generally put our forms into the helper classes so that multiple controllers can access them. But in this case it might make sense to move the form into the controller.. I'll take a look.
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 2139
Wow! Admin module is done and will be available in upcomming release of the theme - 1.5.8.
Many good settings are now available. Thank you for the help.
Small request (it may be available already) - provide the way to insert [HR] into form to break settings into groups (do not mix with current groups).
Posts: 7934
Awesome!
Our approach (instead of using <hr>) is to create multiple groups inside the form, then use CSS to theme them as page separators. Let me know if that doesn't work for you. Oh-- I didn't read your message carefully enough. What do you mean by "do not mix with current groups" ?
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git
Posts: 2139
It is just personal, I like groups in some cases and <hr> in others.
In this case, I have no direct access to CSS for admin theme per se, so I am trying to go by markup itself.
If it is not much hassle, something like $group->divider() would be nice.
Posts: 7934
Can you file a ticket for that? It's probably not that hard (but I have to investigate a bit since Forge, the form library, is a 3rd party creation)
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git