Allow setting the link colours for wxHyperlinkCtrl in wxGTK3

[wxtrac]

Issue migrated from trac ticket # 17089

component: wxGTK | priority: low | resolution: fixed | keywords: GTK3 hyperlink alignment color

2015-08-01 17:25:14: @oneeyeman1 created the issue

---

Attached please find a patch that removes deprecated function for GTK+3 for setting the alignment for the wxHyperlinkCtrl.

Please review and apply.

[wxtrac]

2015-08-01 17:31:34: @oneeyeman1 commented

---

Sorry, I will have to redo the patch.

Apologies. Please hold on..

[wxtrac]

2015-08-02 00:41:43: @oneeyeman1 uploaded file hyperlink_align.patch (4.6 KiB)

Patch fixing the the ticket

[wxtrac]

2015-08-02 01:11:07: @oneeyeman1 changed title from Fix the alignment for GTK+3 to Make wxHyperlinkCtrl GTK+-3 complaint

2015-08-02 01:11:07: @oneeyeman1 commented

Vadim, et al, Attached please find a patch which make the wxHyperlistCtrl compile and properly work on GTK+ 3 port.

Please review and apply.

[wxtrac]

2015-08-02 01:18:28: @oneeyeman1 uploaded file hyperlink_align.2.patch (4.6 KiB)

Second version (fixes GTK+ 2 - ticket 13813)

[wxtrac]

2015-08-02 01:23:55: @oneeyeman1 commented

---

I made a small modification which resulted in a second version of the patch amd now I am able to open the "http://" URL only once on GTK+ 2.

Just for reference I have GTK+ 2.24 and 3.14.

The second version of the patch work properly on both versions on the widgets sample hyperlink control page.

Please review and apply.

The "Blocked By" field is not actually a blocked ticket, but rather a reference to already opened one (for convenience).

[wxtrac]

2015-08-02 03:03:38: @vadz commented

---

I don't think the version checks are correct. Their form should be

#!cpp_ NB: the preprocessor check is only necessary if there is some new API, not
_     if there is a new signal or something like this which would compile
//     with any GTK version.
#if GTK_CHECK_VERSION(x,y,z)
if ( gtk_check_version(x,y,z) == 0 )
{
  // code using API new in x.y.z
}
else // GTK is new enough at run-time
#endif // GTK is new enough at compile-time
{
  // code using old API
}

As always, splitting the patch in two parts (alignment fixes and clicked handling) would be very welcome too. TIA!

[wxtrac]

2015-08-02 15:43:33: @oneeyeman1 uploaded file 17809_1.patch (1.3 KiB)

Fix the event generation for hyperlink ctrl

[wxtrac]

2015-08-02 15:44:23: @oneeyeman1 uploaded file 17809_2.patch (1.2 KiB)

Fix alignment for hyperlink control

[wxtrac]

2015-08-02 15:44:59: @oneeyeman1 uploaded file 17809_3.patch (1.8 KiB)

Fix coloring of hyperlink control

[wxtrac]

2015-08-02 15:46:33: @oneeyeman1 commented

---

Vadim, Patch is fixed and splitted.

However, there will be one more patch for the hyperlink control, so don't close it yet.

[wxtrac]

2015-08-02 16:42:33: @oneeyeman1 uploaded file 17809_4.patch (1.2 KiB)

Correctly set the visited count for hyperlink control

[wxtrac]

2015-08-02 16:43:43: @oneeyeman1 commented

---

This ticket is complete. Please apply.

Most likely the code in the last patch was removed by mistake. I simply re-add it.

[wxtrac]

2015-08-03 07:09:49: @paulcor commented

---

You really need to learn to test your changes. Although in this case it would not have revealed your obvious gaff of checking for wxHL_ALIGN_RIGHT twice, you would have made the more important discovery that, in spite of the GTK+3 documentaion, gtk_widget_set_halign() does not actually seem to work as a replacement for gtk_button_set_alignment().

[wxtrac]

2015-08-03 14:05:03: @oneeyeman1 uploaded file hyperlink_left.png (190.7 KiB)

Screenshot of the sample on GTK+

[wxtrac]

2015-08-03 14:05:31: @oneeyeman1 uploaded file hyperlink_right.png (173.5 KiB)

Screenshot of the sample on GTK+

[wxtrac]

2015-08-03 14:06:34: @oneeyeman1 commented

---

Paul, Please look at the screenshot sample. Everything works as expected.

Please apply.

[wxtrac]

2015-08-04 05:58:14: @oneeyeman1 commented

---

Paul, This set of patches is not just about warnings, it also fixes the visited count (and so is coloring of the visited links) and it properly sets the signal to click the link.

[wxtrac]

2015-08-29 06:57:12: @oneeyeman1 uploaded file 17809.patch (3.0 KiB)

Patch fixing the ticket

[wxtrac]

2015-08-29 07:00:44: @oneeyeman1 commented

---

Paul, Vadim, Attached please find a consolidated patch that get rid of the deprecation warning for wxHyperlinkCtrl.

I made it on top of 13813.

Please review and apply.

[wxtrac]

2015-08-31 07:26:19: @Hanmac commented

---

i tryed to make it similar, but also add feature to Set the Normal and visited color, but its not optimal yet.

i do not make extra checks because i use GdkRGBA in gtk 3.0+ and GdkColor in gtk 2.0+

void wxHyperlinkCtrl::SetNormalColour(const wxColour &colour)
{
    if ( UseNative() )
    {

#ifdef __WXGTK3__
        const GdkRGBA *col = colour;

        GtkCssProvider *provider = gtk_css_provider_new();

        char *css = g_strdup_printf("* { link-color: %s; }", gdk_rgba_to_string(col));
        gtk_css_provider_load_from_data(provider, css, -1, NULL);
        g_free(css);

        // TODO : find a way to remove this provider again when the function is called again.
        gtk_style_context_add_provider(gtk_widget_get_style_context(m_widget),
                                       GTK_STYLE_PROVIDER(provider),
                                       GTK_STYLE_PROVIDER_PRIORITY_APPLICATION);
        g_object_unref(provider);
#endif
        // simply do nothing: GTK+ does not allow us to change it :(
    }
    else
        wxGenericHyperlinkCtrl::SetNormalColour(colour);
}

wxColour wxHyperlinkCtrl::GetNormalColour() const
{
    wxColour ret;
    if ( UseNative() )
    {
#ifdef __WXGTK3__
        GdkRGBA *link_color = NULL;
#else
        GdkColor *link_color = NULL;
#endif

        // convert GdkColor in wxColour
        gtk_widget_style_get(m_widget, "link-color", &link_color, NULL);
        if (link_color)
            ret = wxColour(*link_color);
#ifdef __WXGTK3__
        gdk_rgba_free (link_color);
#else
        gdk_color_free (link_color);
#endif
    }
    else
        ret = wxGenericHyperlinkCtrl::GetNormalColour();

    return ret;
}

[wxtrac]

2015-09-04 15:44:16: @Hanmac commented

---

i add a patch for using style provider to remove them again if a new one is created. (or can i simply load them with new data?)

might be a good idea for adding into the widget samples to set the normal and the visited color.

[wxtrac]

2015-09-08 07:54:59: @Hanmac commented

---

updated patch against recent changes from Paul. only disabling the warnings without using the new functions might bite us.

like to seen in the patch, there is also some new stuff, like making that the color can be set on Native GTK control. (using style provider)

[wxtrac]

2016-02-23 17:06:04: @vadz changed status from new to infoneeded_new

2016-02-23 17:06:04: @vadz commented

This is rather confusing as there are 2 different patches here. First one changes the alignment stuff while the second one seems to deal with colours only. I think it would be worth opening a separate ticket for the second patch and also describe what exactly does it do because it's not really obvious.

Also, for the second patch: it would be better to avoid including gtk.h from our public header, you can just forward declare stuff you need or even put them behind some m_impl pointer.

And for the first one, I'm not sure if there remains anything to be applied there, it looks like the problems it tried to fix were already fixed since then. If not, could you please explain what the remaining problems are and redo the patch against master? TIA!

[wxtrac]

2016-03-05 16:56:34: @oneeyeman1 changed status from infoneeded_new to new

2016-03-05 16:56:34: @oneeyeman1 commented

Should this ticket be closed? I just compiled 3.1 against GTK+3.14 and I don't see any warnings.

[wxtrac]

2016-03-05 17:44:48: @vadz changed status from new to infoneeded_new

2016-03-05 17:44:48: @vadz commented

The deprecation warnings are globally disabled for wxGTK3 since 36d6ddb8de4a9c29b4fc04a7450ac5bfd7839b17. I'm not sure if there is anything left to apply here, this was why I asked. I hope Hanmac can confirm that there is nothing left or update the patch, please don't reset its status to "new" if you don't know the answer.

[wxtrac]

2016-03-05 20:12:16: @oneeyeman1 commented

---

Vadim, Replying to [comment:17 vadz]:

The deprecation warnings are globally disabled for wxGTK3 since 36d6ddb8de4a9c29b4fc04a7450ac5bfd7839b17. I'm not sure if there is anything left to apply here, this was why I asked. I hope Hanmac can confirm that there is nothing left or update the patch, please don't reset its status to "new" if you don't know the answer. I modified Makefile to remove that option, recompile and I didn't see any warnings for hyperlink.cpp.

OT: should this option be applied to just some files, at least for now? The user code can turn those off, but for wx developers they are important as they will indicate the point of failure.

[wxtrac]

2016-03-06 09:39:06: @Hanmac changed status from infoneeded_new to new

2016-03-06 09:39:06: @Hanmac commented

Replying to [comment:17 vadz]:

The deprecation warnings are globally disabled for wxGTK3 since 36d6ddb8de4a9c29b4fc04a7450ac5bfd7839b17. I'm not sure if there is anything left to apply here, this was why I asked. I hope Hanmac can confirm that there is nothing left or update the patch, please don't reset its status to "new" if you don't know the answer.

i tested the patch, it still does what it should do. i removed a whitespace error from it (git did warn me)

hm what i want to do now is a to make the set-style more generic as you can see now the SetNormalColour and SetVisitedColour does share big chunks of code, and i want to make a function in wxWindowGTK for applying style. That will help us with other Style functions too.

So checkout for a 2.0 version of the patch that will happen soon.

[wxtrac]

2016-03-06 09:58:00: @Hanmac uploaded file gtkhyperlink.patch (5.4 KiB)

[wxtrac]

2016-03-06 22:19:57: @vadz changed priority from normal to low

2016-03-06 22:19:57: @vadz changed status from new to confirmed

2016-03-06 22:19:57: @vadz changed title from Make wxHyperlinkCtrl GTK+-3 complaint to Allow setting the link colours for wxHyperlinkCtrl in wxGTK3

2016-03-06 22:19:57: @vadz commented

We should avoid including gtk/gtk.h from the public headers, forward declaring GtkCssProvider should be enough. I also wonder if it wouldn't be better to use an opaque m_impl instead of putting this stuff into the public header; this would also allow us to use wxGtkObject (see include/wx/gtk/private/object.h) instead of error-prone manual g_object_unref() calls

It would be also better, in any case, to use wxGtkString (see include/wx/gtk/private/string.h) instead of the manual g_free() calls.

Last and least, but, still, I'd also appreciate if you could please use wx coding style, i.e. put opening braces on their own lines.

TIA!

[wxtrac]

2016-03-07 08:17:15: @Hanmac uploaded file gtkhyperlink2.0.patch (6.1 KiB)

[wxtrac]

2016-03-07 08:25:43: @Hanmac commented

---

Replying to [comment:20 vadz]:

We should avoid including gtk/gtk.h from the public headers, forward declaring GtkCssProvider should be enough. I also wonder if it wouldn't be better to use an opaque m_impl instead of putting this stuff into the public header; this would also allow us to use wxGtkObject (see include/wx/gtk/private/object.h) instead of error-prone manual g_object_unref() calls

It would be also better, in any case, to use wxGtkString (see include/wx/gtk/private/string.h) instead of the manual g_free() calls.

Last and least, but, still, I'd also appreciate if you could please use wx coding style, i.e. put opening braces on their own lines.

TIA!

i added a totally new patch where i also did try to apply your suggestions.

first i did move the apply css style to provider and widget to wxWindowGTK because we will need it later for other style settings like margin too. like #14809

second i added a wxHyperCtrlImpl class with added as private property. (so i don't get collide from other impl properties that might happen.

i added your suggestions from wxGtkObject (used in the impl) and wxGtkString (used only temporally while creating the css)

i tested the patch against trunk and the widgets example does still work.

if you have critic or different name suggestions, feel free to tell me.

An Idea would be to put even more logic into the Impl, but i need to think about what would be better.

[wxtrac]

2016-03-07 15:03:30: @vadz commented

---

I've created a PR with a slightly modified version of the patch, please let me know if you see any problems with it.

[wxtrac]

2016-03-07 15:12:00: @Hanmac commented

---

nope i am totally okay with that. i wouldn't had the idea with the LinkKind.

i will now go to other stuff like adding more checks to chkconf.

[wxtrac]

2016-03-07 16:22:40: @vadz changed status from confirmed to closed

2016-03-07 16:22:40: @vadz changed resolution from * to fixed*

2016-03-07 16:22:40: @vadz commented

Committed as 6ac52b3babe6986ef6ff28a782a17f520b12ff5d (not noticed by Trac again, argh), so let's close this. Please open new tickets for the remaining problems, if any. TIA!

[wxtrac]

2016-03-07 16:42:57: @Hanmac changed status from closed to reopened

2016-03-07 16:42:57: @Hanmac changed resolution from fixed to **

2016-03-07 16:42:57: @Hanmac commented

not finished yet, ScopedPtr seems incomplete:

In instantiation of ‘wxScopedPtr::~wxScopedPtr() [with T = wxHyperlinkCtrlColData]’ invalid application of ‘sizeof’ to incomplete type ‘wxHyperlinkCtrlColData’

bigger output:

./include/wx/scopedptr.h: In instantiation of ‘wxScopedPtr<T>::~wxScopedPtr() [with T = wxHyperlinkCtrlColData]’:
./include/wx/gtk/hyperlink.h:31:23:   required from here
./include/wx/checkeddelete.h:32:37: error: invalid application of ‘sizeof’ to incomplete type ‘wxHyperlinkCtrlColData’
         typedef char complete[sizeof(*ptr)] WX_ATTRIBUTE_UNUSED;              \
                                     ^
./include/wx/scopedptr.h:45:22: note: in expansion of macro ‘wxCHECKED_DELETE’
     ~wxScopedPtr() { wxCHECKED_DELETE(m_ptr); }
                      ^
./include/wx/checkeddelete.h:33:9: warning: possible problem detected in invocation of delete operator: [-Wdelete-incomplete]
         delete ptr;                                                           \
         ^
./include/wx/scopedptr.h:45:22: note: in expansion of macro ‘wxCHECKED_DELETE’
     ~wxScopedPtr() { wxCHECKED_DELETE(m_ptr); }
                      ^
./include/wx/checkeddelete.h:33:9: warning: invalid use of incomplete type ‘class wxHyperlinkCtrlColData’
         delete ptr;                                                           \
         ^
./include/wx/scopedptr.h:45:22: note: in expansion of macro ‘wxCHECKED_DELETE’
     ~wxScopedPtr() { wxCHECKED_DELETE(m_ptr); }
                      ^
In file included from ./include/wx/hyperlink.h:138:0,
                 from ./src/common/hyperlnkcmn.cpp:32:
./include/wx/gtk/hyperlink.h:19:7: note: forward declaration of ‘class wxHyperlinkCtrlColData’
 class wxHyperlinkCtrlColData;
       ^
In file included from ./include/wx/scopedptr.h:31:0,
                 from ./include/wx/translation.h:24,
                 from ./include/wx/intl.h:17,
                 from ./include/wx/wx.h:22,
                 from ./include/wx/wxprec.h:42:
./include/wx/checkeddelete.h:33:9: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined
         delete ptr;                                                           \
         ^
./include/wx/scopedptr.h:45:22: note: in expansion of macro ‘wxCHECKED_DELETE’
     ~wxScopedPtr() { wxCHECKED_DELETE(m_ptr); }

[wxtrac]

2016-03-07 17:14:31: @vadz commented

---

I thought this would work because the dtor is defined in src/gtk/hyperlink.cpp where wxHyperlinkCtrlColData is defined too and Travis didn't show any errors for this build. I'm going to fix it anyhow, but, just out of curiosity, which compiler gives you this error?

[wxtrac]

2016-03-07 17:22:44: @vadz commented

---

Also, as it seems to require wxScopedPtr dtor in wxHyperlinkCtrl ctor, would making this (or rather both of them) non-inline help?

[wxtrac]

2016-03-07 17:59:30: @Hanmac commented

---

i don't know if or how doing it inline would help.

the error does happen in ./src/common/hyperlnkcmn.cpp so it does need to be fixed in the header somehow.

my compiler:

gcc --version
gcc (Ubuntu 5.2.1-22ubuntu2) 5.2.1 20151010

[wxtrac]

2016-03-07 19:18:15: @vadz commented

---

Weird, with 5.3.1-10 it compiles just fine. Could you please check if this fixes it:

#!diff
diff --git a/include/wx/gtk/hyperlink.h b/include/wx/gtk/hyperlink.h
index 79aa619..a79f6f6 100644
--- a/include/wx/gtk/hyperlink.h
+++ b/include/wx/gtk/hyperlink.h
@@ -28,7 +28,7 @@ class WXDLLIMPEXP_ADV wxHyperlinkCtrl : public wxGenericHyperlinkCtrl
     typedef wxGenericHyperlinkCtrl base_type;
 public:
     // Default constructor (for two-step construction).
-    wxHyperlinkCtrl() { }
+    wxHyperlinkCtrl();

     // Constructor.
     wxHyperlinkCtrl(wxWindow *parent,
@@ -37,10 +37,7 @@ class WXDLLIMPEXP_ADV wxHyperlinkCtrl : public wxGenericHyperlinkCtrl
                     const wxPoint& pos = wxDefaultPosition,
                     const wxSize& size = wxDefaultSize,
                     long style = wxHL_DEFAULT_STYLE,
-                    const wxString& name = wxHyperlinkCtrlNameStr)
-    {
-        (void)Create(parent, id, label, url, pos, size, style, name);
-    }
+                    const wxString& name = wxHyperlinkCtrlNameStr);

     ~wxHyperlinkCtrl();

diff --git a/src/gtk/hyperlink.cpp b/src/gtk/hyperlink.cpp
index 2af1c6f..ea449dc 100644
--- a/src/gtk/hyperlink.cpp
+++ b/src/gtk/hyperlink.cpp
@@ -110,6 +110,21 @@ class wxHyperlinkCtrlColData
 // wxHyperlinkCtrl
 // ----------------------------------------------------------------------------

+wxHyperlinkCtrl::wxHyperlinkCtrl()
+{
+}
+
+wxHyperlinkCtrl::wxHyperlinkCtrl(wxWindow *parent,
+                wxWindowID id,
+                const wxString& label, const wxString& url,
+                const wxPoint& pos = wxDefaultPosition,
+                const wxSize& size = wxDefaultSize,
+                long style = wxHL_DEFAULT_STYLE,
+                const wxString& name = wxHyperlinkCtrlNameStr)
+{
+    (void)Create(parent, id, label, url, pos, size, style, name);
+}
+
 wxHyperlinkCtrl::~wxHyperlinkCtrl()
 {
 #ifndef __WXGTK3__

[wxtrac]

2016-03-07 19:28:34: @Hanmac commented

---

i did this, and it seems to fix the problem:

diff --git a/include/wx/gtk/hyperlink.h b/include/wx/gtk/hyperlink.h
index 79aa619..a79f6f6 100644
--- a/include/wx/gtk/hyperlink.h
+++ b/include/wx/gtk/hyperlink.h
@@ -28,7 +28,7 @@ class WXDLLIMPEXP_ADV wxHyperlinkCtrl : public wxGenericHyperlinkCtrl
     typedef wxGenericHyperlinkCtrl base_type;
 public:
     // Default constructor (for two-step construction).
-    wxHyperlinkCtrl() { }
+    wxHyperlinkCtrl();

     // Constructor.
     wxHyperlinkCtrl(wxWindow *parent,
@@ -37,10 +37,7 @@ public:
                     const wxPoint& pos = wxDefaultPosition,
                     const wxSize& size = wxDefaultSize,
                     long style = wxHL_DEFAULT_STYLE,
-                    const wxString& name = wxHyperlinkCtrlNameStr)
-    {
-        (void)Create(parent, id, label, url, pos, size, style, name);
-    }
+                    const wxString& name = wxHyperlinkCtrlNameStr);

     ~wxHyperlinkCtrl();

diff --git a/src/gtk/hyperlink.cpp b/src/gtk/hyperlink.cpp
index 2af1c6f..7d828f3 100644
--- a/src/gtk/hyperlink.cpp
+++ b/src/gtk/hyperlink.cpp
@@ -110,6 +110,20 @@ public:
 // wxHyperlinkCtrl
 // ----------------------------------------------------------------------------

+wxHyperlinkCtrl::wxHyperlinkCtrl() {  }
+
+// Constructor.
+wxHyperlinkCtrl::wxHyperlinkCtrl(wxWindow *parent,
+                    wxWindowID id,
+                    const wxString& label, const wxString& url,
+                    const wxPoint& pos,
+                    const wxSize& size,
+                    long style,
+                    const wxString& name)
+{
+    (void)Create(parent, id, label, url, pos, size, style, name);
+}
+
 wxHyperlinkCtrl::~wxHyperlinkCtrl()
 {
 #ifndef __WXGTK3__

[wxtrac]

2016-03-19 00:22:57: @vadz changed status from reopened to closed

2016-03-19 00:22:57: @vadz changed resolution from * to fixed*

2016-03-19 00:22:57: @vadz commented

This was closed by changeset:6f8002195011602e33b39d9a5b4fe96efa8dd967 but missed by Trac

[wxtrac]

2016-11-23 18:28:49: @paulcor commented

---

In 0ef695ef9b10429e89358af31d8b2f6ded948ff5: Revert d30673e5, it's completely broken. See #17089

[wxtrac]

2016-11-23 18:39:28: @paulcor commented

---

Sigh. It's hard to believe anyone actually tested this. It could not possibly work, and indeed, testing shows that it does not. First, there is no link-color CSS style property. It's a GtkWidget style property. Not the same thing. Next, the property is a GdkColor, so the code treating it as a GdkRGBA is completely borked. And link-color is deprecated and does nothing since GTK+ 3.12, released 2 years before this patch was applied.

Perhaps someone got confused about which `SetNormalColour() they were calling, as the base class version actually works. Sort of. Sometimes.

It would require considerable effort to implement this, if it's possible at all. IMHO having SetNormalColour()/SetVisitedColour() was a bad idea in the first place. If someone wants to spend the inordinate effort to correctly implement this (mis)feature, go ahead. In the meantime, I'm reverting d30673e5, it's completely broken.

[wxtrac]

2016-11-23 18:57:12: @oneeyeman1 commented

---

Paul, Replying to [comment:33 pcor]:

Sigh. It's hard to believe anyone actually tested this. It could not possibly work, and indeed, testing shows that it does not. First, there is no link-color CSS style property. It's a GtkWidget style property. Not the same thing. Next, the property is a GdkColor, so the code treating it as a GdkRGBA is completely borked. And link-color is deprecated and does nothing since GTK+ 3.12, released 2 years before this patch was applied.

Perhaps someone got confused about which `SetNormalColour() they were calling, as the base class version actually works. Sort of. Sometimes.

Do you have a code which does not work? Or I can check it with the sample?

Any idea what I need to do in order to make it not to work properly?

[snip]