[Geany-devel] Sidebar and MsgWindow Notebook Patches

[Colomban Wendling]

Le 20/03/2011 22:35, Matthew Brush a écrit :
> Hi,
> 
> I have a few patches working towards making the sidebar and message
> window notebooks more friendly.
> 
> #0001
> Makes original sidebar tabs and any new tabs added reorderable.

I think it'd be better to add another listener to tab-added signal and
only set_reorderable() on this very tab and 0003 would add a
set_detachable() in this handler (like you did in patch 0002).
This would avoid setting the flags on just removed tabs, and re-set the
flags on all tabs.

> #0002
> Makes original message window tabs and any new tabs added reorderable.

Nothing more to say, but some general notes, see below.

> #0003
> Enables drag and drop support between the sidebar notebook and message
> window notebook tabs.  This will reduce code in plugins who all do this
> independently and make projects such as geany-sidebarterm pointless.

I think it'd be a nice addition, I didn't know GTK+ provided this
directly :)
I think however the GTK checks to set the group would better be provided
with a wrapper since used in more than one place, something like:

void ui_notebook_set_group_name(GtkNotebook *nb, const gchar *name)
{
#if GTK_CHECK_VERSION(2, 24, 0)
	gtk_notebook_set_group_name(nb, name);
#elif GTK_CHECK_VERSION(2, 12, 0)
	guint h = g_str_hash(name);

	gtk_notebook_set_group(nb, GUINT_TO_POINTER(h));
#elif GTK_CHECK_VERSION(2, 10, 0)
	guint h = g_str_hash(name);

	gtk_notebook_set_group_id(nb, ABS((gint)h));
#endif
}

then simply use:

ui_notebook_set_group_name(GTK_NOTEBOOK(nb), "my-name");

> I have not thoroughly tested the implications of this, like if somewhere
> Geany is using the tab order or location within a specific notebook to
> access the children.  If this is the case, it will be an opportunity to
> fix the code that is doing that.

I think it need to be better tested before adding it, but I agree that
fixing code depending on tab ordering would be good (it there is some).

> I need some guidance on how to make the order and location of the tabs
> to be persistent so it it's saved/loaded with the session.  Any hints
> would be appreciated.

This is the big problem I guess, and it probably need to be addressed
before applying the patches, because it's likely to need some tuning.

I think the problem is that we need to be able to identify a tab in
order to save its state. Maybe simply needing the child to have a name
(set with gtk_widget_set_name()) would work, though it'd need plugins to
be adapted to benefit of the change.

More difficult will be the tab restoration I guess. Not only you have to
be able to restore tab ordering in a notebook, and this even if some
tabs are missing (e.g. a simple insertion at the appropriate position
isn't enough); but it'll need to set add the tab to the right notebook.
Quite a peace of code I guess ^^


And finally, some notes on your patches:

1) Rule of the thumb: always double check your patches.

2) Don't send 2 patches if the second corrects the first; just send 1
that is directly OK. (you did this in Class Builder UI patches -- here
it's only for a blank line)
You can easily use "git commit --amend" or "git rebase" to fixup your
patches if you already made a commit of them.

2) Style! There is some style issues in your patches:
2.1) You added some useless blank lines (e.g., second addition to 0001)
2.2) You left tabs on blank lines, we don't
2.3) Always surround assignation operator with spaces, even in for() loops
2.4) We don't place one argument per line on function defs/decls

3) Most important: GTK version checking. You seem to have though of it,
but you did it wrong in some places:
Runtime checking, like gtk_check_version() is done... at runtime. This
means that the code need to be compilable even with older version, so
you can't use any newer function (like gtk_notebook_get_tab_reorderable()).
Generally these runtime checks are used together with setting some
properties by name: the check makes sure the property exists, but the
code itself is compatible with older versions. You'll need
gtk_container_child_set() ;)


Cheers,
Colomban