Explain why the color module was disabled on theme page

Can you please test the following?

1. Whether javascript is enabled in your browser
2. Your download method is set to Public
3. Your file system path is writeable

You can check 2 and 3 on admin/logs/status and change on admin/settings/file-system.

Nevermind,

I was able to determine the download method from the image on your site. Garlands color picker only works when the download method is set to public. That is by design.

However, a message would be nice, if only to prevent thousands of support requests.

Track this issue.

Changing components/title as this appears to be a color.module issue.

the comment is misleading:
// TODO: Disables the color changer when private files are used. (...)

This is already done. see #36 in the issue referenced in the comment: http://drupal.org/node/92059.

Read the entire comment please:

// TODO: Disables the color changer when private files are used. This should be solved in a different way. See issue #92059.

I did read the entire comment, as I have indicated above with (...).
My point remains that the wording of the comment should be improved, because without knowing the issue, it is confusing.
What comes immediately after TODO: should be the thing that is actually to be done. Split the comment in two lines, with what was done in the first line, and what's actually to be done in the second line. That second line would have the mention TODO.

Thank you for clarifying.

We don't have a 'warning' message type in core.

The comment is much clearer, now. Sorry for not explaining better the first time.

I tested the patch and it works fine.

The message must be improved, though.
It currently is:
"The color picker only works if the download method is set to public, and has been disabled."
which is again misleading.

I suggest writing simply this:
"The color picker only works if the download method is set to public."

I tested the patch and looked at the code, and I think that once the message is changed, it can be RTBC.

Will the color picker ever be available with the private method being used? I've got content I need to protect this way and it seems that two of the newest features in drupal (color picker and CSS speedup) are specifically not designed to work with the "private" method. Is this the way it will always be?

spyres: using the color picker with private files is not practical because private files require a full bootstrap to serve the file. This would be for the CSS file, for the images (more than a dozen afaik), etc. And it would not just be the first load, but also all subsequent pages too, because the browser needs to check if its copy of the image/css is still recent. So, it would essentially kill your site, or at least make it so slow as to be unusable.

The plan for the future is to change the "private" setting so it is actually "private and public". Modules such as color would always have a publically accessible directory where it can place its files, but other modules can respect your setting.

Yep, color.module should use /files/public/ with a separate .htaccess, or something to that effect.

I don't see the message as 'misleading'. Feel free to improve.

I've lost interested.

Steven: no need to activate the picker with private dnloads. But it would help if one read a msg saying that he can simply "temporary" use public dnloads until he did succeed in configuring his theme as needed. At that point one goes "private" and forgets about the picker. A msg is definetely needed, though. I just spent 4 hours trying to understand where and how I had spoiled my new install and was darn close to start it all over from scratch.

I agree that the message in #12 is just fine, and it would be very helpful until a better solution is developed...

I am sorry, but NO, message #12 is not fine.
It is not grammatically correct, and it implies something that is not true.
In essence, it says:

The color picker only works if the download method is set to public,
AND
[The color picker only works if it] has been disabled

It must be what it is saying, since the "download method" cannot be "disabled" in any way. Only set as private or public.

I understand that English is not your first language. I maintain that the statement needs work: it is inaccurate as it is.

Thank you for elaborating. I read it as

The color picker only works if the download method is set to public,
and [the color picker] has been disabled.

If the comma weren't there, I'd read it as you mention, but I'd change it to

    The color picker only works if the download method is set to public
    and if it has been disabled.

for clarity. To get the correct meaning, I'd keep the comma and rephrase it as

    The color picker only works if the download method is set to public,
    and it has been disabled.

As it stands, it's somewhere in between, but even with my limited abilities I wouldn't have any trouble understanding it correctly.

Anyway, it's a pity that this patch (with either wording) hasn't made it into 5.2 and 6. It would save a lot of hair-pulling...

At the bottom of http://drupal.org/patch/review it says "If you can, include an improved patch" — would it help if I took Heine's patch, changed the wording to what you suggested, and re-rolled the patch for 5.x-dev and 6.x-dev? I'd feel a bit stupid doing this, but if that's what it takes to get it RTBC, I'll do it.

(I'm not sure how to post a patch for two versions though...)

and against 6

Thanks, Heine!

Thanks.
The patch applies cleanly. The code change is the same as in earlier patches and is ok. The comment is good.

Looked at the 6.x patch and have the following two observations:

- http://drupal.org/node/92059 does not seem like some TODO for this code, it had patches already committed and deals with safe mode and out of memory issues, which are not related... I feel like missing something there, or this is a bad/outdated(?) issue reference.
- I am on the fence to suggest printing this message with 'error' and not simply a message, to be more visible and because this is kind of a settings conflict.

Let's discuss these two points. Otherwise the approach taken seems to be sane with me for now, until we have private and public methods decided per file, which is not going to happen before D7.

The rest of admin/build/themes/settings/garland is perfectly usable without the color picker, as is the result of applying the color picker and setting the downloading method back to private.

We should not lead anyone to believe that there is a conflict between customizing colors and the private downloading method. On the contrary, it might be reassuring to state that it's perfectly all right to change the downloading method to public, use the color picker, and change the downloading method back to private.

Forgetting to change it back to private could cause serious problems, and a corresponding warning might be helpful.

If you want to make the message more visible, how about putting it into the "Color scheme" fieldset?

Patch for Drupal5 apply with no errors on Drupal-5.2.

-thePanz-

Marking to needs work for Gabor's comments. Patch still applies with offset.

To address Gábor's comments:

- http://drupal.org/node/92059 does not seem like some TODO for this code, it had patches already committed and deals with safe mode and out of memory issues, which are not related... I feel like missing something there, or this is a bad/outdated(?) issue reference.

http://drupal.org/node/92059#comment-462398, for example, talks about "using the private files method". According to http://drupal.org/node/92059#comment-462492, this aspect of the issue wasn't fixed by the committed patch.

Another effort to fix the problem in 5.x was started in http://drupal.org/node/181003, so I guess the comment should now refer to that new thread.

- I am on the fence to suggest printing this message with 'error' and not simply a message, to be more visible and because this is kind of a settings conflict.

I gave my opinion in #28 above. No one else has cared to comment so far...

Patch rerolled for HEAD and comment pointed to new thread.

Hello,

I am not able to see the color picker for the Garland theme. I have checked my browser to verify scripting is enabled, my download method is set to public, and my file system path is set to "files".

I am testing on a WAMP5 personal webserver, what else might I be overlooking?

Thank you for any assitance you can provide.

Mikall~

@Mikall:
Check the info here http://drupal.org/handbook/modules/color and if that doesn't help then please open a new forum topic http://drupal.org/node/add/forum/22 rather than hijacking the discussion here. Thanks!

There is now a 'warning' message type in core, so I have amended the patch accordingly. I believe this finally resolves Gábor's remaining concern at #27.

@salvis: I have commented on using color.module with "private" download method at http://drupal.org/handbook/modules/color#comment-657434.

Cheekily setting status to RTBC since the change was so minor...

$ patch -p0 < color_picker_7.patch
(Stripping trailing CRs from patch.)
patching file modules/color/color.module
Hunk #1 FAILED at 28.
1 out of 1 hunk FAILED -- saving rejects to file modules/color/color.module.rej

Re-rolled patch for failed hunk. Not tested myself, setting status back.

Thanks hass, have tested this patch with HEAD (color.module 1.32), all looks good (when download method set to "private" warning message appears instead of picker for Garland but not Bluemarine which never shows the picker nor the warning message).

Great, committed, thanks.

We should backport this for D5... i think this is an important usability bug.

Seems to work as advertised.

Have used 'error' message type for the warning message since type 'warning' does not exist in D5.

I think a regular notice would be better. There is no particular error, just disabled functionality.

Fair enough!
Revised patch attached, a trivial change to the .patch file compared with #41.

Committed to 5.x.

Thanks!