Re: Composite GtkBuilder template

[Tristan Van Berkom <tristanvb openismus com>]

On Thu, 2013-04-11 at 13:20 +0900, Tristan Van Berkom wrote:
> On Wed, 2013-04-10 at 20:30 -0500, Federico Mena Quintero wrote:
> > On Tue, 2013-04-02 at 17:59 +0900, Tristan Van Berkom wrote:
> >
> > > And while it's a huge list of changes, any thorough peer reviews
> > > would be greatly appreciated of course.
> >
> > This work makes merging my places-sidebar branch completely impossible.
> > There is a highly nontrivial amount of work in that branch and I really
> > don't feel like essentially rewriting the whole file due to merge
> > conflicts.
>
> Federico,
>    I feel your pain, and have also been on the receiving
> end of a merge conflict before.
>
> I don't think it's sensible to be doing this kind of
> surgery on master directly (and I'm just not willing
> to repeat the 2 full days work I did on filechooser from
> scratch) but I'll be happy to help you merge this
> in your branch so that it applies cleanly.

Eeek, looks like I hit send by accident too soon (I think
that perhaps the 'send' button should not 'can-focus' in
Evolution)... luckily the above text is already what I meant
to write :)

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.

    You should be able to do this easily enough
    with an interactive rebase on top of master.

    You will find a single commit in the log
    which atomically does GtkFileChooserDefault.

  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).

    So before I touch this, please make sure that
    there is a GtkFileChooserDefaultPrivate _and_
    a GtkSideBarPrivate structure defined in their
    C files.

  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.

  o At this point, I'll look into your branch
    and make both the GtkSideBar & GtkFileChooserDefault
    use the .ui files again.

I know you're not going to like the mandatory usage
of the private structure, but I think that supporting
public struct offsets in the templates API is a path
we don't want to walk (as it should be generally discouraged
to use publicly accessible object members).

Even in private filechooser code, defining the members in
the .c files will have its benefits.

Cheers,
      -Tristan


> > I'm going to ask you to do these:
> >
> > 1. Revert the commits that modify gtkfilechooserdefault.*, and ensure
> > that the code compiles.
> >
> > 2. Tell me about it, and I'll merge places-sidebar into master.
> >
> > 3. Then you can go back and re-do your changes for
> > gtkfilechooserdefault, BUT do not use a ->priv field.
> > GtkFileChooserDefault *is* a private widget, and it doesn't need a priv
> > structure.  Having it also makes the code unreadable.  (I'd like the
> > other internal widgets in GtkFileChooser to be the same, but I can live
> > with just the main gtkfilechooserdefault being clean).
> >
> > I can only imagine how much work it was to complete the composite
> > templates branch; please think that the places-sidebar branch is a
> > similar investment on my part :)
> >
> > Thanks,
> >
> >   Federico