add wxAuiPaneInfo::FloatingClientSize() (PR #25483)
Bill Su
Using wxWindow::SetClientSize() is more platform-independent than wxWindow::SetSize(). Provide that same functionality for wxAuiPaneInfo.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25483
Commit Summary
- b3158b9 add wxAuiPaneInfo::FloatingClientSize()
File Changes
(8 files)
- M include/wx/aui/framemanager.h (5)
- M include/wx/aui/serializer.h (1)
- M interface/wx/aui/framemanager.h (14)
- M samples/aui/auidemo.cpp (12)
- M src/aui/auibar.cpp (1)
- M src/aui/floatpane.cpp (13)
- M src/aui/framemanager.cpp (40)
- M src/xrc/xh_aui.cpp (1)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25483.patch
- https://github.com/wxWidgets/wxWidgets/pull/25483.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for your work on this!
I think it's mostly good but please see the comments below and please check if using "layout2" still works — I think it doesn't, i.e. existing applications would give an error when restoring the new data. And the simplest way to fix it would be to just not update this code at all, we have wxAuiSerializer which is more flexible.
> + result += wxString::Format(wxT("floath=%d;"), pane.floating_size.y);
+ result += wxString::Format(wxT("cli_floatw=%d;"), pane.floating_client_size.x);
+ result += wxString::Format(wxT("cli_floath=%d"), pane.floating_client_size.y);
I wouldn't bother extending the old serialization format, but if we do, we probably need to change the layout version, i.e. use layout3 instead of layout2.
In interface/wx/aui/framemanager.h:
> @@ -765,11 +765,25 @@ class wxAuiPaneInfo
//@{
/**
FloatingSize() sets the size of the floating pane.
+ FloatingClientSize() has precedence over this.
I think we need a blank line here for the docs to appear correctly, or did Doxygen relax its rules about it? In any case, it doesn't hurt the readability and we could also improve the description a bit.
⬇️ Suggested change- FloatingClientSize() has precedence over this. + + FloatingClientSize() has precedence over this, i.e. this size is ignored + if the floating client size is specified.
(also below)
> @@ -2532,10 +2544,22 @@ void wxAuiManager::Update()
{
// frame already exists, make sure its position
// and size reflect the information in wxAuiPaneInfo
- if ((p.frame->GetPosition() != p.floating_pos) || (p.frame->GetSize() != p.floating_size))
+ // give floating_client_size precedence over floating_size
But we still end up calling SetSize()? I thought the whole idea was to call SetClientSize().
> @@ -193,6 +193,7 @@ wxObject *wxAuiXmlHandler::DoCreateResource()
if ( HasParam(wxS("floatable")) ) paneInfo.Floatable( GetBool(wxS("floatable")) );
// Sizes
if ( HasParam(wxS("floating_size")) ) paneInfo.FloatingSize( GetSize(wxS("floating_size")) );
+ if ( HasParam(wxS("floating_client_size")) ) paneInfo.FloatingClientSize( GetSize(wxS("floating_client_size")) );
Could you please also add this to docs/doxygen/overviews/xrc_format.h and misc/schema/xrc_schema.rnc, near the existing floating_size?
> + /* client size has precedence over wnd size, + so I don't think we need to set wnd size */
Minor, but let's make this comment C++ and more authoritative:
⬇️ Suggested change- /* client size has precedence over wnd size, - so I don't think we need to set wnd size */ + // Setting floating client size is enough, there is no need to set floating + // size, as it won't be used if the client size is set.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
wsu-cb left a comment (wxWidgets/wxWidgets#25483)I think it's mostly good but please see the comments below and please check if using "layout2" still works — I think it doesn't, i.e. existing applications would give an error when restoring the new data. And the simplest way to fix it would be to just not update this code at all, we have
wxAuiSerializerwhich is more flexible.
What do you mean by "existing applications" in this context? The addition of the floating_client_size member already makes the new wxWidgets library ABI-incompatible, so existing applications compiled against old libraries can't possibly work with the new wxWidgets code no matter what we do with layout2.
If you mean an old application that reads an external data source containing a layout string generated by the new code, then I'm almost certain you are correct that the old application would wxFAIL(), so I agree that we should change to layout3 to instead result in return false from LoadPerspective().
However, I don't want to abandon that old format. My demonstration of how the new FloatingClientSize() works, and that it works as intended, is to use the auidemo Perspectives | All Panes menu item, and that uses the SavePerspective()/LoadPerspective() code.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
In interface/wx/aui/framemanager.h:
> @@ -765,11 +765,25 @@ class wxAuiPaneInfo
//@{
/**
FloatingSize() sets the size of the floating pane.
+ FloatingClientSize() has precedence over this.
I have never used Doxygen, and have no reason to think its syntax has changed, so I will apply your suggestions here and elsewhere.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -2532,10 +2544,22 @@ void wxAuiManager::Update()
{
// frame already exists, make sure its position
// and size reflect the information in wxAuiPaneInfo
- if ((p.frame->GetPosition() != p.floating_pos) || (p.frame->GetSize() != p.floating_size))
+ // give floating_client_size precedence over floating_size
The code uses ClientToWindowSize() to convert floating_client_size to a window size, and then calls SetSize(). The reason to use SetSize() is that it can set both position and size, but I don't see a way for SetClientSize() to set the window position. (My reading of the SetClientSize(const wxRect&) implementation is that it ignores the position!) Are you suggesting rewriting the code to use separate SetPosition() and SetClientSize() calls?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- 8c9c2d6 documentation changes based on code review
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
In interface/wx/aui/framemanager.h:
> @@ -765,11 +765,25 @@ class wxAuiPaneInfo
//@{
/**
FloatingSize() sets the size of the floating pane.
+ FloatingClientSize() has precedence over this.
Done. See 8c9c2d6.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -193,6 +193,7 @@ wxObject *wxAuiXmlHandler::DoCreateResource()
if ( HasParam(wxS("floatable")) ) paneInfo.Floatable( GetBool(wxS("floatable")) );
// Sizes
if ( HasParam(wxS("floating_size")) ) paneInfo.FloatingSize( GetSize(wxS("floating_size")) );
+ if ( HasParam(wxS("floating_client_size")) ) paneInfo.FloatingClientSize( GetSize(wxS("floating_client_size")) );
Done. See 8c9c2d6.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> + /* client size has precedence over wnd size, + so I don't think we need to set wnd size */
Done. See 8c9c2d6,
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 2 commits.
- 68285ee new AUI layout string content requires changing version id
- b2ee11c fixup! add wxAuiPaneInfo::FloatingClientSize()
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -2532,10 +2544,22 @@ void wxAuiManager::Update()
{
// frame already exists, make sure its position
// and size reflect the information in wxAuiPaneInfo
- if ((p.frame->GetPosition() != p.floating_pos) || (p.frame->GetSize() != p.floating_size))
+ // give floating_client_size precedence over floating_size
I think using SetPosition() and SetClientSize() might be a safer bet, because while I hope ClientToWindowSize() works, I'm pretty sure that SetClientSize() does.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25483)If you mean an old application that reads an external data source containing a layout string generated by the new code, then I'm almost certain you are correct that the old application would wxFAIL(), so I agree that we should change to
layout3to instead result inreturn falsefromLoadPerspective().
Yes, this is what I meant, but while I agree with the change to "layout3", I think we still need to keep support for reading "layout2" in the code: otherwise upgrading an application to newer wx version will lose the saved layout information, which is definitely not what we want. AFAICS, this should be simple to do (we just need to accept either on input).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In docs/doxygen/overviews/xrc_format.h:
> @@ -731,6 +731,9 @@ wxAuiPaneInfo objects have the following properties:
Sets the ideal size for the pane.}
@row3col{floating_size, @ref overview_xrcformat_type_size,
Sets the size of the floating pane.}
+@row3col{floating_client_size, @ref overview_xrcformat_type_size,
+ Sets the client size of the floating pane. This has precedence over
+ floating_size.}
Sorry, a minor addition:
⬇️ Suggested change- floating_size.} + floating_size. This attribute is available since wxWidgets 3.3.1.}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
wsu-cb left a comment (wxWidgets/wxWidgets#25483)If you mean an old application that reads an external data source containing a layout string generated by the new code, then I'm almost certain you are correct that the old application would wxFAIL(), so I agree that we should change to
layout3to instead result inreturn falsefromLoadPerspective().Yes, this is what I meant, but while I agree with the change to "layout3", I think we still need to keep support for reading "layout2" in the code: otherwise upgrading an application to newer wx version will lose the saved layout information, which is definitely not what we want. AFAICS, this should be simple to do (we just need to accept either on input).
-
Should the parser
wxFAIL()if alayout2string containsfloating_client_sizedata? That would require passing the layout version toLoadPaneInfo(). Or can we allowLoadPaneInfo()to accept any field name it recognizes independent of layout version? -
This discussion of versioning has made me realize that old (external)
wxAuiSerializer::SavePane()implementations will silently ignorewxAuiPaneLayoutInfo::floating_client_size, and that data will be lost. I don't know how to avoid that.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 2 commits.
- bb1a194 fixup! documentation changes based on code review
- 15c3a44 fixup! add wxAuiPaneInfo::FloatingClientSize()
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -2532,10 +2544,22 @@ void wxAuiManager::Update()
{
// frame already exists, make sure its position
// and size reflect the information in wxAuiPaneInfo
- if ((p.frame->GetPosition() != p.floating_pos) || (p.frame->GetSize() != p.floating_size))
+ // give floating_client_size precedence over floating_size
Done. See 15c3a44
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
In docs/doxygen/overviews/xrc_format.h:
> @@ -731,6 +731,9 @@ wxAuiPaneInfo objects have the following properties:
Sets the ideal size for the pane.}
@row3col{floating_size, @ref overview_xrcformat_type_size,
Sets the size of the floating pane.}
+@row3col{floating_client_size, @ref overview_xrcformat_type_size,
+ Sets the client size of the floating pane. This has precedence over
+ floating_size.}
Done. See bb1a194
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
wsu-cb left a comment (wxWidgets/wxWidgets#25483)If you mean an old application that reads an external data source containing a layout string generated by the new code, then I'm almost certain you are correct that the old application would wxFAIL(), so I agree that we should change to
layout3to instead result inreturn falsefromLoadPerspective().Yes, this is what I meant, but while I agree with the change to "layout3", I think we still need to keep support for reading "layout2" in the code: otherwise upgrading an application to newer wx version will lose the saved layout information, which is definitely not what we want. AFAICS, this should be simple to do (we just need to accept either on input).
- Should the parser
wxFAIL()if alayout2string containsfloating_client_sizedata? That would require passing the layout version toLoadPaneInfo(). Or can we allowLoadPaneInfo()to accept any field name it recognizes independent of layout version?- This discussion of versioning has made me realize that old (external)
wxAuiSerializer::SavePane()implementations will silently ignorewxAuiPaneLayoutInfo::floating_client_size, and that data will be lost. I don't know how to avoid that.
@vadz Did you overlook this? Do you have any guidance on these issues?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -3684,8 +3709,15 @@ void wxAuiManager::OnFloatingPaneResized(wxWindow* wnd, const wxRect& rect)
// try to find the pane
wxAuiPaneInfo& pane = GetPane(wnd);
wxASSERT_MSG(pane.IsOk(), wxT("Pane window not found"));
+ // if frame isn't fully set up, don't stomp on pos/size info
Why not, actually? I don't really know how can frame be null here, but shouldn't we still remember the restored values to be able to use them when it is finally created?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25483)Sorry, I just didn't have time to return to this, but I just had another quick look and unfortunately some things are still not obvious to me.
FWIW I don't think the code should assert/fail when given any external data.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> part = input.BeforeFirst(wxT('|'));
input = input.AfterFirst(wxT('|'));
part.Trim(true);
part.Trim(false);
- if (part != wxT("layout2"))
+ if (part != wxT("layout3"))
Ideal would be to recognize both "layout2" (for importing existing saved data) and "layout3" (and only try to read new fields when it is used).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
wsu-cb left a comment (wxWidgets/wxWidgets#25483)FWIW I don't think the code should assert/fail when given any external data.
Before this PR, wxAuiManager::LoadPaneInfo() returns void, but wxFAILs when the string starts with a valid layout2 prefix, but nevertheless contains bad data. Would you like me to rewrite this code so that wxAuiManager::LoadPaneInfo() returns bool and does not wxFAIL?
FWIW, my own view is that it is quite reasonable to wxFAIL() when the passed string is invalid because that string might well have been generated by incorrect wxWidgets code.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25483)FWIW I don't think the code should assert/fail when given any external data.
Before this PR,
wxAuiManager::LoadPaneInfo()returnsvoid, butwxFAILs when the string starts with a validlayout2prefix, but nevertheless contains bad data. Would you like me to rewrite this code so thatwxAuiManager::LoadPaneInfo()returnsbooland does not wxFAIL?
I think this would be better, yes, but I don't necessarily want to ask you to fix problems in the existing code. I'd like to avoid introducing, or reproducing, them in the new code, however.
FWIW, my own view is that it is quite reasonable to
wxFAIL()when the passed string is invalid because that string might well have been generated by incorrect wxWidgets code.
My position is that an assert should only be used for programming error, not for any bad data coming from the external sources. Sure, it's possible that this data was generated incorrectly. But it's also possible that it was corrupted in some other way and we must not assert in this case. And because we can't distinguish the two cases, we shouldn't assert in any case.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -3684,8 +3709,15 @@ void wxAuiManager::OnFloatingPaneResized(wxWindow* wnd, const wxRect& rect)
// try to find the pane
wxAuiPaneInfo& pane = GetPane(wnd);
wxASSERT_MSG(pane.IsOk(), wxT("Pane window not found"));
+ // if frame isn't fully set up, don't stomp on pos/size info
Why not, actually? I don't really know how can
framebe null here, but shouldn't we still remember the restored values to be able to use them when it is finally created?
p.frame is null here during the floating pane setup when a docked pane is dragged to become floating because wxAuiManager::Update() calls frame->SetPaneWindow() before setting p.frame = frame. SetPaneWindow() sets the window size, so it reaches OnFloatingPaneResized() when p.frame is null. I tested setting p.frame = frame before frame->SetPaneWindow(), but that causes problems because then the if (!p.IsFloating() && p.frame) test thinks we are UN-floating a floating pane, and destroys windows.
Also, even if those problems were solved, when the pane has a MinSize(), SetPaneWindow() does some size operations before applying the pane's floating size, and that overwrites the floating size the pane had at the beginning of SetPaneWindow(), so when it does actually apply the pane's floating size, it is has been stomped on.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 3 commits.
- c189933 remove wxNavigationEnabled<> from wxVListBox
- 4cd95b8 auidemo: add MinSize() to demo overriding size problem
- 14c47a3 fixup! new AUI layout string content requires changing version id
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 2 commits.
- 277b221 auidemo: add MinSize() to demo overriding size problem
- 227c215 fixup! new AUI layout string content requires changing version id
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
> part = input.BeforeFirst(wxT('|'));
input = input.AfterFirst(wxT('|'));
part.Trim(true);
part.Trim(false);
- if (part != wxT("layout2"))
+ if (part != wxT("layout3"))
Done. See ebd7585
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
This looks almost good, but let's not make backwards-incompatible changes unnecessary. TIA!
> + result += wxString::Format(wxT("cli_floatw=%d;"), pane.floating_client_size.x);
+ result += wxString::Format(wxT("cli_floath=%d"), pane.floating_client_size.y);
This is pretty minor, but why change the order of words in "floating client size" here? I'd use something like
⬇️ Suggested change- result += wxString::Format(wxT("cli_floatw=%d;"), pane.floating_client_size.x);
- result += wxString::Format(wxT("cli_floath=%d"), pane.floating_client_size.y);
+ result += wxString::Format(wxT("floatw_cli=%d;"), pane.floating_client_size.x);
+ result += wxString::Format(wxT("floath_cli=%d"), pane.floating_client_size.y);
for these keys.
In include/wx/aui/framemanager.h:
> @@ -465,7 +470,7 @@ class WXDLLIMPEXP_AUI wxAuiManager : public wxEvtHandler
// Older functions using bespoke text format, prefer using the ones using
// wxAuiSerializer and wxAuiDeserializer above instead in the new code.
wxString SavePaneInfo(const wxAuiPaneInfo& pane);
- void LoadPaneInfo(wxString panePart, wxAuiPaneInfo &pane);
+ bool LoadPaneInfo(wxString layoutVersion, wxString panePart, wxAuiPaneInfo &pane);
Sorry, but I don't think we should change this function, which is part of the public API, in an incompatible way — especially when it should be simple to keep it by doing the following:
- Add a new function, e.g. `LoadVersionedPaneInfo(const wxString& version, const wxString& panePart, wxAuiPaneInfo& pane).
- Call it from
LoadPaneInfo()passing"layout2"as its first argument.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
In include/wx/aui/framemanager.h:
> @@ -465,7 +470,7 @@ class WXDLLIMPEXP_AUI wxAuiManager : public wxEvtHandler
// Older functions using bespoke text format, prefer using the ones using
// wxAuiSerializer and wxAuiDeserializer above instead in the new code.
wxString SavePaneInfo(const wxAuiPaneInfo& pane);
- void LoadPaneInfo(wxString panePart, wxAuiPaneInfo &pane);
+ bool LoadPaneInfo(wxString layoutVersion, wxString panePart, wxAuiPaneInfo &pane);
Sorry, but I don't think we should change this function, which is part of the public API, in an incompatible way — especially when it should be simple to keep it by doing the following:
- Add a new function, e.g. `LoadVersionedPaneInfo(const wxString& version, const wxString& panePart, wxAuiPaneInfo& pane).
- Call it from
LoadPaneInfo()passing"layout2"as its first argument.
But this PR modifies SavePaneInfo() to generate layout3 data that 2.'s implementation of LoadPaneInfo() can't read. That doesn't seem like a sensible API to me. Are you implying that there should also be SaveVersionedPaneInfo()?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In include/wx/aui/framemanager.h:
> @@ -465,7 +470,7 @@ class WXDLLIMPEXP_AUI wxAuiManager : public wxEvtHandler
// Older functions using bespoke text format, prefer using the ones using
// wxAuiSerializer and wxAuiDeserializer above instead in the new code.
wxString SavePaneInfo(const wxAuiPaneInfo& pane);
- void LoadPaneInfo(wxString panePart, wxAuiPaneInfo &pane);
+ bool LoadPaneInfo(wxString layoutVersion, wxString panePart, wxAuiPaneInfo &pane);
Hmm, good point. I'm not sure how/if LoadPaneInfo() is used outside of wx (I wouldn't have made it public in the first place), but I think it may be used if the application stores pane information in some custom storage and so it should be able to load the data in v2 format currently stored there. But it should be also able to read the data in v3 format which will be written by SavePaneInfo()...
So we need to have a function which can load either v3 (this could be called internally when we know that we're using v3) or accept v2-or-v3, which LoadPaneInfo() would need to do.
Sorry about all the complications, this is why I had initially said that it would be better to avoid modifying this code at all. But if you want to do it, we really need to avoid breaking the existing code ("first, do no harm").
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 2 commits.
- 0946611 rename cli_floatw/cli_floath to floatw_cli/floath_cli
- e775f31 restore LoadPaneInfo() backward compatibility
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> + result += wxString::Format(wxT("cli_floatw=%d;"), pane.floating_client_size.x);
+ result += wxString::Format(wxT("cli_floath=%d"), pane.floating_client_size.y);
Done. See 0946611
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
In include/wx/aui/framemanager.h:
> @@ -465,7 +470,7 @@ class WXDLLIMPEXP_AUI wxAuiManager : public wxEvtHandler
// Older functions using bespoke text format, prefer using the ones using
// wxAuiSerializer and wxAuiDeserializer above instead in the new code.
wxString SavePaneInfo(const wxAuiPaneInfo& pane);
- void LoadPaneInfo(wxString panePart, wxAuiPaneInfo &pane);
+ bool LoadPaneInfo(wxString layoutVersion, wxString panePart, wxAuiPaneInfo &pane);
What do you think of e775f31?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, I think it should be good now.
Looking at the latest diff, it's clear that we could make our life much simpler by just always loading pane info as "layout3" (as it's a strict superset of "layout2"), this would only lose us detecting errors due to extra "cli" fields in "layout2" strings, which is probably not a big deal... But I don't know if you want to change this further.
> input = input.AfterFirst(wxT('|'));
- part.Trim(true);
- part.Trim(false);
- if (part != wxT("layout2"))
+ layoutVersion.Trim(true);
+ layoutVersion.Trim(false);
+ if (layoutVersion != wxT("layout3") &&
This is a typo, right? Should be
⬇️ Suggested change- if (layoutVersion != wxT("layout3") &&
+ if (layoutVersion != wxT("layout2") &&
> + // This is redundant since layout3
+ // recognizes all the layout2 keys,
+ // so should this be removed?
+ LoadPaneInfoVersioned("layout2", pane_part, pane);
Yes, I think it should be removed, but a comment added above saying that we use "layout3" here because it is capable of loading both "layout2" and "layout3" data and we need to do it in this function for compatibility.
> @@ -3684,8 +3709,15 @@ void wxAuiManager::OnFloatingPaneResized(wxWindow* wnd, const wxRect& rect)
// try to find the pane
wxAuiPaneInfo& pane = GetPane(wnd);
wxASSERT_MSG(pane.IsOk(), wxT("Pane window not found"));
+ // if frame isn't fully set up, don't stomp on pos/size info
Do I understand correctly that this change (i.e. just this conditional return) is completely independent of the other ones and fixes a separate bug which (again, IIUC) means that currently we lose the pane size when floating it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -3684,8 +3709,15 @@ void wxAuiManager::OnFloatingPaneResized(wxWindow* wnd, const wxRect& rect)
// try to find the pane
wxAuiPaneInfo& pane = GetPane(wnd);
wxASSERT_MSG(pane.IsOk(), wxT("Pane window not found"));
+ // if frame isn't fully set up, don't stomp on pos/size info
No, this is a new problem. The original code was using window size, not client size, and the wxAuiFloatingFrame ctor can take window size as a parameter, and that allows everything to work. However, there is no good way to get from client size to window size before the window exists, and client size can't be passed to the ctor. This means that setting the client size doesn't get done until later, and if min_size is set, the min_size operations trigger OnFloatingPaneResized() before the client size gets applied to wxAuiFloatingFrame. This conditional return is the least-invasive change I could find that avoided problems.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
> input = input.AfterFirst(wxT('|'));
- part.Trim(true);
- part.Trim(false);
- if (part != wxT("layout2"))
+ layoutVersion.Trim(true);
+ layoutVersion.Trim(false);
+ if (layoutVersion != wxT("layout3") &&
You are correct. See aab24ad
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
> + // This is redundant since layout3
+ // recognizes all the layout2 keys,
+ // so should this be removed?
+ LoadPaneInfoVersioned("layout2", pane_part, pane);
Done. See 8a96664
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25483)Great, thanks! This looks finally good to go, so I'm merging and pushing it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.