On 03/21/11 10:38, Colomban Wendling wrote:
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.
Agreed. I actually did it like this first, but I changed it so it would handle the case of the original tabs. I will fix this.
#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");
Agreed.
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've been using since I patched it, so far so good, but it's still very little testing. I did notice Webhelper doesn't remove his tab if it's been moved to the other notebook, FYI (I didn't check gwh code to investigate).
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.
Not all tabs in a notebook need to be detachable/reorderable, and it still works fine if only some are, so plugins could adapt anytime in the future. So really we could only care about the "core" tabs for now, if that would make it easier.
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 ^^
Agreed. I will need to think more on a proper solution for saving and loading though.
And finally, some notes on your patches:
- Rule of the thumb: always double check your patches.
Believe it or not I did (see below).
- 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.
THANK YOU! I noticed that one line and didn't think I could fix it so I just fixed it in the next patch. I will make sure not to do this now.
- Style! There is some style issues in your patches:
2.1) You added some useless blank lines (e.g., second addition to 0001)
I also noticed this, but see above :)
2.2) You left tabs on blank lines, we don't
I didn't have "Strip trailing spaces and tabs" enabled in Geany and also not showing whitespace. I will make sure to check this next time.
2.3) Always surround assignation operator with spaces, even in for() loops
This is a hard habit to break, but I see what you mean from other code in Geany. I will check this also in the future.
2.4) We don't place one argument per line on function defs/decls
I mis-read the HACKING file, I thought it meant to do this on long functions, but it clearly states "...lines should be wrapped and indented once more to show that the line is continued". Nothing about next arguments being on their own line, I'm just used to this from other people's code I guess.
- 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() ;)
Yeah I absolutely see what you mean, and I was even aware of this after finding GTK_CHECK_VERSION because of trying to use a function from 2.24 and it didn't work inside a gtk_check_version block. In all honesty, this is the first time I've ever actually had to care what GTK+ version since I normally code to a recent/common version. Thanks for pointing it out though, because I totally missed that mistake.
Moving forward, do you think I should keep working on this? If so, is it best to just keep sending patches to the ML, or should I setup a repository+branch for this on GitHub and just let you pull out what you want?
Thanks for looking at the patches and thanks especially for the feedback on the other stuff. If no one tells me, I'd just keep making those mistakes :)
Cheers, Matthew Brush