Merge lp:~elementary-apps/pantheon-photos/unfullscreen into lp:~pantheon-photos/pantheon-photos/trunk

[Danielle Foré]

use symbolic window restore icon

Changes out the stock.unfullscreen icon for symbolic window-restore

[Victor Martinez (victored)]

Looks good!

One thing though. Let's stick to Shotwell's coding style. There are thousands of lines of code using it, and new code just look bad and inconsistent when we impose our own style.

I'm talking about leaving no spaces before opening parenthesis (diff lines 10, 18, and 19).

[Danielle Foré (danrabbit)]

Hey Victor, do we really want to stick to Shotwell's coding style? I think long term it might make more sense for new code to be our style so that we can eventually convert

[Erasmo Marín (erasmo-marin)]

In my opinion we should stick to the Yorba code style, because using 2 differents styles hurts your brain, so, there are only 2 available options:

1) Keep the current style (easy)
2) Change everything to the elementary style (hard)

Also, keeping the current style is good for shotwell and pantheon-photos. It's better for colaboration between the projects. I did the same comment a moment ago in another merge propose.

[David Gomes (davidgomes)]

I agree with Victor and Erasmo.

[Jim Nelson (yorba-jim)]

For whatever it's worth, I'd be pretty miffed if you guys converted the entire code base to your style just to get your preferred brace style or whatever. That's the kind of thing that makes a fork no longer a fork.

[Victor Martinez (victored)]

Agreed, that's totally not worth it and even harmful to the revision history.

The purpose of coding style is having a *consistent* codebase, and the thousands of lines of code in Shotwell have have a well-defined style already.

I'm glad the existing codebase is extremely coherent in this regard :)

review: Needs Fixing

[lp:~elementary-apps/pantheon-photos/unfullscreen]

2517. By Danielle Foré on 2014-05-23

change paranthens to yorba style

[Danielle Foré (danrabbit)]

Okay changed to Yorba style.

I gotta say that I disagree though. Since all of us are working on multiple apps we're already dealing with 2 code styles. Imo, This will make it harder to contribute to elementary development as a whole.

[Victor Martinez (victored)]

Thank you for the fix Daniel!

I can totally see your point. I don't see it as a big problem though because developers should always adapt to the style of the file they're working on.

I've noticed many have issues to comply with this even when we're trying to enforce our own style, and this delays our merges.

review: Approve

=== modified file 'src/AppWindow.vala'
--- src/AppWindow.vala	2014-05-20 08:26:35 +0000
+++ src/AppWindow.vala	2014-05-23 02:38:49 +0000
@@ -9,8 +9,7 @@
     public const int TOOLBAR_DISMISSAL_SEC = 2;
     public const int TOOLBAR_CHECK_DISMISSAL_MSEC = 500;
     
-    private Gtk.Window toolbar_window = new Gtk.Window(Gtk.WindowType.POPUP);
-    private Gtk.ToolButton close_button = new Gtk.ToolButton.from_stock(Gtk.Stock.LEAVE_FULLSCREEN);
+    private Gtk.Window toolbar_window = new Gtk.Window(Gtk.WindowType.POPUP);  
     private Gtk.ToggleToolButton pin_button = new Gtk.ToggleToolButton.from_stock(Resources.PIN_TOOLBAR);
     private bool is_toolbar_shown = false;
     private bool waiting_for_invoke = false;
@@ -50,6 +49,8 @@
         pin_button.set_tooltip_text(_("Pin the toolbar open"));
         pin_button.clicked.connect(update_toolbar_dismissal);
         
+        Gtk.Image img = new Gtk.Image.from_icon_name("window-restore-symbolic", Gtk.IconSize.LARGE_TOOLBAR);
+        Gtk.ToolButton close_button = new Gtk.ToolButton(img, null);
         close_button.set_tooltip_text(_("Leave fullscreen"));
         close_button.clicked.connect(on_close);