[Spice-devel] client: fix endless recursion in rearrange_monitors, RHBZ #692976

Submitted by Yonit Halperin on July 21, 2011, 2:07 p.m.

Details

Message ID 1311232077-19913-1-git-send-email-yhalperi@redhat.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Yonit Halperin July 21, 2011, 2:07 p.m.
I changed RedScreen::resize not to call rearrange_monitors. Instead,
the monitor should be configured correctly from Application, before
calling resize.
In addition, I made some cleanups to allow reusing rearrange_monitors code.
---
 client/application.cpp |   94 +++++++++++++++++++++++++-----------------------
 client/application.h   |    4 ++-
 client/screen.cpp      |    4 --
 client/screen.h        |    2 +
 4 files changed, 54 insertions(+), 50 deletions(-)

Patch hide | download patch | download mbox

diff --git a/client/application.cpp b/client/application.cpp
index f19bd58..ce91b2a 100644
--- a/client/application.cpp
+++ b/client/application.cpp
@@ -664,25 +664,9 @@  RedScreen* Application::get_screen(int id)
 
         if (id != 0) {
             if (_full_screen) {
-                bool capture;
-
                 mon = get_monitor(id);
-                capture = release_capture();
                 screen->set_monitor(mon);
-                prepare_monitors();
-                position_screens();
-                screen->show_full_screen();
-                if (screen->is_out_of_sync()) {
-                    _out_of_sync = true;
-                    /* If the client monitor cannot handle the guest resolution
-                       drop back to windowed mode */
-                    exit_full_screen();
-                }
-
-                if (capture) {
-                    _main_screen->activate();
-                    _main_screen->capture_mouse();
-                }
+                rearrange_monitors(false, true, screen);
             } else {
                 screen->show(false, _main_screen);
             }
@@ -770,13 +754,7 @@  void Application::on_screen_destroyed(int id, bool was_captured)
     }
     _screens[id] = NULL;
     if (reposition) {
-        bool capture = was_captured || release_capture();
-        prepare_monitors();
-        position_screens();
-        if (capture) {
-            _main_screen->activate();
-            _main_screen->capture_mouse();
-        }
+        rearrange_monitors(was_captured, false);
     }
 }
 
@@ -1387,20 +1365,51 @@  void Application::on_screen_unlocked(RedScreen& screen)
     screen.resize(SCREEN_INIT_WIDTH, SCREEN_INIT_HEIGHT);
 }
 
-bool Application::rearrange_monitors(RedScreen& screen)
+void Application::rearrange_monitors(bool force_capture,
+                                     bool enter_full_screen,
+                                     RedScreen* screen)
 {
-    if (!_full_screen) {
-        return false;
+    bool capture;
+    bool toggle_full_screen;
+
+    if (!_full_screen && !enter_full_screen) {
+        return;
+    }
+
+    toggle_full_screen = enter_full_screen && !screen;
+    capture = release_capture();
+#ifndef WIN32
+    if (toggle_full_screen) {
+        /* performing hide during resolution changes resulted in
+           missing WM_KEYUP events */
+        hide();
     }
-    bool capture = release_capture();
+#endif
     prepare_monitors();
     position_screens();
-    if (capture && _main_screen != &screen) {
-        capture = false;
-        _main_screen->activate();
+    if (enter_full_screen) {
+        // toggling to full screen
+        if (toggle_full_screen) {
+            show_full_screen();
+            _main_screen->activate();
+
+        } else { // already in full screen mode and a new screen is displayed
+            screen->show_full_screen();
+            if (screen->is_out_of_sync()) {
+                _out_of_sync = true;
+                /* If the client monitor cannot handle the guest resolution
+                    drop back to windowed mode */
+                exit_full_screen();
+            }
+        }
+    } 
+
+    if (force_capture || capture) {
+        if (!toggle_full_screen) {
+            _main_screen->activate();
+        }
         _main_screen->capture_mouse();
     }
-    return capture;
 }
 
 Monitor* Application::find_monitor(int id)
@@ -1517,20 +1526,8 @@  void Application::enter_full_screen()
 {
     LOG_INFO("");
     _changing_screens = true;
-    bool capture = release_capture();
     assign_monitors();
-#ifndef WIN32
-    /* performing hide during resolution changes resulted in
-       missing WM_KEYUP events */
-    hide();
-#endif
-    prepare_monitors();
-    position_screens();
-    show_full_screen();
-    _main_screen->activate();
-    if (capture) {
-        _main_screen->capture_mouse();
-    }
+    rearrange_monitors(false, true);
     _changing_screens = false;
     _full_screen = true;
     /* If the client monitor cannot handle the guest resolution drop back
@@ -1592,7 +1589,14 @@  bool Application::toggle_full_screen()
 
 void Application::resize_screen(RedScreen *screen, int width, int height)
 {
+    Monitor* mon;
+    if (_full_screen) {
+        if ((mon = screen->get_monitor())) {
+            mon->set_mode(width, height);
+        }
+    }
     screen->resize(width, height);
+    rearrange_monitors(false, false);
     if (screen->is_out_of_sync()) {
         _out_of_sync = true;
         /* If the client monitor cannot handle the guest resolution
diff --git a/client/application.h b/client/application.h
index 4133dfe..8079753 100644
--- a/client/application.h
+++ b/client/application.h
@@ -216,7 +216,6 @@  public:
     void on_disconnecting();
     void on_visibility_start(int screen_id);
 
-    bool rearrange_monitors(RedScreen& screen);
     void enter_full_screen();
     void exit_full_screen();
     bool toggle_full_screen();
@@ -308,6 +307,9 @@  private:
     void assign_monitors();
     void restore_monitors();
     void prepare_monitors();
+    void rearrange_monitors(bool force_capture,
+                            bool enter_full_screen,
+                            RedScreen* screen = NULL);
     void position_screens();
     void show_full_screen();
     void send_key_down(RedKey key);
diff --git a/client/screen.cpp b/client/screen.cpp
index caa5d4f..e085781 100644
--- a/client/screen.cpp
+++ b/client/screen.cpp
@@ -186,11 +186,7 @@  void RedScreen::resize(int width, int height)
     _size.y = height;
     create_composit_area();
     if (_full_screen) {
-        bool cuptur = _owner.rearrange_monitors(*this);
         __show_full_screen();
-        if (cuptur) {
-            capture_mouse();
-        }
     } else {
         bool cuptur = is_mouse_captured();
         if (cuptur) {
diff --git a/client/screen.h b/client/screen.h
index 2b40d77..e7db4ef 100644
--- a/client/screen.h
+++ b/client/screen.h
@@ -62,6 +62,8 @@  public:
     void attach_layer(ScreenLayer& layer);
     void detach_layer(ScreenLayer& layer);
     void on_layer_changed(ScreenLayer& layer);
+    /* When resizing on full screen mode, the monitor must be configured 
+     * correctly before calling resize*/
     void resize(int width, int height);
     void set_name(const std::string& name);
     uint64_t invalidate(const SpiceRect& rect, bool urgent);

Comments

Hi,
forgot to mention that the endless recursion happened due to 
Application::prepare_monitors calling RedScreen::resize calling 
Application::rearrange_monitors calling Application::prepare_monitors
I will change the commit message.


On 07/21/2011 10:07 AM, Yonit Halperin wrote:
> I changed RedScreen::resize not to call rearrange_monitors. Instead,
> the monitor should be configured correctly from Application, before
> calling resize.
> In addition, I made some cleanups to allow reusing rearrange_monitors code.
> ---
>   client/application.cpp |   94 +++++++++++++++++++++++++-----------------------
>   client/application.h   |    4 ++-
>   client/screen.cpp      |    4 --
>   client/screen.h        |    2 +
>   4 files changed, 54 insertions(+), 50 deletions(-)
>
> diff --git a/client/application.cpp b/client/application.cpp
> index f19bd58..ce91b2a 100644
> --- a/client/application.cpp
> +++ b/client/application.cpp
> @@ -664,25 +664,9 @@ RedScreen* Application::get_screen(int id)
>
>           if (id != 0) {
>               if (_full_screen) {
> -                bool capture;
> -
>                   mon = get_monitor(id);
> -                capture = release_capture();
>                   screen->set_monitor(mon);
> -                prepare_monitors();
> -                position_screens();
> -                screen->show_full_screen();
> -                if (screen->is_out_of_sync()) {
> -                    _out_of_sync = true;
> -                    /* If the client monitor cannot handle the guest resolution
> -                       drop back to windowed mode */
> -                    exit_full_screen();
> -                }
> -
> -                if (capture) {
> -                    _main_screen->activate();
> -                    _main_screen->capture_mouse();
> -                }
> +                rearrange_monitors(false, true, screen);
>               } else {
>                   screen->show(false, _main_screen);
>               }
> @@ -770,13 +754,7 @@ void Application::on_screen_destroyed(int id, bool was_captured)
>       }
>       _screens[id] = NULL;
>       if (reposition) {
> -        bool capture = was_captured || release_capture();
> -        prepare_monitors();
> -        position_screens();
> -        if (capture) {
> -            _main_screen->activate();
> -            _main_screen->capture_mouse();
> -        }
> +        rearrange_monitors(was_captured, false);
>       }
>   }
>
> @@ -1387,20 +1365,51 @@ void Application::on_screen_unlocked(RedScreen&  screen)
>       screen.resize(SCREEN_INIT_WIDTH, SCREEN_INIT_HEIGHT);
>   }
>
> -bool Application::rearrange_monitors(RedScreen&  screen)
> +void Application::rearrange_monitors(bool force_capture,
> +                                     bool enter_full_screen,
> +                                     RedScreen* screen)
>   {
> -    if (!_full_screen) {
> -        return false;
> +    bool capture;
> +    bool toggle_full_screen;
> +
> +    if (!_full_screen&&  !enter_full_screen) {
> +        return;
> +    }
> +
> +    toggle_full_screen = enter_full_screen&&  !screen;
> +    capture = release_capture();
> +#ifndef WIN32
> +    if (toggle_full_screen) {
> +        /* performing hide during resolution changes resulted in
> +           missing WM_KEYUP events */
> +        hide();
>       }
> -    bool capture = release_capture();
> +#endif
>       prepare_monitors();
>       position_screens();
> -    if (capture&&  _main_screen !=&screen) {
> -        capture = false;
> -        _main_screen->activate();
> +    if (enter_full_screen) {
> +        // toggling to full screen
> +        if (toggle_full_screen) {
> +            show_full_screen();
> +            _main_screen->activate();
> +
> +        } else { // already in full screen mode and a new screen is displayed
> +            screen->show_full_screen();
> +            if (screen->is_out_of_sync()) {
> +                _out_of_sync = true;
> +                /* If the client monitor cannot handle the guest resolution
> +                    drop back to windowed mode */
> +                exit_full_screen();
> +            }
> +        }
> +    }
> +
> +    if (force_capture || capture) {
> +        if (!toggle_full_screen) {
> +            _main_screen->activate();
> +        }
>           _main_screen->capture_mouse();
>       }
> -    return capture;
>   }
>
>   Monitor* Application::find_monitor(int id)
> @@ -1517,20 +1526,8 @@ void Application::enter_full_screen()
>   {
>       LOG_INFO("");
>       _changing_screens = true;
> -    bool capture = release_capture();
>       assign_monitors();
> -#ifndef WIN32
> -    /* performing hide during resolution changes resulted in
> -       missing WM_KEYUP events */
> -    hide();
> -#endif
> -    prepare_monitors();
> -    position_screens();
> -    show_full_screen();
> -    _main_screen->activate();
> -    if (capture) {
> -        _main_screen->capture_mouse();
> -    }
> +    rearrange_monitors(false, true);
>       _changing_screens = false;
>       _full_screen = true;
>       /* If the client monitor cannot handle the guest resolution drop back
> @@ -1592,7 +1589,14 @@ bool Application::toggle_full_screen()
>
>   void Application::resize_screen(RedScreen *screen, int width, int height)
>   {
> +    Monitor* mon;
> +    if (_full_screen) {
> +        if ((mon = screen->get_monitor())) {
> +            mon->set_mode(width, height);
> +        }
> +    }
>       screen->resize(width, height);
> +    rearrange_monitors(false, false);
>       if (screen->is_out_of_sync()) {
>           _out_of_sync = true;
>           /* If the client monitor cannot handle the guest resolution
> diff --git a/client/application.h b/client/application.h
> index 4133dfe..8079753 100644
> --- a/client/application.h
> +++ b/client/application.h
> @@ -216,7 +216,6 @@ public:
>       void on_disconnecting();
>       void on_visibility_start(int screen_id);
>
> -    bool rearrange_monitors(RedScreen&  screen);
>       void enter_full_screen();
>       void exit_full_screen();
>       bool toggle_full_screen();
> @@ -308,6 +307,9 @@ private:
>       void assign_monitors();
>       void restore_monitors();
>       void prepare_monitors();
> +    void rearrange_monitors(bool force_capture,
> +                            bool enter_full_screen,
> +                            RedScreen* screen = NULL);
>       void position_screens();
>       void show_full_screen();
>       void send_key_down(RedKey key);
> diff --git a/client/screen.cpp b/client/screen.cpp
> index caa5d4f..e085781 100644
> --- a/client/screen.cpp
> +++ b/client/screen.cpp
> @@ -186,11 +186,7 @@ void RedScreen::resize(int width, int height)
>       _size.y = height;
>       create_composit_area();
>       if (_full_screen) {
> -        bool cuptur = _owner.rearrange_monitors(*this);
>           __show_full_screen();
> -        if (cuptur) {
> -            capture_mouse();
> -        }
>       } else {
>           bool cuptur = is_mouse_captured();
>           if (cuptur) {
> diff --git a/client/screen.h b/client/screen.h
> index 2b40d77..e7db4ef 100644
> --- a/client/screen.h
> +++ b/client/screen.h
> @@ -62,6 +62,8 @@ public:
>       void attach_layer(ScreenLayer&  layer);
>       void detach_layer(ScreenLayer&  layer);
>       void on_layer_changed(ScreenLayer&  layer);
> +    /* When resizing on full screen mode, the monitor must be configured
> +     * correctly before calling resize*/
>       void resize(int width, int height);
>       void set_name(const std::string&  name);
>       uint64_t invalidate(const SpiceRect&  rect, bool urgent);
On Thu, Jul 21, 2011 at 10:07:57AM +0300, Yonit Halperin wrote:
> I changed RedScreen::resize not to call rearrange_monitors. Instead,
> the monitor should be configured correctly from Application, before
> calling resize.
> In addition, I made some cleanups to allow reusing rearrange_monitors code.

ACK.

> ---
>  client/application.cpp |   94 +++++++++++++++++++++++++-----------------------
>  client/application.h   |    4 ++-
>  client/screen.cpp      |    4 --
>  client/screen.h        |    2 +
>  4 files changed, 54 insertions(+), 50 deletions(-)
> 
> diff --git a/client/application.cpp b/client/application.cpp
> index f19bd58..ce91b2a 100644
> --- a/client/application.cpp
> +++ b/client/application.cpp
> @@ -664,25 +664,9 @@ RedScreen* Application::get_screen(int id)
>  
>          if (id != 0) {
>              if (_full_screen) {
> -                bool capture;
> -
>                  mon = get_monitor(id);
> -                capture = release_capture();
>                  screen->set_monitor(mon);
> -                prepare_monitors();
> -                position_screens();
> -                screen->show_full_screen();
> -                if (screen->is_out_of_sync()) {
> -                    _out_of_sync = true;
> -                    /* If the client monitor cannot handle the guest resolution
> -                       drop back to windowed mode */
> -                    exit_full_screen();
> -                }
> -
> -                if (capture) {
> -                    _main_screen->activate();
> -                    _main_screen->capture_mouse();
> -                }
> +                rearrange_monitors(false, true, screen);
>              } else {
>                  screen->show(false, _main_screen);
>              }
> @@ -770,13 +754,7 @@ void Application::on_screen_destroyed(int id, bool was_captured)
>      }
>      _screens[id] = NULL;
>      if (reposition) {
> -        bool capture = was_captured || release_capture();
> -        prepare_monitors();
> -        position_screens();
> -        if (capture) {
> -            _main_screen->activate();
> -            _main_screen->capture_mouse();
> -        }
> +        rearrange_monitors(was_captured, false);
>      }
>  }
>  
> @@ -1387,20 +1365,51 @@ void Application::on_screen_unlocked(RedScreen& screen)
>      screen.resize(SCREEN_INIT_WIDTH, SCREEN_INIT_HEIGHT);
>  }
>  
> -bool Application::rearrange_monitors(RedScreen& screen)
> +void Application::rearrange_monitors(bool force_capture,
> +                                     bool enter_full_screen,
> +                                     RedScreen* screen)
>  {
> -    if (!_full_screen) {
> -        return false;
> +    bool capture;
> +    bool toggle_full_screen;
> +
> +    if (!_full_screen && !enter_full_screen) {
> +        return;
> +    }
> +
> +    toggle_full_screen = enter_full_screen && !screen;
> +    capture = release_capture();
> +#ifndef WIN32
> +    if (toggle_full_screen) {
> +        /* performing hide during resolution changes resulted in
> +           missing WM_KEYUP events */
> +        hide();
>      }
> -    bool capture = release_capture();
> +#endif
>      prepare_monitors();
>      position_screens();
> -    if (capture && _main_screen != &screen) {
> -        capture = false;
> -        _main_screen->activate();
> +    if (enter_full_screen) {
> +        // toggling to full screen
> +        if (toggle_full_screen) {
> +            show_full_screen();
> +            _main_screen->activate();
> +
> +        } else { // already in full screen mode and a new screen is displayed
> +            screen->show_full_screen();
> +            if (screen->is_out_of_sync()) {
> +                _out_of_sync = true;
> +                /* If the client monitor cannot handle the guest resolution
> +                    drop back to windowed mode */
> +                exit_full_screen();
> +            }
> +        }
> +    } 
> +
> +    if (force_capture || capture) {
> +        if (!toggle_full_screen) {
> +            _main_screen->activate();
> +        }
>          _main_screen->capture_mouse();
>      }
> -    return capture;
>  }
>  
>  Monitor* Application::find_monitor(int id)
> @@ -1517,20 +1526,8 @@ void Application::enter_full_screen()
>  {
>      LOG_INFO("");
>      _changing_screens = true;
> -    bool capture = release_capture();
>      assign_monitors();
> -#ifndef WIN32
> -    /* performing hide during resolution changes resulted in
> -       missing WM_KEYUP events */
> -    hide();
> -#endif
> -    prepare_monitors();
> -    position_screens();
> -    show_full_screen();
> -    _main_screen->activate();
> -    if (capture) {
> -        _main_screen->capture_mouse();
> -    }
> +    rearrange_monitors(false, true);
>      _changing_screens = false;
>      _full_screen = true;
>      /* If the client monitor cannot handle the guest resolution drop back
> @@ -1592,7 +1589,14 @@ bool Application::toggle_full_screen()
>  
>  void Application::resize_screen(RedScreen *screen, int width, int height)
>  {
> +    Monitor* mon;
> +    if (_full_screen) {
> +        if ((mon = screen->get_monitor())) {
> +            mon->set_mode(width, height);
> +        }
> +    }
>      screen->resize(width, height);
> +    rearrange_monitors(false, false);
>      if (screen->is_out_of_sync()) {
>          _out_of_sync = true;
>          /* If the client monitor cannot handle the guest resolution
> diff --git a/client/application.h b/client/application.h
> index 4133dfe..8079753 100644
> --- a/client/application.h
> +++ b/client/application.h
> @@ -216,7 +216,6 @@ public:
>      void on_disconnecting();
>      void on_visibility_start(int screen_id);
>  
> -    bool rearrange_monitors(RedScreen& screen);
>      void enter_full_screen();
>      void exit_full_screen();
>      bool toggle_full_screen();
> @@ -308,6 +307,9 @@ private:
>      void assign_monitors();
>      void restore_monitors();
>      void prepare_monitors();
> +    void rearrange_monitors(bool force_capture,
> +                            bool enter_full_screen,
> +                            RedScreen* screen = NULL);
>      void position_screens();
>      void show_full_screen();
>      void send_key_down(RedKey key);
> diff --git a/client/screen.cpp b/client/screen.cpp
> index caa5d4f..e085781 100644
> --- a/client/screen.cpp
> +++ b/client/screen.cpp
> @@ -186,11 +186,7 @@ void RedScreen::resize(int width, int height)
>      _size.y = height;
>      create_composit_area();
>      if (_full_screen) {
> -        bool cuptur = _owner.rearrange_monitors(*this);
>          __show_full_screen();
> -        if (cuptur) {
> -            capture_mouse();
> -        }
>      } else {
>          bool cuptur = is_mouse_captured();
>          if (cuptur) {
> diff --git a/client/screen.h b/client/screen.h
> index 2b40d77..e7db4ef 100644
> --- a/client/screen.h
> +++ b/client/screen.h
> @@ -62,6 +62,8 @@ public:
>      void attach_layer(ScreenLayer& layer);
>      void detach_layer(ScreenLayer& layer);
>      void on_layer_changed(ScreenLayer& layer);
> +    /* When resizing on full screen mode, the monitor must be configured 
> +     * correctly before calling resize*/
>      void resize(int width, int height);
>      void set_name(const std::string& name);
>      uint64_t invalidate(const SpiceRect& rect, bool urgent);
> -- 
> 1.7.4.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel