On Thu, 2013-04-11 at 17:49 -0500, Federico Mena Quintero wrote:
> On Thu, 2013-04-11 at 13:36 +0900, Tristan Van Berkom wrote:
>
> First, let me apologize for the rather harsh tone in my message
> yesterday. I had a big "WTF" moment when I saw how the composite
> templates patches played badly with my branch. Your message made things
> look easier to fix than I expected.
>
> > So, this is how I propose we handle the situation:
> >
> > o First, you rebase your branch in such a way
> > that the filechooserdefault is reverted as
> > the first commit in your branch.
>
> I'll do something like this. First, revert the commit. Then, merge my
> branch. Doing a straight rebase is not trivial, as places-sidebar has
> gotten master merged into it a few times to keep up with general
> development. And finally, apply your commit again with lots of changes.
>
> > o Second, I know you wont like this part but
> > I need you to put the instance members on
> > a private structure.
> >
> > We do not support automatically assigning
> > component pointers to public structure offsets.
> >
> > And frankly, using a public structure defined
> > openly in gtkfilechooserprivate.h is an open
> > invitation for other components to access
> > the components of GtkFileChooserDefault directly
> > (which I think we both feel is unintended).
>
> I totally agree with this for *public* widgets, those that go into the
> public API.
>
> But for GtkFileChooserDefault, I have two objections:
>
> 1. It's a private, internal widget, never meant to be exported.
>
> 2. I'd really really really like to keep the file chooser's code as
> similar as possible between gtk2 and gtk3. Otherwise, cherry-picking
> fixes becomes much harder.
I can understand the second argument here, but access to components
created from a .ui file can't be done on the public scope of an
instance (whether it's type is private or public).
To illustrate this, this line of code in _class_init():
gtk_widget_class_bind_child (widget_class,
GtkFileChooserDefaultPrivate,
browse_files_tree_view);
... makes the 'browse_files_tree_view' variable on the widget's
private data point to the GtkTreeView built by GtkBuilder
for a given instance, automatically, for the lifetime of
the GtkFileChooserDefault's instance.
Now, GtkFileChooserDefault is not public but the
gtk_widget_class_bind_child() API is public.
We have previously decided (Benjamin and I) that the
gtk_widget_class_bind_child() API should not allow automation
of pointers on the public scope of the instance structure.
Supporting the binding of components to the public scope
of an instance would send a sort of message in the API,
like "It's OK and even encouraged, to write code with
members declared on the public scope of a GObject's
instance structure".
This is the main reason for not supporting the public
scope variables.
Now, at the cost of adding more code to GtkFileChooserDefault,
you could call the function gtk_widget_class_automate_child()
with a negative structure offset, which will avoid assigning
the pointer to the private data... and after calling
gtk_widget_init_template(), you could write a bunch of
calls that would look like:
chooser->browse_files_tree_view =
gtk_widget_get_automated_child (chooser,
GTK_TYPE_FILE_CHOOSER_DEFAULT,
"browse_files_tree_view");
However, I think the above is really undesirable, but it may
improve the cherry picking situation between master and gtk-2-24.
Note that the above is available for the sake of language bindings,
which might not be able to use instance private data on the types
that they create.
>
> I do appreciate having the private stuff in the .c file. And I
> definitely don't like the current state (well, before your patches)
> where the GtkFileChooserDefault struct is not in
> gtkfilechooserdefault.h, but in a gtkfilechooserprivate.h file. I don't
> remember why it ended up there; probably so that the unit tests would be
> able to poke at internal widgets. *That* is not the right thing to do,
> anyway, so I'm happy to see the struct move elsewhere. But the
> objections still stand.
>
> I haven't even seen how the code for composite templates pokes at
> structs... but why does it have to care whether the struct is private or
> public? Could we have:
>
> gtkfilechooserdefault.h:
>
> /* no struct definitions at all */
> typedef struct GtkFileChooserDefault *GtkFileChooserDefault;
> typedef struct GtkFileChooserDefaultClass *GtkFileChooserDefaultClass;
>
> gtkfilechooserdefault.c:
>
> /* complete structure definitions */
> struct GtkFileChooserDefault {
> GtkBox parent;
> blah blah;
> }
>
> ?
>
> > o If you have made any changes to the UI, i.e.
> > changes like spacing settings, expand/align
> > settings of any widgets in the filechooser,
> > any newly added widgets, anything that actually
> > changes the UI components, I would like you
> > to list those changes to me so I can make
> > the changes while splitting up gtkfilechooserdefault.ui
> > into 2 .ui files.
>
> Sorry, you lost me - what would those two files be for?
>
> (GtkPlacesSidebar is a self-contained thing which is mostly a
> GtkTreeView...)
Yes, I recall walking through it's creation line by line, ensuring
that I've replicated the cell renderer properties just right, and
adding the 'inline-toolbar' class to the toolbar below with the
add/remove buttons.
So it should probably end up as a gtkplacessidebar.ui, the
existing gtkfilechooserdefault.ui will need to replace the
whole definition of the places sidebar with a reference
to a private <object class="GtkPlacesSidebar" id="sidebar"/>
And before calling gtk_widget_init_template(), the file
chooser will need to call g_type_ensure (GTK_TYPE_PLACES_SIDEBAR)
(it's important that private types exist before GtkBuilder
tries to access them).
Cheers,
-Tristan
_______________________________________________
gtk-devel-list mailing list
[email protected]
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
