Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wxGUI: adding a button for undocking an AuiNotebook tab to wx.Frame (Single-Window GUI) #2667

Merged
merged 43 commits into from
Jun 2, 2023

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented Nov 27, 2022

The implementation of the ability to undock and dock a map window is needed for those of you who would like to still use GRASS as a Multi-Window in the future. It is also very convenient for example when using g.gui.tangible module.

The map panel can be undocked and then docked back into AuiNotebook using the "un/dock" button:

Screenshot from 2022-11-27 13-39-53

After undocking, the independent map display frame is created. This frame can be moved around as a regular frame (you can have it on the second monitor for example).
Screenshot from 2022-11-27 13-49-23

This PR is still under process.

Things that need to be issued:

  • undocked and docked functionality implemented via a new "un/dock" button

  • resizing undocked frame to the size of the original MapPanel

  • when docking, focus on the newly added AuiNotebook page

  • closing the undocked wx.Frame needs to close the associated Layers tab

  • closing the Layers tab must close the associated wx.Frame

  • when closing GRASS, undocked frames and Aui.Notebook pages need to be also closed.

  • fix adding a new Map Notebook page (the AuiNotebook page indexing is id being changed by undocking)

  • fix switching to 3D/2D in undocked frame (meantime found the bug wxGUI: After switching to 3D the map display toolbar is hidden [Bug]  #2672)

  • when undocked, focus on the associated Layers tab

  • check whether Multi-Window still works and adapt

  • create nicer icon for Map Display undock/dock

  • fix close event on undocked wx.Frame

  • fix renaming a page in Layers tab

  • add support for workspaces (isDocked parameter will be stored in a workspace file and after loading a workspace file, GRASS decide which map displays should be undocked right away)

  • fix error related to closing undocked map frame wrapped C/C++ object of type MapPageFrame has been deleted (appeared when before loading a workspace in the moment when all map displays are being closed)

Successful merge of this PR closes the Issue #1748.

@lindakarlovska lindakarlovska marked this pull request as draft November 27, 2022 12:51
@lindakarlovska lindakarlovska added GUI wxGUI related enhancement New feature or request labels Nov 27, 2022
@lindakarlovska lindakarlovska added this to the 8.3.0 milestone Nov 27, 2022
@petrasovaa
Copy link
Contributor

Perhaps an icon may look better? See for example https://github.com/qgis/QGIS/blob/master/images/themes/default/mDockify.svg
maybe we could tweak it.

@lindakarlovska
Copy link
Contributor Author

Perhaps an icon may look better? See for example https://github.com/qgis/QGIS/blob/master/images/themes/default/mDockify.svg maybe we could tweak it.

@petrasovaa What you do think about this idea?
The docking functionality is similarly as New Display or Display Settings related to the whole window. I tried to tweak a bit the Map Display Settings icon.
First suggestion:
Screenshot from 2022-12-03 14-10-49

Second suggestion:
Screenshot from 2022-12-03 14-26-16

if self.GetMapDisplay().IsDocked():
self.mapnotebook.DeletePage(self.GetMapDisplayIndex())
else:
self.GetMapDisplay().GetParent().Destroy()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just self.GetMapDisplay().Destroy() should be enough thanks to the FrameMixin.

@lindakarlovska
Copy link
Contributor Author

Tested now with slightly newer configuration on Ubuntu 20.04 and it works fine here.

Python 3.10.10 (main, Feb  8 2023, 14:50:01) [GCC 9.4.0]
4.2.0 gtk3 (phoenix) wxWidgets 3.2.0

That is a very similar version as mine. Have you tested the problem with storing position in workspace file?

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Feb 11, 2023

Is this to be found in wxdemo?

I have tried to find the methods "Reparent" and "RemovePage" that we use and I have not found them in wxdemo. But both those methods could be simply found in docs - https://docs.wxpython.org/wx.Window.html#wx.Window.Reparent and https://docs.wxpython.org/wx.lib.agw.aui.auibook.AuiNotebook.html#wx.lib.agw.aui.auibook.AuiNotebook.RemovePage.

"Undock" and "Dock", both are called on wx.Panel in our case. Undock is basically the wx.Panel reparent from wx.lib.agw.aui.Notebook to wx.Frame, and the Dock is the opposite operation.

I can share the minimum working undock/dock example which just undock and dock a wx.Panel from one wx.Frame to a different wx.Frame.

Could you try that code?


import wx

class MainFrame(wx.Frame):

    def __init__(self, parent, id=-1, title="", pos=wx.DefaultPosition,
                 size=wx.DefaultSize, style=wx.DEFAULT_FRAME_STYLE |
                                            wx.SUNKEN_BORDER |
                                            wx.CLIP_CHILDREN):

        wx.Frame.__init__(self, parent, id, title, pos, size, style)

        # tell FrameManager to manage this frame
        self.panel = MyPanel(self)
        self.panel.SetCallback(self.Undock)
        self.sizer = wx.BoxSizer()
        self.sizer.Add(self.panel, 1, wx.EXPAND)
        self.SetSizer(self.sizer)

    def Undock(self, panel):
        fr = SecondFrame(self)
        fr.SetTitle("Panel")
        panel.Reparent(fr)
        panel.SetCallback(self.Dock)
        self.sizer.Detach(panel)
        fr.sizer.Add(panel)
        fr.Show()

    def Dock(self, panel):
        fr = panel.GetParent()
        panel.Reparent(self)
        panel.SetCallback(self.Undock)
        fr.Close()


class MyPanel(wx.Panel):
    def __init__(self, parent):
        wx.Panel.__init__(self, parent=parent)
        self.parent = parent
        self.callback = None
        sizer = wx.BoxSizer(wx.VERTICAL)
        self.btn = wx.Button(self, label="(un)dock")
        sizer.Add(self.btn, proportion=0, flag=wx.EXPAND | wx.ALL, border=5)
        sizer.Add(wx.StaticText(self, label="This panel can be separated to a different wx.Frame"),
                  proportion=0, flag=wx.EXPAND | wx.ALL, border=5)
        self.SetSizerAndFit(sizer)
        self.btn.Bind(wx.EVT_BUTTON, self.OnButton)

    def SetCallback(self, function):
        self.callback = function

    def OnButton(self, event):
        self.callback(self)


class SecondFrame(wx.Frame):
    def __init__(self, parent):
        wx.Frame.__init__(self, parent=parent)
        self.sizer = wx.BoxSizer(wx.HORIZONTAL)
        self.SetSizerAndFit(self.sizer)


if __name__ == '__main__':
    app = wx.App()
    fr = MainFrame(None)
    fr.Show()
    app.MainLoop()

@nilason
Copy link
Contributor

nilason commented Feb 11, 2023

Could you try that code?

Thanks! This will probably simplify a lot the tracking down of this issue on Mac. Clicking the button erases the frame in a similar way as in GRASS. Will look into it.

wx.Frame.__init__(self, parent=parent, size=size, pos=pos, title=title)
self.mapdisplay = mapdisplay
self.sizer = wx.BoxSizer(wx.HORIZONTAL)
self.SetSizerAndFit(self.sizer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.SetSizerAndFit(self.sizer)
self.SetSizer(self.sizer)
self.Centre()

This solves the problem on Mac, specifically self.SetSizerAndFit was the culprit.
self.Centre() was needed as the detached panel was placed partly below the macOS main menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicklas, thank you for testing it!
Unfortunately, on my Ubuntu the result does not look good after that change:
Screenshot from 2023-02-13 17-37-37
The content has been put a bit lower and a status bar is not visible.

So we will probably need to apply different solutions based on OS. It would be nice if someone tested it also on Windows. @hellik could I ask you? We would specifically need to verify that undocking and docking using the new Undock/Dock button works fine here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I had the same problem. But I was happy to see THAT much ! :-) , a manual window resize and everything good.
I'll see if I can come up with something regarding this detail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried everything possible, that I could think of, but was unable to make this work as it should (so far). Only resizing "manually", even with as little as a pixel size change, would re-render the status bar and all content correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you tried, but right now the mapdisplay is added to be managed by the sizer after the frame is created (see the UndockMapDisplay method), so it is not completely surprising this doesn't quite work. I suggest to move the frame.sizer.Add(...) call into the MapPageFrame.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right @petrasovaa .. it is not so easy because we have the reparent method here. I have tried several options and the only thing I could think of is putting self.mapdisplay.Reparent(self) directly inside MapPageFrame init. SetSizer needs to be called after reparenting otherwise it does not work.

Now it works for me as expected. Please @nilason, could you test it one more time?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With e2f1db8 it works for me too!

I still need a frame.Centre(wx.BOTH) for correct positioning though, not only will the undocked window be placed partially under macOS menu:

Screenshot

but with multiple displays, the undocked window will always open in primary display (even if single-window is in secondary. With centre command the undocked window opens centred in the same display.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I'm not following this fully, but centering is best done on the parent than on the screen. Center on screen does not work well on extremely large displays like this one:

Screenshot from 2023-02-20 09-43-25

@petrasovaa
Copy link
Contributor

I tested again all the use cases and I did not come to any bugs. Please, could @petrasovaa test it as well?

Another problem is when you close an undocked window by closing the associated layer tree with the x. The layer tree is removed, but the window stays.

@petrasovaa
Copy link
Contributor

Tested now with slightly newer configuration on Ubuntu 20.04 and it works fine here.

Python 3.10.10 (main, Feb  8 2023, 14:50:01) [GCC 9.4.0]
4.2.0 gtk3 (phoenix) wxWidgets 3.2.0

That is a very similar version as mine. Have you tested the problem with storing position in workspace file?

Yes, no problems there. Not sure what's going on, but maybe focus on the other issues now.

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Feb 13, 2023

I tested again all the use cases and I did not come to any bugs. Please, could @petrasovaa test it as well?

Another problem is when you close an undocked window by closing the associated layer tree with the x. The layer tree is removed, but the window stays.

Thanks for catching it. Should be okay now.:)

@petrasovaa
Copy link
Contributor

When I drag a second map display to have 2 displays side by side I get:

Traceback (most recent call last):
  File "/home/akratoc/dev/grass/venv/lib/python3.10/site-
packages/wx/lib/agw/aui/auibook.py", line 5281, in
OnTabEndDrag

self.SetSelectionToPage(page_info)
  File "/home/akratoc/dev/grass/grass_main/dist.x86_64-pc-
linux-gnu/gui/wxpython/main_window/notebook.py", line 120,
in SetSelectionToPage

if not page.IsDocked():
AttributeError
:
'AuiNotebookPage' object has no attribute 'IsDocked'

@lindakarlovska
Copy link
Contributor Author

When I drag a second map display to have 2 displays side by side I get:

Traceback (most recent call last):
  File "/home/akratoc/dev/grass/venv/lib/python3.10/site-
packages/wx/lib/agw/aui/auibook.py", line 5281, in
OnTabEndDrag

self.SetSelectionToPage(page_info)
  File "/home/akratoc/dev/grass/grass_main/dist.x86_64-pc-
linux-gnu/gui/wxpython/main_window/notebook.py", line 120,
in SetSelectionToPage

if not page.IsDocked():
AttributeError
:
'AuiNotebookPage' object has no attribute 'IsDocked'

Unfortunately, I have still a problem in the moving operation itself:
#2671
so I cannot reproduce it.. I am wondering if not to establish an Issue on wxPython on it...

The problem here is in SetSelectionToPage, we need to define a custom SetSelectionToMapPage because we need the wx.Panel as the input, not AuiNotebookPage. Please could you @petrasovaa test it after the changes I made in c3a161e ?

@petrasovaa
Copy link
Contributor

The problem here is in SetSelectionToPage, we need to define a custom SetSelectionToMapPage because we need the wx.Panel as the input, not AuiNotebookPage. Please could you @petrasovaa test it after the changes I made in c3a161e ?

Works great!

@nilason nilason added the Python Related code is in Python label Feb 15, 2023
@petrasovaa petrasovaa modified the milestones: 8.3.0, 8.4.0 Feb 16, 2023
@petrasovaa
Copy link
Contributor

I changed milestone for 8.4.0 since this PR involves a lot of changes that need more testing and while this feature would be nice to have in 8.3, it's not completely necessary.

@nilason
Copy link
Contributor

nilason commented Feb 20, 2023

All fine with d5609a3 too!

Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have now release branch 8.3, we can merge this. More testing welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request GUI wxGUI related Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants