GNOME Bugzilla – Bug 615797
There is just one tab in Properties, that looks slightly odd
Last modified: 2010-12-05 11:44:52 UTC
Created attachment 158767 [details] The single tab The track properties dialog sports a tabbed interface, but there is only one tab, so that looks odd and it makes it not really a tabbed interface. It also looks a bit odd. This is probably happening because it's pretty much shares everything with the Edit Track dialog, including this tab.
That's exactly what's going on. Further, the whole dialog is extensible, so theoretically extensions could add their own properties pages to that notebook. We probably should special case a single tab though. It's not exactly a highly trafficked dialog though :-)
Yeah, I just ran across it today and went like "Hey, what's going on here, is this broken?"
Created attachment 175860 [details] [review] [TrackEditorDialog] Only show tabs if there is more than one This is my attempt at fixing this. It works for me for the Track Properties and Edit Track Information dialogs, although it is not a perfect solution. The created notebook seems to contain one extra tab than is shown. For example, on my installation, notebook.NPages is 2 for the Track Properties dialog, when only 1 tab is shown, and notebook.NPages is 5 for the Edit Track Information dialog, when only 4 tabs are shown. This patch assumes that note.NPages will be one more than the amount of shown tabs, and calls ShowTabs accordingly. Any insight into how the notebook and it's pages function would be welcome.
The issue with the above patch is how to determine the amount of pages that are actually shown. Can it be assumed that it is one less than notebook.NPages? If not, how can this amount be found so that it can be used to switch notebook.ShowTabs?
I've found the culprit here. The "Podcasts" extension adds a page of type Banshee.Podcasting.Gui.PodcastEpisodePage, which is not actually shown in the Notebook. Why this is, I have no idea. Debugging in MonoDevelop, I am yet to come across any discernible way to tell which pages will or will not be shown in the dialog.
Created attachment 175861 [details] [review] [TrackEditorDialog] Only show tabs if there is more than one Sorry about cluttering up this bug, but (hopefully) this will be my last post here. I found that the value of page.Order could be used to determine which pages were actually shown in the notebook. The podcast page was duplicating an Order value of another page, and thus not being shown. This patch uses a dictionary to determine the unique values for page.Order, representing the visible pages, and then calls notebook.ShowTabs when there is only one tab to be shown.
Comment on attachment 175861 [details] [review] [TrackEditorDialog] Only show tabs if there is more than one I am not going to comment on the use of a Dictionary here as there are far more people qualified to determine whether that's a good solution in this case but it has the feel of a hacky work-around. Maybe the podcast extension should be looked at as to why it is adding a page here. Secondly, I personally would check for == 1 if that's possible. I don't know enough of the surrounding code to determine whether it will ever get here when there are 0 pages, but if you only get here with >= 1 page, I would change the check from < 2 to == 1 to improve readability. But on to the actual patch. >@@ -257,6 +259,7 @@ namespace Banshee.Gui.TrackEditor > if (page_to_focus != null) { > notebook.CurrentPage = notebook.PageNum (page_to_focus); > } >+ if (order_values.Count < 2) notebook.ShowTabs = false; > } Banshee's code base tries to enforce the use of {}, even for one-line if-statements. Take a look at the page_to_focus check above. if (order_values.Count < 2) { notebook.ShowTabs = false; }
Oh, I forgot to say thanks for that patch and all!
Created attachment 175862 [details] [review] Updated patch with the above comments
Review of attachment 175862 [details] [review]: Thanks for the patch and the investigations ! Using the Order is not the right way to do it. The fact that PodcastEpisodePage.Order has the same value as another page is just a coincidence. PodcastEpisodePage is not displayed for tracks that are not podcast tracks because it hides itself. See here : http://git.gnome.org/browse/banshee/tree/src/Extensions/Banshee.Podcasting/Banshee.Podcasting.Gui/PodcastEpisodePage.cs#n80 That happens when the track is loaded in the dialog, in the page.LoadTrack() call. So we need to count the number of pages that are visible (page.Widget.Visible) after that call.
Created attachment 175863 [details] [review] Count visible pages after LoadTrackToEditor
Comment on attachment 175863 [details] [review] Count visible pages after LoadTrackToEditor Wow that was fast ! Committed with a few changes : - Rename the method and make it private - Make sure the notebook is not visible at all by removing borders http://git.gnome.org/browse/banshee/commit/?id=527edee
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.