Code review comment for lp:~cyphermox/unity/multimonitor

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Hi Mathieu,

Great initiative here - just nitpicking a few things before we merge this:

1) You've got merge markers in the source (eg <<<<<<< TREE MERGE SOURCE), you need to remove them or it won't build :p
2) You have strutHackTimeout in UnityScreen::Relayout - this means that on init you'll have strutHackTimeout being called twice. Remove it from UnityScreen::Relayout
3) s/g_warning/g_debug/ The screen size change is *not* a warning
4) you've got:

+ g_signal_connect_swapped (gdk_screen_get_default (), "monitors-changed", G_CALLBACK (&UnityScreen::Relayout), (void*) this);
+ g_signal_connect_swapped (gdk_screen_get_default (), "size-changed", G_CALLBACK (&UnityScreen::Relayout), (void*) this);

That is going to cause UnityScreen::Relayout to be called twice in cases where the monitors changed and the size changed. That's bad

5) This is probably going to be a point of contention between me and the other developers, but I'd actually prefer it if we hooked up this ::outputChangeNotify wrappable function in compiz. The reason why is because outputs are user-configurable in compiz in case your graphics driver is horribly broken or you need to do multihead testing. In addition, it means that we're reacting to the output change once the window manager has done everything it needs to do, meaning racy weirdness when compiz reshuffles windows about to handle the output change and we've already moved before that.

review: Needs Fixing

« Back to merge proposal