From b3d7bf1c1e5b042d94ff20682de2da6bf8426932 Mon Sep 17 00:00:00 2001 From: Vojtech Kral Date: Fri, 11 Jan 2019 18:09:21 +0100 Subject: [PATCH 1/4] Fix window geometry persistence #1557 --- src/slic3r/GUI/GUI.cpp | 1 + src/slic3r/GUI/GUI_App.cpp | 109 ++++++++++++++++++++++------------- src/slic3r/GUI/GUI_App.hpp | 10 ++-- src/slic3r/GUI/GUI_Utils.cpp | 19 ++++-- src/slic3r/GUI/GUI_Utils.hpp | 5 +- src/slic3r/GUI/MainFrame.cpp | 21 ++----- 6 files changed, 100 insertions(+), 65 deletions(-) diff --git a/src/slic3r/GUI/GUI.cpp b/src/slic3r/GUI/GUI.cpp index bb5ab29e0..bccf211ef 100644 --- a/src/slic3r/GUI/GUI.cpp +++ b/src/slic3r/GUI/GUI.cpp @@ -348,6 +348,7 @@ bool get_current_screen_size(wxWindow *window, unsigned &width, unsigned &height return true; } +// XXX: remove these void save_window_size(wxTopLevelWindow *window, const std::string &name) { const wxSize size = window->GetSize(); diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 9618aaf00..861312f82 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -143,7 +143,6 @@ bool GUI_App::OnInit() init_fonts(); // application frame - std::cerr << "Creating main frame..." << std::endl; if (wxImage::FindHandler(wxBITMAP_TYPE_PNG) == nullptr) wxImage::AddHandler(new wxPNGHandler()); mainframe = new MainFrame(); @@ -370,6 +369,33 @@ void GUI_App::update_ui_from_settings() mainframe->update_ui_from_settings(); } +void GUI_App::persist_window_geometry(wxTopLevelWindow *window) +{ + const std::string name = into_u8(window->GetName()); + + window->Bind(wxEVT_CLOSE_WINDOW, [=](wxCloseEvent &event) { + window_pos_save(window, name); + event.Skip(); + }); + + window_pos_restore(window, name); +#ifdef _WIN32 + // On windows, the wxEVT_SHOW is not received if the window is created maximized + // cf. https://groups.google.com/forum/#!topic/wx-users/c7ntMt6piRI + // so we sanitize the position right away + window_pos_sanitize(window); +#else + // On other platforms on the other hand it's needed to wait before the window is actually on screen + // and some initial round of events is complete otherwise position / display index is not reported correctly. + window->Bind(wxEVT_SHOW, [=](wxShowEvent &event) { + CallAfter([=]() { + window_pos_sanitize(window); + }); + event.Skip(); + }); +#endif +} + void GUI_App::load_project(wxWindow *parent, wxString& input_file) { input_file.Clear(); @@ -404,45 +430,6 @@ void GUI_App::CallAfter(std::function cb) callback_register.unlock(); } -void GUI_App::window_pos_save(wxTopLevelWindow* window, const std::string &name) -{ - if (name.empty()) { return; } - const auto config_key = (boost::format("window_%1%") % name).str(); - - WindowMetrics metrics = WindowMetrics::from_window(window); - app_config->set(config_key, metrics.serialize()); - app_config->save(); -} - -void GUI_App::window_pos_restore(wxTopLevelWindow* window, const std::string &name) -{ - if (name.empty()) { return; } - const auto config_key = (boost::format("window_%1%") % name).str(); - - if (! app_config->has(config_key)) { return; } - - auto metrics = WindowMetrics::deserialize(app_config->get(config_key)); - if (! metrics) { return; } - - window->SetSize(metrics->get_rect()); - window->Maximize(metrics->get_maximized()); -} - -void GUI_App::window_pos_sanitize(wxTopLevelWindow* window) -{ - const auto display_idx = wxDisplay::GetFromWindow(window); - if (display_idx == wxNOT_FOUND) { return; } - - const auto display = wxDisplay(display_idx).GetClientArea(); - - auto metrics = WindowMetrics::from_window(window); - - metrics.sanitize_for_display(display); - if (window->GetScreenRect() != metrics.get_rect()) { - window->SetSize(metrics.get_rect()); - } -} - // select language from the list of installed languages bool GUI_App::select_language( wxArrayString & names, wxArrayLong & identifiers) @@ -789,6 +776,48 @@ wxNotebook* GUI_App::tab_panel() const return mainframe->m_tabpanel; } +void GUI_App::window_pos_save(wxTopLevelWindow* window, const std::string &name) +{ + if (name.empty()) { return; } + const auto config_key = (boost::format("window_%1%") % name).str(); + + WindowMetrics metrics = WindowMetrics::from_window(window); + app_config->set(config_key, metrics.serialize()); + app_config->save(); +} + +void GUI_App::window_pos_restore(wxTopLevelWindow* window, const std::string &name) +{ + if (name.empty()) { return; } + const auto config_key = (boost::format("window_%1%") % name).str(); + + if (! app_config->has(config_key)) { return; } + + auto metrics = WindowMetrics::deserialize(app_config->get(config_key)); + if (! metrics) { return; } + + window->SetSize(metrics->get_rect()); + window->Maximize(metrics->get_maximized()); +} + +void GUI_App::window_pos_sanitize(wxTopLevelWindow* window) +{ + unsigned display_idx = wxDisplay::GetFromWindow(window); + wxRect display; + if (display_idx == wxNOT_FOUND) { + display = wxDisplay(0u).GetClientArea(); + window->Move(display.GetTopLeft()); + } else { + display = wxDisplay(display_idx).GetClientArea(); + } + + auto metrics = WindowMetrics::from_window(window); + metrics.sanitize_for_display(display); + if (window->GetScreenRect() != metrics.get_rect()) { + window->SetSize(metrics.get_rect()); + } +} + // static method accepting a wxWindow object as first parameter // void warning_catcher{ // my($self, $message_dialog) = @_; diff --git a/src/slic3r/GUI/GUI_App.hpp b/src/slic3r/GUI/GUI_App.hpp index a0e20d7d4..6d81b3795 100644 --- a/src/slic3r/GUI/GUI_App.hpp +++ b/src/slic3r/GUI/GUI_App.hpp @@ -121,13 +121,11 @@ public: // wxMessageDialog* message_dialog, const std::string& err); // void notify(/*message*/); + + void persist_window_geometry(wxTopLevelWindow *window); void update_ui_from_settings(); void CallAfter(std::function cb); - void window_pos_save(wxTopLevelWindow* window, const std::string &name); - void window_pos_restore(wxTopLevelWindow* window, const std::string &name); - void window_pos_sanitize(wxTopLevelWindow* window); - bool select_language(wxArrayString & names, wxArrayLong & identifiers); bool load_language(); void save_language(); @@ -172,6 +170,10 @@ public: PrintHostJobQueue& printhost_job_queue() { return *m_printhost_job_queue.get(); } +private: + void window_pos_save(wxTopLevelWindow* window, const std::string &name); + void window_pos_restore(wxTopLevelWindow* window, const std::string &name); + void window_pos_sanitize(wxTopLevelWindow* window); }; DECLARE_APP(GUI_App) diff --git a/src/slic3r/GUI/GUI_Utils.cpp b/src/slic3r/GUI/GUI_Utils.cpp index 43b3c38f7..db0264459 100644 --- a/src/slic3r/GUI/GUI_Utils.cpp +++ b/src/slic3r/GUI/GUI_Utils.cpp @@ -120,19 +120,28 @@ boost::optional WindowMetrics::deserialize(const std::string &str void WindowMetrics::sanitize_for_display(const wxRect &screen_rect) { rect = rect.Intersect(screen_rect); + + // Prevent the window from going too far towards the right and/or bottom edge + // It's hardcoded here that the threshold is 80% of the screen size + rect.x = std::min(rect.x, screen_rect.x + 4*screen_rect.width/5); + rect.y = std::min(rect.y, screen_rect.y + 4*screen_rect.height/5); } -std::string WindowMetrics::serialize() +std::string WindowMetrics::serialize() const { return (boost::format("%1%; %2%; %3%; %4%; %5%") - % rect.GetX() - % rect.GetY() - % rect.GetWidth() - % rect.GetHeight() + % rect.x + % rect.y + % rect.width + % rect.height % static_cast(maximized) ).str(); } +std::ostream& operator<<(std::ostream &os, const WindowMetrics& metrics) +{ + return os << '(' << metrics.serialize() << ')'; +} } diff --git a/src/slic3r/GUI/GUI_Utils.hpp b/src/slic3r/GUI/GUI_Utils.hpp index 256d47253..8ca4d9383 100644 --- a/src/slic3r/GUI/GUI_Utils.hpp +++ b/src/slic3r/GUI/GUI_Utils.hpp @@ -3,6 +3,7 @@ #include #include +#include #include @@ -149,9 +150,11 @@ public: bool get_maximized() const { return maximized; } void sanitize_for_display(const wxRect &screen_rect); - std::string serialize(); + std::string serialize() const; }; +std::ostream& operator<<(std::ostream &os, const WindowMetrics& metrics); + }} diff --git a/src/slic3r/GUI/MainFrame.cpp b/src/slic3r/GUI/MainFrame.cpp index 326d59c5e..8192de17e 100644 --- a/src/slic3r/GUI/MainFrame.cpp +++ b/src/slic3r/GUI/MainFrame.cpp @@ -31,7 +31,7 @@ namespace Slic3r { namespace GUI { MainFrame::MainFrame() : -wxFrame(NULL, wxID_ANY, SLIC3R_BUILD, wxDefaultPosition, wxDefaultSize, wxDEFAULT_FRAME_STYLE), +wxFrame(NULL, wxID_ANY, SLIC3R_BUILD, wxDefaultPosition, wxDefaultSize, wxDEFAULT_FRAME_STYLE, "mainframe"), m_printhost_queue_dlg(new PrintHostQueueDialog(this)) { // Load the icon either from the exe, or from the ico file. @@ -80,8 +80,7 @@ wxFrame(NULL, wxID_ANY, SLIC3R_BUILD, wxDefaultPosition, wxDefaultSize, wxDEFAUL event.Veto(); return; } - // save window size - wxGetApp().window_pos_save(this, "mainframe"); + // Save the slic3r.ini.Usually the ini file is saved from "on idle" callback, // but in rare cases it may not have been called yet. wxGetApp().app_config->save(); @@ -93,18 +92,9 @@ wxFrame(NULL, wxID_ANY, SLIC3R_BUILD, wxDefaultPosition, wxDefaultSize, wxDEFAUL event.Skip(); }); - // NB: Restoring the window position is done in a two-phase manner here, - // first the saved position is restored as-is and validation is done after the window is shown - // and initial round of events is complete, because on some platforms that is the only way - // to get an accurate window position & size. - wxGetApp().window_pos_restore(this, "mainframe"); - Bind(wxEVT_SHOW, [this](wxShowEvent&) { - CallAfter([this]() { - wxGetApp().window_pos_sanitize(this); - }); - }); + wxGetApp().persist_window_geometry(this); - update_ui_from_settings(); + update_ui_from_settings(); // FIXME (?) } void MainFrame::init_tabpanel() @@ -447,7 +437,8 @@ void MainFrame::init_menubar() SetMenuBar(menubar); #ifdef __APPLE__ - // This fixes a bug (?) on Mac OS where the quit command doesn't emit window close events + // This fixes a bug on Mac OS where the quit command doesn't emit window close events + // wx bug: https://trac.wxwidgets.org/ticket/18328 wxMenu *apple_menu = menubar->OSXGetAppleMenu(); if (apple_menu != nullptr) { apple_menu->Bind(wxEVT_MENU, [this](wxCommandEvent &) { From 984b1bc1c16f24fe3cf9300582f027df3d6948f9 Mon Sep 17 00:00:00 2001 From: Vojtech Kral Date: Thu, 17 Jan 2019 17:41:07 +0100 Subject: [PATCH 2/4] GUI_App: Remove custom CallAfter --- src/slic3r/GUI/GUI_App.cpp | 29 ----------------------------- src/slic3r/GUI/GUI_App.hpp | 6 ------ 2 files changed, 35 deletions(-) diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 861312f82..9991fee75 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -164,27 +164,8 @@ bool GUI_App::OnInit() // to correct later layouts }); - // This makes CallAfter() work Bind(wxEVT_IDLE, [this](wxIdleEvent& event) { - std::function cur_cb{ nullptr }; - // try to get the mutex. If we can't, just skip this idle event and get the next one. - if (!callback_register.try_lock()) return; - // pop callback - if (m_cb.size() != 0) { - cur_cb = m_cb.top(); - m_cb.pop(); - } - // unlock mutex - this->callback_register.unlock(); - - try { // call the function if it's not nullptr; - if (cur_cb != nullptr) cur_cb(); - } - catch (std::exception& e) { - std::cerr << "Exception thrown: " << e.what() << std::endl; - } - if (app_config->dirty()) app_config->save(); @@ -420,16 +401,6 @@ void GUI_App::import_model(wxWindow *parent, wxArrayString& input_files) dialog.GetPaths(input_files); } -void GUI_App::CallAfter(std::function cb) -{ - // set mutex - callback_register.lock(); - // push function onto stack - m_cb.emplace(cb); - // unset mutex - callback_register.unlock(); -} - // select language from the list of installed languages bool GUI_App::select_language( wxArrayString & names, wxArrayLong & identifiers) diff --git a/src/slic3r/GUI/GUI_App.hpp b/src/slic3r/GUI/GUI_App.hpp index 6d81b3795..c398c449e 100644 --- a/src/slic3r/GUI/GUI_App.hpp +++ b/src/slic3r/GUI/GUI_App.hpp @@ -73,11 +73,6 @@ class GUI_App : public wxApp { bool app_conf_exists{ false }; - // Lock to guard the callback stack - std::mutex callback_register; - // callbacks registered to run during idle event. - std::stack> m_cb{}; - wxColour m_color_label_modified; wxColour m_color_label_sys; wxColour m_color_label_default; @@ -124,7 +119,6 @@ public: void persist_window_geometry(wxTopLevelWindow *window); void update_ui_from_settings(); - void CallAfter(std::function cb); bool select_language(wxArrayString & names, wxArrayLong & identifiers); bool load_language(); From 0d9f26f10b2ceaa9be326859012f55e0a323588b Mon Sep 17 00:00:00 2001 From: Vojtech Kral Date: Thu, 17 Jan 2019 18:06:47 +0100 Subject: [PATCH 3/4] GUI: Remove old window size persistence code --- src/slic3r/GUI/GUI.cpp | 45 ------------------------------------------ src/slic3r/GUI/GUI.hpp | 5 ----- 2 files changed, 50 deletions(-) diff --git a/src/slic3r/GUI/GUI.cpp b/src/slic3r/GUI/GUI.cpp index bccf211ef..0ff4ed161 100644 --- a/src/slic3r/GUI/GUI.cpp +++ b/src/slic3r/GUI/GUI.cpp @@ -348,51 +348,6 @@ bool get_current_screen_size(wxWindow *window, unsigned &width, unsigned &height return true; } -// XXX: remove these -void save_window_size(wxTopLevelWindow *window, const std::string &name) -{ - const wxSize size = window->GetSize(); - const wxPoint pos = window->GetPosition(); - const auto maximized = window->IsMaximized() ? "1" : "0"; - - get_app_config()->set((boost::format("window_%1%_size") % name).str(), (boost::format("%1%;%2%") % size.GetWidth() % size.GetHeight()).str()); - get_app_config()->set((boost::format("window_%1%_maximized") % name).str(), maximized); -} - -void restore_window_size(wxTopLevelWindow *window, const std::string &name) -{ - // XXX: This still doesn't behave nicely in some situations (mostly on Linux). - // The problem is that it's hard to obtain window position with respect to screen geometry reliably - // from wxWidgets. Sometimes wxWidgets claim a window is located on a different screen than on which - // it's actually visible. I suspect this has something to do with window initialization (maybe we - // restore window geometry too early), but haven't yet found a workaround. - - const auto display_idx = wxDisplay::GetFromWindow(window); - if (display_idx == wxNOT_FOUND) { return; } - - const auto display = wxDisplay(display_idx).GetClientArea(); - std::vector pair; - - try { - const auto key_size = (boost::format("window_%1%_size") % name).str(); - if (get_app_config()->has(key_size)) { - if (unescape_strings_cstyle(get_app_config()->get(key_size), pair) && pair.size() == 2) { - auto width = boost::lexical_cast(pair[0]); - auto height = boost::lexical_cast(pair[1]); - - window->SetSize(width, height); - } - } - } catch(const boost::bad_lexical_cast &) {} - - // Maximizing should be the last thing to do. - // This ensure the size and position are sane when the user un-maximizes the window. - const auto key_maximized = (boost::format("window_%1%_maximized") % name).str(); - if (get_app_config()->get(key_maximized) == "1") { - window->Maximize(true); - } -} - void about() { AboutDialog dlg; diff --git a/src/slic3r/GUI/GUI.hpp b/src/slic3r/GUI/GUI.hpp index 79b21b503..e33be8f58 100644 --- a/src/slic3r/GUI/GUI.hpp +++ b/src/slic3r/GUI/GUI.hpp @@ -70,11 +70,6 @@ boost::filesystem::path into_path(const wxString &str); // Returns the dimensions of the screen on which the main frame is displayed bool get_current_screen_size(wxWindow *window, unsigned &width, unsigned &height); -// Save window size and maximized status into AppConfig -void save_window_size(wxTopLevelWindow *window, const std::string &name); -// Restore the above -void restore_window_size(wxTopLevelWindow *window, const std::string &name); - // Display an About dialog extern void about(); // Ask the destop to open the datadir using the default file explorer. From 1685a30ee0601ca97647bfa69e74b82898b2be29 Mon Sep 17 00:00:00 2001 From: Lukas Matena Date: Mon, 21 Jan 2019 15:03:20 +0100 Subject: [PATCH 4/4] Added static asserts on classes initialized by memset to make sure it is possible to do --- src/libslic3r/Fill/FillBase.hpp | 1 + src/libslic3r/Slicing.hpp | 3 +++ 2 files changed, 4 insertions(+) diff --git a/src/libslic3r/Fill/FillBase.hpp b/src/libslic3r/Fill/FillBase.hpp index b67d14339..5ff6d4a31 100644 --- a/src/libslic3r/Fill/FillBase.hpp +++ b/src/libslic3r/Fill/FillBase.hpp @@ -38,6 +38,7 @@ struct FillParams // in this case we don't try to make more continuous paths bool complete; }; +static_assert(std::is_trivially_copyable::value, "FillParams class is not POD (and it should be - see constructor)."); class Fill { diff --git a/src/libslic3r/Slicing.hpp b/src/libslic3r/Slicing.hpp index 395fedc9f..bd3bf7c0b 100644 --- a/src/libslic3r/Slicing.hpp +++ b/src/libslic3r/Slicing.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include "libslic3r.h" namespace Slic3r @@ -91,6 +92,8 @@ struct SlicingParameters coordf_t object_print_z_min; coordf_t object_print_z_max; }; +static_assert(std::is_trivially_copyable::value, "SlicingParameters class is not POD (and it should be - see constructor)."); + // The two slicing parameters lead to the same layering as long as the variable layer thickness is not in action. inline bool equal_layering(const SlicingParameters &sp1, const SlicingParameters &sp2)