Created attachment 280775 [details] [review]
Basic designing

Created attachment 280776 [details] [review]
basic design - corrected whitespaces mistakes

Created attachment 280780 [details] [review]
basic design with few changes

Review of attachment 280780 [details] [review]:

::: data/city.ui
@@ +112,3 @@
+                          </object>
+                          <packing>
+                            <property name="name">Today</property>

You should probably stick to "GObject case" (lowercase, separated by hyphens) for identifiers (so "today" and "tomorrow" for the Stack child name, "day-stack-switcher", "today-button", etc.)

@@ +181,3 @@
+                                </child>
+                                <child>
+                                  <placeholder/>

Placeholders  are something that glade adds to make interactive editing easier, but are not necessary and cause confusion when the UI file is edited manually (which happens, because glade does not support most modern gtk widgets). Please remove them.

::: src/app/city.js
@@ +23,3 @@
 const Gnome = imports.gi.GnomeDesktop;
 const Lang = imports.lang;
+const Gdk = imports.gi.Gdk;

Unused?

@@ +81,3 @@
+        this._forecastGrid.set_halign(Gtk.Align.CENTER);
+        this._forecasts = new Forecast.ForecastBox({ hexpand: false });
+        this._forecastGrid.attach(this._forecasts, 0, 0, 1, 1);

You're using a mixture of glade and manual code here. It would be better to have everything in glade, except what glade cannot do (ie, ForecastBox).

Note that by glade I don't mean the IDE, I mean handediting the ui files. They are simple enough that it's easier to edit them by hand.

@@ +135,3 @@
+            this._start = new Date().getTime();
+            this._end = this._start + 800;
+            this._tickId = this._forecastGrid.add_tick_callback(Lang.bind(this, this._animate));

There is the some duplicated code here that can probably be consolidated.

Also, Gtk supports GtkAdjustment animation natively with "animate_to_value()".

@@ +147,3 @@
+        if (now < this._end) {
+            t = (now - this._start) / 800;
+            t = this._ease_out_cubic (t);

If you keep the manual animation code, use the easeOutCubic function from imports.tweener.equations (or just use Tweener!)

@@ +191,1 @@
+        if (!forecasts || (forecasts.length == 0)) {

Is it possible that we get null here? A NULL GList should be an empty array in JS, not null.

::: src/app/forecast.js
@@ +95,3 @@
+        if (infos && infos.length > 0) {
+            let now = GLib.DateTime.new_now_local();
+            if (day == "Tomorrow")

I don't see the

@@ +112,3 @@
         } else {
+            //Translators: this is label showing that forecast is not available
+            let label = new Gtk.Label({ label: "Forecast not available",

You're not really translating this, you need _("string")

@@ +149,3 @@
     },
 
+    updateWeekly: function(infos, wForecastGrid) {

This method is taking the grid as parameter, which sounds wrong. Why isn't it an attribute?

@@ +163,1 @@
+            let timeFormat = "%A";

Time formats need to be translatable (like they were before)

::: src/app/window.js
@@ +103,3 @@
+            this._providers = GWeather.Provider.METAR | GWeather.Provider.YR_NO |
+                GWeather.Provider.OWM;
+        }

Please share this code with world.js

@@ +231,3 @@
+                                                visible: true });
+        this._recentViewLabel.get_style_context().add_class('dim-label');
+        this._grid.attach(this._recentViewLabel, 0, 2, 1, 1);

As for city.js, please use UI files as much as possible for creating GTK widgets. I know the syntax is not awesome, but it's easier to test and update for minor fixes.

(For the popover you would create a new toplevel object in the UI file, which references the menu button by ID)

@@ +248,3 @@
+        this._listBox.connect('row-activated', Lang.bind(this, function(box,  row) {
+            popover.hide();
+            this._rowChange(row);

You don't need this indirection, just connect the method and hide the popover there.

@@ +356,3 @@
+        } else if (locations.length > 0) {
+            this.showInfo(this._infoList[0]);
+        }

This code should be in the model (in world.js or in a new class), and window should only see the current location info as well as any updates.

@@ +359,3 @@
+    },
+
+    _rowChange: function(row) {

rowChange reads to me as "the content (or associated model item) for a row changed"

maybe rowActivated?

@@ +433,3 @@
+
+        let info = new GWeather.Info({ location: location,
+                                        enabled_providers: this._providers });

See above, I don't like this

@@ +498,3 @@
     _goToPage: function(page) {
+        /*if (page == this._currentPage)
+            return; */

Why did you comment this code?

Created attachment 281178 [details] [review]
New design. Some source code removal is still left but new design is implemented.

Created attachment 281333 [details] [review]
New design.

Created attachment 281363 [details] [review]
Weather redesign patch

Created attachment 281378 [details] [review]
weather redesign. It hides the row of currently loaded location.

Review of attachment 281378 [details] [review]:

Nice work indeed.

From testing, besides a missing Lang.bind(), you need to remove the remnants of World View (including the button that brings you there and changes the window title), but other than that it's fine.

::: data/org.gnome.Weather.Application.gschema.xml
@@ +9,3 @@
       </description>
     </key>
+    <key name="auto-location" type="b">

Maybe automatic-location?

(Definitely the summary should be Automatic and not Auto)

::: src/app/currentLocationController.js
@@ +112,1 @@
         this._clientProxy.StartRemote(function(result, e) {

Need to bind this one, or it crashes at startup.

::: src/shared/world.js
@@ +96,3 @@
+        this.addedCurrentLocation = false;
+
+        this._currentLocationController = new CurrentLocationController.CurrentLocationController(this);

I'm not a fan of having the currentlocationcontroller owned by the worldmodel (because worldmodel is used by the background service too, and we don't want to run geolocation there).

How about moving the autolocation stuff to CurrentLocationController?
For the use in WorldContentView, there is not problem to access autoLocation on CurrentLocationController instead of WorldModel, and for the use in fillCityView, it can maybe be a parameter?

@@ +100,3 @@
+        this.autoLocation = this._settings.get_value('auto-location').deep_unpack();
+        if(this.autoLocation)
+            this._currentLocationController.startGeolocationService();

Shouldn't you watch for changes to the GSettings?

(Or maybe use a GObject property, and bind it to the settings)

@@ +125,3 @@
+            }
+        }
+        this.emit('load-window');

Why a new load-window signal, and not the existing notify::loaded?
With that, you could avoid the duplicate code in app.js

Created attachment 281853 [details] [review]
Redesign - city-view changes.

Created attachment 281854 [details] [review]
Redesign - world-view and city-view changes, removal of libgd.(Total redesign)

Created attachment 281855 [details] [review]
Final redesign patch with all changes(city-view, world-view and libgd removal).  "tests" and screenshots are yet to be updated.

Review of attachment 281855 [details] [review]:

A couple of comments from testing...

::: src/shared/world.js
@@ +22,3 @@
 const GWeather = imports.gi.GWeather;
 const Lang = imports.lang;
+const Signals = imports.signals;

Don't use Signals, use GObject subclasses.

@@ -61,3 @@
 const WorldModel = new Lang.Class({
     Name: 'WorldModel',
-    Extends: Gtk.ListStore,

You must Extends: GObject.Object (or another GObject subclass)

@@ -71,2 @@
     _init: function(world, enableGtk) {
-        this.parent();

And therefore you must chain up here.

@@ +82,3 @@
+            }
+        }
+        this.emit('notify::loading');

You don't need this emission, _updateLoadingCount takes care of it.

@@ +143,2 @@
         if (wasLoading != isLoading)
+            this.emit('notify::loading');

Go back to notify('loading') please.

@@ +391,3 @@
     }
 });
+Signals.addSignalMethods(WorldModel.prototype);

And kill this.

Another thing that would make sense would be to replace deprecated properties (margin-left, margin-right, xalign, etc.) that are introduced in the patch with the non-deprecated version.

After design review:

- today/tomorrow and </> buttons should use OSD style ("osd" style class)
- the time should be shown only if it's in a different timezone than here
- the layout should be responsive and look decent when maximized too
  * for the sidebar, I got a good result by making the grid inside the WeeklyForecastFrame vexpand=TRUE,valign=FILL, and each item grid vexpand=FALSE,valign=CENTER
  * for center content, we probably needs a custom GtkBin that requests the size of the child (which would be the minimum and natural size, because we don't have reflowable content), but then allocates with a linear proportion between the requested size and the allocated size
    (this is sort of advanced, because writing custom containers is not easy and not really well documented, so it's ok if you want me to take care of it)

Created attachment 282097 [details] [review]
Final patch for new design. One slot still remains empty in today's forecasting around midnight. That is yet to be solved.

So, I've been testing this some more, and I noticed that the search provider is completely broken. You must update it to the new WorldModel interface before this can be merged.

Two requirements:
1) Do a separate patch on top of the first one, do not squash into an already enormous commit
2) The background service must not use Gtk in any way or form

Created attachment 282254 [details] [review]
Improving model-view split. The patch is on top of the previous patch.

Pushed to master, and pushed some more clean-ups on top of those,
if you want to take a look.

Created attachment 282413 [details] [review]
strings.js file is removed but imports.shared.strings is not removed from forecast.js and weeklyforecast.js

Comment on attachment 282413 [details] [review]
strings.js file is removed but imports.shared.strings is not removed from forecast.js and weeklyforecast.js

Pushed.