1198256 - [GTK3] GtkColorSelectionDialog is deprecated

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Petr Jašíček]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150518070114

Steps to reproduce:

GtkColorSelectionDialog is deprecated according to https://developer.gnome.org/gtk3/stable/GtkColorSelectionDialog.html and compile deprecation warnings with GTK3 toolkit and it should be replaced by GtkColorChooserDialog.

Comment 1

[Petr Jašíček]

Attachment: 002_gtk3_color_chooser.patch

Patch replacing deprecated GtkColorSelectionDialog with GtkColorChooserDialog when compiled with GTK3 toolkit. It is compatible with GTK2.

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a162ff197e4a

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8652318 [details] [diff] [review]
002_gtk3_color_chooser.patch

nsIColorPicker only supports opaque colors as described at
https://html.spec.whatwg.org/multipage/infrastructure.html#simple-colour
so the “use-alpha” property on GtkColorChooser should be set to FALSE.

>+  return (int)(color_component * 255);

color_component * 255 + 0.5

>+  return (gdouble)(color_component / 255);

return color_component / 255.0;

>+  if (parent_window)
>+      gtk_window_set_destroy_with_parent(GTK_WINDOW(color_chooser), TRUE);

Mozilla style is to brace.
(Jump statements are sometimes accepted as exceptions.)

>+void nsColorPicker::ReadSelectedColor(GdkRGBA* color)

Please call this SetColor(const GdkRGBA* color) to clarify that color is not
an outparam but used to set the color rather than the method looking for a
selected color anywhere.

Comment 3

[Petr Jašíček]

Attachment: 003_gtk3_color_chooser.patch

Thanks for the feedback Karl, I modified it.

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11bd4281d73b

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8653344 [details] [diff] [review]
003_gtk3_color_chooser.patch

>+void nsColorPicker::ReadSelectedColor(GdkRGBA* color)

Please call this SetColor(const GdkRGBA* color) to clarify that color is not
an outparam but used to set the color in the nsColorPicker rather than the method looking for a selected color anywhere.

Comment 5

[Petr Jašíček]

Attachment: 004_gtk3_color_chooser.patch

I'm sorry about that Karl, I modified the patch.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8655263 [details] [diff] [review]
004_gtk3_color_chooser.patch

>+void nsColorPicker::SetColor(GdkRGBA* color)

Thanks.  r+ with (const GdkRGBA* color) here and in the declaration.

Comment 7

[Petr Jašíček]

Attachment: patch for check-in

Added (const GdkRGBA* color) to definition and declaration.

Copying r+ from previous comment.

Comment 8

[Petr Jašíček]

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a6d2fbce87c

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39f959c45807

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39f959c45807

Comment 11

[Arnaud Bienner]

One remark about this, as I worked on the initial implementation of Gtk's nsColorPicker:
I know GtkColorSelectionDialog is deprecated, but I don't like GtkColorChooserDialog for one reason that makes me like <input type=color>: the old one lets the user pick a color on the screen, and the new one isn't able to do this.

IMHO this was one of the nice feature of input type color, since there was no other way for web developers to use a "color picker than can pick a color on the screen" (e.g. when using a javascript based color picker).

Actually, Windows' native color picker can't do this either (OS X's color picker can do this though) and I believe it's better to use the most up-to-date native color picker of the platform, for coherency (another nice thing about using input type color IMO).

So I believe this is definitely a GTK3 problem.
I had a quick look around and didn't find a way to customize GTK3 color picker to do this, nor an alternative, official and non deprecated GTK3 color picker that might have this feature.

But if anyone with more GTK experience than me has an idea, or know if it is actually possible to do this...

Comment 12

[Karl Tomlinson (:karlt)]

I'd be happy if you'd like to back this out until we have solution, if you prefer, Arnaud.

I'd guess it is possible to customize the dialog, but I don't know how much work that would be.

GDK_VERSION_MIN_REQUIRED can be used to suppress deprecated warnings.

Comment 13

[Arnaud Bienner]

(In reply to Karl Tomlinson (back Oct 8 ni?:karlt) from comment #12)
> I'd be happy if you'd like to back this out until we have solution, if you
> prefer, Arnaud.

I'm really ambivalent about this... I don't like the idea of loosing coherency over other Gtk applications (which will almost all use eventually the new color picker) but "picking a color on a screen" was one of the nicest feature of input type color.

I've opened a GTK bug to discuss the issue:
https://bugzilla.gnome.org/show_bug.cgi?id=756132

Comment 14

[Arnaud Bienner]

Seems like GTK people are not against having this feature integrated [1] though they still need someone motivated to implement this.

I would suggest to rollback this until the problem is fixed on GTK side.
More precisely, as some good work has been done here, maybe we should just keep this code and add a define to disable it (with a comment explaining the reason and a bug number) to be sure we don't loose it?

Also, another issue with this color picker is that "color-activated" signal isn't working (at least for me) so the color button isn't updated (thus no input event) when the value is being changed by the user. This would prevent a web developer to do some "preview" of the result achieved by changing the color (example of what I mean: [2])
But our code looks good, so I have no idea what is wrong...

Petr, would that be OK for you? Or do you have any reason to prefer the new color picker over the old one?

[1]: https://bugzilla.gnome.org/show_bug.cgi?id=756132#c5
[2]: http://jsfiddle.net/arnaudbienner/fe050234/1/

Comment 15

[Petr Jašíček]

I am fine with your suggestion, Arnaud.

Comment 16

[Arnaud Bienner]

Attachment: bug1198256_deactivate_gtk3_color_picker.patch

Deactivate the new code for now using another macro check.

karlt: I tried your suggestion for removing the deprecated warnings but didn't managed to get it work :(
I guess it might work if gtk.h is included in .cpp files compiled independently, which is not the case here since several .h files including gtk.h header are in nsWidgetFactory.cpp (otherwise this would impact all the compilation unit I guess).
Otherwise I don't understand how this can work.

I tried to look at examples on our codebase but didn't find anything :( so if you have any ideas, they are welcome :) (but maybe it's not such a big deal to have these warnings?)

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to Arnaud Bienner from comment #16)
> I tried to look at examples on our codebase but didn't find anything :( so
> if you have any ideas, they are welcome :) (but maybe it's not such a big
> deal to have these warnings?)

The issue is not specific to this code, but see
https://bugzilla.mozilla.org/show_bug.cgi?id=1214953#c1

Comment 18

[Arnaud Bienner]

(In reply to Karl Tomlinson (ni?:karlt) from comment #17)
> The issue is not specific to this code, but see
> https://bugzilla.mozilla.org/show_bug.cgi?id=1214953#c1

OK, I thought you wanted to deactivate the warnings for the nsColorPicker.* files only, but regarding your last comment you want to turn off deprecated GTK3 warnings everywhere IIUC, and this is what bug 1214953 is about.

So I guess my patch is OK like this. It just misses a proper commit message: I will reupload another patch.

Comment 19

[Arnaud Bienner]

Attachment: bug1198256_deactivate_gtk3_color_picker_2.patch

Same patch, but with a proper commit message

Comment 20

[Arnaud Bienner]

bug1198256_deactivate_gtk3_color_picker_2.patch needs to be checked in.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69e01c8ae9d0

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69e01c8ae9d0

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69e01c8ae9d0

Comment 24

[Arnaud Bienner]

Now this has been integrated, I think we should backport this to beta, don't you think?
To not deliver to final users a version with the new GTK3 color picker, and have the next version using the old color picker again.

Comment 25

[Karl Tomlinson (:karlt)]

Bug 1207310 already disabled GTK3 builds for 42 Beta, but yes, it makes sense to uplift to 43 (currently Aurora).

The deactivate patch would have been better in a different bug so that it could have its own status/tracking flags.

Comment 26

[Arnaud Bienner]

Comment on attachment 8675285 [details] [diff] [review]
bug1198256_deactivate_gtk3_color_picker_2.patch

Approval Request Comment
[Feature/regressing bug #]:
Remove new feature added from the first patch of this bug.
New feature was to use new GTK3 color picker instead of GTK2 color picker for input type color, but GTK3 color picker is missing some features, so use GTK2 color picker for now.

[User impact if declined]:
According to comment 25, no impact since GTK3 builds are deactivated for 43, but it would better for coherency to have the same code everywhere.

[Describe test coverage new/current, TreeHerder]:

[Risks and why]: 
Low: this just reactivate old, tested code

[String/UUID change made/needed]:

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Agreed with Karl, this is a bit confusing to keep in the same bug. 
I'll call 43 "affected" for now. GTK3 builds aren't deactivated for 43 by the way; they are for 42.
We can set it to "disabled" after the patch lands on aurora if that will make the most sense (if we need to revisit this again)

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8675285 [details] [diff] [review]
bug1198256_deactivate_gtk3_color_picker_2.patch

Approved for uplift to aurora. 
This reverses some of the changes landed in September when 43 was in the nightly channel. Keeping GTK2's color picker for now.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b93831e01df