GNOME Bugzilla – Bug 611923
Launch TrackEditor in ReadOnly mode when metadata save operation doesn't work
Last modified: 2010-05-24 06:57:59 UTC
Created attachment 155339 [details] [review] Patch v1 I'm in the process of fixing similar bugs like bug 609411 and bug 559416, but it's taking time and their fixes may not get ready for Banshee 1.6 (as they have some dependencies I'm taking care of). In the meantime (and because it's main cause is clear: SaveTrackMetadataJob only works for MusicLibrary for now), I think we should make the TrackEditorDialog read-only for the cases which are affected by this problem, and this is a simpler patch to do.
Created attachment 155340 [details] [review] Patch v2 Small nitpick corrected.
Review of attachment 155340 [details] [review]: The patch looks fine except for a nitpick below. However, I'm not sure why we should open the dialogue in read-only mode instead of just showing the properties. The properties make it clear that the user cannot edit anything while the read-only dialogue will make them wonder about why they can't. I bet there would be a flood of bug reports from confused users. So I suggest to keep things simple. If we can edit metadata - show the edit dialogue, if we cannot - show the properties. ::: src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs @@ +270,3 @@ + return source is PlaylistSource || + { + private bool CanEditMetadata (ITrackModelSource source) PlaylistSource can be from the video library or from FSQ (when opening m3u files). Also, considering MusicLibrary is never null, this can be simplified: return ServiceManager.SourceManager.MusicLibrary.Equals (source) || ServiceManager.SourceManager.MusicLibrary.Equals (source.Parent);
Created attachment 155494 [details] [review] Patch v3 (In reply to comment #2) > Review of attachment 155340 [details] [review]: > > The patch looks fine except for a nitpick below. > > However, I'm not sure why we should open the dialogue in read-only mode instead > of just showing the properties. The properties make it clear that the user > cannot edit anything while the read-only dialogue will make them wonder about > why they can't. I bet there would be a flood of bug reports from confused > users. I don't agree, because the following reasons: a) The same user that would open a bug because he cannot edit the metadata, would open a bug as well if he cannot see the the option "Edit the metadata" but just "View properties". b) I much prefer *receiving* bugs about *missing* functionality (i.e.: cannot edit the metadata when using XYZ source) than *having* (and maybe not receiving because users wouldn't notice in the first place) bugs about *broken functionality* (i.e.: editing the metadata in the XYZ source doesn't work). c) Furthermore, if we commit this patch and start receiving bugs like explained in (b), we can start to: * Either start creating the functionality to fix those bugs one by one (so we have some clue as of what is more requested by users). * Or just put an informational message (next to the Close button) that says why the user cannot edit the metadata (this is doable with the ReadOnly approach, but not doable if we disable or make the EditMetadata contextual command disappear). > So I suggest to keep things simple. If we can edit metadata - show the edit > dialogue, if we cannot - show the properties. > > ::: src/Core/Banshee.ThickClient/Banshee.Gui/TrackActions.cs > @@ +270,3 @@ > + return source is PlaylistSource || > + { > + private bool CanEditMetadata (ITrackModelSource source) > > PlaylistSource can be from the video library or from FSQ (when opening m3u I just tried opening an M3U file with double-click in my desktop (to make it be opened by Banshee's FSQ) and nothing happened. Is this a bug? I was mainly thinking about not disabling tag editing in the PlayQueue source when I wrote the "is PlaylistSource" condition. > files). Also, considering MusicLibrary is never null, this can be simplified: Cool, this is fixed in this version of the patch.
Comment on attachment 155340 [details] [review] Patch v2 Marking patch v2 obsolete.
(In reply to comment #3) > ... > I don't agree, because the following reasons: > ... Forgot the most important one: d) In the properties tab, you cannot *see* the tag contents, only the properties of the file.
Created attachment 155600 [details] [review] Patch v4 Addressing last conversations on IRC. If this gets committed quickly, there's a chance I can hurry up to fix bug 609411 as well for 1.5.5.
Review of attachment 155600 [details] [review]: Looks good, thanks Andrés! ::: src/Core/Banshee.Services/Banshee.Playlist/AbstractPlaylistSource.cs @@ +105,3 @@ } + get { return PrimarySource.CanEditMetadata; } + public override bool CanEditMetadata { May be `return PrimarySource != null && PrimarySource.CanEditMetadata` ? You won't need the override in PlayQueueSource then.
Created attachment 155601 [details] [review] Patch v5 (In reply to comment #7) > Review of attachment 155600 [details] [review]: > > Looks good, thanks Andrés! > > ::: src/Core/Banshee.Services/Banshee.Playlist/AbstractPlaylistSource.cs > @@ +105,3 @@ > } > + get { return PrimarySource.CanEditMetadata; } > + public override bool CanEditMetadata { > > May be `return PrimarySource != null && PrimarySource.CanEditMetadata` ? You > won't need the override in PlayQueueSource then. We want PlayQueueSource to return true, so we still need the override. But false is better than NRE so I've included your change as well in this patch.
Review of attachment 155601 [details] [review]: Isn't it possible to just set each editable page of the notebook's Sensitive = false, instead of each field? Also, if the track editor is Read Only, I'm not wild about having both Properties and View Track Information - basically the same thing then. Maybe hide the Properties action in that case? ::: src/Core/Banshee.Core/Resources/translators.xml @@ +67,3 @@ <language code="fr" name="French"> <person>Alexandre Franke</person> + <person>Bruno Brouard</person> Please don't include unrelated changes. ::: src/Core/Banshee.Services/Banshee.Playlist/AbstractPlaylistSource.cs @@ +105,3 @@ + public override bool CanEditMetadata { + get { return PrimarySource != null && PrimarySource.CanEditMetadata; } why not make this PrimarySource == null || PrimarySource.CanEditMetdata that way you don't need the override in PlayQueueSource, and you default to true like Source does ::: src/Core/Banshee.Services/Banshee.Sources/ITrackModelSource.cs @@ +57,3 @@ bool Indexable { get; } + + bool CanEditMetadata { get; } This doesn't seem necessary since Source.cs contains this property ::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/AlbumArtistEntry.cs @@ +85,3 @@ } + public void SetAsReadOnly () Isn't it possible to just set each editable page of the notebook's Sensitive = false, instead of each field? Also, if the track editor is Read Only, I'm not wild about having both Properties and View Track Information - basically the same thing then. Maybe hide the Properties action in that case?
Created attachment 155607 [details] [review] Patch v6 Thanks for the review. Attaching new patch with comments addressed. Some additions inline: (In reply to comment #9) > Review of attachment 155601 [details] [review]: > ::: src/Core/Banshee.Core/Resources/translators.xml > @@ +67,3 @@ > <language code="fr" name="French"> > <person>Alexandre Franke</person> > + <person>Bruno Brouard</person> > > Please don't include unrelated changes. Yes sorry, this was autogenerated. > ::: src/Core/Banshee.Services/Banshee.Playlist/AbstractPlaylistSource.cs > @@ +105,3 @@ > > + public override bool CanEditMetadata { > + get { return PrimarySource != null && > PrimarySource.CanEditMetadata; } > > why not make this PrimarySource == null || PrimarySource.CanEditMetdata > > that way you don't need the override in PlayQueueSource, and you default to > true like Source does I like it. Done. > ::: src/Core/Banshee.Services/Banshee.Sources/ITrackModelSource.cs > @@ +57,3 @@ > bool Indexable { get; } > + > + bool CanEditMetadata { get; } > > This doesn't seem necessary since Source.cs contains this property It's necessary because in TrackActions.cs, curren_track is ITrackModelSource, not Source. > ::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/AlbumArtistEntry.cs > @@ +85,3 @@ > } > > + public void SetAsReadOnly () > > Isn't it possible to just set each editable page of the notebook's Sensitive = > false, instead of each field? Also, if the track editor is Read Only, I'm not > wild about having both Properties and View Track Information - basically the > same thing then. Maybe hide the Properties action in that case? Left as is becuase it was discussed on IRC; for the record: knocte> gabaug: I just tried marking the notebook pages insensitive and the main drawback of that is that it makes the widgets insensitive, not ineditable, so then you cannot select text, IMO it's prettier with non-editable widgets gabaug> knocte: I'd prefer you not have to modify every Field of the editor; maybe edit the place where they're added, and check if it's a widget, and if so set Sensitive = false? knocte> but for TextEntries I'm just setting IsEditable=false, not Sensitive; besides, there are things that are not widgets but containers such as PageNavigationEntry knocte> gabaug: what they have in common is indeed the IEditorField interface gabaug> knocte: containers are widgets gabaug> knocte: and you can check for is Gtk.Editable and set IsEditable=false before checking is Gtk.Widget and setting Sensitive=false knocte> I see, knocte> but there's still another problem knocte> there are things that need Sensitive=false AND IsEditable=false knocte> for example the SpinButton knocte> (because it's a gtk+ bug I already reported) knocte> gabaug: but I could do an exception in that case and just check if widget is SpinButton, then act accordingly? gabaug> knocte: maybe - maybe you can get abock's input knocte> gabaug: ok, I've looked more into it, and it gets quite complicated because widgets like PageNavigationEntry and TextViewEntry don't implement Gtk.Editable, and if I add the interface to them, I have to define a lot of behaviors besides IsEditable (SelectRegion(int, int), GetChars(int, int), DeleteText(int, int), CopyClipboard(), DeleteSelection(), etc.) gabaug> knocte: ok, mention this on the bug please
Created attachment 155609 [details] [review] Patch v7 Simplified 2 parts of last patch.
Created attachment 155647 [details] [review] Patch v8 New addition: with this patch, now the SpinButtons are read-only as well instead of just disabled.
Created attachment 155688 [details] [review] Patch for the string freeze I was adviced by gabaug to prepare this in case there's no time to review Patch_v8 for the 1.5.5 release (tomorrow).
Review of attachment 155688 [details] [review]: Committed, thanks!
Created attachment 155691 [details] [review] Patch v9 It includes the removal of the artificial string addition for the l10n freeze
(In reply to comment #3) > I just tried opening an M3U file with double-click in my desktop (to make it be > opened by Banshee's FSQ) and nothing happened. Is this a bug? FYI I filed this problem as bug 612515.
(In reply to comment #0) > ...I'm in the process of fixing similar bugs like bug 609411 and bug 559416... Another issue related to this is bug 550276, which is a generalisation of bug 559416.
Review of attachment 155691 [details] [review]: We already have HasEditableTrackProperties and HasViewableTrackProperties in Source.cs - adding CanEditMetadata on top of that is very confusing. What would it mean to have HasEditableTrackProperties false and CanEditMetadata true? It's a very confusing API. The actual bug here is (AFAICT): Sometimes we make it seem like the user can edit metadata when they can't. What about removing EditorMode.ReadOnly, and just display these other tabs in the View (aka Properties) mode? And set HasEditableTrackProperties = false in the sources where we can't save the metadata.
Created attachment 156323 [details] [review] Patch v10 (In reply to comment #18) > Review of attachment 155691 [details] [review]: > > We already have HasEditableTrackProperties and HasViewableTrackProperties in > Source.cs - adding CanEditMetadata on top of that is very confusing. What > would it mean to have HasEditableTrackProperties false and CanEditMetadata > true? It's a very confusing API. True! I didn't know about that HasEditableTrackProperties property, I've used it in this patch. > The actual bug here is (AFAICT): Sometimes we make it seem like the user can > edit metadata when they can't. > > What about removing EditorMode.ReadOnly, and just display these other tabs in > the View (aka Properties) mode? And set HasEditableTrackProperties = false in > the sources where we can't save the metadata. I've done this, however I still needed a "readonlyTabs" param because I made these 3 use cases available (in order to preserve old behaviour): 1. editable track -> Edit track information -> TrackEditor in Edit mode (Properties tab shown) 2. editable track -> Properties -> TrackEditor in View mode (Properties tab is the only one shown) 3. non-editable track -> Properties (there's no other menu) -> TrackEditor in View mode (Properties tab, plus other tabs in readonly mode)
Review of attachment 156323 [details] [review]: The patch doesn't apply, could you rebase it?
Created attachment 161744 [details] [review] Patch v11 (In reply to comment #20) > Review of attachment 156323 [details] [review]: > > The patch doesn't apply, could you rebase it? Rebased. Thanks for taking a look at it.
Review of attachment 161744 [details] [review]: Looks fine, great job Andrés! Please commit after fixing the following style nitpicks: ::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/TrackEditorDialog.cs @@ +225,3 @@ + show = true; + if (mode == EditorMode.Edit && (page.PageType != PageType.ViewOnly)) { + bool show = false; Put on one line or indent with 4 spaces. @@ +230,3 @@ + show = true; + if (mode == EditorMode.Edit && (page.PageType != PageType.ViewOnly)) { + bool show = false; Add {} @@ +254,3 @@ + notebook.CurrentPage = notebook.PageNum (page_to_focus); + if (page_to_focus != null) Add {}
Comment on attachment 161744 [details] [review] Patch v11 Awesome. Committed and pushed in 454df5ee268aa6b5b9c9b76a0c6b9df67bb625a2 after fixing the nitpicks
Thanks a ton!