GNOME Bugzilla – Bug 675245
Provide an app menu
Last modified: 2012-09-01 17:05:30 UTC
See - https://live.gnome.org/GnomeGoals/PortToGMenu You should be able to replace the menu bar with the app menu. Suggested app menu format: Disc > --- Submit Track Names --- Preferences --- Help About Sound Juicer Quit Where disc is a submenu containing: Re-read Duplicate Eject I'm a bit unsure about the submit track names entry - the label could be clearer.
Created attachment 213445 [details] [review] main: Port to GtkApplication
Created attachment 213446 [details] [review] Remove the now unused bacon files
Created attachment 213447 [details] [review] ui: Use GtkApplicationWindow rather than GtkWindow
Created attachment 213448 [details] [review] Add an application menu
Created attachment 213449 [details] [review] Remove the remaining menubar
Created attachment 213450 [details] [review] Add an application menu Woops, forgot to add the menu's ui file ...
(In reply to comment #1) > Created an attachment (id=213445) [details] [review] > main: Port to GtkApplication There was already a patch for than in bug #661363 :((
So, should we get one of the patches landed, then ?
Created attachment 216560 [details] [review] main: Port to GtkApplication
Created attachment 216561 [details] [review] Remove the now unused bacon files
Created attachment 216562 [details] [review] ui: Use GtkApplicationWindow rather than GtkWindow
Created attachment 216563 [details] [review] Add an application menu
Created attachment 216564 [details] [review] Remove the remaining menubar
Here are the patches rebased against master. I've squashed in the last patch to get rid of a runtime warning. diff --git a/src/sj-main.c b/src/sj-main.c index b59f677..71f4025 100644 --- a/src/sj-main.c +++ b/src/sj-main.c @@ -590,7 +590,7 @@ static void update_ui_for_album (AlbumDetails *album) set_action_enabled ("play", TRUE); set_action_enabled ("select-all", FALSE); set_action_enabled ("deselect-all", TRUE); - set_action_enabled ("prev-track", FALSE); + set_action_enabled ("previous-track", FALSE); set_action_enabled ("next-track", FALSE); set_duplication (TRUE);
I've enough confidence in Christophe to approve this series without reviewing it. Hey Christophe, want to maintain SJ?
So, should we get the patches landed, then ?!
wanted to have a look at these last evening, but got sidetracked with boxes :( Ross, I'm fine with being a co-maintainer and do occasional releases, but I probably won't have much time either to work on it, and I'm also a morituri user these days, so a bit complicated ;)
Created attachment 219414 [details] [review] main: Port to GtkApplication
Created attachment 219415 [details] [review] Remove the now unused bacon files
Created attachment 219416 [details] [review] ui: Use GtkApplicationWindow rather than GtkWindow
Created attachment 219417 [details] [review] Add an application menu
Created attachment 219418 [details] [review] Remove the remaining menubar
I've finally looked over these, and they look good. The series I've just attached does 2 things: it removes the set_duplication function and calls directly set_action_enabled instead, and it fixes translation of the app menu (it was missing from POTFILES.in, and for some reason, intltool wants "" around attribute values, not '') Just a few questions: - this removes 'Extract CD' from the menu, this makes sense to me, but I prefer to check it's intentional - the app menu entries have no accelerators, same question, no idea if this is just something that was missed or if this is on purpose
Filed https://bugs.launchpad.net/intltool/+bug/1027631 for the ' VS " issue
{Un}select-all seems to be totally gone from the UI, I've sometimes had use for it, so I'm not sure we want to get rid of it altogether.
app menus intentionally don't show accelerators
Removing (Un)select All would probably result in bug reports pretty quickly so it should be bought back in some form. Apart from that if teuf's reviewed the patches I'm happy to see them land. After all teuf is a SJ maintainer now (congrats teuf!).
(In reply to comment #27) > Removing (Un)select All would probably result in bug reports pretty quickly so > it should be bought back in some form. (Un)select All is no longer exposed in the UI, but the functionality is still there ((<Shift>)<Control>a). I don't think it is suited for the application menu, however I'll talk to the designers about other means to expose it.
Any update on this from the designers ?
We chatted yesterday on IRC. I suggested adding a Select All / Select None button, in line with Play and Extract, but justified to the right. (Note that you should probably select all by default.)
Fwiw, I'll be making a release today as Ross wants to get one out so that the GStreamer 1.0 stuff gets testing, hopefully we can merge this code soon after the release.
Created attachment 220950 [details] [review] Add "{Un,}Select All" button The Unselect all/Select all menu entries have been removed as part of the addition of the app menu an were only accessible through a keyboard shortcut. After a discussion with the design team, readd a button to provide this functionality.
Here is the missing bit for this bug. Florian, Ross, it would be great if you could review this patch, then we can merge all of this. First comment is that I used "Unselect All" in the patch while the wording suggested by Allan was "Select None", I'll change that.
Ah and some c&p fail in the gtksizegroup stuff
Created attachment 220951 [details] [review] Add "{Un,}Select All" button The Unselect all/Select all menu entries have been removed as part of the addition of the app menu an were only accessible through a keyboard shortcut. After a discussion with the design team, readd a button to provide this functionality.
Created attachment 220952 [details] [review] Add "{Un,}Select All" button The Unselect all/Select all menu entries have been removed as part of the addition of the app menu an were only accessible through a keyboard shortcut. After a discussion with the design team, readd a button to provide this functionality.
Created attachment 220953 [details] [review] Add "{Un,}Select All" button The Unselect all/Select all menu entries have been removed as part of the addition of the app menu an were only accessible through a keyboard shortcut. After a discussion with the design team, readd a button to provide this functionality.
(sorry about the mess, took me a few tries to get things right :(
Created attachment 220954 [details] [review] suggested changes to "Remove the remaining menubar" (In reply to comment #22) > Created an attachment (id=219418) [details] [review] > Remove the remaining menubar This patch has 2 small issues: it adds whitespace in sj-play.h, and the handling of the play action can be simplified a bit, I'd squash the attached patch into that one.
And one last thing I just noticed is that there is no action, no keyboard shortcut for Extract. This surprises me as this is what sound-juicer is meant for.
Review of attachment 220953 [details] [review]: Hmm, so if a single track is selected, I have to press the button twice to select all? Still, if the designers are happy, I'm happy :-) The code looks good - thanks for pushing this forward!
Review of attachment 220954 [details] [review]: Looks great to me, thanks! ::: src/sj-main.c @@ +1733,3 @@ gtk_button_set_label(GTK_BUTTON(select_button), _("Select None")); gtk_actionable_set_action_name(GTK_ACTIONABLE(select_button), "win.deselect-all"); + gtk_actionable_set_action_name(GTK_ACTIONABLE(play_button), "win.play"); Good catch! ::: src/sj-play.h @@ -38,3 @@ void toggle_play (void); - void play_next_track (void); Ugh, how did I end up doing that in the first place?
Finally pushed all of this.