Created attachment 277755 [details] [review]
Add Check-In capabilities

This only adds check-in capabilities necessary
to implement a check-in UI.

Created attachment 277756 [details] [review]
Add Check-In dialog

This only adds the dialog code to be called in future
Maps version.

You can test the check-in dialog using the following code:

---

let dialog = new imports.checkInDialog.CheckInDialog({
    transient_for: this.window,
    place: new Geocode.Place({
        name: '<place name>',
        place_type: null,
        location: new Geocode.Location({
            latitude: <place latitude>,
            longitude: <place longitude>,
            accuracy: 0
        })
    })
});
let response = dialog.run();
dialog.destroy();

print('Check-In dialog response: ' + response);

---

Just put it somewhere in Maps code.

To test Foursquare support you will need to apply this patch in gnome-online-accounts and use the patched goa-daemon:

https://bugzilla.gnome.org/attachment.cgi?id=276201

The related bug report is:

https://bugzilla.gnome.org/show_bug.cgi?id=729837

You will need a Foursquare client ID and configure gnome-online-accounts --with-foursquare-client-id with that ID. You'll need to create a Foursquare app to get a client ID or if you want, I can provide you the ID for my testing app, just send me a private e-mail/chat.

Created attachment 277762 [details] [review]
Add Check-In capabilities - v2

Add copyright header.

Created attachment 277763 [details] [review]
Add Check-In dialog - v2

Add copyright headers.

TODO: We need to save the privacy/bradcast options to GSettings so user does not need to configure it each time she check-ins if she does not like default options.

Review of attachment 277762 [details] [review]:

Thanks for the patch!

Some initial comments below, will look closer when I'm not so tired :)

::: src/checkIn.js
@@ +44,3 @@
+        this._goaClient.connect('account-added', Lang.bind(this, this._refreshGoaAccounts));
+        this._goaClient.connect('account-changed', Lang.bind(this, this._refreshGoaAccounts));
+        this._goaClient.connect('account-removed', Lang.bind(this, this._refreshGoaAccounts));

Please use Maps way of binding: this._refreshGoaAccounts.bind(this);

@@ +58,3 @@
+            let backend = backendsArray[i];
+            this._backends[backend.getName()] = backend;
+        }

Do we really need the loop here? Can't we just instansiate the backends above and and them to this._backends?

Like:

    this._backends[facebookBackend.getName(), foursquareBackend.getName()];

@@ +67,3 @@
+        this._authorizers = {};
+
+        accounts.forEach(Lang.bind(this, function(object) {

In Maps we have been using the construct

    .forEach((function() {

    }).bind(this));

@@ +72,3 @@
+
+            if (!object.get_maps())
+                return;

Do we need any more error handling here? What does it signify that these are non-truthy?

@@ +83,3 @@
+            } else {
+                //not reached
+            }

If it's not reached, does it need to be there then? Either add error handling (log or utils.debug maybe?) or remove it.

Also no brackets needed on these constructs, right?

@@ +103,3 @@
+    getAuthorizers: function() {
+        return this._authorizers;
+    },

Maybe these getters can be in the form of

get Client() { ...

get Accounts() { ...

[...]

instead?

@@ +120,3 @@
+        this.getBackendForAuthorizer(authorizer)
+            .performCheckInAsync(authorizer, checkIn, callback, cancellable);
+    },

If there is no sync version of this then I think we can drop the Async part of the name.

@@ +125,3 @@
+        this.getBackendForAuthorizer(authorizer)
+            .getPlacesAsync(authorizer, latitude, longitude, distance, callback, cancellable);
+    }

Same here.

::: src/socialService/facebookBackend.js
@@ +50,3 @@
+        return data == null?
+            restCall.get_status_code():
+            (data.error? data.error.code:null);

Some spaces around the conditional '?' would be nice

@@ +57,3 @@
+            restCall.get_status_message():
+            (data.error? data.error.message:null);
+    },

Same here.

::: src/socialService/foursquareGoaAuthorizer.js
@@ +36,3 @@
+        if (!params.goa_object) {
+            logError('FoursquareGoaAuthorizer requires goa_object parameter');
+            return;

Why not have this take a goaObject instead of 'params' ?

@@ +39,3 @@
+        }
+
+        //this._mutex = new GLib.Mutex();

Remove this and all other commented out mutex code

@@ +47,3 @@
+    },
+
+    get goa_object() {

Why not goaObject?

@@ +51,3 @@
+    },
+
+    set goa_object(object) {

Same here.

@@ +60,3 @@
+    },
+
+    process_call: function(restCall) {

We use camelCase naming in Maps, fix here and below.

::: src/socialService/serviceBackend.js
@@ +33,3 @@
+    getName: function() {
+        //Virtual
+    },

Do we gain anything by having all these empty methods?

::: src/socialService/socialPlace.js
@@ +47,3 @@
+    getOriginalData: function() {
+        return this._originalData;
+    }

Why not get originalData() {

?

(In reply to comment #4)
>
> You will need a Foursquare client ID and configure gnome-online-accounts
> --with-foursquare-client-id with that ID. You'll need to create a Foursquare
> app to get a client ID or if you want, I can provide you the ID for my testing
> app, just send me a private e-mail/chat.

Wasn't the conclusion of the whole discussion with foursquare guy (the emails I forwarded to you) that we should use the foursq API that doesn't require any secret client ID?

(In reply to comment #8)
> @@ +72,3 @@
> +
> +            if (!object.get_maps())
> +                return;
> 
> Do we need any more error handling here? What does it signify that these are
> non-truthy?
> 

I am iterating over all GOA accounts, if get_maps() returns false, that means that the account does not provide "Maps" feature or that the account has the "Maps" feature disabled, so we don't have to use it.

> @@ +83,3 @@
> +            } else {
> +                //not reached
> +            }
> 
> If it's not reached, does it need to be there then? Either add error handling
> (log or utils.debug maybe?) or remove it.
> 
> Also no brackets needed on these constructs, right?

There is no other account providers that supports Maps feature, that line should never reached, the line was intended to help programmers to understand that, this line is safely removable.

Sorry, I am used to put brackets in single line bodies... improves legibility for me and is more errorproof, I'm changing it now.

> ::: src/socialService/foursquareGoaAuthorizer.js
> @@ +36,3 @@
> +        if (!params.goa_object) {
> +            logError('FoursquareGoaAuthorizer requires goa_object parameter');
> +            return;
> 
> Why not have this take a goaObject instead of 'params' ?

To follow the same conventions of Gjs GObject constructors.

> @@ +47,3 @@
> +    },
> +
> +    get goa_object() {
> 
> Why not goaObject?
> 
> @@ +51,3 @@
> +    },
> +
> +    set goa_object(object) {
> 
> Same here.
> 
> @@ +60,3 @@
> +    },
> +
> +    process_call: function(restCall) {
> 
> We use camelCase naming in Maps, fix here and below.

I did that to follow the syntax of a native library, the I was planning to move FoursquareGoaAuthorizer to a native library.

> 
> ::: src/socialService/serviceBackend.js
> @@ +33,3 @@
> +    getName: function() {
> +        //Virtual
> +    },
> 
> Do we gain anything by having all these empty methods?

It helps to know which is the interface to implement to the concrete backends.

(In reply to comment #9)
> (In reply to comment #4)
> >
> > You will need a Foursquare client ID and configure gnome-online-accounts
> > --with-foursquare-client-id with that ID. You'll need to create a Foursquare
> > app to get a client ID or if you want, I can provide you the ID for my testing
> > app, just send me a private e-mail/chat.
> 
> Wasn't the conclusion of the whole discussion with foursquare guy (the emails I
> forwarded to you) that we should use the foursq API that doesn't require any
> secret client ID?

Foursquare API has a client ID and a secret key, the secret key is not required but the client ID is required on all authentication methods.

If not, how Foursquare can prints the app information in the login dialog and how a user can disable GNOME in her Foursquare account?

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #4)
> > >
> > > You will need a Foursquare client ID and configure gnome-online-accounts
> > > --with-foursquare-client-id with that ID. You'll need to create a Foursquare
> > > app to get a client ID or if you want, I can provide you the ID for my testing
> > > app, just send me a private e-mail/chat.
> > 
> > Wasn't the conclusion of the whole discussion with foursquare guy (the emails I
> > forwarded to you) that we should use the foursq API that doesn't require any
> > secret client ID?
> 
> Foursquare API has a client ID and a secret key, the secret key is not required
> but the client ID is required on all authentication methods.

Ah ok so its not the secret thing. I got confused because you didn't want to share the app ID here?

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (In reply to comment #4)
> > > >
> > > > You will need a Foursquare client ID and configure gnome-online-accounts
> > > > --with-foursquare-client-id with that ID. You'll need to create a Foursquare
> > > > app to get a client ID or if you want, I can provide you the ID for my testing
> > > > app, just send me a private e-mail/chat.
> > > 
> > > Wasn't the conclusion of the whole discussion with foursquare guy (the emails I
> > > forwarded to you) that we should use the foursq API that doesn't require any
> > > secret client ID?
> > 
> > Foursquare API has a client ID and a secret key, the secret key is not required
> > but the client ID is required on all authentication methods.
> 
> Ah ok so its not the secret thing. I got confused because you didn't want to
> share the app ID here?

Ah haha... no, that was to avoid abuse of the testing app :)

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #9)
> > > > (In reply to comment #4)
> > > > >
> > > > > You will need a Foursquare client ID and configure gnome-online-accounts
> > > > > --with-foursquare-client-id with that ID. You'll need to create a Foursquare
> > > > > app to get a client ID or if you want, I can provide you the ID for my testing
> > > > > app, just send me a private e-mail/chat.
> > > > 
> > > > Wasn't the conclusion of the whole discussion with foursquare guy (the emails I
> > > > forwarded to you) that we should use the foursq API that doesn't require any
> > > > secret client ID?
> > > 
> > > Foursquare API has a client ID and a secret key, the secret key is not required
> > > but the client ID is required on all authentication methods.
> > 
> > Ah ok so its not the secret thing. I got confused because you didn't want to
> > share the app ID here?
> 
> Ah haha... no, that was to avoid abuse of the testing app :)

Unless they have something in their ToS that forbids it, don't care too much about that. In open source world, you only hide information when you really have to. :) The actual GNOME ID we'll use in the end will go through this bugzilla anyway.

Created attachment 278368 [details] [review]
Add Check-In capabilities - v3

I've changed the code according Jonas review except FoursquareGoaAuthorizer 
methods/attributes syntax and the "virtual methods" in ServiceBackend, 
because I had not feedback about my review comments.

Created attachment 278369 [details] [review]
Add Check-In dialog - v3

The dialog code also needed adaptations and also I've found a bug during the 
CheckIn privacy setting initialization.

Created attachment 278422 [details] [review]
Add Check-In capabilities - v4

A fix, ServiceBackend::isTokenInvalid must not be called with data == null.

Created attachment 278423 [details] [review]
Add Check-In dialog - v4

- Adds check-in advanced options to GSettings so Maps will remember the last 
used options.
 - Make the privacy comboboxes liststores IDs not-translatable in Glade.

Created attachment 278939 [details] [review]
Add Check-In capabilities - v5

Lot of code style fixes

Created attachment 278940 [details] [review]
Add Check-In dialog - v5

- Lot of code style fixes
 - Added files to POTFILES.in
 - Add an option to the dialog to tell that we are going to check-in into plain coordinates instead of POI 
(this will be used in the user location bubble)

Review of attachment 278939 [details] [review]:

Thx!

Just some small comments.

::: src/application.js
@@ +103,3 @@
         application = this;
         this._initServices();
+        settings = new Settings.Settings('org.gnome.maps');

Error in rebase? This is already done in initServices.

::: src/socialService/facebookBackend.js
@@ +86,3 @@
+                       callback,
+                       cancellable);
+    },

Could you spell out what the difference between a method starting with_ and a method starting with internal?

::: src/socialService/foursquareGoaAuthorizer.js
@@ +30,3 @@
+ * This code should be converted to a native library
+ * in the near future.
+ */

How near a future? Do you have this on your TODO list?

@@ +92,3 @@
+
+    return restCall;
+}

Even if you plan on converting this to a native library, right now it is in Maps and then should follow the coding style of Maps.
So convert the syntax to camelCase in this file.

::: src/socialService/serviceBackend.js
@@ +63,3 @@
+        restCall.set_function(func);
+
+        log('DEBUG: ' + this.getName() + ': ' + func);

If it's a DEBUG line that you think is useful going forward, convert to Utils.debug, otherwise remove this.

(In reply to comment #20)
> Created an attachment (id=278940) [details] [review]
> Add Check-In dialog - v5
> 
> - Lot of code style fixes
>  - Added files to POTFILES.in
>  - Add an option to the dialog to tell that we are going to check-in into plain
> coordinates instead of POI 
> (this will be used in the user location bubble)

Could I get a test patch to try it out or is that a hassle?

(In reply to comment #20)
> Created an attachment (id=278940) [details] [review]
> Add Check-In dialog - v5
> 
> - Lot of code style fixes
>  - Added files to POTFILES.in
>  - Add an option to the dialog to tell that we are going to check-in into plain
> coordinates instead of POI 
> (this will be used in the user location bubble)

Could you also add the bugs that this bug depends on in the Depends On field?

(In reply to comment #22)
> (In reply to comment #20)
> > Created an attachment (id=278940) [details] [review] [details] [review]
> > Add Check-In dialog - v5
> > 
> > - Lot of code style fixes
> >  - Added files to POTFILES.in
> >  - Add an option to the dialog to tell that we are going to check-in into plain
> > coordinates instead of POI 
> > (this will be used in the user location bubble)
> 
> Could I get a test patch to try it out or is that a hassle?

I usually put the code with hardcoded values mentioned in comment #3 in the about dialog method. To do it right without being a hassle, I need the markers/bubble code, I'll put the related bug as dependency and rebase/add patches to this one when markers/bubbles are ready.

(In reply to comment #21)
> Review of attachment 278939 [details] [review]:
> 
> ::: src/socialService/facebookBackend.js
> @@ +86,3 @@
> +                       callback,
> +                       cancellable);
> +    },
> 
> Could you spell out what the difference between a method starting with_ and a
> method starting with internal?
> 

Name the two methods with the same name, only _ as the difference is pretty odd, the "no-internal" method is just used as a wrapper of the "internal" method to perform some common parameters validation and result data manipulation.

Perhaps instead of prefixing with "internal" those methods should be prefixed with "_real", but using just "_" is a bit odd.

Created attachment 279432 [details] [review]
Add Check-In capabilities - v6

- Changed ServiceBackend::internal* methods for 
ServiceBackend::_real* methods.
- Now the authorizers are only used internally by 
CheckInManager.
- Simplified CheckInManager public interface.
- Changed FoursquareGoaAuthorizer code style.
- New ServiceBackend refreshAuthorization, createAuthorizer 
and getAuthorizerAccount methods

Created attachment 279433 [details] [review]
Add Check-In dialog - v6

- Changes according the previous patch.
- Use halign instead of xalign (sadly Glade is still writting 
the xalign property in the XML).

There is some TODO lines in this and previous versions of 
this patch, I couldn't save a SocialPlace object (native Gjs 
object) into the a GtkListStore, I don't know how to specify 
the type for that column, I'm saving the ID of the place 
instead of the object as a workaround. Anyone knows how to 
fix this?

(In reply to comment #27)
> Created an attachment (id=279433) [details] [review]
> Add Check-In dialog - v6
> 
> - Changes according the previous patch.
> - Use halign instead of xalign (sadly Glade is still writting 
> the xalign property in the XML).
> 
> There is some TODO lines in this and previous versions of 
> this patch, I couldn't save a SocialPlace object (native Gjs 
> object) into the a GtkListStore, I don't know how to specify 
> the type for that column, I'm saving the ID of the place 
> instead of the object as a workaround. Anyone knows how to 
> fix this?

You could have SocialPlace be a subclass of GObject.

Review of attachment 279432 [details] [review]:

Looks good otherwise.

Maybe SocialPlace should be a GObject.

::: src/checkIn.js
@@ +121,3 @@
+
+    broadcastTwitter: false
+});

No need for empty lines between these, I feel.

Created attachment 279799 [details] [review]
Add Check-In capabilities - v7

- Change CheckIn class code style.
- SocialPlace is now a GObject (to solve the next patch problem)

Created attachment 279800 [details] [review]
Add Check-In dialog - v7

- Changed calls to insert(-1) to append() in Gtk.ListStore's
- Removed CheckInDialog._socialPlaces

Review of attachment 279799 [details] [review]:

::: src/socialService/facebookBackend.js
@@ +35,3 @@
+    getName: function() {
+        return 'facebook';
+    },

get name() {

?

@@ +72,3 @@
+            restCall.get_status_message() :
+            (data.error ? data.error.message : null);
+    },

should these be getters with the getter syntax instead?

@@ +114,3 @@
+                                                      link: link,
+                                                      originalData: place }));
+        }

could this be a map? Something like:

return rawData.map(function(place) {
    new SocialPlace.SocialPlace({ ... });
});

::: src/socialService/foursquareBackend.js
@@ +67,3 @@
+    getCallResultMessage: function(restCall, data) {
+        return data === null ? restCall.get_status_message() : data.meta.errorDetail;
+    },

Same comment as other backend, could these getXYZ functions be getters with the get XYZ() format?

@@ +121,3 @@
+        }
+
+        return places;

same comment as other backend, could this be a map?

Review of attachment 279800 [details] [review]:

::: src/check-in-dialog.ui
@@ +3,3 @@
+<interface>
+  <requires lib="gtk+" version="3.12"/>
+  <object class="GtkBox" id="box-account">

Grid

@@ +41,3 @@
+    </child>
+  </object>
+  <object class="GtkBox" id="box-loading">

Grid and below as well.

::: src/checkInDialog.js
@@ +111,3 @@
+};
+
+const SocialPlaceListModel = new Lang.Class({

A file of its own I think. This module is largish and I think a split up might be in order.

@@ +129,3 @@
+                    //We need to remove the "Show more results" after
+                    //a time out to avoid trigger row-activated in a
+                    //new created row

Please use the /* [..] */ type of comments for multi line comments.

@@ +156,3 @@
+        }
+    },
+

set matches() ?

Zeeshan said he wanted to try this out, do these patches apply to master or do they need rebasing?

Yes, this patch applies on master (it won't apply over #722871 though)

Created attachment 284253 [details] [review]
application: Make mainWindow globally accesible

This allow to have main window dependant helpers, i.e. helpers for creation of
dialogs.

---

Needed for showCheckInDialog method.

Created attachment 284254 [details] [review]
Add Check-In capabilities

- ServiceBackend: getName -> get name
- FoursquareBackend and FacebooBackend: for -> Array.prototype.forEach
- FoursquareGoaAuthorizer: request access token to GOA when we are 
going to make a request instead of in startup (avoiding the need to 
unlock the keyring on startup)

Created attachment 284255 [details] [review]
Add Check-In dialog

- Box -> Grid.
- NO_PLACES dialog response code.
- Whitespaces damages fixes.
- Get rid of xalign, margin_left and margin_right usages.
- Place name and category escaping in SocialPlaceListModel
- Fix "(gnome-maps:31436): Gtk-WARNING **: gtkliststore.c:836: Unable to convert from gpointer 
to GObject"

Created attachment 284256 [details] [review]
UserLocationBubble: add Check-In button

This allows user to perform a check-in in a social network from his/her
current location.

Forgot to mention, I rebased the patch set on #722871 changes.

Created attachment 284350 [details] [review]
Add Check-In dialog

- Typo: Ocurred -> Occurred

(In reply to comment #39)
> Created an attachment (id=284256) [details] [review]
> UserLocationBubble: add Check-In button

+ <property name="label" translatable="yes">Check-In here</property>

Do you intentionally use a noun instead of a verb? 
Is there a reason why this button has no mnemonic?

(In reply to comment #42)
> (In reply to comment #39)
> > Created an attachment (id=284256) [details] [review] [details] [review]
> > UserLocationBubble: add Check-In button
> 
> + <property name="label" translatable="yes">Check-In here</property>
> 
> Do you intentionally use a noun instead of a verb? 

Check-In is a weird word, Foursquare (more precisely, Swarm) seems to use it as a verb, for example, to make a check-in in the mobile app there is a button with the label "Check-in". A valid phrase can be "Check-in in Foursquare", also there are phrases in Swarm like "You can still check-in where you are".

I don't know if you speak spanish, but a valid translation for "Check-In here" is "Hacer check-in aquí".

Now I see that I need to check the capitalization of the words "check" and "in" and be consistent, Foursquare/Swarm itself uses many variations: Check In, Check-In, Check-in. I think Check-in and check-in is the best alternative.

> Is there a reason why this button has no mnemonic?

No, there's not, I'll send a patch with the fix.

Review of attachment 284350 [details] [review]:

::: src/checkInDialog.js
@@ +111,3 @@
+};
+
+const SocialPlaceListModel = new Lang.Class({

I think this should go in socialPlaceListModel.js, if you disagree please motivate. A policy we have in maps is that non-trivial classes goes in its own file.

@@ +193,3 @@
+        let iter = this.append();
+
+        let markup = '<span foreground="#777777">' + _('Show more results') + '</span>';

_("...")

@@ +204,3 @@
+    min: function(values) {
+        if (values.length == 2)
+            return values[0] < values[1]? values[0]:values[1];

I would prefer this as an if statement.

But if you insist at least make it more readable:
return (a < b) ? a : b;

Or couldn't it be:

return Math.min(a, b)?

@@ +212,3 @@
+        let i;
+        let j;
+        let d = new Array();

let d = [] ?

@@ +215,3 @@
+
+        for (i = 0; i <= a.length; i++) {
+            d[i] = new Array();

= [] ?

@@ +382,3 @@
+        this.get_header_bar().show_close_button = false;
+
+        let cancelButton = new Gtk.Button({ label: _('Cancel') });

_("...")

@@ +388,3 @@
+        }).bind(this));
+
+        this.ui.buttonDone = new Gtk.Button({ label: _('Done') });

_("...")

@@ +462,3 @@
+            }
+        }).bind(this));
+    },

Could some or all of this be moved to ui file?

@@ +486,3 @@
+            }
+        }).bind(this));
+    },

Same here.

@@ +569,3 @@
+                else
+                    this.ui.labelPlaceNotFound.label = _('Maps could not find the place to check-in in %s, please select one from this list')
+                                                         .format(this._account.get_account().provider_name);

_("...")

@@ +584,3 @@
+
+        this.ui.labelMessageInfo.label =
+            _('You are going to check-in in %s with your %s account. Put a message for the check-in below')

The style guide tells us to use "-type quotes in the gettext function.

@@ +641,3 @@
+                                                        buttons: Gtk.ButtonsType.OK,
+                                                        modal: true,
+                                                        text: _('An error has occurred'),

Change from ' to "

Review of attachment 284253 [details] [review]:

I do not like this. What is preventing the mapView from showing the dialog?

Review of attachment 284256 [details] [review]:

::: src/mainWindow.js
@@ +327,3 @@
+        }
+    },
+

Same question as in previous patch, does this have to live on mainWindow?

::: src/userLocationBubble.js
@@ +48,3 @@
+        this._buttonCheckIn = ui.buttonCheckIn;
+        this._buttonCheckIn.connect('clicked', this._onCheckInButtonClicked.bind(this));
+        Application.checkInManager.connect('accounts-refreshed', this._updateCheckInButtonVisibility.bind(this));

Maybe shorten lines with second argument on next line?

@@ +49,3 @@
+        this._buttonCheckIn.connect('clicked', this._onCheckInButtonClicked.bind(this));
+        Application.checkInManager.connect('accounts-refreshed', this._updateCheckInButtonVisibility.bind(this));
+        this._updateCheckInButtonVisibility();

Maybe a function is overkill for this?

Review of attachment 284254 [details] [review]:

::: src/socialService/facebookBackend.js
@@ +113,3 @@
+                                                      link: link,
+                                                      originalData: place }));
+        });

Why not a map instead of forEach?

::: src/socialService/foursquareBackend.js
@@ +121,3 @@
+
+        return places;
+    }

Why not a map?

Review of attachment 284256 [details] [review]:

::: src/mainWindow.js
@@ +322,3 @@
+                                                        modal: true,
+                                                        text: _('An error has occurred'),
+                                                        secondary_text: message });

All these gettext calls should be _("...) I think:

"Use 'single quotes' for programming strings that should not be translated and "double quotes" for strings that the user may see. This allows us to quickly find untranslated or mistranslated strings by grepping through the sources for double quotes without a gettext call around them."

https://wiki.gnome.org/Projects/Gjs/StyleGuide

Perhaps a grep for _(' is in order to make sure we get them all and convert to => _("

Review of attachment 284350 [details] [review]:

::: src/checkInDialog.js
@@ +123,3 @@
+    initView: function(treeview) {
+        treeview.connect('row-activated', (function(view, path, column, userData) {
+            if (path != null) {

!==

@@ +149,3 @@
+
+        if (this._matches.exactMatches.length +
+            this._matches.goodMatches.length == 0) {

===

@@ +203,3 @@
+
+    min: function(values) {
+        if (values.length == 2)

===

@@ +500,3 @@
+        if (accounts.length > 1)
+            this.startAccountStep();
+        else if (accounts.length == 1) {

===

@@ +551,3 @@
+    _onFindPlacesFinished: function(account, places, error) {
+        if (!error) {
+            if (places.length == 0) {

===

@@ +558,3 @@
+            let matches = SocialPlaceMatcher.match(this._place, places);
+
+            if (matches.exactMatches.length == 1 && !this._coordinatesBased) {

===

Review of attachment 284350 [details] [review]:

::: src/checkInDialog.js
@@ +111,3 @@
+};
+
+const SocialPlaceListModel = new Lang.Class({

I absolutely agree, but I'll feel a lot more comfortable if we move every class on this file to a separate file: accountListModel.js, socialPlaceListModel.js, socialPlaceMatcher.js; are you ok with that?

@@ +123,3 @@
+    initView: function(treeview) {
+        treeview.connect('row-activated', (function(view, path, column, userData) {
+            if (path != null) {

What about "if (path)"?
What is our style for this anyways? I got very confused with some reviews in #722871 about what is our style for null checking.

Review of attachment 284254 [details] [review]:

::: src/socialService/facebookBackend.js
@@ +113,3 @@
+                                                      link: link,
+                                                      originalData: place }));
+        });

rawData.data is an array, not an map (JS object).

::: src/socialService/foursquareBackend.js
@@ +121,3 @@
+
+        return places;
+    }

rawData.response.venues is an array, not an map (JS object).

Review of attachment 284256 [details] [review]:

::: src/mainWindow.js
@@ +327,3 @@
+        }
+    },
+

I'll respond you in the review of the patch that converts mainWindow to global.

::: src/userLocationBubble.js
@@ +49,3 @@
+        this._buttonCheckIn.connect('clicked', this._onCheckInButtonClicked.bind(this));
+        Application.checkInManager.connect('accounts-refreshed', this._updateCheckInButtonVisibility.bind(this));
+        this._updateCheckInButtonVisibility();

Do you prefer a closure as 'accounts-refreshed''s callback and duplicate the code for that callback as a constructor sentence?

I prefer the function instead :)

Review of attachment 284253 [details] [review]:

Yes, it doesn't look good, but the creation of dialogs conceptually depends on a window (the transient_for property, technically speaking) and not on a window widget like mapView, I mean, is a window system related stuff and not map view related stuff (like zooming, markers management, etc). mapView should only contains code related to the map view (that's because move the code from mapWalker to mapView is more feasible that move showCheckInDialog method to mapView).
 
Maybe you'd feel better if we move the code of showCheckInDialog to UserLocationBubble class and we only use the mainWindow global to set the transient_for property, but we also will need to create check-ins dialogs from POIs bubbles and the code to show that dialog will be the same except for the coordinatesBased dialog option.

For any better idea, I'm all ears :)

Review of attachment 284253 [details] [review]:

Maybe adding a reference to MainWindow instance to MapView and then, in UserLocationBubble, do "this.mapView.mainWindow.showCheckInDialog(...)", that avoid us to make mainWindow global.

Review of attachment 284350 [details] [review]:

::: src/checkInDialog.js
@@ +111,3 @@
+};
+
+const SocialPlaceListModel = new Lang.Class({

I am ok with that.

@@ +123,3 @@
+    initView: function(treeview) {
+        treeview.connect('row-activated', (function(view, path, column, userData) {
+            if (path != null) {

Well, it's about the truthy and falsy aspect in JS (http://www.sitepoint.com/javascript-truthy-falsy/).

If you really want to check that something is or is not null, then you use the '!==' or '===' construct, otherwise...

var l = (false == null); // true
var m = (false === null); // false

If you want to check is something is "falsy" or truthy".

Like from the link, the falsy values are:
    false
    0 (zero)
    "" (empty string)
    null
    undefined
    NaN (a special Number value meaning Not-a-Number!)

If you just want to check for falsy, you can use '==':

var c = (false == 0); // true
var d = (false == ""); // true
var e = (0 == ""); // true

Review of attachment 284253 [details] [review]:

Yeah, I agree with you. And maybe it's the right thing todo. I just feel we are moving more and more stuff to being global :)
I would love to here Mattias opinion on this. Dunno if we can manage him to take a look.

But barring his objections I am fine with this code as is.

Created attachment 284517 [details] [review]
Make mainWindow accesible from mapView

This allow to have main window dependant helpers and be called from
markers and bubbles code, i.e. helpers for creation of dialogs.

Created attachment 284518 [details] [review]
Add Check-In capabilities

- forEach -> map.
- Conditional clean.

Created attachment 284519 [details] [review]
Add Check-In dialog

- Conditionals clean.
- _('...') -> _("...")
- Don't show place category in the list if category doesn't exists.
- Fix in AccountListModel construction (it doesn't accept params).
- Add some translation comments and fixes some translatable 
strings.
- Add mnemonics to dialog buttons
- Replace SocialPlaceMatcher.min with Math.min.
- new Array -> []
- Initialize GtkTreeViewColumn and GtkCellRenderer with GtkBuilder.
- Separate all checkInDialog.js classes into files.
- SocialPlaceMatcher moved to socialPlaceMatcher.js as a helper 
function file (like utils.js).
- Rename AccountListModelColumn and SocialPlaceListModelColumn to 
Columns to follow the style in SearchPopup.
- Rename CheckInDialogResponse to Response.
- Removed debug code from socialPlaceMatcher.

Created attachment 284520 [details] [review]
UserLocationBubble: add Check-In button

- _('...') -> _("...")
- Add translation comments.
- Add mnemonic to check-in button.
- Code style fixes.
- mainWindow non-global refactorization.
- Rename CheckInDialogResponse to Response.

Review of attachment 284518 [details] [review]:

Ack.

Review of attachment 284519 [details] [review]:

Ack.

Review of attachment 284520 [details] [review]:

Looks fine.

Review of attachment 284517 [details] [review]:

Ok.

Awww yeah!

Created attachment 284621 [details] [review]
Add Check-In dialog

- Found some untranslated strings.

Comment on attachment 284621 [details] [review]
Add Check-In dialog

>+                    /* Translators: %s is the social service name (Facebook, Foursquare) */
>+                    this.ui.labelPlaceNotFound.label = _("Maps could not find the place to check-in in %s, please select one from this list")
>+                                                         .format(this._account.get_account().provider_name);

From an I18N point that string is pretty much broken. 
See https://en.wikipedia.org/wiki/Declension for the linguistic concept.

Also, why is there a comma between those two separate sentences? 
Why is there no punctuation at the end of that string? 
Shouldn't the "in" rather be "on", assuming you refer to random websites?
Where are styleguides when you need them, GNOME project?

(In reply to comment #68)
> (From update of attachment 284621 [details] [review])
> >+                    /* Translators: %s is the social service name (Facebook, Foursquare) */
> >+                    this.ui.labelPlaceNotFound.label = _("Maps could not find the place to check-in in %s, please select one from this list")
> >+                                                         .format(this._account.get_account().provider_name);
> 
> From an I18N point that string is pretty much broken. 
> See https://en.wikipedia.org/wiki/Declension for the linguistic concept.

Are you talking about the -in suffix?
Check-in is the word used by Swarm/Foursquare and maybe has some marketing meaning, according to Foursquare translations, in Spanish is "check-in" or "check in", in Russian is "Чекин", which seems to be a phonetic equivalent to "check-in".

(In reply to comment #69)
> Also, why is there a comma between those two separate sentences? 
> Why is there no punctuation at the end of that string? 
> Shouldn't the "in" rather be "on", assuming you refer to random websites?
> Where are styleguides when you need them, GNOME project?

Sorry, take into account that my English skills are horrible.

Maybe this is more acceptable?

"Maps cannot find the place to check-in on %s. Please select one from this list."

(In reply to comment #70)
> > >+ /* Translators: %s is the social service name (Facebook, Foursquare) */
> > >+ this.ui.labelPlaceNotFound.label = _("Maps could not find the place 
> > >+ to check-in in %s, please select one from this list")
>
> Are you talking about the -in suffix?

No, I was talking about nouns. See https://wiki.gnome.org/TranslationProject/DevGuidelines/Never%20split%20sentences

And if I remember Russian correctly (it's been a while) it's the same problem:
+ /* "Translators: %s is the place (e.g. oтель) */
+ this.ui.labelPlaceNotFound.label = _("Я в %s")

Now read aloud the constructed sentence and either find the mistake, or tell me that my Russian sucks. :)

Review of attachment 284517 [details] [review]:

I don't think this is needed at all. For creation of dialogs we can use this.get_toplevel() to set the transient_for property.

Review of attachment 284520 [details] [review]:

Should be done using new button-template

Review of attachment 284621 [details] [review]:

Thanks!

Is it not possible to construct the headerbar/buttons/stack in ui file so we could avoid all that initing in js code?

::: src/checkInDialog.js
@@ +53,3 @@
+
+        this._coordinatesBased = params.coordinatesBased;
+        delete params.coordinatesBased;

What exactly is this 'coordinatesBased', it is never set to anything other than true what I could see? Is it needed?
Could we at least omit it until something needs it?

@@ +113,3 @@
+        }).bind(this));
+
+        this.ui.buttonDone = new Gtk.Button({ label: _("_Done"),

Could "Done" be "Check in" ?

@@ +138,3 @@
+        this._initPlacesTreeView();
+
+        this.ui.textviewMessage.buffer.connect('changed', this._refreshDoneButtonSensitivity.bind(this));

Is it not allowed to have an empty message?

@@ +168,3 @@
+
+        this.ui.treeviewAccountsColumn.add_attribute(this.ui.treeviewAccountsRendererAttentionNeeded,
+                                                     'visible', AccountListModel.Columns.ATTENTION_NEEDED);

These add_attributes can be set in ui-file.

@@ +184,3 @@
+
+        this.ui.treeviewPlacesColumn.add_attribute(this.ui.treeviewPlacesRendererText,
+                                                   'markup', SocialPlaceListModel.Columns.MARKUP);

ui file

@@ +204,3 @@
+    },
+
+    startup: function() {

_startup?

@@ +241,3 @@
+    },
+
+    startAccountStep: function() {

_startAccountStep?

@@ +246,3 @@
+    },
+
+    startPlaceStep: function() {

_startPlaceStep?

@@ +289,3 @@
+    },
+
+    startMessageStep: function() {

_startMessageStep?

@@ +317,3 @@
+    },
+
+    startCheckInStep: function() {

_startCheckInStep?

@@ +359,3 @@
+            messageDialog.destroy();
+
+            this.startMessageStep();

I would prefer using notificationManager for errors instead of using a dialog.

::: src/socialPlaceListModel.js
@@ +63,3 @@
+            }
+        }).bind(this));
+    },

Does this really belong as a method on the model?

(In reply to comment #72)
> Review of attachment 284517 [details] [review]:
> 
> I don't think this is needed at all. For creation of dialogs we can use
> this.get_toplevel() to set the transient_for property.

So _showDialog could be in mapBubble.js or maybe even in the checkinManager and take a toplevel as an argument. And mapBubble can pass this.get_toplevel()

Review of attachment 284517 [details] [review]:

Ah, yeah, that'd be far better.

Review of attachment 284621 [details] [review]:

::: src/checkInDialog.js
@@ +53,3 @@
+
+        this._coordinatesBased = params.coordinatesBased;
+        delete params.coordinatesBased;

There are two kinds of check-in:

 - When you try to check-in to a specific coordinate, in that case Maps will search for nearby places for that coordinate (coordinatesBased == true)
 - When you try to check-in to a Point of Interest, in that case Maps will search for the best match in the social service for a given POI (coordinatesBased == false)

I changed it to usePlaceMatcher instead since it feels more clear.

@@ +113,3 @@
+        }).bind(this));
+
+        this.ui.buttonDone = new Gtk.Button({ label: _("_Done"),

Yup, according to HIG.

@@ +138,3 @@
+        this._initPlacesTreeView();
+
+        this.ui.textviewMessage.buffer.connect('changed', this._refreshDoneButtonSensitivity.bind(this));

You are right, Foursquare and Facebook accepts empty messages for check-in and they are actually very common

@@ +168,3 @@
+
+        this.ui.treeviewAccountsColumn.add_attribute(this.ui.treeviewAccountsRendererAttentionNeeded,
+                                                     'visible', AccountListModel.Columns.ATTENTION_NEEDED);

Hmmm... but I cannot reuse the Column constants collection.

@@ +359,3 @@
+            messageDialog.destroy();
+
+            this.startMessageStep();

The check-in dialog is not closed in this case, I think it'd be nice to allow user to try again without losing the check-in message.

Created attachment 289912 [details] [review]
Add Check-in capabilities

This only adds check-in capabilities necessary
to implement a check-in UI.

Created attachment 289913 [details] [review]
Add Check-in dialog

Created attachment 289914 [details] [review]
mapBubble: Add Check-in button

Created attachment 289915 [details] [review]
userLocationBubble: Enable Check-in button

Comment on attachment 289913 [details] [review]
Add Check-in dialog

>+                    this.ui.labelPlaceNotFound.label = _("Maps could not find the place to check-in in %s, please select one from this list")

If there are reasons to ignore comments 68-71 you might want to elaborate? :)

Created attachment 289918 [details] [review]
Add Check-in dialog

(In reply to comment #82)
> (From update of attachment 289913 [details] [review])
> >+                    this.ui.labelPlaceNotFound.label = _("Maps could not find the place to check-in in %s, please select one from this list")
> 
> If there are reasons to ignore comments 68-71 you might want to elaborate? :)

Yes, I can elaborate: I completely forgot it :P

+  this.ui.labelPlaceNotFound.label = _("Maps cannot find the place to check-in on %s. Please select one from this")
+  _("You are going to check-in in %1$s with your %2$s account. Put a message for the check-in below")

Are there reasons that the last sentences have no full stop? Plus I still think that the first sentence does not work in many languages. See comment 71 and https://wiki.gnome.org/TranslationProject/DevGuidelines/Never%20split%20sentences

From your friendly native speakers on #docs:

<shaunm> andre_: as a verb, it's two words: "Check in"
<mdhill> this is the same as aday's question about set-up or setup vs set up
 "set up in" works better than "check in in" though
<amigadave> wouldn't you "check in at" anyway?
<amigadave> i suppose that "to" feels more natural there

<shaunm> andre_: "check in using..." ?
<shaunm> or "with"

Created attachment 289936 [details] [review]
Add Check-in dialog

Created attachment 289939 [details] [review]
Add Check-in dialog

----

Hope this works for the languages spoken in Mars and Jupiter.

Probably, though "check in to with Facebook" still sounds weird to me, but I'm not a native English speaker. :)

Review of attachment 289914 [details] [review]:

Thanks!

::: src/mapBubble.js
@@ +56,3 @@
         delete params.routeFrom;
 
+        let checkInUsePlaceMatcher = params.checkInUsePlaceMatcher;

I think we can do better with the name. The bubble users should not have to know about some "PlaceMatcher". How about checkInUseName?

@@ +57,3 @@
 
+        let checkInUsePlaceMatcher = params.checkInUsePlaceMatcher;
+        if (checkInUsePlaceMatcher === undefined)

if (!checkInUse...) so we catch setting it to null as well.

@@ +69,3 @@
                                                    'bubble-button-area',
+                                                   'bubble-route-button',
+                                                   'bubble-check-in-button' ]);

extra space here.

@@ +78,3 @@
             if (buttonFlags & Button.ROUTE)
                 this._initRouteButton(ui.bubbleRouteButton, routeFrom);
+

I would remove this empty line. Up to you tho.

@@ +133,3 @@
+    },
+
+    _showCheckInDialog: function(usePlaceMatcher) {

Could this move to checkinManager? And take a place and a toplevel?

@@ +168,3 @@
+            messageDialog.run();
+            messageDialog.destroy();
+        }

Still dialog for error?

Review of attachment 289912 [details] [review]:

::: src/checkIn.js
@@ +87,3 @@
+    isCheckInAvailable: function() {
+        return this._accounts.length > 0;
+    },

Maybe a property?

get hasCheckIn() {

}

?

Review of attachment 289939 [details] [review]:

::: src/check-in-dialog.ui
@@ +19,3 @@
+        <property name="shadow_type">in</property>
+        <child>
+          <object class="GtkTreeView" id="treeview-accounts">

Would we benefit from using a GtkListBox instead? Will the logic improve by avoiding models?

@@ +165,3 @@
+        <property name="min_content_height">300</property>
+        <child>
+          <object class="GtkTreeView" id="treeview-places">

Same question here regarding GtkListBox. Would the UI be nicer?

We don't want this for searchResults? All the POIs we will be able to get through Rishis patches we will be able to get by searching for them as well.

Review of attachment 289914 [details] [review]:

::: src/mapBubble.js
@@ +56,3 @@
         delete params.routeFrom;
 
+        let checkInUsePlaceMatcher = params.checkInUsePlaceMatcher;

But the bubble users should not have to know that SocialPlaceMatcher uses the name to match a place, that's an internal behaviour.

What about checkInMatchPlace or checkInEnableMatchPlace?

@@ +57,3 @@
 
+        let checkInUsePlaceMatcher = params.checkInUsePlaceMatcher;
+        if (checkInUsePlaceMatcher === undefined)

What about if bubble pass { checkInUserPlaceMatcher: false }?

Maybe "if (checkInUse... === undefined || checkInUse... === null)" is more appropiated.

@@ +69,3 @@
                                                    'bubble-button-area',
+                                                   'bubble-route-button',
+                                                   'bubble-check-in-button' ]);

Really? I was using the same style for every class I created :(
Also, I checked some gnome-shell files that also adds a space before ]

@@ +78,3 @@
             if (buttonFlags & Button.ROUTE)
                 this._initRouteButton(ui.bubbleRouteButton, routeFrom);
+

Ok.

@@ +133,3 @@
+    },
+
+    _showCheckInDialog: function(usePlaceMatcher) {

Ok

@@ +168,3 @@
+            messageDialog.run();
+            messageDialog.destroy();
+        }

Yes... not convinced with the NotificationManager approach (tried it).

Review of attachment 289939 [details] [review]:

::: src/check-in-dialog.ui
@@ +19,3 @@
+        <property name="shadow_type">in</property>
+        <child>
+          <object class="GtkTreeView" id="treeview-accounts">

I don't know, I used GtkTreeView since I got inspired by G-C-C's online accounts panel. I like how we separate some list code into models and I like how it looks.

Created attachment 290227 [details] [review]
Add Check-in capabilities

This only adds check-in capabilities necessary
to implement a check-in UI.

Created attachment 290228 [details] [review]
Add Check-in dialog

Created attachment 290229 [details] [review]
mapBubble: Add Check-in button

Created attachment 290230 [details] [review]
userLocationBubble: Enable Check-in button

Review of attachment 290228 [details] [review]:

I think maybe we should hold off this until templates for gjs works? (https://bugzilla.gnome.org/show_bug.cgi?id=739739)

Since then the dialog ui file can actually be a dialog and we can do the packing to content-area in the ui file and will not have to construct them in code.
Maybe you could rebase as if that bug was applied to gjs and use template syntax?

Regarding treeview/listbox, is the UI look of treeView that different from listBox? My impression is that GNOME is moving towards using ListBox instead of TreeView.
You get some seperation since a row will be a class / ui file of its own. And there is more freedom since a row is widgets and easier to style.

::: src/checkIn.js
@@ +143,3 @@
+                                                        secondary_text: message });
+            messageDialog.run();
+            messageDialog.destroy();

Could you expand what you didn't like about using in-app notifications? Having different types of error reporting in different features feels icky.

Review of attachment 290229 [details] [review]:

Nice!

::: src/mapBubble.js
@@ +56,3 @@
 
+        let checkInMatchPlace = params.checkInMatchPlace;
+        if (checkInMatchPlace === undefined || checkInMatchPlace === null)

Yeah, or maybe if (checkInMatchPlace !== false) ?

@@ +128,3 @@
+        }).bind(this));
+
+        button.visible = Application.checkInManager.hasCheckIn;

if we made hasCheckIn a proper GObject property we could use bind_property for this?
That would avoid the need for the accounts-refreshed listener?

Review of attachment 290230 [details] [review]:

Looks fine.

(In reply to comment #94)
> We don't want this for searchResults? All the POIs we will be able to get
> through Rishis patches we will be able to get by searching for them as well.

Ping

Review of attachment 290228 [details] [review]:

So let's change it to Template and ListBox (I hope we can merge this in 3.16 xD).

::: src/checkIn.js
@@ +143,3 @@
+                                                        secondary_text: message });
+            messageDialog.run();
+            messageDialog.destroy();

Some reasons are:

 - I feel the error widget in the map view to be unrelated (or unattached) to the check-in dialog, that can conclude on users missing the error message and thinking that the check-in was committed successfully, I don't want to take that risk.
 - I need to show a details of the reason of a check-in failure, if I show "An error has occurred during check-in", as a user, I would feel destitute and confused. The user needs to know if they need to sign-in in GNOME Online Accounts, if it was an error on the Internet connection or whatever error the social service can throw. Maybe showing all this stuff in the map view error widget is too much.
 - If the user reach the check-in message step and something failed during the check-in, I want to give her another try without discarding the check-in message. Take for example the case of a user in a bar with a really unstable Internet connection, a timeout error is possible in the final step, if I force the user to do all the steps again, I would feel somewhat angry.

Anyway, from my PoV, the first of the three reasons is the main one.

Do you feel that is icky from the UX or technical (we are not reusing the NotificationManager to show this error) point of view?

Created attachment 290529 [details] [review]
Add Check-in capabilities

This only adds check-in capabilities necessary
to implement a check-in UI.

Created attachment 290530 [details] [review]
Add Check-in dialog

Created attachment 290531 [details] [review]
mapBubble: Add Check-in button

Created attachment 290533 [details] [review]
userLocationBubble: Enable Check-in button

Review of attachment 290228 [details] [review]:

We will merge this in 3.16, it will be done. So say we all. (so say we all).

So I shouldn't review this more now? You are changing to ListBox and templates?

::: src/checkIn.js
@@ +143,3 @@
+                                                        secondary_text: message });
+            messageDialog.run();
+            messageDialog.destroy();

Ok.

Well I dislike the dialog in dialog a bit. Would there be a way to display the error in the Checkindialog? Like a error view in the stack that you could switch to?

I also do not want to show needlessly complicated or technical error messages that the users can't do anything about. But I also trust you. So do it up the way you feel best and we will let those pesky designers have the last word :)

Review of attachment 290530 [details] [review]:

So this will change to ListBox/templateS?

Review of attachment 290531 [details] [review]:

Looks good!

::: src/mapBubble.js
@@ +124,3 @@
+                                                 button, 'visible',
+                                                 GObject.BindingFlags.DEFAULT |
+                                                 GObject.BindingFlags.SYNC_CREATE);

nice!

Review of attachment 290533 [details] [review]:

Looks nice!

::: src/userLocationBubble.js
@@ +37,2 @@
         params.routeFrom = true;
+        params.checkInMatchPlace = false;

Only one nit: Should this be default false?

Review of attachment 290529 [details] [review]:

::: src/checkIn.js
@@ +23,3 @@
+const GObject = imports.gi.GObject;
+const Goa = imports.gi.Goa;
+

Remove empty line.

::: src/socialService/foursquareGoaAuthorizer.js
@@ +24,3 @@
+const Goa = imports.gi.Goa;
+const Rest = imports.gi.Rest;
+

Remove empty line

::: src/socialService/serviceBackend.js
@@ +31,3 @@
+    _init: function(params) {
+
+    },

No need for the _init?

Review of attachment 290530 [details] [review]:

Yes, let's commit something that we know we don't want to change it the next week :P, I'll work on that in the following days.

Review of attachment 290530 [details] [review]:

Awesome! I can do conversion of the searchPopup to ListBox as part of the renovate UX bug perhaps.

Created attachment 290670 [details] [review]
Add Check-in capabilities

This only adds check-in capabilities necessary
to implement a check-in UI.

---

- Minor code style fixes
- Imports reorganization

Created attachment 290671 [details] [review]
Add Check-in dialog

---

- Port to Gtk+'s Template (it requires to apply #739739 to gjs
to get this working.)
- Convert accounts and place treeviews to GtkListBox.

(FTW!!)

Created attachment 290672 [details] [review]
mapBubble: Add Check-in button

Created attachment 290673 [details] [review]
userLocationBubble: Enable Check-in button

Review of attachment 290670 [details] [review]:

Looks fine

Review of attachment 290671 [details] [review]:

This looks great! I have only done a quick review atm, will do more indepth when I have more time.

Could you maybe do a screencast? (ctrl-shift-alt-r) And post the result in the bug or youtube if it is to large? That way designers can have a look as well.
And include some error path so that we can see how that dialog from dialog looks.

Thanks!

::: data/gnome-maps.css
@@ +87,3 @@
+    -GtkTreeView-horizontal-separator: 20;
+}
+

Should this be removed?

::: src/checkInDialog.js
@@ +104,3 @@
+
+    _initWidgets: function() {
+        // Because Gjs + GtkBuilder = crap

Maybe instead: // Limitations in Gjs means we can't do this in UI files yet.

:)

::: src/socialPlaceListBox.js
@@ +69,3 @@
+                    this.remove(row);
+                    this._showBadMatches();
+                }).bind(this));

This seems magical, is there other signal this could connect to? What _exactly_ is the issue? Could this be done another way? Just waiting 10 ms seems weird. Would an idle achieve the same?

::: src/socialPlaceMatcher.js
@@ +21,3 @@
+ */
+
+function _getLevenshteinDistance(a, b) {

A small comment about what this is would be nice for the less educated of us.

Review of attachment 290672 [details] [review]:

Looks fine.

Review of attachment 290673 [details] [review]:

Ack.

Review of attachment 290671 [details] [review]:

::: data/gnome-maps.css
@@ +87,3 @@
+    -GtkTreeView-horizontal-separator: 20;
+}
+

Yes

::: src/checkInDialog.js
@@ +104,3 @@
+
+    _initWidgets: function() {
+        // Because Gjs + GtkBuilder = crap

We need to promote self-criticism ;)

::: src/socialPlaceListBox.js
@@ +69,3 @@
+                    this.remove(row);
+                    this._showBadMatches();
+                }).bind(this));

I need to try it again since I'm using ListBox, maybe it behaves differently. When I was using treeview and pressed the "Show more results" row, a second 'row-activated' event was triggered for the row that took the "Show more results" position and an idle was not enough to prevent this.

::: src/socialPlaceMatcher.js
@@ +21,3 @@
+ */
+
+function _getLevenshteinDistance(a, b) {

NP

Created attachment 290714 [details] [review]
Add Check-in dialog

----

- Add some comments
- Add the signals place-selected and account-selected to the 
SocialPlaceListBox and AccountListBox so we can get more
abstraction and simplicity in the CheckInDialog code
- Remove the magical "Show more results" timeout, not needed
with the ListBox approach
- CSS Fixes

Created attachment 290718 [details] [review]
Add Check-in dialog

---

- Hide check-in button during submitting

Screencast:

http://youtu.be/c38zAeOhq-4

Review of attachment 290718 [details] [review]:

Cool! Great work.

::: data/gnome-maps.css
@@ +85,3 @@
+.maps-check-in GtkTextView {
+    font-size: 1.2em;
+}

Would it be possible to use a less magical value? Like x-small/small/large/normal or something named like that?

::: src/checkInDialog.js
@@ +141,3 @@
+        this.parent();
+        this._startup();
+        this.show();

What is this show command needed? Isn't the call to this.parent() enough?

::: src/social-place-more-results-list-box-row.ui
@@ +20,3 @@
+    </child>
+  </template>
+</interface>

The name for this file is not pretty :) Could it be nicer? Or is this small enough to construct from code instead of template?
Or general enough to be "more-results-row.ui".

And while on the subject maybe we could do this some other way? We do not have "more result" item anywhere else. Could it be done by just having a scrollbar? And loading more when you scroll down? Or will that be to complicated?

::: src/socialPlaceMatcher.js
@@ +26,3 @@
+ * delete to transform string a into string b.
+ * We use it to compare the similarities of two place names.
+ *

Thanks! Maybe we should use this when we filter against recent and favourites in the search-popup as well?
Sort by levenshtein, or is it to slow?

Review of attachment 290718 [details] [review]:

::: data/gnome-maps.css
@@ +85,3 @@
+.maps-check-in GtkTextView {
+    font-size: 1.2em;
+}

Yup, large is fine (actually, I think large == 1.2em :D )

::: src/checkInDialog.js
@@ +141,3 @@
+        this.parent();
+        this._startup();
+        this.show();

Oh, what a shame!

::: src/social-place-more-results-list-box-row.ui
@@ +20,3 @@
+    </child>
+  </template>
+</interface>

Hmm, if I rename it to a more general name, I will be forced to move the more result class to a separated file since I'll feel that it doesn't belongs to socialPlaceListBox.js anymore. Also, I prefer to be consistent in this case and use the Template approach (believe me, you won't like to have styling related code like foreground color and margins in JS).

If we find in the future another UC where the Show more results is required, then we could move it to a more general ui file name and a more accessible class.

I think the scrolling approach is not applicable here, I want to hide the bad matches for the list in the first look and there will be high probabilities to have only 1 or 2 rows before the "Show more results", so you cannot scroll.

Let's make everyone happier, I will strip the list-box part of the names:

social-place-list-box-row -> social-place-row
social-place-more-results-list-box-row -> social-place-more-results-row
account-list-box-row -> account-row

Created attachment 291371 [details] [review]
Add Check-in capabilities

This only adds check-in capabilities necessary
to implement a check-in UI.

Created attachment 291372 [details] [review]
Add Check-in dialog

- Use "add" instead of "insert" in ListBoxes.
- Explicitly destroy rows instead of removing from container.
- Remove GtkExpander for privacy and broadcasting options.
- Show account icon in the last step
- Use large instead of 1.2em in GtkTextView style
- Don't call CheckInDialog::show explicitly
- Strip list-box and ListBox for the ui and classes names
of ListBoxRow widgets.

Created attachment 291373 [details] [review]
mapBubble: Add Check-in button

Created attachment 291374 [details] [review]
userLocationBubble: Enable Check-in button

Cast v2, this doesn't show error check and that stuff, it only show the recent changes.

https://youtu.be/2sUHeDyAnYU

Looks nice in general! Good work!

For "Check in" vs. "Check-in", Facebook [1] seems to favor the first one in their documentation.
Foursquare [2] seems to differ between the Verb "check in", and the Noun "A check-in". I trust whatever the documentation team suggested though.

"Put a message" doesn't sound like correct English, and it also doesn't capture the whole action of the dialog, as the dialog also carries out the actual Check in action. I would either name it "Check In" or "Check In to Name-of-place". This could then be taken out of the sentence below.

For the actual message box, it is way to big for a short check in message. I expect these to be in the short in nature, such as "having a blast with my friends" or similar. I would size it to fit about 140 characters.

Visibility sounds like a good term. Nothing to change.

The sentences "Broadcast to my Facebook wall" and "Broadcast to my Twitter account" can be changed to be shorter and reflect the vocabulary used on those sites. Facebook generally uses "Post" and Twitter "Tweet". Standardizing on one of them, I think the better is "Post". This would then be "Post on Facebook", "Post on Twitter" 

1. https://www.facebook.com/help/461075590584469/
2. https://support.foursquare.com/hc/en-us/articles/201065340-Check-ins

Apart from those grammar fixes and layout fixes, I think it looks good.

(In reply to comment #136)

> For the actual message box, it is way to big for a short check in message. I
> expect these to be in the short in nature, such as "having a blast with my
> friends" or similar. I would size it to fit about 140 characters.

I fully understand that you want to adapt to the size of the dialog in the other steps though.

Review of attachment 291371 [details] [review]:

A part from the question this looks great!

::: src/socialService/socialPlace.js
@@ +27,3 @@
+const SocialPlace = new Lang.Class({
+    Name: 'SocialServiceSocialPlace',
+    Extends: GObject.Object,

Tell me again why this needs to be a GObject?

Review of attachment 291372 [details] [review]:

Well, If you fix Andreas comments I think this is ok now.
However Andreas should also investigate if this could be under the share-menu. I think that would be nice. We would avoid an extra button and an extra step in the check-in dialog.

The concern would be how we would integrate it. Would popping up a new dialog when you press Facebook or FourSquare in the share dialog be weird?

So the design team has spoken. We will not move this to share, and in a twist, share will disappear! And be replaced with some kind of 'open with' mechanism.

So fix these up and keep them under your own button and we should be able to merge this really soon!

Review of attachment 291373 [details] [review]:

Make sure we are using the verb proper then it is fine!

::: src/map-bubble.ui
@@ +98,3 @@
+            <child>
+              <object class="GtkButton" id="bubble-check-in-button">
+                <property name="label" translatable="yes" comments="Translators: Check-in is used as a verb">C_heck-in</property>

Should be 'Check In'?

Review of attachment 291374 [details] [review]:

Great!

If you feel you have fixed up Andreas grammar and layout fixes fine, and maybe even get an ok from him. Then feel free to push this.

Thanks for your great work!

The release team says that they will take tar-balls all of today, so if you want to get this in 3.15.2 then fix this up and then release a Maps 3.15.2.1 and push the tar-balls.

You can ask in #release-team if there is time still.

Guide for making releases here:
https://wiki.gnome.org/MaintainersCorner

I can do the signed tag later.

Review of attachment 291371 [details] [review]:

::: src/socialService/socialPlace.js
@@ +27,3 @@
+const SocialPlace = new Lang.Class({
+    Name: 'SocialServiceSocialPlace',
+    Extends: GObject.Object,

Originally it was because I was saving SocialPlace instances in a GtkListStore column, now it's still necessary for the SocialPlaceListBox::place-selected signal.

Damian said on IRC that some of the things I pointed out above was a bit unclear, so I arranged it in a mockup:
https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/checkin-dialog.png

Note that in order to simplify this dialog, I removed the text above the text box and put it as a placeholder text instead. This text needs to go away when the text area is clicked.

(In reply to comment #146)
> Damian said on IRC that some of the things I pointed out above was a bit
> unclear, so I arranged it in a mockup:
> https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/checkin-dialog.png
> 
> Note that in order to simplify this dialog, I removed the text above the text
> box and put it as a placeholder text instead. This text needs to go away when
> the text area is clicked.

Thanks!

I see two problems with this:

1) Bastien told me on G+ that is not obvious for the user which account is using to perform the check-in in the case of having just one configured account for Maps, in that case, the account selection step is skipped. That's why I added the account icon in the last step in my very last patch.

2) In my original design, I opted to use a textview placeholder instead of a separated label, then I realized that GtkTextView doesn't support placeholders and switched to an easier solution :( . Anyway, I think it could be implemented without much problem.

(In reply to comment #147)

> 1) Bastien told me on G+ that is not obvious for the user which account is
> using to perform the check-in in the case of having just one configured account
> for Maps, in that case, the account selection step is skipped. That's why I
> added the account icon in the last step in my very last patch.

That's a really good point.
Since you don't know what service you are using in all cases, but since you always will know the place from the previous step, the provider identifier will have to win over the location in this case.
I would therefore suggest changing the dialog title to "Check in with Foursquare" or "Check in with Facebook".

I've updated my mockup accordingly.
https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/checkin-dialog.png

Created attachment 292495 [details] [review]
Add Check-in capabilities

This only adds check-in capabilities necessary
to implement a check-in UI.

Created attachment 292496 [details] [review]
Add Check-in dialog

Created attachment 292497 [details] [review]
mapBubble: Add Check-in button

Created attachment 292498 [details] [review]
userLocationBubble: Enable Check-in button

Review of attachment 292495 [details] [review]:

Thanks!

Review of attachment 292496 [details] [review]:

(o/

Review of attachment 292497 [details] [review]:

Great (o/

Review of attachment 292498 [details] [review]:

Awesome \o/