GNOME.org
 

Re: Composite GtkBuilder template

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
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]