Correct client area origin and navigation back from secondary from on IOS (PR #25841)
RobertRoeb
- Add extra space for iPhone status bar, currently hard coded as a temporary fix until we can calculate it
- Use it in GetClientAreaOrigin() and use that, respectively, in wxTopLevelWindow::Layout to place the client window correctly
- Add default wxToolbar in all secondary frames as they have no close button with an [X], but most usually a < like sign as a way to return to the previous wxFrame = "screen"
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25841
Commit Summary
- bfa3db9 Correct client area pos and navigation back from secondary wxFrame on iOS
File Changes
(4 files)
- A art/prev_screen.xpm (121)
- M src/common/toplvcmn.cpp (8)
- M src/osx/carbon/frame.cpp (36)
- M src/osx/nonownedwnd_osx.cpp (25)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25841.patch
- https://github.com/wxWidgets/wxWidgets/pull/25841.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 requested changes on this pull request.
Sorry but this is really more of a draft-quality than a ready-to-merge PR. Could you please polish it at least a little bit? TIA!
> @@ -0,0 +1,121 @@ +/* XPM */
We should be really using PNGs for any new icons IMNSHO.
> @@ -496,7 +496,13 @@ bool wxTopLevelWindowBase::Layout()
int clientW, clientH;
DoGetClientSize(&clientW, &clientH);
- child->SetSize(0, 0, clientW, clientH);
+#ifndef __WXOSX_IPHONE__
Can we please avoid this, either by using GetClientAreaOrigin() under all platforms if this is indeed the right thing to do or by adding another virtual function, e.g. GetUsableAreaOrigin() if it is not. But using such preprocessor checks is just not a good idea.
> @@ -60,6 +66,20 @@ bool wxFrame::Create(wxWindow *parent,
if ( !wxTopLevelWindow::Create(parent, id, title, pos, size, style, name) )
return false;
+#ifdef __WXOSX_IPHONE__
+ if (parent != NULL) {
⬇️ Suggested change
- if (parent != NULL) {
+ if (parent != nullptr)
+ {
> @@ -60,6 +66,20 @@ bool wxFrame::Create(wxWindow *parent,
if ( !wxTopLevelWindow::Create(parent, id, title, pos, size, style, name) )
return false;
+#ifdef __WXOSX_IPHONE__
+ if (parent != NULL) {
+ // We are on the next screen, provide a back button and title
+ wxToolBar *tb = CreateToolBar();
+ // tb->AddTool( wxID_CLOSE, wxEmptyString, wxArtProvider::GetBitmap(wxART_PREV_SCREEN) ); doesn't work for some reason
+ tb->AddTool( wxID_CLOSE, wxEmptyString, wxBitmap( prev_screen_xpm ) );
+ tb->AddStretchableSpace(); // doesn't yet stretch on iOS, so at least add two of them
+ tb->AddStretchableSpace();
+ tb->AddControl( new wxStaticText( tb, -1, title ) );
⬇️ Suggested change
- tb->AddControl( new wxStaticText( tb, -1, title ) ); + tb->AddControl( new wxStaticText( tb, wxID_ANY, title ) );
> @@ -60,6 +66,20 @@ bool wxFrame::Create(wxWindow *parent,
if ( !wxTopLevelWindow::Create(parent, id, title, pos, size, style, name) )
return false;
+#ifdef __WXOSX_IPHONE__
+ if (parent != NULL) {
+ // We are on the next screen, provide a back button and title
+ wxToolBar *tb = CreateToolBar();
+ // tb->AddTool( wxID_CLOSE, wxEmptyString, wxArtProvider::GetBitmap(wxART_PREV_SCREEN) ); doesn't work for some reason
It really should, but if it doesn't, it would be worth opening an issue about it and removing this commented out line.
> @@ -60,6 +66,20 @@ bool wxFrame::Create(wxWindow *parent,
if ( !wxTopLevelWindow::Create(parent, id, title, pos, size, style, name) )
return false;
+#ifdef __WXOSX_IPHONE__
+ if (parent != NULL) {
+ // We are on the next screen, provide a back button and title
+ wxToolBar *tb = CreateToolBar();
+ // tb->AddTool( wxID_CLOSE, wxEmptyString, wxArtProvider::GetBitmap(wxART_PREV_SCREEN) ); doesn't work for some reason
+ tb->AddTool( wxID_CLOSE, wxEmptyString, wxBitmap( prev_screen_xpm ) );
+ tb->AddStretchableSpace(); // doesn't yet stretch on iOS, so at least add two of them
Again, instead of working around this here, why not make AddStretchableSpace() in wxiOS implementation add 2 spaces?
> @@ -397,8 +417,22 @@ void wxFrame::PositionToolBar()
else
{
#if !wxOSX_USE_NATIVE_TOOLBAR
+#ifdef __WXOSX_IPHONE__
This should also be a virtual function in wxWidgetImpl.
In src/osx/nonownedwnd_osx.cpp:
> @@ -253,6 +253,20 @@ wxPoint wxNonOwnedWindow::GetClientAreaOrigin() const
{
int left, top, width, height;
m_nowpeer->GetContentArea(left, top, width, height);
+
+#ifdef __WXOSX_IPHONE__
And by having it in a virtual function we'd at least avoid duplicating this.
In src/osx/nonownedwnd_osx.cpp:
> @@ -470,6 +484,17 @@ void wxNonOwnedWindow::DoGetClientSize( int *width, int *height ) const
// status bar, therefore we use the content view's area
#ifdef __WXOSX_IPHONE__
GetPeer()->GetContentArea(left, top, w, h);
+
+ // TODO, find a way to find the safe area not covered at the top
Sorry, triplicating.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb pushed 1 commit.
- b4313bf Streamlined GetClientAreaOrigin(), enable stretchable toolbar items in IOS
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
Not sure why- all icons that we currently ship are XPM
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> @@ -496,7 +496,13 @@ bool wxTopLevelWindowBase::Layout()
int clientW, clientH;
DoGetClientSize(&clientW, &clientH);
- child->SetSize(0, 0, clientW, clientH);
+#ifndef __WXOSX_IPHONE__
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> @@ -60,6 +66,20 @@ bool wxFrame::Create(wxWindow *parent,
if ( !wxTopLevelWindow::Create(parent, id, title, pos, size, style, name) )
return false;
+#ifdef __WXOSX_IPHONE__
+ if (parent != NULL) {
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
> @@ -60,6 +66,20 @@ bool wxFrame::Create(wxWindow *parent,
if ( !wxTopLevelWindow::Create(parent, id, title, pos, size, style, name) )
return false;
+#ifdef __WXOSX_IPHONE__
+ if (parent != NULL) {
+ // We are on the next screen, provide a back button and title
+ wxToolBar *tb = CreateToolBar();
+ // tb->AddTool( wxID_CLOSE, wxEmptyString, wxArtProvider::GetBitmap(wxART_PREV_SCREEN) ); doesn't work for some reason
+ tb->AddTool( wxID_CLOSE, wxEmptyString, wxBitmap( prev_screen_xpm ) );
+ tb->AddStretchableSpace(); // doesn't yet stretch on iOS, so at least add two of them
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> @@ -60,6 +66,20 @@ bool wxFrame::Create(wxWindow *parent,
if ( !wxTopLevelWindow::Create(parent, id, title, pos, size, style, name) )
return false;
+#ifdef __WXOSX_IPHONE__
+ if (parent != NULL) {
+ // We are on the next screen, provide a back button and title
+ wxToolBar *tb = CreateToolBar();
+ // tb->AddTool( wxID_CLOSE, wxEmptyString, wxArtProvider::GetBitmap(wxART_PREV_SCREEN) ); doesn't work for some reason
Tried, but failed
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
In src/osx/nonownedwnd_osx.cpp:
> @@ -470,6 +484,17 @@ void wxNonOwnedWindow::DoGetClientSize( int *width, int *height ) const
// status bar, therefore we use the content view's area
#ifdef __WXOSX_IPHONE__
GetPeer()->GetContentArea(left, top, w, h);
+
+ // TODO, find a way to find the safe area not covered at the top
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb pushed 1 commit.
- 4fc8359 Use nullptr instead of NULL
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> @@ -60,6 +66,20 @@ bool wxFrame::Create(wxWindow *parent,
if ( !wxTopLevelWindow::Create(parent, id, title, pos, size, style, name) )
return false;
+#ifdef __WXOSX_IPHONE__
+ if (parent != NULL) {
+ // We are on the next screen, provide a back button and title
+ wxToolBar *tb = CreateToolBar();
+ // tb->AddTool( wxID_CLOSE, wxEmptyString, wxArtProvider::GetBitmap(wxART_PREV_SCREEN) ); doesn't work for some reason
+ tb->AddTool( wxID_CLOSE, wxEmptyString, wxBitmap( prev_screen_xpm ) );
+ tb->AddStretchableSpace(); // doesn't yet stretch on iOS, so at least add two of them
+ tb->AddStretchableSpace();
+ tb->AddControl( new wxStaticText( tb, -1, title ) );
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#25841)I tested the changes on wxMac and wxGTK and didn't see anything broken. Indeed, I reverted all or nearly all changes in non-OSX or non-IOS code.
BackNavi.IOS.png (view on web)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#25841)Here is my hack over a hack to allow stretchable spacer in iOS toolbar. The other solution would be to remove both hacks and add a flag to CreateTool() in all ports. I didn't do that because someone must have thought about that when he added the hack that is currently in the code and decided against the clean solution.
BTW, I tried really hard to also use Realize() by removing the unstretchable spacers in the toolbar and inserting stretchable ones if they were marked with MarkStretchable(). That almost worked but ruined the layout of the toolbar for reasons I could not figure out after 2h.
virtual wxToolBarToolBase *CreateTool(int toolid,
const wxString& label,
const wxBitmapBundle& bmpNormal,
const wxBitmapBundle& bmpDisabled = wxBitmapBundle(),
wxItemKind kind = wxITEM_NORMAL,
wxObject *clientData = nullptr,
const wxString& shortHelp = wxEmptyString,
const wxString& longHelp = wxEmptyString,
bool stretchable = false ) = 0;
<<<< This is in the PR right now
wxToolBarToolBase *wxToolBarBase::InsertStretchableSpace(size_t pos)
{
#ifdef WXOSX_IPHONE
// The hack below does not work on iPhone as the tools are
// created in the constructor not Realize(). I hack around the hack
// by adding a distinct ID
const int id = wxID_STRETCHABLE_SEPARATOR;
#else
const int id = wxID_SEPARATOR;
#endif
wxToolBarToolBase * const tool = CreateSeparator( id );
if ( tool )
{
// this is a hack but we know that all the current implementations
// don't really use the tool when it's created, they will do it
// InsertTool() at earliest and maybe even in Realize() much later
//
// so we can create the tool as a plain separator and mark it as being
// a stretchable space later
tool->MakeStretchable();
}
return DoInsertNewTool(pos, tool);
}
—
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#25841)The "clean" solution is not really clean because bool stretchable only makes sense when combined with wxITEM_SEPARATOR and nothing else. A decent API shouldn't allow specifying nonsensical combinations of parameters, like wxITEM_CHECK and stretchable=true. Of course, wxToolBar is already guilty of it, but it's not a reason to make things worse.
The only alternative API I see is to add wxITEM_STRETCHABLE_SEPARATOR but this doesn't seem appealing. Making an existing separator stretchable doesn't seem very out of place as it's similar to making an existing check tool checked etc. Of course, if this really can't be implemented for iOS we may want to revisit it, but for now I guess we can live with wxID_STRETCHABLE_SEPARATOR hack, although I think making CreateSeparator() virtual and overriding it in wxiOS wxToolBar would be slightly less ugly than checking for iOS in common code.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb pushed 1 commit.
- 79ce11e Corrected typos and whitespace issues
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.