GNOME Bugzilla – Bug 618122
Move BANSHEE_DISABLE_GRID env var switch to an option in the preferences
Last modified: 2011-02-08 21:36:45 UTC
Created attachment 160592 [details] [review] Proposed patch, v1 Some people requested to have the classic album layout back because presumably the new one was less performant. Aaron said it was available by the use of the BANSHEE_DISABLE_GRID env var and that because of lack of time he couldn't convert it to a banshee preference.
I started looking yesterday at the code on how to do this. I hope to have a patch ready in a few days.
Jeroen, please note, this bug already includes a patch. Let's not duplicate efforts.
@Andrés oops, did not notice the attachment :) Thx!
Review of attachment 160592 [details] [review]: It also crashed on me, stack trace below. ::: src/Core/Banshee.Services/Banshee.Library/MusicLibrarySource.cs @@ +75,3 @@ SetFileNamePattern (MusicFileNamePattern); + Catalog.GetString ("Albums options"), 10)); + Section album = PreferencesPage.Add (new Section ("albumopts", I think adding a new section just for 2 options is a bit excessive. Just re-use the misc section. ::: src/Core/Banshee.Services/Banshee.Sources/SourceManager.cs @@ +164,3 @@ + Source = source + Position = position, + SourceAdded.SafeInvoke (new SourceAddedArgs () { Why this was moved?
Steps to reproduce: * Disable grid view * Restart Banshee * Enable grid view * At this point the album pane looks garbled. * Click on it → crash Stack trace: System.ArgumentNullException: Argument cannot be null. Parameter name: source at System.Linq.Check.SourceAndSelector (System.Object source, System.Object selector) [0x00022] in /home/ise/svn-packages/mono/trunk/src/mono-2.6.4/mcs/class/System.Core/System.Linq/Check.cs:74 at System.Linq.Enumerable.Count[Column] (IEnumerable`1 source, System.Func`2 selector) [0x00000] in /home/ise/svn-packages/mono/trunk/src/mono-2.6.4/mcs/class/System.Core/System.Linq/Enumerable.cs:418 at Hyena.Data.Gui.Accessibility.ListViewAccessible`1[Banshee.Collection.AlbumInfo].get_n_columns () [0x00000] in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Data.Gui/Accessibility/ListViewAccessible.cs:150 at Hyena.Data.Gui.Accessibility.ListViewAccessible`1[Banshee.Collection.AlbumInfo].get_NColumns () [0x00000] in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Data.Gui/Accessibility/ListViewAccessible_Table.cs:52 at Hyena.Data.Gui.Accessibility.ListViewAccessible`1[Banshee.Collection.AlbumInfo].RefAt (Int32 row, Int32 column) [0x00000] in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Data.Gui/Accessibility/ListViewAccessible_Table.cs:160 at Hyena.Data.Gui.Accessibility.ListViewAccessible`1[Banshee.Collection.AlbumInfo].get_ActiveCell () [0x00022] in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Data.Gui/Accessibility/ListViewAccessible.cs:145 at Hyena.Data.Gui.Accessibility.ListViewAccessible`1[Banshee.Collection.AlbumInfo].OnSelectionFocusChanged (System.Object o, System.EventArgs a) [0x00000] in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Data.Gui/Accessibility/ListViewAccessible.cs:137 at Hyena.Collections.SelectionProxy.OnFocusChanged () [0x0000d] in /home/alex/Projects/banshee/src/Libraries/Hyena/Hyena.Collections/SelectionProxy.cs:75 at Hyena.Collections.SelectionProxy.HandleSelectionFocusChanged (System.Object o, System.EventArgs args) [0x00000] in /home/alex/Projects/banshee/src/Libraries/Hyena/Hyena.Collections/SelectionProxy.cs:94 at Hyena.Collections.Selection.set_FocusedIndex (Int32 value) [0x00014] in /home/alex/Projects/banshee/src/Libraries/Hyena/Hyena.Collections/Selection.cs:70 at Hyena.Data.Gui.ListView`1[T].FocusModelRow (Int32 index) [0x00000] in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Data.Gui/ListView/ListView_Interaction.cs:896 at Hyena.Data.Gui.ListView`1[T].OnListButtonPressEvent (Gdk.EventButton evnt) [0x00191] in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Data.Gui/ListView/ListView_Interaction.cs:618 at Hyena.Data.Gui.ListView`1[T].OnButtonPressEvent (Gdk.EventButton evnt) [0x0006f] in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Data.Gui/ListView/ListView_Interaction.cs:515 at Gtk.Widget.buttonpressevent_cb (IntPtr widget, IntPtr evnt) [0x00000] in <filename unknown>:0 at GLib.ExceptionManager.RaiseUnhandledException(System.Exception e, Boolean is_terminal) at Gtk.Widget.buttonpressevent_cb(IntPtr widget, IntPtr evnt) at Gtk.Application.gtk_main() at Gtk.Application.Run() at Banshee.Gui.GtkBaseClient.Run() in /home/alex/Projects/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:line 205 at Banshee.Gui.GtkBaseClient.Startup() in /home/alex/Projects/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:line 82 at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup) in /home/alex/Projects/banshee/src/Libraries/Hyena.Gui/Hyena.Gui/CleanRoomStartup.cs:line 54 at Banshee.Gui.GtkBaseClient.Startup() in /home/alex/Projects/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:line 77 at Banshee.Gui.GtkBaseClient.Startup(System.String[] args) in /home/alex/Projects/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:line 67 at Nereid.Client.Main(System.String[] args) in /home/alex/Projects/banshee/src/Clients/Nereid/Nereid/Client.cs:line 54
Created attachment 160868 [details] [review] Patch v2 (In reply to comment #4) > Review of attachment 160592 [details] [review]: > > It also crashed on me, stack trace below. Good catch. What a painful bug BTW, but I found a fix! Just switching to non-grid mode receiving the first expose event. > ::: src/Core/Banshee.Services/Banshee.Library/MusicLibrarySource.cs > @@ +75,3 @@ > SetFileNamePattern (MusicFileNamePattern); > + Catalog.GetString ("Albums options"), 10)); > + Section album = PreferencesPage.Add (new Section ("albumopts", > > I think adding a new section just for 2 options is a bit excessive. Just re-use > the misc section. I actually think the same. But, if I don't do it, it would be 3 options for misc. Isn't that excessive too for misc? > ::: src/Core/Banshee.Services/Banshee.Sources/SourceManager.cs > @@ +164,3 @@ > + Source = source > + Position = position, > + SourceAdded.SafeInvoke (new SourceAddedArgs () { > > Why this was moved? Because if I don't move, I get the SourceAdded event with a MusicLibrary property that has not yet assigned, so it's a small fix for that case. Thanks for the review!
s/move/move it/ s/assigned/been assigned/
(In reply to comment #6) > > ::: src/Core/Banshee.Services/Banshee.Library/MusicLibrarySource.cs > > @@ +75,3 @@ > > SetFileNamePattern (MusicFileNamePattern); > > + Catalog.GetString ("Albums options"), 10)); > > + Section album = PreferencesPage.Add (new Section ("albumopts", > > > > I think adding a new section just for 2 options is a bit excessive. Just re-use > > the misc section. > > I actually think the same. > But, if I don't do it, it would be 3 options for misc. Isn't that excessive too > for misc? I think 3 options for misc is fine, we already have 4 in the General tab. It also keeps the whole dialog a bit more compact, as the additional section adds some height. > > ::: src/Core/Banshee.Services/Banshee.Sources/SourceManager.cs > > @@ +164,3 @@ > > + Source = source > > + Position = position, > > + SourceAdded.SafeInvoke (new SourceAddedArgs () { > > > > Why this was moved? > > Because if I don't move, I get the SourceAdded event with a MusicLibrary > property that has not yet assigned, so it's a small fix for that case. I'm not sure I understand : does this have anything to do with the original purpose of the patch ? If not, it should be a separate patch. Another question : it seems both the classic_layout and the grid_layout are always instantiated, even though one of them will never be really used. Would it be possible to only instantiate a layout when it's needed ?
Created attachment 161388 [details] [review] Patch v3 Thanks for your review! See inline: (In reply to comment #8) > (In reply to comment #6) > > > ::: src/Core/Banshee.Services/Banshee.Library/MusicLibrarySource.cs > > > @@ +75,3 @@ > > > SetFileNamePattern (MusicFileNamePattern); > > > + Catalog.GetString ("Albums options"), 10)); > > > + Section album = PreferencesPage.Add (new Section ("albumopts", > > > > > > I think adding a new section just for 2 options is a bit excessive. Just re-use > > > the misc section. > > > > I actually think the same. > > But, if I don't do it, it would be 3 options for misc. Isn't that excessive too > > for misc? > > I think 3 options for misc is fine, we already have 4 in the General tab. It > also keeps the whole dialog a bit more compact, as the additional section adds > some height. Ok, 2 vs 1, you win, I've addressed this in this new patch :) > > > ::: src/Core/Banshee.Services/Banshee.Sources/SourceManager.cs > > > @@ +164,3 @@ > > > + Source = source > > > + Position = position, > > > + SourceAdded.SafeInvoke (new SourceAddedArgs () { > > > > > > Why this was moved? > > > > Because if I don't move, I get the SourceAdded event with a MusicLibrary > > property that has not yet assigned, so it's a small fix for that case. > > I'm not sure I understand : does this have anything to do with the original > purpose of the patch ? If not, it should be a separate patch. Yes, it's related to this patch. Sorry if I wasn't clear enough in previous comment. If I don't change the order of that, the following line in the change would return always false: + if (!args.Source.Equals (ServiceManager.SourceManager.MusicLibrary)) { > Another question : it seems both the classic_layout and the grid_layout are > always instantiated, even though one of them will never be really used. Would > it be possible to only instantiate a layout when it's needed ? Yes, good point, but only for the case of classic_layout (otherwise we would be bitten by the bug that Alex discovered in comment#5). So, in this new patch, classic_layout is lazily initialized.
Review of attachment 161388 [details] [review]: Looks good, please commit.
Comment on attachment 161388 [details] [review] Patch v3 Committed and pushed
Thousand thanks for the reviews! (Commit was 8ba16f8c2c0a9188a2f4ea7cd085a768200b0998)
*** Bug 614738 has been marked as a duplicate of this bug. ***