*** Bug 723019 has been marked as a duplicate of this bug. ***

I have created a branch for the work on initialSetup which should contain 4 commits, implementing a better initial setup.
https://git.gnome.org/browse/polari/?h=wip/bastianilso/initial-setup

More info in this blog entry:
https://blogs.gnome.org/bastian/2015/06/04/a-first-time-polari-experience/

I think the branch is ready for review.

(In reply to Bastian from comment #2)
> I have created a branch for the work on initialSetup which should contain 4
> commits, implementing a better initial setup.

So I've finally had some time to play around with this a bit, here are some initial impressions:
 - the normal window quickly flashes up before the initial setup window
 - do we need to hint to the connection editor in the app menu to avoid
   falling into the "wizard trap"[0]? Or maybe it would make sense to merge
   the initial setup window into ChatPlaceholder?
 - related to the last thought: if I disable all accounts, I get a placeholder
   that tells me to use the (+) button to join a room, but that dialog doesn't
   let me use the existing accounts (read: re-enable them)

Regarding the patch series, it currently looks roughly like this:

(1) some initial work by Carlos
(2) update it so that it actually works
(3) update its looks and feel so that it doesn't feel alien
(4) add chat placeholder

I do see the point of crediting Carlos for his initial work, but it's so WIP and incomplete that I think it's fine to base the commit series on logical changes rather than history - namely squash the first three commits into one (taking over authorship of course)

[0] http://stef.thewalter.net/installer-anti-pattern.html

Created attachment 307662 [details] [review]
roomStack: add visuals to chatPlaceholder

(In reply to Florian Müllner from comment #3)
>    [..] Or maybe it would make sense to merge
>    the initial setup window into ChatPlaceholder?

I played with that today and I think that's the right way to go. Besides the design advantages you mention it's also less code - It's one single patch now.

Some additional things I would like input on:
 * It might be nice to make a link to the application menu, similar to how GNOME music links to the music folder in its empty state's description text[0]. That is, so pressing the "application menu" link will open the application menu.
 * We could also add an empty state text to connection-list-dialog.ui too (so far testing the waters results in lots of funky errors atm though[1] ... ).


[0]: https://github.com/gnome-design-team/gnome-mockups/blob/master/empty-app-states/music.png
[1]: http://fpaste.org/245690/37233267/

(In reply to Bastian from comment #4)
> Created attachment 307662 [details] [review] [review]
> roomStack: add visuals to chatPlaceholder
> 
> (In reply to Florian Müllner from comment #3)
> >    [..] Or maybe it would make sense to merge
> >    the initial setup window into ChatPlaceholder?
> 
> I played with that today and I think that's the right way to go. Besides the
> design advantages you mention it's also less code - It's one single patch
> now.

Ah, that's not what I was suggesting - I meant merging the accounts list from the previous version into the ChatPlaceholder. But maybe what you are doing here is indeed better than duplicating that UI in two places (with the more up-front one having no obvious way to get back to once the users starts joining channels) ...


> Some additional things I would like input on:
>  * It might be nice to make a link to the application menu, similar to how
> GNOME music links to the music folder in its empty state's description
> text[0]. That is, so pressing the "application menu" link will open the
> application menu.

I do see a difference between a content location and some UI element, and I'm not aware of any precedent of an application doing this - there's certainly no clean way to do it, which we'd want to add first should this pattern become a thing. However I'm not really convinced that this is a good idea - users have certain expectation how standard UI elements like menus behave and we shouldn't mess lightly with this. How about turning the "Connections" text into a link to the connection dialog though?


>  * We could also add an empty state text to connection-list-dialog.ui too
> (so far testing the waters results in lots of funky errors atm though[1] ...

Yeah, you are trying to add an additional child to a container (GtkScrolledWindow) that only allows a single child. I don't see why you would ever want to scroll the placeholder, so the parent (a GtkBox) is probably a good pick.

reply to Florian Müllner from comment #5)
> [..] How about turning the "Connections" text into a link to the 
> connection dialog though?

Then we kinda end back to the in the "no obvious way to get back to once the users starts joining channels" issue, no? I see more value in teaching the user how to open connections from the application menu by letting them to try doing so then, hmm..


> > (so far testing the waters results in lots of funky errors atm though[1] ...
> 
> Yeah, you are trying to add an additional child to a container
> (GtkScrolledWindow) that only allows a single child. I don't see why you
> would ever want to scroll the placeholder, so the parent (a GtkBox) is
> probably a good pick.

ah thanks. I made another branch to confuse you. I made no changes to the roomStack commit, but it includes a new commit which adds a placeholder text to Connections.[0]

[0]: https://git.gnome.org/browse/polari/log/?h=wip/bastianilso/initial-setup-2

(In reply to Bastian from comment #6)
> reply to Florian Müllner from comment #5)
> > [..] How about turning the "Connections" text into a link to the 
> > connection dialog though?

Dunno. At this point, I expect users to understand an instruction like "Open Connections in the application menu" - and if not, then maybe the concept of the menu needs rethinking before adding hacks to control the system UI from applications ...

(In reply to Florian Müllner from comment #7)
> (In reply to Bastian from comment #6)
> > reply to Florian Müllner from comment #5)
> > > [..] How about turning the "Connections" text into a link to the 
> > > connection dialog though?
> 
> Dunno. At this point, I expect users to understand an instruction like "Open
> Connections in the application menu" - and if not, then maybe the concept of
> the menu needs rethinking before adding hacks to control the system UI from
> applications ...

True. I guess it comes down to design philosophy - whether it's app's responsibility to teach the patterns or whether it should just focus on enforcing/following them.

'Connections' is now a link, (part of roomStack commit):
https://git.gnome.org/browse/polari/log/?h=wip/bastianilso/initial-setup-2

(In reply to Bastian Ilsø from comment #8)
> 'Connections' is now a link, (part of roomStack commit):
> https://git.gnome.org/browse/polari/log/?h=wip/bastianilso/initial-setup-2

I like it. I have some (minor) comments on the code, so can you attach the patches for review?

Also I'd appreciate if you could rebase your branches on top of 3.17.3 - there has been a change in GTK+'s style handling internals this cycle which makes Polari go absolutely bonkers without commit a6d7dc231.

Created attachment 307674 [details] [review]
roomStack: add visuals to chatPlaceholder

Currently there is a lot of empty grey space when Polari's
main window is started with no rooms in it. The patch adds a
recognizeable Polari logo to the background and a helping text
for adding connections, enabling accounts or joining a chatroom.

Created attachment 307675 [details] [review]
Connections: Add placeholder text when state is empty

As part of the effort to improve the initial setup experience
in Polari, a message is now shown telling the user how to
add a new connection as a convenience. The message is only
visible should when no connections have been added.

initial-setup-2[0] has been rebased to master. I'll rebase the rest later today.

[0]: https://git.gnome.org/browse/polari/log/?h=wip/bastianilso/initial-setup-2

Review of attachment 307674 [details] [review]:

::: src/roomStack.js
@@ +90,3 @@
+        this._image = new Gtk.Image({ icon_name: 'polari-symbolic',
+                                      pixel_size: 96, halign: Gtk.Align.END,
+                                      margin_end: 14 });

image and title are not modified after being added to the grid, so normal local variables could be used instead of object properties

@@ +103,3 @@
+        this._instruction = new Gtk.Label({ halign: Gtk.Align.CENTER,
+                                            use_markup: true, wrap: true });
+        this._hrefText = _('<a href="%s">%s</a>').format('connections', _("Connections"));

It doesn't make sense to mark '<a href="%s">%s</a>' for translation - it *must* be the same for all languages. Should be '<a href="connections">%s</a>'.format(_("Connections"));

@@ +108,3 @@
+            let action = app.lookup_action(actionName);
+            action.activate(null);
+            return action;

action? The handler should return a boolean to indicate whether the "url" has been opened - if you want to do something fancier than returning true, it should be "action != null", but then you should also handle the action == null case properly ("action = null; action.activate();" will throw an exception)

@@ +109,3 @@
+            action.activate(null);
+            return action;
+            }));

Weird indentation

@@ +127,3 @@
+        let accounts = this._accountsMonitor.dupAccounts();
+        if (accounts.length != 0) {
+            let enabledAccount = false;

More concise as:

let enabledAccount = accounts.some(function(a) { return a.enabled; });

@@ +136,3 @@
+            if (enabledAccount) {
+                this._addRoomText();
+                return;

I don't like using an "early" return here - rather than a mix of nested ifs and returns, I'd prefer something cleaner like:

let accounts = this._accountsMonitor.dupAccounts();
if (accounts.length == 0)
    this._addAccountText();
else if (accounts.some(function(a) { return a.enabled; }))
    this._addRoomText();
else
    this._enableAccountText();

@@ +145,3 @@
+    },
+
+    _addAccountText: function() {

The function name is a bit of a misnomer - it suggests to add a label somewhere, but it actually changes existing labels' text. Personally I wouldn't bother coming up with a better name, and just move this inline into checkAccounts().

@@ +146,3 @@
+
+    _addAccountText: function() {
+        this._description.label = "Begin chatting by adding a new connection.";

Needs to be marked for translation

@@ +148,3 @@
+        this._description.label = "Begin chatting by adding a new connection.";
+        this._instruction.label = "Open " + this._hrefText
+                                + " in the application menu.";

+ string concatenation does not work with gettext, so use format() with %s; will also need a translator comment to explain what %s will be.
Also not sure whether we can reuse hrefText like this - "Connections" may need to be inflected differently in the two phrases. So maybe use something like:
/* translators: This will be used in the phrase: "Open Connections in the application menu" */
let href = '<a href="connections">%s</a>'.format(_("Connections"));
this._instruction.label = _("Open %s in the application menu").format(href);

Review of attachment 307675 [details] [review]:

I wonder whether the GtkStack here is a bit overkill compared to just putting list and empty state into the box and setting their visibility on account changes - the transition between "empty" and "non-empty" happens when the dialog is covered by a modal child dialog, so I doubt the crossfade makes a noticeable difference.

::: src/connections.js
@@ -42,3 @@
         this._listBox = builder.get_object('accounts_list');
         this._stack = builder.get_object('stack');
-

Unrelated whitespace change

@@ +54,3 @@
         context.set_junction_sides(Gtk.JunctionSides.BOTTOM);
 
+        this._emptyState = builder.get_object('empty_state');

If you assign names to the stack children, you don't have to keep the widgets around and can set GtkStack:visible-child-name instead.

@@ -129,3 @@
             if (rows[i]._account == account) {
                 rows[i].destroy();
-                return;

Minor optimization: change to "break" instead of removing it altogether

Created attachment 307711 [details] [review]
roomStack: add visuals to chatPlaceholder

Currently there is a lot of empty grey space when Polari's
main window is started with no rooms in it. The patch adds a
recognizeable Polari logo to the background and a helping text
for adding connections, enabling accounts or joining a chatroom.

Created attachment 307712 [details] [review]
Connections: Add placeholder text when state is empty

As part of the effort to improve the initial setup experience
in Polari, a message is now shown telling the user how to
add a new connection as a convenience. The message is only
visible should when no connections have been added.

Review of attachment 307711 [details] [review]:

LGTM

Review of attachment 307712 [details] [review]:

::: src/connections.js
@@ +68,3 @@
 
+        if (this._listBox.get_children() == 0) {
+            this._emptyState.visible = true;

This is called before any accounts are added to the list, so you could just fix up the visibility in the .ui file.
In any case, I'd split this into a separate function for readability:

  _syncVisibility: function() {
      let isEmpty = this._listBox.get_children() == 0;
      this._emptyState.visible = isEmpty;
      this._scrolledWindow.visible = !isEmpty;
  }

and use that in the appropriate places (or as handler to the GtkContainer::add and GtkContainer::remove signals)

Created attachment 307740 [details] [review]
Connections: Add placeholder text when state is empty

As part of the effort to improve the initial setup experience
in Polari, a message is now shown telling the user how to
add a new connection as a convenience. The message is only
visible should when no connections have been added.

Review of attachment 307740 [details] [review]:

::: src/connections.js
@@ +75,3 @@
         this.widget.connect('destroy', Lang.bind(this, this._onDestroy));
+
+        this._syncVisibility();

You don't need this if you make the empty state visible in the .ui file

Created attachment 307745 [details] [review]
Add placeholder text when state is empty

As part of the effort to improve the initial setup experience
in Polari, a message is now shown telling the user how to
add a new connection as a convenience. The message is only
visible should when no connections have been added.

Review of attachment 307745 [details] [review]:

Yay!

pushed:
https://git.gnome.org/browse/polari/commit/?id=c824ad3ec37d482ea52a03ba0e59956b70e38a58
https://git.gnome.org/browse/polari/commit/?id=c7db2a04e553f6a2500302612126609d974e8808