[GTK3] GtkColorSelectionDialog is deprecated

RESOLVED FIXED in Firefox 43

Status

()

Core
Widget: Gtk
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Petr Jašíček, Assigned: Arnaud Bienner)

Tracking

38 Branch
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed, firefox44 disabled)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8652318 [details] [diff] [review]
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
Attachment #8652318 - Flags: review?(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.
Attachment #8652318 - Flags: review?(karlt) → review-
(Reporter)

Comment 3

3 years ago
Created attachment 8653344 [details] [diff] [review]
003_gtk3_color_chooser.patch

Thanks for the feedback Karl, I modified it.

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11bd4281d73b
Attachment #8652318 - Attachment is obsolete: true
Attachment #8653344 - Flags: review?(karlt)

Updated

3 years ago
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: Gtk
Ever confirmed: true
Product: Firefox → Core
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.
Attachment #8653344 - Flags: review?(karlt) → review-
(Reporter)

Comment 5

3 years ago
Created attachment 8655263 [details] [diff] [review]
004_gtk3_color_chooser.patch

I'm sorry about that Karl, I modified the patch.
Attachment #8653344 - Attachment is obsolete: true
Attachment #8655263 - Flags: review?(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.
Attachment #8655263 - Flags: review?(karlt) → review+
(Reporter)

Comment 7

3 years ago
Created attachment 8655369 [details] [diff] [review]
patch for check-in

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

Copying r+ from previous comment.
Attachment #8655263 - Attachment is obsolete: true
Attachment #8655369 - Flags: review+
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39f959c45807
Assignee: nobody → pjasicek
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 11

3 years ago
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...
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.
(Assignee)

Comment 13

3 years ago
(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
(Assignee)

Comment 14

3 years ago
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/
Flags: needinfo?(pjasicek)
(Reporter)

Comment 15

3 years ago
I am fine with your suggestion, Arnaud.
Flags: needinfo?(pjasicek)
(Assignee)

Comment 16

3 years ago
Created attachment 8674584 [details] [diff] [review]
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?)
Assignee: pjasicek → arnaud.bienner
Status: RESOLVED → REOPENED
Attachment #8674584 - Flags: review?(karlt)
Resolution: FIXED → ---
Attachment #8674584 - Flags: review?(karlt) → review+
(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
(Assignee)

Comment 18

3 years ago
(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.
(Assignee)

Comment 19

3 years ago
Created attachment 8675285 [details] [diff] [review]
bug1198256_deactivate_gtk3_color_picker_2.patch

Same patch, but with a proper commit message
Attachment #8674584 - Attachment is obsolete: true
Attachment #8675285 - Flags: review+
(Assignee)

Comment 20

3 years ago
bug1198256_deactivate_gtk3_color_picker_2.patch needs to be checked in.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/69e01c8ae9d0
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 24

3 years ago
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.
Flags: needinfo?(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.
status-firefox44: fixed → disabled
Flags: needinfo?(karlt)
(Assignee)

Comment 26

3 years ago
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]:
Attachment #8675285 - Flags: approval-mozilla-aurora?
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)
status-firefox43: fixed → affected
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.
Attachment #8675285 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b93831e01df
status-firefox43: affected → fixed
You need to log in before you can comment on or make changes to this bug.