Not sure if I had to file this as bug, feature request, or support request.
When building the skinr ui form I need to render or omit things based on theme settings of the theme that is being 'skinned'. However, because the seven theme is loaded @ the ui form a call to theme_get_setting request a setting from the Seven theme. So we would have to know the theme-context within the skinr ui form so that you can do theme_get_setting('setting', $theme) and get the right setting for the theme that is acted on.
Comment | File | Size | Author |
---|---|---|---|
#33 | skinr-custom_widget_default-1299162-33.patch | 4.54 KB | moonray |
#28 | skinr-form_callback_default-1299162-28.patch | 574 bytes | rooby |
#17 | pass-by-ref-1299162-17.patch | 490 bytes | nedjo |
#14 | patch_commit_a94ef004e0dc.patch | 3.96 KB | moonray |
#13 | patch_commit_f944e3e256b8.patch | 2 KB | moonray |
Comments
Comment #1
Jeff Burnz CreditAttribution: Jeff Burnz commentedSubscribe :)
Comment #2
moonray CreditAttribution: moonray commentedSkinr UI adds a set of form elements with the following structure. You can deduce the currently used theme from there:
Comment #3
noland CreditAttribution: noland commentedjust wondering if this has anything to do with blocks only being rendered with the skinr settings applied for the site-wide default theme and ignoring their setting for another (enabled) theme whose template is actually being used?
I have 2 sub themes of Acquia Marina and each use skins from that base. One of my sub themes, ThemeA is the default, ThemeB is also enabled and used only in the Organic Groups context (using OG Theme). The group context rendered with ThemeB in that the logo and slogan of the site change but all the blocks and panes retain their ThemeA skins even though I set (and the settings persist) different skins for them under ThemeB. Theme Dev says that the blocks use ThemeB template.
Is this related to this issue?
Comment #4
moonray CreditAttribution: moonray commentedngermain: doesn't sound realted at all. please open a separate ticket. Also, going to need a bit better description. can't quite make out the problem you're having.
Comment #5
noland CreditAttribution: noland commentedOk. I think it actually is a bug and I think I have a fix for it. see http://drupal.org/node/1311046
Comment #6
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedThe $form variable appears to be unavailable in the .inc files in my theme. get_defined_vars() also doesn't give me any theme-context in the .inc file or within the hook implementations.
Maybe the solution to this, and other advanced theme integrations is to complete the @todo at skinr_ui.module#490:
// Create widget.
$field = array();
if (!empty($skin_info['form callback']) && function_exists($skin_info['form callback'])) {
// @todo Allow for custom form callbacks.
}
Comment #7
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedI'm trying to create a patch that does allows a skin to implement a callback function, but as we would want the callback function to be included, the MY_THEME/core.inc the inc file must be loaded otherwise the function does not exist when loading the skinr form.
Do you know if there is a function in the current skinr codebase that rounds up all the .inc files and includes them? We'll have to make it work so that we only call the .inc files that implement callbacks but that shouldn't be hard. I'm just asking to make sure I don't replicate any code in my patch.
Right now the .inc files are only loaded once after you clear cache and after that the form seems to be cached or something.
This what I got so far.
in skinr_ui.module around line #490
in my .inc file
Comment #8
moonray CreditAttribution: moonray commentedUse skinr_load_include() to load your includes. See skinr_hook() for an example of how to get the right path for your include and how to invoke skinr_load_include().
Comment #9
moonray CreditAttribution: moonray commentedAnother thought... the form callback should probably include a $context array with relevant data (such as $theme, $skin_name, and maybe $skin_info; these are derived from the variables used in the foreach loops in skinr_ui_form_alter()).
Comment #10
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedAllright sorry for the delay, here is my patch. I've reworked it a few times and I think now I better understand the Skinr code. I've tried to mimic skinr's variable names, caching strategy and code organisation so I've put the heavy stuff in a separate function in skinr.module and it only runs if there are actually callback functions that need to be loaded.
This is the first time I've made a patch file for a module, let me know if anything needs fixing.
Currently the callback function just loads skinr info files, searches for callback functions, and runs them with a contextual function argument that provides the currently enabled theme.
Comment #11
moonray CreditAttribution: moonray commentedFinally got around to reviewing this patch. It doesn't seem quite right...
What is the purpose of these 'skin callback' functions? What are you trying to accomplish with it? Do you have an example callback function?
What we envisioned originally is for a 'form callback' to be an alternative to the automatic widget creation.
If you look at skinr_ui.module around line 491 you'll see the following code which should be expanded upon:
I'm working on a patch that does the original intended behavior, and should (hopefully) work for your purposes.
Comment #12
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedEasiest way to see what's going on is hitting the edit-skin form in this profile I made that has the patch applied:
http://drupal.org/project/starterkit_arti
Generally speaking, the skin_callback functions are just hooks that any skin can use to do something before the form is built. In my case I use it to determine how many form fields I need and to theme the form.
This is one example callback function, just adding some CSS:
Another example of stuff I need to do in the skinr form to communicate with the theme settings:
And then after this code when I define my widgets I loop over the number of media queries that the theme is using to target different devices:
Comment #13
moonray CreditAttribution: moonray commentedGot the code for it written, just need tests.
Below is an example skin plugin file that uses a form callback.
The $context parameter is an array with the following items:
$context['theme'] // theme name
$context['skin_name'] // current skin name
$context['skin_info'] // current skin info array
Comment #14
moonray CreditAttribution: moonray commentedAnd here's a patch that includes tests.
Please review.
Comment #15
moonray CreditAttribution: moonray commentedDue to lack of feedback I've decided to commit this patch. If you have problems with it, reopen this issue.
Comment #16
nedjoThe commit contained a stray pass by reference that is causing fatal errors as reported in #1395386: Skinr Jan 5th dev release makes web site inaccessible.
Comment #17
nedjoPatch attached.
Comment #18
moonray CreditAttribution: moonray commentedThanks for catching that, nedjo. Committed.
Comment #20
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedThanks bala, well done you did a much cleaner job than I did. Sorry for being slow I was preoccupied with some projects.
I've done some small preliminary tests and it seems to work properly. I'll put this in my base theme soon.
Comment #21
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedThis patch is not working as intended. I had used it to ensure the my inc file gets loaded but now I actually need the intended purpose of this patch, namely to provide the theme context and use it to produce skins.
Problems with the patch:
1. The color-white example doesn't work, if I safe the form it doesn't remember the state of the checkbox, it's always false. Even if it does work, it seems that the callback is not supposed to be used for adding more or less skinr classes right? so what is this form callback intended for?
2. It doesn't provide theme context to the scope where it is useful. The $context['theme'] variable is not available in the scope of the hook_skinr_skin_PLUGIN_info info so it can't be used to create theme-specific skins.
What I want to do:
Create different sets of skins for different themes in hook_skinr_skin_PLUGIN_info, by querying theme settings.
For example: in the edit-skin form I see 2 fieldsets for 2 enabled themes e.g. bartik and seven.
Bartik and seven both have a theme setting called 'device_count' that specifies how many layout contexts they support.
Now if Bartik has this setting set to 5, in the edit-skin form I might need 5 "Block width for device X" skins where X is the index number of the device. If Seven has this setting set to 2, it will only need 2 layout skin widgets in the edit skin form.
This example is a simplification of the code I posted in comment #12, where I produce sets of skins in a loop, where the theme setting determines how many times the loop loops.
Because this patch provides the $context_theme after skins are defined in hook_skinr_skin_PLUGIN_info, there is still no way to define theme-specific skins as far as I know.
Do you think it's at all possible the get the theme context in the inc file? Because I notice the inc file gets loaded only once, not once for every theme.
Comment #22
moonray CreditAttribution: moonray commentedThe color white example listed above worked perfectly for me.
Note that it's being limited to system blocks, comments on the page node, and the page node (see the 'theme hooks' variable).
Context outputs the following for me, which I believe should be sufficient for you to get the theme settings you require.
Do remember that all possible options need to be defined in the hook_skinr_skin_PLUGIN_info() for them to be available...
Comment #23
lolmaus CreditAttribution: lolmaus commentedI'm on Skinr 7.x-2.0-beta1.
I've got a custom setting in my theme, that i access with
theme_get_setting
.In my
skins/myplugin/myplugin.inc
i have two functions:mytheme_skinr_group_myplugin_info() and mytheme_skinr_skin_myplugin_info().
When i call
theme_get_setting
from mytheme_skinr_skin_myplugin_info(), i get the setting. But from mytheme_skinr_group_myplugin_info(),theme_get_setting
returns an empty result. :(Thus, i fail to create groups dynamically based on that theme setting. I have to put all my settings in one bloated group. :(
How can i call
theme_get_setting
from mytheme_skinr_group_myplugin_info()?Comment #24
rooby CreditAttribution: rooby commentedThis feature does not seem to be documented.
Can someone give some information on its purpose/what you would use it to do?
I'm not sure I understand from reading back over some of the comments because it seems to me like something that could be done via form_alter as per #2 (although then it would not be within the plugin itself but the module that the plugin belongs to - is that the reason this is here?)
Comment #25
moonray CreditAttribution: moonray commented@rooby See comment #13. The purpose for having form callbacks is to allow custom form elements not defined by Skinr. You could use a textfield instead of checkboxes/etc. or add a set of form elements to set up the skin settings you wish to capture.
Comment #26
moonray CreditAttribution: moonray commented@lolmaus I'm not sure why you're getting the results you are. There isn't any code in Skinr that would cause
theme_get_settings
to return empty results. If you can get it inmytheme_skinr_skin_myplugin_info()
then you should get it inmytheme_skinr_group_myplugin_info()
(the callbacks are called right after each other inskinr_ui_form_alter()
, and in all other cases skin info is initialized before groups, so it's wouldn't be a matter of the theme settings not being available yet).Comment #27
rooby CreditAttribution: rooby commentedThat is actually exactly what I currently need.
Should mean I can do it cleaner than with form alter.
Comment #28
rooby CreditAttribution: rooby commentedThere is a problem I have come across that might be what the others are experiencing.
The default value is not passed through so you set a skin, then edit it and your setting is not repopulated.
This patch addresses it for me.
All you should have to do is add this to your element:
<?php 'default_value' => $context['default_value'], ?>
Comment #29
moonray CreditAttribution: moonray commentedCan you either provide a test or describe the steps required to repeat this error?
Comment #30
rooby CreditAttribution: rooby commentedI can't even remember now. When I get a chance I will roll back my patch and see what the error was.
I don't know how you would do without the default value as per the patch though. How else would you set the existing value when someone loads the form?
Comment #31
moonray CreditAttribution: moonray commentedI was confused with other issues.
You are right, we're not passing the default value to the custom form.
The patch in #28 should fix that. However, I prefer using (value instead of default_value for the context)
'value' => isset($defaults[$theme->name][$skin_name]) ? $defaults[$theme->name][$skin_name] : array(),
Comment #32
moonray CreditAttribution: moonray commentedAlso, we're going to need tests to make sure this doesn't get broken again in the future.
Comment #33
moonray CreditAttribution: moonray commentedAttached patch adds tests, and documentation in skinr.api.php.
Comment #34
moonray CreditAttribution: moonray commentedCommitted.