966417 - Call Update from nsColorPicker

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Tom S [:evilpie]]

Attachment: color-picker

Comment 1

[Tom S [:evilpie]]

Attachment: update-color

Wrong patch!

Comment 2

[Arnaud Bienner]

Even if I wasn't asked for the review, just adding my two cents ;)
I remember we deliberately implemented the "Update" only for OS X because color pickers in native applications work like this IIRC (i.e. the button is updated while the user is changing the color, not only when it closes it)
And for Gtk applications, the color button isn't updated until "OK" is clicked, on applications we checked.
So we decided to go that way to be platform consistent.

That being said, it makes sense to also handle "Update" on other platforms indeed (e.g. live update while choosing a user, as you said on IRC).

However, I realized the Update isn't working as I expect currently (with your patch, but also very certainly for OS X): when user clicked "Cancel", the button isn't restored to the initial color (and IMHO, user expects "Cancel" to drop what he did and get him back to whatever color was chosen previously).

I think we should have something like:
    case GTK_RESPONSE_CANCEL:
    case GTK_RESPONSE_CLOSE:
    case GTK_RESPONSE_DELETE_EVENT:
        mCallback->Done(mInitialColor);

So in that case we keep initialcolor as an class attribute in Init function, and we delay the conversion (from string to GdkColor) to Open.

Comment 3

[Arnaud Bienner]

Actually
mCallback->Update(mInitialColor);
might be better, to avoid triggering a "change" event (which seems unnecessary if the user canceled the color selection).

Comment 4

[Tom S [:evilpie]]

Do you want to take over? I actually just did this to test the Update function in Bug 963294 on Linux.

Comment 5

[Arnaud Bienner]

As you prefer, but I can take care of this if you want, indeed.
If so, I will apply my modifications on top of your patch, which looks great to me (except the missing part I already mentioned).

Karl, what do you think about this?
- Are you OK to implement Update for color picker on Linux? (i.e. update the color button immediately according to what the user is selecting in the color picker)
- Do you agree with me that closing the color picker or hitting cancel should restore the original color?

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #5)
> Karl, what do you think about this?
> - Are you OK to implement Update for color picker on Linux? (i.e. update the
> color button immediately according to what the user is selecting in the
> color picker)

Yes.  It could even be useful to help associate multiple color pickers with their inputs.

> - Do you agree with me that closing the color picker or hitting cancel
> should restore the original color?

Yes.  Cancel should restore the original color, or it won't be cancelling anything.
The ideal behavior on closing the dialog is less clear.  I compared the behavior of the preferences dialog, which doesn't have a Cancel button, but I don't see a reasonable way to remove the Cancel button from the color picker.  When there is a Cancel button I think users expect closing the dialog to behave similarly to Cancel, so I agree with you.

The only other change I would suggest is renaming WidgetToColorSelection to WidgetGetColorSelection.

Comment 7

[Arnaud Bienner]

Attachment: Restore initial color when closing color picker/canceling color selection

Patch to restore the initial color when canceling/closing the color picker (on top of Tom's patch).

So now we delay the color conversion from Init to Open. I don't think it is a problem as it is not specified in nsIColorPicker.idl when we should return an error if initial color doesn't follow the specification.

Using a temporary object (color_gdk) in Open instead of mDefaultColor member isn't a problem either as the object passed to gtk_color_selection_set_current_color isn't kept (color components values are copied in gtk_color_selection_set_current_color).

I also renamed WidgetToColorSelection to WidgetGetColorSelection, as you suggested.

With this patch we are still calling mCallback->Done, which triggers a (useless) "change" event.
Using mCallback->Update (as I suggested in comment 3) to avoid triggering this event when canceling will not work, as some additional work is done in "Done" (mainly mInput->PickerClosed(); is called, so another color picker can be open after).
We might want to change nsIColorPickerShownCallback::done to add a flag to specify if we want to trigger the change event or not (to handle this special case), or something similar. But I'm not sure if it's worth the extra work needed, as I'm not sure it's a big deal to trigger an unneeded change event.

Comment 8

[Arnaud Bienner]

Attachment: two_diffs.patch

As I'm not sure you reviewed Tom's patch, here if the diff of our 2 patches.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8376874 [details] [diff] [review]
two_diffs.patch

Please land as one patch with both authors in the user field.
(We don't need to have a method land with one name in one changeset, and then be changed to a new name in the next changeset.)

>-  return (int(color_component)*255 + 127)/65535;
>+  return (int(color_component)* 255 + 127) / 65535;

I don't really mind whether there are spaces around '*', but one space on *each* side please, not just one side.

While here, can you remove the int() cast please because integral promotion will do that anyway.

>+  static_cast<nsColorPicker*>(user_data)->
>+    Update(colorselection);

Please unwrap here.

Comment 10

[Arnaud Bienner]

Attachment: bug966417-part1.patch

Comment 11

[Arnaud Bienner]

Attachment: bug966417-part2.patch

Comment 12

[Arnaud Bienner]

(In reply to Karl Tomlinson (:karlt) from comment #9) 
> Please land as one patch with both authors in the user field.

Not sure how to do this (after doing some googling, it looks like hg doesn't allow multiple authors for one changeset), so I updated Tom's patch. As it was only trivial changes, I think that it's fine.

Comment 13

[Karl Tomlinson (:karlt)]

The user field is just a string so it can be
"Tom Schuster <[e-mail]> and Arnaud Bienner <[e-mail]>"
but the separate patches are now good, thanks.

Comment 14

[Arnaud Bienner]

Marking the patches r+ as already reviewed by Karl in comment 9.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08a317d9cf0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/77c291ce6be5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08a317d9cf0f
https://hg.mozilla.org/mozilla-central/rev/77c291ce6be5