We don't handle resolution changes at all right now; this is just one example.

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

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

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

Created attachment 150259 [details] [review]
Patch for this bug

Added signal 'screen-resolution-changed'.
Connect to it '_relayout'.

Review of attachment 150259 [details] [review]:

Thanks a lot for tackling this, it's been bothering people for a long time!

Trying it out, the only real problem I saw was that if the clock was up, then the clock wouldn't get centered in the new screen width.

Patch looks generally good, some comments.

::: js/ui/main.js
@@ +194,3 @@
     panel.actor.set_size(primary.width, Panel.PANEL_HEIGHT);
     overview.relayout();
+    overview.hide();

While it seems a bit strange to kick the user out of the overview when the resolution changes, I think it's OK to do it if it saves a lot of code.

*But* you need a comment explaining that and saying what doesn't update properly.

::: src/shell-global.c
@@ +220,3 @@
 
+  shell_global_signals[SCREEN_RESOLUTION_CHANGED] =
+    g_signal_new ("screen-resolution-changed",

I'd prefer 'screen-size-changed'

@@ +464,3 @@
+                             clutter_actor_get_width (stage));
+  g_object_notify (G_OBJECT (global), "screen-width");
+  g_signal_emit (G_OBJECT (global), shell_global_signals[SCREEN_RESOLUTION_CHANGED], 0);

The only reason to have a signal in addition to the notified properties would be to "de-duplicate" the emission so that you don't do all the work twice when both the width and size change at once. But you don't currently do that. You can use
'meta_later_add (META_LATER_BEFORE_REDRAW, ...)' to do the de-duplication.

@@ +494,3 @@
+  g_signal_connect (stage, "notify::height",
+                    G_CALLBACK (global_stage_notify_height), data);
+  g_signal_emit (G_OBJECT (global), shell_global_signals[SCREEN_RESOLUTION_CHANGED], 0);

I don't think you want this emission.

@@ +508,3 @@
+
+  g_signal_connect (plugin, "notify::screen",
+                    G_CALLBACK (global_plugin_notify_screen), global);

You need a comment explaining why you do it this way (because the screen property hasn't yet been set at this point), and pointing out the screen will never change, or otherwise it looks like you are connecting to screen changes but forgetting to disconnect signal handlers from the old screen.

Created attachment 151705 [details] [review]
Corrected patch

Review of attachment 151705 [details] [review]:

Looks great, just a few minor style points.

::: js/ui/main.js
@@ +204,3 @@
+
+    // Need update position and size of overview._workspaces
+    // or just hide it.

This is too cryptic and sketchy. It sounds like hide() either updates or hides the position. I would suggest:

 // To avoid updating the position and size of the workspaces
 // in the overview, we just hide the overview. The positions
 // will be updated when it is next shown. We do the same for
 // the calendar popdown.

(And then move the calendar popdown hiding down here.)

::: src/shell-global.c
@@ +195,3 @@
+
+  global->screen_width = 0;
+  global->screen_height = 0;

Can you call these 'last_change_screen_width/height' instead ... because they don't correspond to the current screen width exactly, and we don't use them when querying the screen width/height.

@@ +538,3 @@
   global->wm = shell_wm_new (plugin);
+
+  /*Here screen = NULL*/

Please be more verbose (and the comment should have spaces around it).

> You need a comment explaining why you do it this way (because the screen
> property hasn't yet been set at this point), and pointing out the screen will
> never change, or otherwise it looks like you are connecting to screen changes
> but forgetting to disconnect signal handlers from the old screen.

So, maybe:

 /* At this point screen is NULL, so we can't yet do signal connections
  * to the width and height; we wait until the screen property is set
  * to do that. Note that this is a one time thing - screen will never
  * change once first set.
  */

(I've provided suggestions here because, well, I wouldn't want to have to write long comments in anything other than English. However, it's not good grammar that's important, it's the content. As long as its understandable, that's fine.)

Created attachment 151716 [details] [review]
Corrected patch

Review of attachment 151716 [details] [review]:

The patch looks good to commit. The subject of the commit should be 'Handle resolution changes' not 'Doesn't handle resolution changes', since it describes what the patch does.

The body of the commit should be more verbose:

 - What class did you add screen-size-changed to?
 - Where is _relayout? 'Connect to this signal in main.js and run the _relayout() method'
 - You should mention hiding the overview and the clock

The body of the commit doesn't have to describe everything that's in the patch, but it should give a general sense of what the patch is about and the approach you took.

Until you get familiar with operation of 'git push', I would suggest doing 'git push --dry-run' before doing 'git push' for real. That will show which branches are being pushed and the change to that branch. The change will be shown as a range of git IDs - say, '51f12e1..312a341' to see what commits that corresponds to, do 'git log 51f12e1..312a341'

Comment on attachment 151716 [details] [review]
Corrected patch

Attachment 151716 [details] pushed as 900805 - Handle resolution changes