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.
#0002 Makes original message window tabs and any new tabs added reorderable.
#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 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 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.
Cheers, Matthew Brush (codebrainz)
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
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
On 21.03.2011 23:10, Matthew Brush wrote:
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?
Please keep working on this (git's fine for me)! Geany's UI is a bit too static for my taste. I would also love to see your main/documents notebook patch in!
Best regards.
Le 21/03/2011 23:10, Matthew Brush a écrit :
On 03/21/11 10:38, Colomban Wendling wrote:
Le 20/03/2011 22:35, Matthew Brush a écrit :
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).
Nothing really surprising unfortunately; and this will be a real problem for many plugins... any plugin using gtk_notebook_remove_page(), or even gtk_container_remove(nb, child) -- basically everyone that don't simply gtk_widget_destroy(child). Actually after some thinking, one should use gtk_container_remove(gtk_widget_get_parent(child), child), e.g. don't expect the child to be in a particular parent.
Then we need not to make a tab detachable if the plugin don't expect it, e.g. if it haven't set the tab name we need for restoring.
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.
Right, agreed.
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?
I think it's an interesting feature that moves forward to a better UI customization, so if you're interested by it, I definitely say "yes, keep working on it" :) I think patches are better, at least for now we're supposed to use SVN; this will make easier for other people (Nick, Enrico, even interested users) to get the patches. And since when you pushed a patch to a remote Git repository you can't ammend it anymore (well, you willn't be able to push the modified one), it'll be easier for you to update your patches ^^
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 :)
No problem :) And I know, my patches had the same kind of problems when I started sending some ^^
Cheers, Colomban
On 03/21/11 10:38, Colomban Wendling wrote:
Le 20/03/2011 22:35, Matthew Brush a écrit :
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 ^^
Just to see if it was do-able I prototyped this setup in Python:
https://gist.github.com/880598
I wrote this really quickly but it should show the idea and it seems to work well.
Cheers, Matthew Brush
Le 22/03/2011 02:54, Matthew Brush a écrit :
On 03/21/11 10:38, Colomban Wendling wrote:
Le 20/03/2011 22:35, Matthew Brush a écrit :
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 ^^
Just to see if it was do-able I prototyped this setup in Python:
https://gist.github.com/880598
I wrote this really quickly but it should show the idea and it seems to work well.
Looks good so far :) But don't forget we need also to be able to reorder tabs added later (e.g. by plugins), and this even if the tab order has changed (so we can't just reorder all tabs) and even if there are some missing tabs (e.g. plugins that haven't been loaded).
Cheers, Colomban