Created attachment 328878 [details] [review]
mainToolbar: Add fullscreen and nightmode to toolbar

For gnome-books.

This also changes the fullscreen action to a stateful one, which is
useful for multi-screen setups, where the app menu is still visible even
with the window fullscreened.

Review of attachment 328878 [details] [review]:

Few minor comments, looks good otherwise.

::: src/mainToolbar.js
@@ +91,3 @@
     },
 
+    _onFullscreenStateChanged: function(source, actionName, state) {

You could get the state using g_action_group_get_action_state() and simplify the signature of this method (so that you don't need to pass all the parameters below).

@@ +103,3 @@
+                                                action_name: 'app.fullscreen' });
+        this.toolbar.pack_end(fullscreenButton);
+    },

Is fullscreenStateId used?

@@ +106,3 @@
+            Lang.bind(this, this._onFullscreenStateChanged));
+        this._fullscreenButton = fullscreenButton;
+    addFullscreenButton: function() {

Extra whitespace before opening paren

(In reply to Cosimo Cecchi from comment #2)
> Review of attachment 328878 [details] [review] [review]:
> 
> Few minor comments, looks good otherwise.
> 
> ::: src/mainToolbar.js
> @@ +91,3 @@
>      },
>  
> +    _onFullscreenStateChanged: function(source, actionName, state) {
> 
> You could get the state using g_action_group_get_action_state() and simplify
> the signature of this method (so that you don't need to pass all the
> parameters below).

Fixed.

> @@ +103,3 @@
> +                                                action_name:
> 'app.fullscreen' });
> +        this.toolbar.pack_end(fullscreenButton);
> +    },
> 
> Is fullscreenStateId used?

It's not, I removed it.

> @@ +106,3 @@
> +            Lang.bind(this, this._onFullscreenStateChanged));
> +        this._fullscreenButton = fullscreenButton;
> +    addFullscreenButton: function() {
> 
> Extra whitespace before opening paren

Fixed.

Created attachment 328957 [details] [review]
mainToolbar: Add fullscreen and nightmode to toolbar

For gnome-books.

This also changes the fullscreen action to a stateful one, which is
useful for multi-screen setups, where the app menu is still visible even
with the window fullscreened.

Review of attachment 328957 [details] [review]:

Thanks, looks good.

Attachment 328957 [details] pushed as a36a7dd - mainToolbar: Add fullscreen and nightmode to toolbar