Hi,

It would be nice to do a bit of cleanup prior to looking at #352842: Support multi-groups

Here's a patch that fixes a few coding standard issues.

It also fixes the way permissions to edit a field is done. It has been recently changed in CCK. Please, see #336174: Provide a function to restrict view (based on current permissions) of field while using content_format or content_view_field

The javascript file could also be changed so that it uses Drupal javascript behaviors. That way, the namespace of the module can be protected and less prone to cause conflicts with other modules. (TODO).

CommentFileSizeAuthor
#5 editablefields.352884-5.patch8.46 KBmarkus_petrux
editablefields.module.patch10.71 KBmarkus_petrux

Comments

markfoodyburton’s picture

markfoodyburton commented

Markus, thankyou so much for spending the time looking at this, and sorry my coding style isn't clean...
Patch applied to the drupal-6--1 branch,
I get a lot of "warning: Missing argument 3 for content_permissions_field_access() in /home/www/drupal/drupal/sites/all/modules/cck/modules/content_permissions/content_permissions.module on line 21." errors when using this though, and (of course) it has to be used with the very latest CCK...
(I'll take a look at the warnings.....)

For the javascript code - can you point me to the places you think I should update things? I'm not sure what the 'official' javascript API is?

Cheers

Mark.

markus_petrux’s picture

markus_petrux commented

For the warnings... you would have to run the latest CCK2, from the dev package.

For the javascript... yo may wish to look at how Drupal behaviours are used in several .js files of Drupal itself. See textarea.js for a quick example.

Drupal.behaviors.editablefields = function(context) {
  // your code goes here... and it will be executed automagically on page load.
  // you can also put your functions here, or add them to the Drupal object using
  // your module name as a prefix.
};

For the path to the module menu handlers... maybe you could use drupal_add_js with type 'setting'. That way it would work on sites where Drupal is not installed on the root of the web space.

markfoodyburton’s picture

markfoodyburton commented

For the warnings, yched just fixed that, see: http://drupal.org/node/336174
For the JS, I'll get onto it, thanks!

Cheers

Mark.

markfoodyburton’s picture

markfoodyburton commented

For the JS stuff, I have now committed a version which I _think_ is better.
I've tested it a bit, and it seems to work, but I'd appreciate feedback.

I think I should wait to make a release until CCK releases, as this depends on the current -dev version of CCK.

Markus, please add more cleanup suggestions here, or close this one :-)

Cheers

Mark.

markus_petrux’s picture

markus_petrux commented
StatusFileSize
new editablefields.352884-5.patch8.46 KB

Here's a new round. Fixing remaining conding standard issues and a few more things. The issue with on/off widget still remains pending to fix here.

There's a problem I see in this module, it is that one may not wish/need to have the fields editable in node view mode. One possible way would be to behave like this demo:

http://www.quirksmode.org/dom/cms.html

Well, more or less. The idea would be to render the fields in normal view mode, but allow them to swicth to the editable widget when needed.

Also, maybe the module would have to display a small animation during field post processing. Maybe an animation similar to the one used for autocomplete widgets.

I hope you don't mind, but all these minor changes are important, IMHO, in order to approach a potential more complex change such as adding support for the multigroup module.

markfoodyburton’s picture

markfoodyburton commented

Wow - major re-write - and - agreeably cleaner :-)
I've committed this,

For the 'url' settings, this is cleaner than the way I did it, but I was just using the 'built in' settings...
I _love_ the 'disabled' thing, thats neat.

THANKS!

Cheers

Mark.

markfoodyburton’s picture

markfoodyburton commented

For #5...

We could indeed start the field in non-editable mode and have on-click change it to be editable... why not - that would seem like a reasonably easy thing to change? (though maybe this isn't 'obvious', maybe the user WANTS it editable - maybe we should have 'permanently editable' and 'potentially editable' or some such....

For the 'spinning disk': for the D5 version I tried and failed to get it working, then ended up with this 'opacity' effect - and then realised that if I did go with the spinning disk I'd have to 'know' about the layout of the widget... which I dont.... so in the end, I actually LIKE the opacity thing.... I guess one could make the whole thing use the opacity thing, then somehow superimpose some sort of spinning disk (a-la apple) ontop... may look nice.... I could play....

markfoodyburton’s picture

markfoodyburton commented

#5 part one - added to the CVS. I hope I haven't broken anything!

markfoodyburton’s picture

markfoodyburton commented

#5 part 2 now needs more - I'll try adding a 'doing stuff' type thing, also the 'click to edit' mode probably needs some sort of hint that you can 'click' it (at least something that a themer can choose to hide if they dont want to see it)...

markus_petrux’s picture

markus_petrux commented

Oh, it seems your editor introduced indentation issues back in the latest commits. :(

As per the use of content_access(), which makes your -dev version dependent on the -dev version of CCK. Maybe you could do something like this:

function editablefields_check_edit_access($field) {
  if (function_exists('content_access')) {
    return content_access('edit', $field);
  }
  // Backwards compatible code with CCK 2.1.
  $field_access = module_invoke_all('field_access', 'edit', $field);
  foreach ($field_access as $value) {
    if (empty($value)) {
      return FALSE;
    }
  }
  return TRUE;
}

Then:

-  if (!content_access('edit', $field)) {
+  if (!editablefields_check_edit_access($field)) {
markfoodyburton’s picture

markfoodyburton commented

I've just added loads of throbbing stuff (and checked it in) -- it looks OK, I'm no graphics designer, so somebody else can have a hack if they like at making it pretty. The one 'mantra' I've been trying to follow is to make it so that everything can be re-themed (so, the javascript does not call in the images directly, it just adds a class which has a background image in the .css)... So - I hope this is a reasonable solution that people can work with.

Always open for ideas to make it better.

For the 'CCK -dev' thing.
If everybody can test out the editablefields as it stands, with the current CCK, __IF__ CCK isn't going to be released for a long time, and __IF__ too many people complain that they need a stable version, then we should make the release, but I'm not inclined to put backwards compatibility code in at this stage?

What do you think?

(For the indentations, I'm using xemacs - and it didn't seem to me that is had changed anything from the way you had it. Of course, I prefer cuddled else's, but - I'm not bothered :-) --- vi/emacs wars -- love'em!)

markus_petrux’s picture

markus_petrux commented

There seem to be something wrong in your editor. Just see your source from CVS.

http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/editablefie...

markfoodyburton’s picture

markfoodyburton commented

MMm, VERY odd...
I _think_ my editor is putting in tabs where it should be using spaces... YUK
Anyway, either way, the huge indent isn't needed, so - I've made it a bit more reasonable ;-)

(Thanks and sorry!)

Cheers

Mark.

markus_petrux’s picture

markus_petrux commented

One thing you could try is the coder module. If you run this from time to time, it can help you learn the coding standards (there's more than just indentation) and produce code with more quality and easier to browse for others.

You can close this issue when you wish.

markfoodyburton’s picture

markfoodyburton commented
Status: Active » Fixed

Did so - is now passing cleanly (actually, didn't seem to be too many issues, apart from some string concatenation things and some empty lines..)

So, I'll close this for now, but please re-open if you find issues :-)

Cheers

Mark.

Status: Fixed » Closed (fixed)

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