Noise

Merge lp://staging/~tintou/noise/fix-interface into lp://staging/~elementary-apps/noise/trunk

fix-interface
Merge into trunk

Proposed by Corentin Noël on 2014-03-14

Status:

Merged

Approved by:

Danielle Foré on 2014-03-16

Approved revision:

1573

Merged at revision:

1563

Proposed branch:

lp://staging/~tintou/noise/fix-interface

Merge into:

lp://staging/~elementary-apps/noise/trunk

Diff against target:

473 lines (+77/-104)

7 files modified

CMakeLists.txt (+5/-5)
plugins/LastFM/PreferencesSection.vala (+3/-3)
src/Dialogs/MediaEditor.vala (+25/-46)
src/Dialogs/PreferencesWindow.vala (+20/-20)
src/Dialogs/SmartPlaylistEditor.vala (+22/-22)
src/LibraryWindow.vala (+2/-2)
src/Widgets/StatusBar.vala (+0/-6)

To merge this branch:

bzr merge lp://staging/~tintou/noise/fix-interface

Related bugs:

Bug #1289724: "Hide window when a song is being played" doesn't make sense	Low	Fix Released
Bug #1289726: "Show notifications" can be simplified	Low	Fix Released
Bug #1291107: use gtk dialog for song editor	Low	Fix Released
Bug #1291831: Preferences window is too wide	Low	Fix Released
Bug #1292182: remove right padding from statusbar buttons	Low	Fix Released

Link a bug report

Reviewer	Date Requested	Status
Danielle Foré		Approve on 2014-03-16
Victor Martinez (community)	2014-03-14	Needs Information on 2014-03-15
Review via email: mp+211080@code.staging.launchpad.net

Commit message

Fixed some problems with the interface:
* English strings in the preference dialog
* Lastfm string being expanded…
* Luna padding on the statusbar
* Media Editor dialog now uses Gtk.Dialog
* Preference Dialog and Media Editor Dialog now uses Gtk.HeaderBar
* Smart Playlist editor dialog now uses Gtk.HeaderBar
* Gtk.Dialog.get_action_area has been replaced with ButtonBox
* Set suggested action style class on "Save" buttons

Description of the change

Revision history for this message

Victor Martinez (victored) wrote on 2014-03-15:

This branch looks great.

I have some questions:

1. Where is the code that uses Gtk.Popover?

2. What about using the "use-header-bar" property for Gtk.Dialog? Only for GTK >= 3.12: https://developer.gnome.org/gtk3/unstable/GtkDialog.html#GtkDialog--use-header-bar

3. Gtk.Label supports text wrapping. Granite.Widgets.WrapLabel shouldn't be necessary.

4. Cast the value of Gtk.Dialog.get_content_area to Gtk.Container instead of the implementation-specific Gtk.Box.

review: Needs Information

lp://staging/~tintou/noise/fix-interface updated on 2014-03-15

1565. By Corentin Noël on 2014-03-15: media editor: Now uses a Gtk.Dialog with HeaderBar.
1566. By Corentin Noël on 2014-03-15: preference dialog: use Headerbar for better looking and consistency
1567. By Corentin Noël on 2014-03-15: dependencies: Now required gtk+ >= 3.11.6 and vala >= 0.23.2
1568. By Corentin Noël on 2014-03-15: media editor: Using Gtk+ 3.12 integrated HeaderBar

Revision history for this message

Corentin Noël (tintou) wrote on 2014-03-15:

Thanks for the review!

1. Indeed, I wanted to say Gtk.Dialog instead of Gtk.Popover, it's fixed in trunk.

2. I didn't know that this property existed, I've pushed some revisions to use this property instead.

3. I tried with Gtk.Label but as the dialog has no size limit it doesn't wrap the label, Granite WrapLabel is needed.

4. Indeed, fixed.

Revision history for this message

Danielle Foré (danrabbit) wrote on 2014-03-15:

Hrm, I'm not sure I like removing the close button from the bottom right of dialogs. I think this creates an inconsistency since gtk.dialog retains the explicit buttons, but removes the close window decoration instead. I would agree that it's better to retain the label button since it an give a more explicit explanation of what closing the dialog does (maybe not applicable in this particular dialog, but in the scheme of platform consistency it is important in other dialogs).

I think we should:
1. revert the window decoration/close button situation in the prefs dialog
2. consider adding a "Cancel" button to the media editor and use the standard gtk dialog without a window decoration.

Revision history for this message

Victor Martinez (victored) wrote on 2014-03-16:

The code looks good at this point.

I believe you will implement Dan's suggestions before merging. If so please consider this as well:

1. Leave a single space after casts: (TypeName)var => (TypeName) var
2. Allocate the parent container size before packing the label or setting the text to avoid issues with text wrapping: https://developer.gnome.org/gtk3/unstable/GtkLabel.html#gtk-label-set-line-wrap
3. Gtk.Dialog.get_action_area has been deprecated since GTK 3.12: https://developer.gnome.org/gtk3/unstable/GtkDialog.html#gtk-dialog-get-action-area

lp://staging/~tintou/noise/fix-interface updated on 2014-03-16

1569. By Corentin Noël on 2014-03-16: media editor and preferences dialog: reverted close button deletion
1570. By Corentin Noël on 2014-03-16: smartplaylisteditor: removed decorated window
1571. By Corentin Noël on 2014-03-16: Gtk 3.12 does pack_end on headerbar on an other way than previously, switched back to the original disposition.
1572. By Corentin Noël on 2014-03-16: fixed style

Revision history for this message

Corentin Noël (tintou) wrote on 2014-03-16:

Daniel:
I reverted the changes, there is now a close button allowing consistency, I also added SUGGESTED_ACTION for button with a "Save" label.

Victor:
1. Okay, changed.
2. Hmm, it's something that could be investigated in a next branch.
3. Yes, I replaced this with a ButtonBox in the content_area.

lp://staging/~tintou/noise/fix-interface updated on 2014-03-16