GNOME Bugzilla – Bug 609315
New option "Open containing folder"
Last modified: 2010-02-18 23:04:07 UTC
Created attachment 153262 [details] [review] Proposed patch If you have music in your library from a different set of imported locations, it's very useful to access right away the folder in which the selection is contained (for example, a friend tells you "I like this song, can you send it to me by email?"). The patch attached adds the functionality. I've tested it and it works. Only thing to determine if it's useful to transform the commented "//Assert..." lines in really useful assertions or not. If not, just removed them and commit as is if you approve the patch. Thanks!
*** Bug 584621 has been marked as a duplicate of this bug. ***
Review of attachment 153262 [details] [review]: ::: src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs @@ +418,3 @@ + var source = ActiveSource as ITrackModelSource; + { + private void OnOpenContainingFolder (object o, EventArgs args) Remove these comments. @@ +419,3 @@ + var source = ActiveSource as ITrackModelSource; + { + private void OnOpenContainingFolder (object o, EventArgs args) Why is it necessary to spawn a new thread? @@ +426,3 @@ + var source = ActiveSource as ITrackModelSource; + { + private void OnOpenContainingFolder (object o, EventArgs args) It's going to be O(n^2), you should rather use a HashSet. Better yet, I think you should just disable the option if more than one item is selected. @@ +429,3 @@ + var source = ActiveSource as ITrackModelSource; + { + private void OnOpenContainingFolder (object o, EventArgs args) I guess it's safer to use xdg-open instead. See the link in the duplicate bug.
Hmm, the review tool didn't insert the right code snippets. But the line numbers look fine, use them instead.
Created attachment 153790 [details] [review] v2 Thanks for the review! Here are my comments and the updated patch. (In reply to comment #2) > Review of attachment 153262 [details] [review]: > > ::: src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs > @@ +418,3 @@ > + //Assert.IsNotNull (source.TrackModel) > + //Assert.IsNotNull (source.TrackModel.SelectedItems) > + //Assert.IsTrue (source.TrackModel.SelectedItems.Count > 0) > > Remove these comments. I was marking this comments as "RFC" on my comment#0. Besides, I've also made a suggestion [1] on how to make this asserts taking advantage of the recently introduced BansheeMetrics. Anyway I've changed them now to an if statement and a Log.Error() call > @@ +419,3 @@ > + //Assert.IsNotNull (source.TrackModel.SelectedItems) > + //Assert.IsTrue (source.TrackModel.SelectedItems.Count > 0) > + ThreadAssist.SpawnFromMain (delegate { > > Why is it necessary to spawn a new thread? Because in case the user selected many files, I didn't want to make Banshee's main thread be responsible of checking the existance of all the folders. However, I have removed this given the last comments below. > @@ +426,3 @@ > + var path = System.IO.Path.GetDirectoryName > (track.Uri.AbsolutePath); > + > + if (!already_opened.Contains (path)) { > > It's going to be O(n^2), you should rather use a HashSet. Mmm, true. However, I have removed this given the last comments below. > Better yet, I think you should just disable the option if more than one item is > selected. True, it's much cleaner this way, and less complicated to code. > @@ +429,3 @@ > + already_opened.Add (path); > + if (Banshee.IO.Directory.Exists (path)) { > + System.Diagnostics.Process.Start (path); > > I guess it's safer to use xdg-open instead. See the link in the duplicate bug. xdg-open is cross-desktop, but not cross-platform. This is the method that MonoDevelop uses to open folders by the way. Besides, remember that the folders are most likely going to be local (as per the check "CanDeleteTracks") so it's mono's decision what to use to open a folder. That being said, I think xdg-open is a great tool, but maybe it's mono who should use it internally (if, at runtime, its existance is confirmed in the system). [1] http://mail.gnome.org/archives/banshee-list/2010-February/msg00076.html
Committed a slightly modified version of the patch, thanks Andrés!
(In reply to comment #5) > Committed a slightly modified version of the patch, thanks Andrés! Thanks. I was also thinking about placing the option in a different place. I agree as well that the use of foreach is much cleaner than using an enumerator, but I was thinking that it makes the developer think at first that there are more elements than 1 to open. Maybe the best way would have been creating an [index] accessor to the SelectedItems property? So you could say track = source.SelectedItems[0];
(In reply to comment #6) > I agree as well that the use of foreach is much cleaner than using an > enumerator, but I was thinking that it makes the developer think at first that > there are more elements than 1 to open. Maybe the best way would have been > creating an [index] accessor to the SelectedItems property? So you could say > track = source.SelectedItems[0]; There's actually a commented out indexer. Regarding the confusion the foreach statement may cause, there's a check for the number of items in the selection just before it; and a return statement inside the loop. I guess this should cover it ;)
A "Reveal in file manager" would be even better, but nautilus is currently missing the needed functionaliy. Opened bug 610408 about that.