See #1029058: [META ISSUE] Change skin configuration for details.

CommentFileSizeAuthor
#12 patch_commit_60ad7e141418.patch35.49 KBmoonray
#10 patch_commit_fe92cfd27cee.patch29.1 KBmoonray
#7 skinr_1044222_7.patch28.13 KBmoonray
#6 skinr_1044222_6.patch28.11 KBmoonray
#3 skinr-HEAD.config-form.3.patch39.89 KBsun

Comments

jacine’s picture

jacine
she/her
Location New York
commented

I'm really happy about this one. Been wanting this for a while. :D

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Ugh. @

function comment_skinr_form_index_handler($op, &$form, &$form_state) {
...
    case 'submit':
      // Clear old variable before we set a new one if the node type has changed
      if ($form_state['values']['old_type'] != $form_state['values']['type']) {
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
StatusFileSize
new skinr-HEAD.config-form.3.patch39.89 KB

Technically, it should go like this...

coltrane’s picture

coltrane
he/him
commented

In #1029058: [META ISSUE] Change skin configuration sun said,

Remove skin configuration integration stuff from third-party forms; i.e., only keep Skinr's own edit form to configure a skin.

and looking at the patch I understand what it's doing, but for sake of newbies to Skinr (like me), can you explain what the purpose of this issue is? What is the ultimate goal?

moonray’s picture

moonray commented
Status: Needs review » Needs work

Well, looks like we're going to run into a big problem with panels (again) if we want to remove third-party forms. The way they do their contextual links is all custom, and they don't have a hook to get in there. Here a link to the issue I opened for it: #1051418: Need a hook_panels_pane_admin_links_alter() in template_preprocess_panels_pane()

As for removing all the above (see patch), I think that's way over the top. We need to keep skinr functional. I'll post a more reasonable patch that removes ONLY the third party forms (though we'd need panels to patch things in order for skinr to work with it... again).

moonray’s picture

moonray commented
Status: Needs work » Needs review
StatusFileSize
new skinr_1044222_6.patch28.11 KB

Attached patch only removes skinr config from third-party forms. It adds a few comments (for clarity), and the panels_panels_pane_admin_links_alter() function which will only work if above patch gets in (we can omit the function, but without it skinr can't be configured for panels).

moonray’s picture

moonray commented
StatusFileSize
new skinr_1044222_7.patch28.13 KB

Here's an updated patch using the suggested hook_panels_pane_content_alter() to make contextual links for panel panes. There is, however, no way to do this for displays or panel regions that I'm aware of (they don't have preprocess hooks for skinr to work with), so we would loose the ability to affect these if we implement this patch.

EDIT: Ran tests locally. No fails.

moonray’s picture

moonray commented
Priority: Normal » Major
rickmanelius’s picture

rickmanelius commented

So is the patch posted by mooray sufficient to fulfill the agreed upon request by sun and jacine?

Which 3rd party form would I test on to prove it's working?

moonray’s picture

moonray commented
Title: Remove skin configuration from third-party forms » Remove skin configuration functionality from third-party forms
StatusFileSize
new patch_commit_fe92cfd27cee.patch29.1 KB

Rerolled the patch.
As noted above, this will make it so there no longer is access to the following:

  • Edit skin for a panel display.
  • Edit skin for a panel region.

You can still edit skins for panel panes, though.
This is due to Panels module limitations (the display and regions both don't have proper preprocess functions and compatible template files).

moonray’s picture

moonray commented
Status: Needs review » Needs work

It looks like views changed things around and made it nearly impossible to add contextual links. Why always these custom implementations of good APIs?

moonray’s picture

moonray commented
StatusFileSize
new patch_commit_60ad7e141418.patch35.49 KB

#1149726: Create an 'add skin configuration' form to edit any element on the site should take care of elements that are inaccessible through contextual links (the cog).

Attached patch fixes views (see #1147912: Views contextual links should allow custom (non-views) contextual links) and panel pane contextual links. I've also updated the element names to use the '__' (double underscore) which is now fairly standard in drupal 7. It also allows us to avoid potential problems with postgres (see #1073108: Use of three-parameter concat function is bad for postgres compatibility). It also fixes a small bug where contextual links for regions would output the same as for a page.

moonray’s picture

moonray commented
Status: Needs work » Needs review

And let's review it.

moonray’s picture

moonray commented
Status: Needs review » Fixed

Committed this. See http://drupalcode.org/project/skinr.git/commit/dd98548

Since I'm not getting any feedback, I'm going to just go ahead and commit things. If stuff breaks, file a bug report.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Thanks for moving forward and making progress, @moonray :)

The committed patch looks good, code-wise. Especially happy to see lots of ugly code being removed.

AFAICS, the next best step in terms of major API changes and clean-up would be to entirely remove/simplify the following fugly construct:

+++ modules/block.skinr.inc
@@ -14,20 +14,6 @@
   $data['block']['form']['skinr_ui_form'] = array(
     'preprocess_hook_callback' => 'block_skinr_preprocess_hook_callback',
     'title' => t('block settings'),

@@ -63,10 +49,10 @@ function block_skinr_config_info() {
 function block_skinr_form_index_handler($op, &$form, $form_state) {
   switch ($op) {
     case 'form':
-      return $form['module']['#value'] . '-' . $form['delta']['#value'];
+      return $form['module']['#value'] . '__' . $form['delta']['#value'];
 
     case 'submit':
-      return $form_state['values']['module'] . '-' . $form_state['values']['delta'];
+      return $form_state['values']['module'] . '__' . $form_state['values']['delta'];
   }
 }

@@ -115,7 +104,7 @@ function block_skinr_preprocess_hook_callback(&$form, $form_state) {
 function block_skinr_preprocess_index_handler(&$variables) {
-  return array($variables['block']->module . '-' . $variables['block']->delta);
+  return array($variables['block']->module . '__' . $variables['block']->delta);
 }

The entire "index handler" thing is a very weird construct. IIRC, I starred for quite some time at it, but wasn't really able to figure out WTF it means and how it's supposed to work.

Given that we now rely on renderable elements (for runtime operation) as well as contextual links (for administrative UI), we should be able to completely revamp this essential part of Skinr (the actual API to integrate with other modules). Ideally, we standardize it to only allow for one way to integrate with Skinr. And that API standard should be very easy to understand for anyone who wants to integrate with Skinr.

This is highly important for Skinr's adoption throughout the Drupal community. Skinr is only going to be successful, if developers are able to grok and easily implement what Skinr asks for.

I'm not sure whether you already have ideas or plans in this direction, but we should definitely create a new issue for it, figure out what kind of tasks the remaining API/include files are performing, and lastly, discuss how that can be completely revamped and simplified.

moonray’s picture

moonray commented

@sun I would love to clean up the functionality plugins. I'm just not sure what a better way to handle this would be.

We have the following callbacks left after this last patch:

preprocess_hook_callback
Returns a list of all theme hooks (hooks used in the preprocess function) that this element could apply to. The given context is $form and $form_state from hook_form_alter().
index_handler for preprocess function
Returns an array of element IDs (e.g. for the User menu block, which is implemented by system module: 'system__user-menu') — rules are the only ones that currently return more than one — derived from the given context (the $variables array from the preprocess function).
contextual_links_handler
Returns an array of contextual links derived from the given context (the $variables array from the preprocess function).

EDIT: Are you suggesting we change these into hooks (e.g. hook_skinr_ui_preprocess_hooks(&$form, $form_state), hook_skinr_elements(&$variables), hook_skinr_preprocess_hooks(&$variables))?

moonray’s picture

moonray commented

Regarding the block_skinr_form_index_handler() function... that was supposed to be removed with the above patch. Did that in this follow-up commit: http://drupalcode.org/project/skinr.git/commit/1c68fa8.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I guess it would make sense to create a new issue for the API changes discussed since #15.

I'd have to have a closer look at the current code. However, it would be ideal if we could write down something along the lines of #16 in the new issue - but with some more essential details; for example, (not having looked at the code yet) the purpose of contextual_links_handler is not clear to me - I don't understand why some other module has to deal or assist with contextual links for Skinr UI at all. In such cases, we should also clarify the difference to having modules simply implement hook_contextual_links_view_alter() instead, or why that wouldn't be feasible.

The goal should be to have a single hook_skinr_config_info() [or whatever it was/is named] with clear and concise info properties being returned, and most of the module integration information being cached or cacheable. If something is based on dynamic values (retrieval/assignment), then there should be a dedicated hook that is being invoked, getting clear function arguments (for example, passing $form and $form_state to a preprocess_hook_callback is... highly confusing for me). Ideally, there should be default implementations provided by Skinr itself for the 80% use-case, so most module integration code doesn't have to implement common logic.

vrajak@gmail.com’s picture

vrajak@gmail.com commented

Late to the thread here, but I just wanted to comment that the patch in #12 works fine. Tested skins after patching and no errors etc.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.