Sounds fine to me.

Created attachment 127840 [details] [review]
[PATCH] Bug 565665 – GtkTargetEntry member target should be const char *

 gtk/gtkselection.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Ping? Can we get this for 2.18 ?

Matthias, can we commit this now?

Why not yet commited? This is really annoying, causing warnings in lots of apps.

I'd like to second Jaroslav's sentiment.  Please commit this patch.

Pushed to master and 2-24.

(In reply to comment #0)
> One is not expected to modify ->target.

So surely that should be documented? It isn't so far:
http://library.gnome.org/devel/gtk/stable/gtk-Selections.html#GtkTargetEntry
http://library.gnome.org/devel/gtk/unstable/gtk3-Selections.html#GtkTargetEntry

In gtkmm, we do set this to the result of a g_strdup(), though only in the implementation of a Gtk::TargetEntry::set_target() method that hopefully nobody uses, though time will tell. Hopefully I can get away with const_cast<>ing and just deprecating all the TargetEntry::set_*() methods.

Was this intended to be committed to gtk-2-24 branch [1]? (this bug has version 2.91.x identified)

This commit breaks Firefox 3.6 build (Version: 1.9.2.12) with:
nsDragService.cpp:1173:38: error: invalid conversion from ‘const void*’ to ‘void*’

http://git.gnome.org/browse/gtk+/commit/?id=b3c5232a9b63f23aca4909a0a47358b53162d70b

(In reply to comment #8)
> (In reply to comment #0)
> > One is not expected to modify ->target.
> 
> So surely that should be documented? It isn't so far:
> http://library.gnome.org/devel/gtk/stable/gtk-Selections.html#GtkTargetEntry
> http://library.gnome.org/devel/gtk/unstable/gtk3-Selections.html#GtkTargetEntry

In fact, the change makes no sense to me. We are obviously meant to set GtkTargetEntry::target at least initially. It might be efficient for a C application to just use a const char* string literal, but I don't see the advantage in suggesting that applications (or language bindings) can't set this from a newly-allocated string and g_free() it later.

I'd like to see what compiler warnings this was actually causing.

The code in comment 0 produces this warning with -Wwrite-strings:

warning: initialization discards qualifiers from pointer target type

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #0)
> > > One is not expected to modify ->target.
> > 
> > So surely that should be documented? It isn't so far:
> > http://library.gnome.org/devel/gtk/stable/gtk-Selections.html#GtkTargetEntry
> > http://library.gnome.org/devel/gtk/unstable/gtk3-Selections.html#GtkTargetEntry
> 
> In fact, the change makes no sense to me. We are obviously meant to set
> GtkTargetEntry::target at least initially. It might be efficient for a C
> application to just use a const char* string literal, but I don't see the
> advantage in suggesting that applications (or language bindings) can't set this
> from a newly-allocated string and g_free() it later.
> 
> I'd like to see what compiler warnings this was actually causing.

It makes sense. GTK functions that expects GtkTargetEntry make copy of the string anyway, setting this from const string literal is less pain, then having to g_strdup the string, check if result is not null, if not, pass it to function which does yet another string copy, then free it. And if one needs to create array of these for multiple targets, one end up with code like:

entries[0].target = g_strdup("target1");
if (entries[0].target)
{
  entries[1].target = g_strdup("target2");
  if (entries[1].target)
  {
    // perhaps 8 more here, making it messy
    call_some_function(entries, 10);
    g_free(entries[1].target);
  }
  g_free(entries[0].target);
}

And there is no reason to keep the array stored somewhere, it is not needed anymore.

It is much easier to write

GtkTargetEntry entries[] = {
  {"target1", ...},
  {"target2", ...},
  {"target3", ...},
  {"target4", ...},
};

And one doesn't need to do any allocations (and then check if they succeeded). In various bindings, they have to copy their on structures/classes into array such as this anyway, so they can add extra field into their bindings or just use ugly typecast.

If not change it for GTK 2.*, than at least change this for GTK 3. It is not final and people creating bindings will have to make changes anyway.

About this: "or just use ugly typecast".
Yes, I could also do typecast in my code, but const string literal in C can be realy const (mapped in read-only memory) and writing to it might cause segfault, so it makes no sense to typecast real const char* to char*, but it makes sense to typecast char* to const char*, because it is writable and readable (if it weren't readable, it would make no sense to pass it to any GTK function using that field). And when one knows it was writable before, they can typecast it back in the bindings.

And even if (lets say) gtk_tree_view_enable_model_drag_dest takes const GtkTargetEntry*, that only guarantees const access to the struct itself, but one could think, that it will modify target string (not the pointer) because it is not const. But it doesn't, so there is no reason to have that field non-const.

(In reply to comment #12)
> It is much easier to write
> 
> GtkTargetEntry entries[] = {
>   {"target1", ...},
>   {"target2", ...},
>   {"target3", ...},
>   {"target4", ...},
> };

Fair enough.

The problem now is only really that code ends up g_free()ing the const char*, causing a warning, but it does indeed make sense for the char* to be const for the purposes of the GtkTargetEntry itself. The application should theoretically have a non-const pointer somewhere that it would call g_free() on, or the application would know what it's doing and use a cast (such as const_cast<> in C++.

So I withdraw my objection and I'll just file this under "bare C structs are awkward for language bindings even though they are efficient for C". Sorry.

I've reverted the commit, since it causes warnings. 

Somebody needs to come up with a proper working patch.

(In reply to comment #15)
> I've reverted the commit, since it causes warnings. 
> 
> Somebody needs to come up with a proper working patch.

What causes warnings? Applying the patch or compiling GTK with the patch applied? I did not test it myself, I just wanted to report this and saw it was already here.

(In reply to comment #9)
> Was this intended to be committed to gtk-2-24 branch [1]? (this bug has version
> 2.91.x identified)
I also think it should go to master. Having this in gtk-2.0 breaks FF/xulrunner...

Maybe we can guard the change with #ifdef __cplusplus?  In C, dropping the const is automatic and not an error, a mere warning.

We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.

As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla.

If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

https://gitlab.gnome.org/GNOME/gtk/issues/new